Skip to content

[WRONG BRANCH] fix(providers): harden Alibaba migration backup #274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
luvs01 wants to merge 1 commit into
base: main
Choose a base branch
from
+33 −2
Draft

[WRONG BRANCH] fix(providers): harden Alibaba migration backup #274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/providers/alibaba-region-backup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { copyFileSync, existsSync, linkSync, readFileSync, rmSync } from "node:fs";
import { chmodSync, copyFileSync, existsSync, linkSync, readFileSync, rmSync } from "node:fs";
import { getConfigPath } from "../config";
import { hardenSecretPath } from "../lib/windows-secret-acl";

export interface AlibabaBackupIO {
exists: (path: string) => boolean;
read: (path: string) => Buffer;
copy: (source: string, destination: string) => void;
harden: (path: string) => void;
/** Publish with no-replace semantics: fails with EEXIST if the destination exists. */
publishNoReplace: (temp: string, destination: string) => void;
remove: (path: string) => void;
Expand All @@ -14,6 +16,10 @@ const DEFAULT_IO: AlibabaBackupIO = {
exists: existsSync,
read: path => readFileSync(path),
copy: (source, destination) => copyFileSync(source, destination),
harden: path => {
try { chmodSync(path, 0o600); } catch { /* platform may ignore chmod */ }

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fail closed when POSIX chmod fails

On a non-Windows filesystem where chmodSync throws, such as some network or removable mounts, this catch reports hardening as successful and the routine proceeds to publish the secret-bearing backup with its copied/inherited permissions. The added failure test only covers an injected harden that throws, so it misses this production path; let chmod failures propagate on POSIX or verify that the resulting mode is 0600, relying on the required ACL operation only on Windows.

AGENTS.md reference: AGENTS.md:L218-L224

Useful? React with 👍 / 👎.

if (process.platform === "win32") hardenSecretPath(path, { required: true });
},
Comment on lines +19 to +22

@coderabbitai coderabbitai Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail
rg -n -C 12 'HardenResult|hardenEntry|hardenSecretPath|required' src/lib/windows-secret-acl.ts

Repository: luvs01/opencodex

Length of output: 15241

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '--- target file ---'
cat -n src/providers/alibaba-region-backup.ts | sed -n '1,180p'

printf '%s\n' '--- harden and backup call sites ---'
rg -n -C 5 'DEFAULT_IO|backupConfigBeforeAlibabaRegionMigration|hardenSecretPath|chmodSync' src

Repository: luvs01/opencodex

Length of output: 50372

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '--- Alibaba backup tests and fixtures ---'
fd -i 'alibaba|backup' . --type f | sort
rg -n -C 8 'AlibabaBackup|pre-alibaba-region|backupConfigBeforeAlibabaRegionMigration|harden.*throw|chmod' --glob '*test*' --glob '*spec*' src test tests 2>/dev/null || true

printf '%s\n' '--- focused hardening tests ---'
rg -n -C 6 'hardenSecretPath|HardenResult|required: true|required: false|chmodSync' --glob '*test*' --glob '*spec*' src test tests 2>/dev/null | head -n 500 || true

Repository: luvs01/opencodex

Length of output: 50373

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

cat -n tests/alibaba-region-backup.test.ts
printf '%s\n' '--- relevant hardening contract tests ---'
cat -n tests/windows-secret-acl.test.ts | sed -n '1368,1425p'

Repository: luvs01/opencodex

Length of output: 8326

Make POSIX hardening fail closed.

At src/providers/alibaba-region-backup.ts:19-22, do not suppress chmodSync failures. If chmodSync fails on POSIX, the credential-bearing temporary file can retain unsafe permissions, but the backup flow continues to verification and publication.

Remove the try/catch around chmodSync. Keep hardenSecretPath(path, { required: true }) on Windows; required mode throws on ACL failures.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/providers/alibaba-region-backup.ts` around lines 19 - 22, Update the
harden callback to call chmodSync directly without catching or suppressing
failures, so POSIX permission errors propagate and stop the backup flow.
Preserve the existing Windows-specific hardenSecretPath(path, { required: true
}) behavior.

publishNoReplace: linkSync,
remove: path => rmSync(path, { force: true }),
};
Expand Down Expand Up @@ -57,6 +63,9 @@ export function backupConfigBeforeAlibabaRegionMigration(
const temp = `${backup}.${process.pid}.tmp`;
try {
io.copy(configPath, temp);
// The snapshot contains credentials. Harden it before publication so the
// stable backup path is never exposed with inherited permissions or ACLs.
io.harden(temp);

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remediate backups created by v2.11.0

When upgrading an installation that already ran v2.11.0, this new hardening call is never reached: a successful prior migration makes runAlibabaRegionStartupMigration return when projection.changed is false, while an aborted prior run finds the backup and returns "reused" before this line. Consequently, the legacy .pre-alibaba-region-v1.bak that motivated this security fix can retain permissive permissions indefinitely and continue exposing credentials; harden the stable legacy path during startup/load, including before returning "reused", rather than only hardening newly created temps.

AGENTS.md reference: AGENTS.md:L218-L224

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear the hardening memo after removing the temp

On Windows, if this hardening call times out, hardenSecretPath records a timeout memo keyed by this temp path. The finally block removes the file without calling forgetEphemeralSecretPath, and because the temp name is deterministic for the process PID, retrying startServer in the same process recreates the same path and immediately receives the cached timeout instead of retrying icacls, even after the transient condition has recovered; clear the ephemeral hardening state after confirmed removal, as the other atomic writers do.

Useful? React with 👍 / 👎.

// Verify before publishing: a short copy must never become the snapshot.
if (!io.read(temp).equals(source)) {
throw new AlibabaBackupIntegrityError(`failed to write a complete backup to ${temp}`);
Expand Down
24 changes: 23 additions & 1 deletion tests/alibaba-region-backup.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, test } from "bun:test";
import { existsSync, linkSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
import { existsSync, linkSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import {
Expand All @@ -22,6 +22,7 @@ test("creates a snapshot, then never replaces it", () => {
writeFileSync(configPath, '{"before":true}', "utf8");
expect(backupConfigBeforeAlibabaRegionMigration(configPath)).toBe("created");
expect(readFileSync(backupPath, "utf8")).toBe('{"before":true}');
if (process.platform !== "win32") expect(statSync(backupPath).mode & 0o777).toBe(0o600);

@coderabbitai coderabbitai Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 'hardenSecretPath|HardenResult|required: true|ACL|process\.platform' src tests

Repository: luvs01/opencodex

Length of output: 50373

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '--- Alibaba test ---'
cat -n tests/alibaba-region-backup.test.ts | sed -n '1,125p'

printf '%s\n' '--- Alibaba implementation ---'
cat -n src/providers/alibaba-region-backup.ts | sed -n '1,110p'

printf '%s\n' '--- Windows ACL tests: declarations and relevant cases ---'
rg -n -C 6 'describe|test\(|hardenSecretPath\(|required: true|required: false|setPlatformForTests|setIcaclsRunnerForTests|HardenResult' tests/windows-secret-acl.test.ts

printf '%s\n' '--- Windows ACL implementation ---'
cat -n src/lib/windows-secret-acl.ts | sed -n '730,825p'

Repository: luvs01/opencodex

Length of output: 50372

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '--- Direct Alibaba backup coverage ---'
rg -n -C 3 'backupConfigBeforeAlibabaRegionMigration|AlibabaBackupIO|DEFAULT_IO' src tests

printf '%s\n' '--- Exact Windows ACL implementation around the requested location ---'
cat -n src/lib/windows-secret-acl.ts | sed -n '775,805p'

printf '%s\n' '--- Relevant Windows ACL failure tests only ---'
sed -n '458,545p' tests/windows-secret-acl.test.ts

printf '%s\n' '--- Test seams that can force Windows behavior ---'
rg -n -C 3 'Object\.defineProperty\(process, "platform"|setPlatformForTests\("win32"\)' tests | head -n 180

Repository: luvs01/opencodex

Length of output: 25575

Add default Windows ACL coverage for AlibabaBackupIO.DEFAULT_IO.

tests/alibaba-region-backup.test.ts:17-29 and 83-100 use custom AlibabaBackupIO objects, so they do not exercise DEFAULT_IO.harden in src/providers/alibaba-region-backup.ts:15-22. Add a focused Windows-seam test that runs the default I/O path, asserts successful hardening before publication, and asserts that a required ACL failure leaves no backup or temporary file. Existing tests/windows-secret-acl.test.ts:464-545 covers hardenSecretPath itself, but not this production call edge.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/alibaba-region-backup.test.ts` at line 25, Add focused Windows-seam
coverage for the production path using AlibabaBackupIO.DEFAULT_IO rather than
custom I/O objects. Verify DEFAULT_IO.harden successfully applies ACL protection
before the backup is published, and add a failure case where a required ACL
error leaves neither the backup nor its temporary file. Keep direct
hardenSecretPath behavior covered by the existing Windows ACL tests.

Source: Path instructions

expect(backupConfigBeforeAlibabaRegionMigration(configPath)).toBe("reused");
expect(readFileSync(backupPath, "utf8")).toBe('{"before":true}');
} finally { rmSync(dir, { recursive: true, force: true }); }
Expand Down Expand Up @@ -52,6 +53,7 @@ test("a short copy is never published", () => {
exists: existsSync,
read: path => readFileSync(path),
copy: (_source, destination) => { writeFileSync(destination, '{"bef', "utf8"); },
harden: () => {},
publishNoReplace: linkSync,
remove: path => rmSync(path, { force: true }),
})).toThrow(AlibabaBackupIntegrityError);
Expand All @@ -69,10 +71,30 @@ test("a failed copy leaves no snapshot and no temp file", () => {
exists: existsSync,
read: path => readFileSync(path),
copy: () => { throw new Error("disk full"); },
harden: () => {},
publishNoReplace: linkSync,
remove: path => { removed.push(path); rmSync(path, { force: true }); },
})).toThrow("disk full");
expect(existsSync(`${configPath}.pre-alibaba-region-v1.bak`)).toBe(false);
expect(removed).toHaveLength(1);
} finally { rmSync(dir, { recursive: true, force: true }); }
});

test("a failed harden never publishes the secret-bearing snapshot", () => {
const dir = mkdtempSync(join(tmpdir(), "ocx-bak-"));
const configPath = join(dir, "config.json");
const backupPath = `${configPath}.pre-alibaba-region-v1.bak`;
try {
writeFileSync(configPath, '{"before":true}', "utf8");
expect(() => backupConfigBeforeAlibabaRegionMigration(configPath, {
exists: existsSync,
read: path => readFileSync(path),
copy: (source, destination) => writeFileSync(destination, readFileSync(source)),
harden: () => { throw new Error("ACL hardening failed"); },
publishNoReplace: linkSync,
remove: path => rmSync(path, { force: true }),
})).toThrow("ACL hardening failed");
expect(existsSync(backupPath)).toBe(false);
expect(existsSync(`${backupPath}.${process.pid}.tmp`)).toBe(false);
} finally { rmSync(dir, { recursive: true, force: true }); }
});
Loading