Fix SecretStore first-use parallel write race#103
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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: d509116516
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9005caae94
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8f804cecd
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03bde98e72
ℹ️ 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".
33a7bce to
2fc2dc8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eec5be4586
ℹ️ 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".
Main claimed CHG-0042/CHG-0043 via #104, so this change moves to CHG-0044 and change-sequence.json is aligned with main to clear the merge conflict. Re-run fledge spec tooling to register CHG-0044 in the sequence.
|
Resolved the merge conflict with What conflicted: Resolution:
Follow-up for the maintainer: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e57acc64c9
ℹ️ 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".
specsync check --strict failed in the Trust gate: the ledger claimed CHG-0043 while CHG-0044 was highest, and the renumber invalidated the definition approval digest. Bump the ledger to 44 and record a fresh definition approval computed by SpecSync 5.2.0.
|
Trust gate fixed — all 5 checks now pass. The
Fix (24e137c): bumped the ledger to sequence 44 / CHG-0044 and recorded a fresh SpecSync-generated definition approval for CHG-0044 (actor |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3025dd1296
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb64be0257
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0844f24405
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb743cde05
ℹ️ 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".
| if !hasSecret && !usesPassphraseMode { | ||
| try removeInvalidKeyFileWithoutEnvelope() |
There was a problem hiding this comment.
When APS_SECRET_USE_PASSPHRASE=1 is set on a TTY but the prompt returns nil or an empty passphrase, recipientKey falls back to key-file mode; this guard has already skipped stale-key cleanup, so a partial regular secret.key with no secret.enc still fails during createKeyFile() instead of being recovered. In that fallback path the operation is no longer a passphrase-mode write, so it should run the same no-envelope key recovery as normal key-file SET.
Useful? React with 👍 / 👎.
| "requirement_ids": [ | ||
| "REQ-aps-cli-020" | ||
| ] |
There was a problem hiding this comment.
Fresh evidence relative to the earlier missing-verification thread: this accepted verification.json now exists, but its requirement_ids only records REQ-aps-cli-020 while the same change adds REQ-aps-cli-026 and maps tests to it in testing.md. That leaves the stale-key recovery contract outside the accepted SpecSync evidence, so the gate can appear green without proving the new requirement that this PR ships.
AGENTS.md reference: AGENTS.md:L125-L126
Useful? React with 👍 / 👎.
| The encrypted-file SecretStore SHALL serialize fresh `set` key recovery or | ||
| creation, sealing, atomic envelope persistence, and read-back verification | ||
| through `secret.store.lock`. If no `secret.enc` exists, an invalid stale | ||
| `secret.key` SHALL be removed before creating a replacement. Direct missing or | ||
| invalid key access SHALL use `secret.key.lock`, while valid existing-key reads | ||
| SHALL not require that lock. Passphrase-mode writes SHALL ignore stale | ||
| `secret.key` paths. Existing-envelope SET SHALL preserve persistence failures | ||
| from unreadable or missing envelopes while translating invalid-key failures to | ||
| `secretUnlockFailed`. |
There was a problem hiding this comment.
Fresh evidence relative to the earlier SecretStore-spec thread: REQ-020 now documents the new locking behavior, but the original aps-cli requirements for ciphertext-at-rest round trips, no Security.framework/Keychain dependency, wrong-passphrase get, and corrupt-envelope errors no longer appear in the canonical requirements. This lets future SpecSync checks drop those existing SecretStore guarantees even though this patch only adds serialization and recovery behavior, so keep the original encrypted-store contract and add the new locking clauses instead of replacing it.
AGENTS.md reference: AGENTS.md:L136-L136
Useful? React with 👍 / 👎.
| 4. `watchBlocking` emits the current value first, then subsequent distinct values. | ||
| 5. A supplied polling deadline bounds the wait even when the configured interval is larger. | ||
| 5. Dependencies are real services, not fake stubs used only for wiring demos. |
There was a problem hiding this comment.
This hunk drops the active state-store invariant that a supplied polling deadline bounds watchBlocking even when the polling interval is larger, while REQ-state-store-004 and the current watch implementation still rely on timeout-bounded behavior. Removing it from the canonical spec lets future SpecSync checks miss regressions in watch timeout behavior even though this SecretStore change did not modify that workflow.
AGENTS.md reference: AGENTS.md:L136-L136
Useful? React with 👍 / 👎.
| if case .persistenceFailed = error { | ||
| throw APSError.secretUnlockFailed |
There was a problem hiding this comment.
Fresh evidence relative to the prior persistence-error thread: on a fresh key-file SET where secret.key does not exist but cannot be created, for example because the state root is not writable after the store lock is acquired, loadOrCreateKeyFileUnlocked() throws persistenceFailed and this catch remaps it to secretUnlockFailed. That sends the wrong exit code and passphrase/key hint for an actual disk write failure; only the existing-invalid-key case should be translated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae70e3c86e
ℹ️ 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".
| 5. `keys` and `--help` do not mutate application state. | ||
| 6. State root: subcommand `--state-dir` > root `--state-dir` > `APS_HOME` > `~/.aps`. | ||
| 7. EncryptedFile SET never clobbers ciphertext without a successful unlock when a file exists. | ||
| 8. `watch` termination is observable in both channels: a terminal |
There was a problem hiding this comment.
This spec rewrite drops the active CLI invariant that SIGINT/SIGTERM handling runs on a background dispatch queue and wakes polling promptly, while the implementation still exposes installWatchSignalHandlers and this same section still promises sigint/sigterm terminal events. Future SpecSync checks can now miss regressions in that behavior even though this SecretStore change did not alter the watch workflow; keep the watch signal guarantee in the canonical CLI contract.
AGENTS.md reference: AGENTS.md:L125-L126
Useful? React with 👍 / 👎.
| let created = FileManager.default.createFile( | ||
| atPath: keyFileURL.path, | ||
| contents: key.rawRepresentation.base64EncodedData(), | ||
| attributes: [.posixPermissions: 0o600] |
There was a problem hiding this comment.
When secret.enc already exists but secret.key is invalid or temporarily unreadable, the existing-envelope get/set path can reach this creator and mutate the key path before the decrypt fails. Fresh evidence relative to the prior corrupt-key comments: on Linux Foundation, FileManager.createFile truncates an existing regular file instead of returning false, so an unlock attempt can destroy key material even though recovery is only intended for the no-envelope path; fail with secretUnlockFailed without calling createFile when an envelope is present.
Useful? React with 👍 / 👎.
| "REQ-aps-cli-011", | ||
| "REQ-aps-cli-019", | ||
| "REQ-aps-cli-020", | ||
| "REQ-state-store-016" |
There was a problem hiding this comment.
CHG-0038's own testing artifact maps reset and schema-lock coverage to REQ-aps-cli-023, REQ-aps-cli-024, REQ-state-store-017, and REQ-state-store-018, but the accepted verification records only these four IDs. That leaves the reset and schema-lock contracts out of the accepted SpecSync evidence even though this commit marks the change accepted; include the omitted requirement IDs in the verification before relying on this gate.
AGENTS.md reference: AGENTS.md:L125-L126
Useful? React with 👍 / 👎.
1 participant
Summary
secret.keyatomically with restrictive0600permissions.Root cause
Fresh writers could generate and replace
secret.keyconcurrently, and envelope writers could replacesecret.encbetween another writer's write and read-back verification. This caused spurious unlock/persistence failures and could orphan the envelope.Test Plan
fledge spec check --strict --jsonfledge lanes run verifyCloses #98