Repository navigation
fix(core): validate secret.key, stop first-run overwrite race - #1
Open
Dextheking1 wants to merge 2 commits into
Open
Dextheking1 wants to merge 2 commits into
Dextheking1 wants to merge 2 commits into
Conversation
Dextheking1
marked this pull request as ready for review
October 23, 2026 03:16
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 72 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideHarden credential key initialization by rejecting malformed key files before key derivation and using exclusive creation so concurrent first runs converge on one stable key instead of overwriting credentials. Sequence diagram for safe credential key initializationsequenceDiagram
participant ProcessA
participant ProcessB
participant KeyFile
par First process
ProcessA->>KeyFile: writeFileSync(flag: wx)
KeyFile-->>ProcessA: key created
and Concurrent process
ProcessB->>KeyFile: writeFileSync(flag: wx)
KeyFile-->>ProcessB: EEXIST
ProcessB->>KeyFile: readFileSync
end
ProcessA->>KeyFile: readFileSync
KeyFile-->>ProcessA: 64 hex characters
KeyFile-->>ProcessB: winner's key
ProcessA->>ProcessA: createHash(sha256)
ProcessB->>ProcessB: createHash(sha256)
Flow diagram for credential key validationflowchart TD
A[Read secret.key] --> B{Exactly 64 hex characters?}
B -->|Yes| C[createHash sha256]
B -->|No| D[Throw Invalid credential key file]
C --> E[Use derived credential key]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/core/src/util/secret.ts" line_range="27" />
<code_context>
+ // "wx" fails instead of overwriting when a concurrent first run won the race.
+ // The loser has to encrypt with the winner's key; replacing it would strand
+ // every credential the winner already stored.
+ writeFileSync(keyFile, randomBytes(32).toString("hex"), { mode: 0o600, flag: "wx" })
+ } catch (error) {
+ if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error
</code_context>
<issue_to_address>
**Credential operations fail on first run**
When a second process reads `secret.key` after `wx` creates it but before the winning write completes, or the writer stops mid-write, `key()` catches `EEXIST` and immediately calls `readKey`; validation rejects the empty or partial file, so credential operations fail. A file left incomplete keeps later operations failing.
Write the key completely to a separate file, then publish it atomically without replacing an existing key.
Also at `packages/core/src/util/secret.ts:28-31`, `packages/core/src/util/secret.ts:33`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if key-file validation or race handling is wrong, credential encryption or decryption can fail or use unintended key material, potentially leaving stored credentials inaccessible. Reverting restores the previous behavior, but credentials already encrypted under a different key cannot be recovered by the revert.
Blocking findings: packages/core/src/util/secret.ts:27
An empty or partial secret.key was hashed as-is, so a truncated write derived the credential key from a known string. Reject any key file that is not 64 hex chars. Create the file with flag wx so two first runs cannot overwrite each other's key and strand stored credentials.
the wx create from the last commit still left a window: secret.key existed before the write finished, so a concurrent first run could read an empty file and fail validation, and a crash mid-write left a broken key file behind for every later run. write the key to a temp file in the same dir, fsync it, then link it into place. link fails with EEXIST instead of replacing, so the real path only ever holds a finished key and the loser of the race reads the winner's key. tests: concurrent first runs in child processes agree on one key, a watcher polling the real path during creation never sees a partial key, a damaged key file still fails closed, and a leftover temp file does not break creation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
No linked issue.
Type of change
What does this PR do?
secret.ts derives the credential encryption key by hashing whatever is in secret.key, with no check on the content.
Two problems:
Fix:
Valid existing key files are 64 hex chars and pass unchanged. Legacy plaintext values still pass through decrypt as before.
How did you verify your code works?
Verified locally with the file logic isolated: encrypt/decrypt roundtrip ok, key file stays 0600 and stable across calls, empty key file rejected, partial key file rejected. packages/core/test/util/secret.test.ts adds coverage for concurrent key creation, atomic key visibility, malformed key rejection, and leftover temporary files.
Screenshots / recordings
Not a UI change.
Checklist
Summary by Sourcery
Harden credential key initialization and validation to fail closed and remain safe during concurrent first runs.
Bug Fixes:
Enhancements:
Tests: