Conversation
Contributor
Summary by CodeRabbit
WalkthroughThis PR adds a machine-secret-backed SecureStore fallback path, shared envelope/KDF helpers, a machine-secret path helper, and validation/integration tests. It also updates two Agent API tests with a masked-key helper and stricter rule-view existence checks. ChangesAgent API test guards
SecureStore machine-secret fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
LLxprt PR Review – PR #2178Issue AlignmentIssue #1986 is fully addressed. The PR replaces the non-secret KDF input (
The v:2 KDF input ( Side Effects
Code QualityStrengths:
Minor observations (not blocking):
Tests and CoverageCoverage impact: Increase
Tests are behavioral, not mock theater. Keyring/filesystem are injected; the actual provider logic is exercised. No tests flagged as inadequate. VerdictReady. The PR fully addresses issue #1986 with high-quality, well-tested implementation. The machine secret is a proper root of trust, v:2/v:1 envelope versioning is correct, backward compatibility with v:1 is preserved, and the downgrade prevention is implemented correctly with appropriate test coverage. No blockers identified. |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/storage/src/secure-store/secure-store.ts (2)
768-821: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe v:2 downgrade guard is still bypassable under concurrent writers.
The version probe happens before the temp file is renamed into place. Another process can create or upgrade
finalPathto v:2 in that window, and this v:1 write will still replace it onfs.rename(). That breaks the “never silently downgrade an existing v:2 file” guarantee for concurrentset()calls. You need a per-key lock or another atomic replace strategy that revalidates at commit time.🤖 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 `@packages/storage/src/secure-store/secure-store.ts` around lines 768 - 821, The v:2 downgrade check in secure-store.ts is still racy because readExistingEnvelopeVersion() runs before the final rename, so a concurrent set() can create or upgrade the target to v:2 and still get overwritten by a v:1 write. Fix this in set() by adding a per-key lock or another atomic commit flow around getFallbackFilePath(), readExistingEnvelopeVersion(), and renameWithRetry() so the version is revalidated immediately before replace. Keep the existing SecureStoreError guard, but move it into the commit path so v:1 never overwrites a v:2 envelope under concurrent writers.
903-923: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard non-object JSON before reading
v.
JSON.parse('null')succeeds, andenv.vthen throws a rawTypeErrorhere instead of the expectedSecureStoreError. Malformed fallback files should stay on the controlled corruption path.Suggested fix
try { envelope = JSON.parse(content); } catch { throw new SecureStoreError( 'Fallback file is corrupt or uses an unrecognized format', 'CORRUPT', 'Re-save the key or re-authenticate', ); } + if (typeof envelope !== 'object' || envelope === null) { + throw new SecureStoreError( + 'Fallback file envelope is malformed', + 'CORRUPT', + 'Re-save the key or re-authenticate', + ); + } + const env = envelope as Record<string, unknown>;🤖 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 `@packages/storage/src/secure-store/secure-store.ts` around lines 903 - 923, Guard the parsed fallback content in secure-store.ts before accessing env.v in the envelope validation block. In the JSON.parse/content handling inside the try/catch near the envelope and env checks, verify the parsed value is a non-null object (not null/array/primitive) and throw the existing SecureStoreError corruption path if it is not. This keeps malformed fallback files from causing a raw TypeError and ensures the SecureStoreError flow remains controlled.
🤖 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 `@packages/storage/src/secure-store/machine-secret.test.ts`:
- Around line 245-261: The failure-path test for getMachineSecret is flaky
because it relies on chmod-based directory permissions, which behave
inconsistently across platforms and privileged CI environments. Update the test
to use the same blocker-file approach as the existing blocker-based case in
machine-secret.test.ts, so the write/persistence path fails due to a filesystem
conflict rather than permission semantics. Keep the test focused on the
getMachineSecret flow and keyringLoader throw path, but make the setup
deterministic by reusing the blocker pattern instead of chmod(tempDir, 0o500).
In `@packages/storage/src/secure-store/machine-secret.ts`:
- Around line 257-267: The secure secret handling only fixes the file mode in
ensureSecurePermissions, but the containing directory can still be writable or
overly permissive. Update the machine-secret flow to also verify and repair the
parent directory of the secret before reading or accepting it, by checking its
permissions and hardening it to a private mode if needed. Use the existing
ensureSecurePermissions helper and the directory setup path around the
machine_secret creation/read logic to ensure both the file and its parent
directory are protected.
- Around line 146-160: The machine secret flow in machine-secret.ts is treating
invalid or unreadable durable data as if it were missing, which causes an
unnecessary secret rotation. Update the read path around loadKeyring,
readFromKeyring, and readFromFile so callers can distinguish “not found” from
“present but unusable” instead of collapsing both to null. Then only call
generatePersistAndReread when both durable sources are truly absent, and
propagate invalid/permission-repair failures instead of generating a new secret.
- Around line 93-103: The cache key in sourceKeyOf() is too coarse because it
only distinguishes injected versus default loaders, so different injected
keyring backends can collide and reuse the same secret. Update sourceKeyOf() to
include a stable identifier for the injected durable source in addition to
filePath, and make the key unique per injected backend while preserving
DEFAULT_SOURCE_KEY for the default machine-secret path. Ensure the change is
localized around sourceKeyOf() and any caller-provided keyringLoader metadata
used to distinguish sources.
In `@packages/storage/src/secure-store/secure-store.ts`:
- Around line 841-845: The secure store read path in readEnvelopeContent() is
treating every fs.readFile failure as “no existing envelope”; update the catch
around fs.readFile(filePath, 'utf8') to return null only for ENOENT and rethrow
or propagate all other errors so writeFallbackFile() does not overwrite an
existing v:2 file on EACCES, EPERM, or transient I/O failures.
---
Outside diff comments:
In `@packages/storage/src/secure-store/secure-store.ts`:
- Around line 768-821: The v:2 downgrade check in secure-store.ts is still racy
because readExistingEnvelopeVersion() runs before the final rename, so a
concurrent set() can create or upgrade the target to v:2 and still get
overwritten by a v:1 write. Fix this in set() by adding a per-key lock or
another atomic commit flow around getFallbackFilePath(),
readExistingEnvelopeVersion(), and renameWithRetry() so the version is
revalidated immediately before replace. Keep the existing SecureStoreError
guard, but move it into the commit path so v:1 never overwrites a v:2 envelope
under concurrent writers.
- Around line 903-923: Guard the parsed fallback content in secure-store.ts
before accessing env.v in the envelope validation block. In the
JSON.parse/content handling inside the try/catch near the envelope and env
checks, verify the parsed value is a non-null object (not null/array/primitive)
and throw the existing SecureStoreError corruption path if it is not. This keeps
malformed fallback files from causing a raw TypeError and ensures the
SecureStoreError flow remains controlled.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef0c0d0b-ef14-438f-8258-0bb25b3adf5f
📒 Files selected for processing (12)
packages/agents/src/api/__tests__/capabilityGaps.integration.spec.tspackages/agents/src/api/__tests__/policyControl.behavior.test.tspackages/storage/src/config/storage.test.tspackages/storage/src/config/storage.tspackages/storage/src/secure-store/envelope.test.tspackages/storage/src/secure-store/envelope.tspackages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/machine-secret.tspackages/storage/src/secure-store/secure-store-integration.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store.fallback2.test.tspackages/storage/src/secure-store/secure-store.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
- GitHub Check: Lint (Javascript)
- GitHub Check: CodeQL
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/storage/src/secure-store/envelope.test.tspackages/storage/src/secure-store/secure-store.fallback2.test.tspackages/agents/src/api/__tests__/policyControl.behavior.test.tspackages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope.tspackages/agents/src/api/__tests__/capabilityGaps.integration.spec.tspackages/storage/src/config/storage.tspackages/storage/src/config/storage.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store-integration.test.tspackages/storage/src/secure-store/machine-secret.tspackages/storage/src/secure-store/secure-store.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/storage/src/secure-store/envelope.test.tspackages/storage/src/secure-store/secure-store.fallback2.test.tspackages/agents/src/api/__tests__/policyControl.behavior.test.tspackages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope.tspackages/agents/src/api/__tests__/capabilityGaps.integration.spec.tspackages/storage/src/config/storage.tspackages/storage/src/config/storage.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store-integration.test.tspackages/storage/src/secure-store/machine-secret.tspackages/storage/src/secure-store/secure-store.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/storage/src/secure-store/envelope.test.tspackages/storage/src/secure-store/secure-store.fallback2.test.tspackages/agents/src/api/__tests__/policyControl.behavior.test.tspackages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope.tspackages/agents/src/api/__tests__/capabilityGaps.integration.spec.tspackages/storage/src/config/storage.tspackages/storage/src/config/storage.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store-integration.test.tspackages/storage/src/secure-store/machine-secret.tspackages/storage/src/secure-store/secure-store.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/storage/src/secure-store/envelope.test.tspackages/storage/src/secure-store/secure-store.fallback2.test.tspackages/agents/src/api/__tests__/policyControl.behavior.test.tspackages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope.tspackages/agents/src/api/__tests__/capabilityGaps.integration.spec.tspackages/storage/src/config/storage.tspackages/storage/src/config/storage.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store-integration.test.tspackages/storage/src/secure-store/machine-secret.tspackages/storage/src/secure-store/secure-store.ts
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/storage/src/secure-store/envelope.test.tspackages/storage/src/secure-store/secure-store.fallback2.test.tspackages/agents/src/api/__tests__/policyControl.behavior.test.tspackages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope.tspackages/agents/src/api/__tests__/capabilityGaps.integration.spec.tspackages/storage/src/config/storage.tspackages/storage/src/config/storage.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store-integration.test.tspackages/storage/src/secure-store/machine-secret.tspackages/storage/src/secure-store/secure-store.ts
📚 Learning: 2026-03-26T00:49:43.150Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.ts:309-324
Timestamp: 2026-03-26T00:49:43.150Z
Learning: In this repository’s Jest (or Jest-like) test files, it is acceptable to use `expect(promiseReturningFunction).resolves.not.toThrow()` when the function returns `Promise<void>`. Do not flag this as an incorrect or suboptimal matcher; for `Promise<void>` it is functionally equivalent to using `resolves.toBeUndefined()` to assert successful resolution.
Applied to files:
packages/agents/src/api/__tests__/policyControl.behavior.test.tspackages/agents/src/api/__tests__/capabilityGaps.integration.spec.ts
📚 Learning: 2026-06-19T17:16:55.805Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/__tests__/hooks.spec.ts:86-119
Timestamp: 2026-06-19T17:16:55.805Z
Learning: In `vybestack/llxprt-code`’s agents package under `packages/agents/src/api/`, the chat-turn flow triggers hooks by directly calling `hookSystem.fireBeforeModelEvent` / `hookSystem.fireAfterModelEvent`. It does not emit bus-mediated `HOOK_EXECUTION_REQUEST` / `HOOK_EXECUTION_RESPONSE` messages. Therefore, `agent.hooks.onHookExecution` (which only observes bus-mediated hook messages) will not surface chat-turn hook executions. During code review, don’t expect chat-turn hooks to appear via `onHookExecution`; if you need them observable, that requires a separate production change to add bus publishing for chat-turn hook executions.
Applied to files:
packages/agents/src/api/__tests__/policyControl.behavior.test.tspackages/agents/src/api/__tests__/capabilityGaps.integration.spec.ts
📚 Learning: 2026-06-23T21:59:35.738Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/__tests__/cli-turn-parity.spec.ts:132-146
Timestamp: 2026-06-23T21:59:35.738Z
Learning: In `packages/agents/src/api/__tests__/`, some plan-phase snapshot spec files may be intentionally hash-frozen (their sha256 hashes are recorded in `project-plans/issue1594remediate/.completed/<phase>-frozen-hashes.txt` and enforced by a guard test). During code review, do not recommend edits to these frozen snapshot files; if you see boundary/correctness concerns, route the issue to the authoritative enforcement test `boundary.adequacy.test.ts` instead.
Applied to files:
packages/agents/src/api/__tests__/policyControl.behavior.test.tspackages/agents/src/api/__tests__/capabilityGaps.integration.spec.ts
🪛 ast-grep (0.44.0)
packages/storage/src/secure-store/machine-secret.test.ts
[warning] 103-103: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tempFilePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 147-147: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(tempFilePath, fileSecret.toString('base64'))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 165-165: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(tempFilePath, fileSecret.toString('base64'))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 232-232: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(blockerFile, 'x')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 273-273: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tempFilePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 366-366: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tempFilePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 408-408: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(pathA, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 409-409: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(pathB, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 465-467: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(tempFilePath, preExisting.toString('base64'), {
mode: 0o644,
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/storage/src/secure-store/secure-store.fallback-v2.test.ts
[warning] 78-78: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(file, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 197-197: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(file, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 234-234: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 262-262: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 289-289: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 306-306: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 312-312: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 347-347: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(file, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 375-375: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(path.join(tempDir, 'bad-version.enc'), badEnvelope)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/storage/src/secure-store/secure-store-integration.test.ts
[warning] 644-644: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/storage/src/secure-store/machine-secret.ts
[warning] 227-227: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/storage/src/secure-store/secure-store.ts
[warning] 842-842: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (5)
packages/agents/src/api/__tests__/capabilityGaps.integration.spec.ts (1)
37-43: LGTM!Also applies to: 178-178
packages/agents/src/api/__tests__/policyControl.behavior.test.ts (1)
155-166: LGTM!packages/storage/src/secure-store/secure-store-integration.test.ts (1)
30-30: LGTM!Also applies to: 612-653
packages/storage/src/secure-store/secure-store.fallback-v2.test.ts (1)
1-437: LGTM!packages/storage/src/secure-store/secure-store.fallback2.test.ts (1)
110-110: LGTM!Also applies to: 437-437
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/storage/src/secure-store/machine-secret.ts (1)
204-227: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not cache a secret that was not confirmed durable.
Lines 217 and 227 return the freshly generated
secretwhen the post-persist reread ismissingorunusable. That can cache and use a root secret that is not actually recoverable from the durable source, producing v2 fallback files that may not decrypt after restart or after another writer wins.Proposed fix
// Re-read the keyring winner in case a concurrent writer persisted a // different secret first. const winner = await readFromKeyring(keyring); - return winner.status === 'found' ? winner.secret : secret; + return winner.status === 'found' ? winner.secret : null; } } @@ } // Re-read the file winner. const winner = await readFromFile(filePath); - return winner.status === 'found' ? winner.secret : secret; + return winner.status === 'found' ? winner.secret : null; }🤖 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 `@packages/storage/src/secure-store/machine-secret.ts` around lines 204 - 227, The generatePersistAndReread flow in machine-secret.ts should not return the freshly generated secret unless it has been confirmed durable by rereading it from the keyring or file. Update the logic in generatePersistAndReread, persistToKeyring, and persistToFile call handling so that a post-persist reread returning missing or unusable yields null (or retries/propagates failure) instead of caching the local secret; only return the reread winner.secret when readFromKeyring/readFromFile reports found.packages/storage/src/secure-store/secure-store.ts (1)
773-777: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClose the remaining v2→v1 downgrade gaps.
The downgrade guard checks only the sanitized final path, but
get()can still read a legacy path for the same key. A degraded write can therefore shadow an existing legacy v2 file with a new v1 file. Also,readExistingEnvelopeVersion()returnsnullfor parsed-but-malformed{ v: 2, ... }, allowing a v1 overwrite even though the file declares a v2 envelope.Proposed fix
if (!useV2) { const finalPathForCheck = this.getFallbackFilePath(key); - const existingVersion = - await this.readExistingEnvelopeVersion(finalPathForCheck); - if (existingVersion === 2) { + const legacyPathForCheck = this.getLegacyFallbackFilePath(key); + const pathsToCheck = [finalPathForCheck]; + if (legacyPathForCheck !== finalPathForCheck) { + pathsToCheck.push(legacyPathForCheck); + } + const existingVersions = await Promise.all( + pathsToCheck.map((candidatePath) => + this.readExistingEnvelopeVersion(candidatePath), + ), + ); + if (existingVersions.includes(2)) { throw new SecureStoreError( 'Refusing to overwrite v:2 fallback file with a weaker v:1 envelope while the machine secret is unavailable', 'UNAVAILABLE', @@ - if (!isValidEnvelope(parsed)) { - return null; - } - return parsed.v; + const version = (parsed as { v?: unknown }).v; + return typeof version === 'number' && ENVELOPE_VERSIONS.has(version) + ? version + : null; }Also applies to: 856-859
🤖 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 `@packages/storage/src/secure-store/secure-store.ts` around lines 773 - 777, The downgrade protection in secure-store still has gaps because get() can resolve a legacy path and readExistingEnvelopeVersion() treats malformed v2 envelopes as null. Update the write path in SecureStore’s guarded fallback flow so it checks both the sanitized fallback path and the legacy path used by get() before allowing a v1 write, and make readExistingEnvelopeVersion() preserve a detected v2 envelope even when parsing is malformed instead of returning null. Keep the fix centered around get(), getFallbackFilePath(), and readExistingEnvelopeVersion() so v2 files cannot be shadowed by a degraded v1 write.
🤖 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.
Outside diff comments:
In `@packages/storage/src/secure-store/machine-secret.ts`:
- Around line 204-227: The generatePersistAndReread flow in machine-secret.ts
should not return the freshly generated secret unless it has been confirmed
durable by rereading it from the keyring or file. Update the logic in
generatePersistAndReread, persistToKeyring, and persistToFile call handling so
that a post-persist reread returning missing or unusable yields null (or
retries/propagates failure) instead of caching the local secret; only return the
reread winner.secret when readFromKeyring/readFromFile reports found.
In `@packages/storage/src/secure-store/secure-store.ts`:
- Around line 773-777: The downgrade protection in secure-store still has gaps
because get() can resolve a legacy path and readExistingEnvelopeVersion() treats
malformed v2 envelopes as null. Update the write path in SecureStore’s guarded
fallback flow so it checks both the sanitized fallback path and the legacy path
used by get() before allowing a v1 write, and make readExistingEnvelopeVersion()
preserve a detected v2 envelope even when parsing is malformed instead of
returning null. Keep the fix centered around get(), getFallbackFilePath(), and
readExistingEnvelopeVersion() so v2 files cannot be shadowed by a degraded v1
write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 959ff5cd-6a66-4497-95dd-b0b6a4945e41
📒 Files selected for processing (4)
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/machine-secret.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: Lint (Javascript)
- GitHub Check: CodeQL
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store.tspackages/storage/src/secure-store/machine-secret.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store.tspackages/storage/src/secure-store/machine-secret.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store.tspackages/storage/src/secure-store/machine-secret.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store.tspackages/storage/src/secure-store/machine-secret.ts
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/secure-store.fallback-v2.test.tspackages/storage/src/secure-store/secure-store.tspackages/storage/src/secure-store/machine-secret.ts
🪛 ast-grep (0.44.0)
packages/storage/src/secure-store/machine-secret.test.ts
[warning] 246-246: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(blockerFile, 'x')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 523-525: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(tempFilePath, preExisting.toString('base64'), {
mode: 0o600,
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 544-544: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(tempFilePath, 'not-a-valid-32-byte-secret')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 554-554: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tempFilePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/storage/src/secure-store/secure-store.fallback-v2.test.ts
[warning] 289-289: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 319-319: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/storage/src/secure-store/machine-secret.ts
[warning] 261-261: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (4)
packages/storage/src/secure-store/machine-secret.ts (1)
72-75: LGTM!Also applies to: 91-128, 165-194, 240-281, 313-330, 347-381
packages/storage/src/secure-store/machine-secret.test.ts (1)
245-257: LGTM!Also applies to: 439-472, 518-557
packages/storage/src/secure-store/secure-store.ts (1)
92-93: LGTM!Also applies to: 252-287, 842-849, 862-973
packages/storage/src/secure-store/secure-store.fallback-v2.test.ts (1)
16-16: LGTM!Also applies to: 278-322
Collaborator
Author
Summary verdict: the fix is genuine ✅PR #2178 genuinely resolves the root cause of #1986. The original
"Hide vs. fix" cheat check ✅ (no rule overrides / suppression)Specifically scanned for ways the problem could be hidden rather than fixed:
Conclusion: this is a real fix, not LLM cheating / suppression. Important findings to consider (not blockers for #1986, but worth tracking)These are from OCR (15 comments total) plus my own static analysis. They do not reopen #1986; they are hardening/robustness items for this module or follow-ups. Security / "silent defeat" paths (defense-in-depth)
Correctness / availability
TOCTOU
Test-quality gaps (valid, lower priority)
Maintainability (minor)
Related (out of scope for this PR, but the same vulnerability class exists elsewhere)While verifying "are there others", I found three live, sibling implementations that still derive an AES-256-GCM key from non-secret data with a hard-coded password — the same weakness #1986 describes, but not routed through
( Generated by LLxprt Code (codeanalyzer subagent + open-code-review |
This was referenced Jun 25, 2026
Stanley00
pushed a commit
to stanley-fork/llxprt-code
that referenced
this pull request
Jun 26, 2026
…et KDF (Fixes vybestack#2187) Follow-up to PR vybestack#2178 (vybestack#1986). PR vybestack#2178 hardened SecureStore's fallback encryption with a machine-secret root of trust and a versioned AES-256-GCM envelope (v:1 legacy host/user KDF, v:2 machine-secret KDF), but three sibling encrypted file stores bypassed SecureStore and still derived keys from hard-coded constants plus non-secret host/user metadata. This migrates the live and reachable siblings onto the same root of trust via a new shared codec, preserves backward-compatible reads of existing files, and makes rotation/downgrade failures fail closed. Phase 1 - shared codec (packages/storage): - Add secure-store/envelope-codec.ts exposing encryptEnvelopeString, decryptEnvelopeString, and readEnvelopeVersion. It is a thin wrapper over the existing envelope.ts primitives (scrypt + AES-256-GCM, layout [salt][iv][authTag][ciphertext]) and introduces no new crypto parameters. Centralizes v:1/v:2 selection, the anti-downgrade guard, and fail-closed decrypt (EnvelopeCodecError). - Export the codec from the barrel and add a deep sub-path export (./storage/envelope-codec.js) so core and mcp can import it without crossing the import-boundary guard. Phase 2 - ToolKeyStorage live .key fallback (packages/core): - Route saveToFile/getFromFile through the codec: new writes are v:2 envelopes when the machine secret is available; legacy iv:authTag:ciphertext files still decrypt; a v:2 file with a missing/rotated secret fails closed instead of being misread as "no key configured"; unrecognized content fails closed. Phase 3 - FileTokenStorage MCP fallback (packages/mcp): - Route loadTokens/saveTokens through the codec with the same v:2/legacy/ anti-downgrade/fail-closed semantics, mapping codec errors to the existing "Token file corrupted" behavior. Phase 4 - deprecate the dead store: - Mark FileTokenStore (file-token-store.ts) @deprecated; it has no production instantiation and is retained only for public-API/backward-compatible reads. Tests: - New behavioral tests (real temp-dir files, injected machine-secret loaders) prove v:2 writes use the secret root of trust, legacy data stays readable, and downgrade/rotation failures fail closed across all touched stores. - Update the three vitest storageExportToSource alias maps for the new sub-path export.
This was referenced Jul 12, 2026
This was referenced Jul 25, 2026
This was referenced Aug 1, 2026
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
1 participant
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.
TLDR
Hardened SecureStore fallback encryption by adding a shared high-entropy machine secret and writing new fallback envelopes as v:2 when that secret is available. Legacy v:1 fallback files remain readable, and SecureStore refuses to silently downgrade an existing v:2 fallback file to v:1 when the machine secret is unavailable.
Dive Deeper
This PR adds a machine-secret root of trust for SecureStore file fallback encryption. The new provider prefers OS keyring storage and falls back to a restrictive file at ~/.llxprt/machine_secret with atomic writes, 0700 parent directories, 0600 file permissions, source-scoped caching, and concurrent first-call deduplication.
The encrypted fallback envelope logic is now version-aware:
The change also adds focused coverage for envelope validation, machine-secret persistence/cache/permissions, v:1 compatibility, v:2 read/write behavior, wrong or missing machine secrets, and downgrade prevention.
Two agents test files were adjusted only to satisfy full-repo lint rules that were exposed while running the prescribed verification; their behavior is unchanged.
Reviewer Test Plan
Recommended validation:
Local verification performed:
Testing Matrix
Linked issues / bugs
Fixes #1986