[WRONG BRANCH] fix(providers): harden Alibaba migration backup - #274
[WRONG BRANCH] fix(providers): harden Alibaba migration backup#274luvs01 wants to merge 1 commit into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughAlibaba backup creation now hardens temporary backup files before verification and publication. The default implementation applies mode ChangesAlibaba backup hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟠 High · up to The change can leave credential-bearing temporary files with unsafe permissions when hardening fails, and can report an existing backup even though publication never succeeded. These fail-closed and backup-publication correctness issues should be fixed before merge. Sequence Diagram(s)sequenceDiagram
participant AlibabaBackupIO
participant TemporaryBackup
participant Filesystem
AlibabaBackupIO->>TemporaryBackup: Create temporary backup
AlibabaBackupIO->>Filesystem: Harden temporary backup
Filesystem-->>AlibabaBackupIO: Return success or error
AlibabaBackupIO->>TemporaryBackup: Verify and publish on success
AlibabaBackupIO->>TemporaryBackup: Remove temporary and unpublished files on error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba23bb43f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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); |
There was a problem hiding this comment.
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 👍 / 👎.
| read: path => readFileSync(path), | ||
| copy: (source, destination) => copyFileSync(source, destination), | ||
| harden: path => { | ||
| try { chmodSync(path, 0o600); } catch { /* platform may ignore chmod */ } |
There was a problem hiding this comment.
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 👍 / 👎.
| 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); |
There was a problem hiding this comment.
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 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/alibaba-region-backup.ts (1)
68-79: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLimit
EEXISThandling to publication.Because
io.harden(temp)runs inside the outertry, a hardening error withcode === "EEXIST"reaches Line 79. The function then returns"reused"even thoughpublishNoReplacedid not run. The backup path can remain absent while the caller believes it exists.Move the
EEXISTcheck into a narrowtry/catcharoundio.publishNoReplace. Add a regression test intests/alibaba-region-backup.test.tswherehardenthrows an error withcode === "EEXIST".Proposed fix
- io.publishNoReplace(temp, backup); + try { + io.publishNoReplace(temp, backup); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "EEXIST") return "reused"; + throw error; + } return "created"; - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "EEXIST") return "reused"; - throw error; } finally {🤖 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 68 - 79, Restrict the EEXIST-to-"reused" handling to the io.publishNoReplace call by wrapping only publication in its own try/catch; allow io.harden and earlier copy/verification failures, including EEXIST, to propagate normally. Add a regression test in the Alibaba backup tests where harden throws an EEXIST error and assert the operation does not report "reused".
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/providers/alibaba-region-backup.ts`:
- Around line 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.
In `@tests/alibaba-region-backup.test.ts`:
- 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.
---
Outside diff comments:
In `@src/providers/alibaba-region-backup.ts`:
- Around line 68-79: Restrict the EEXIST-to-"reused" handling to the
io.publishNoReplace call by wrapping only publication in its own try/catch;
allow io.harden and earlier copy/verification failures, including EEXIST, to
propagate normally. Add a regression test in the Alibaba backup tests where
harden throws an EEXIST error and assert the operation does not report "reused".
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2961c1ac-55e5-4aec-8950-16a721785178
📒 Files selected for processing (2)
src/providers/alibaba-region-backup.tstests/alibaba-region-backup.test.ts
| harden: path => { | ||
| try { chmodSync(path, 0o600); } catch { /* platform may ignore chmod */ } | ||
| if (process.platform === "win32") hardenSecretPath(path, { required: true }); | ||
| }, |
There was a problem hiding this comment.
🔒 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.tsRepository: 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' srcRepository: 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 || trueRepository: 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.
| 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); |
There was a problem hiding this comment.
🔒 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 testsRepository: 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 180Repository: 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
1 participant
Motivation
0600chmod or Windows ACL hardening, which risks exposing secrets on some platforms or directory ACL configurations.Description
hardenstep that sets mode0600and calls the WindowshardenSecretPathwhen applicable insrc/providers/alibaba-region-backup.ts.AlibabaBackupIOinterface with ahardenhook and make the default IO performchmodSync(..., 0o600)plus Windows ACL hardening where available, and callio.harden(temp)before publishing the link.tests/alibaba-region-backup.test.tsto assert the backup is0600on POSIX, and to verify that hardening failures leave no published backup or temp file.Testing
bun test tests/alibaba-region-backup.test.tsand all added and existing tests in that file passed (6/6).bun x tsc --noEmit(bun run typecheck) andbun run privacy:scan, both of which completed successfully.bun run test; the focused backup tests remained green but the full suite encountered unrelated timeouts and failures in other areas (e.g. API-key attribution, provider management, image normalization), so the change is covered by focused regression tests while the unrelated full-suite issues remain outside the scope of this patch.Codex Task
Summary by CodeRabbit