fix(cli): harden context graph ids and keystores#296
fix(cli): harden context graph ids and keystores#296branarakic wants to merge 5 commits intomainfrom
Conversation
Made-with: Cursor
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (!/^[\w:/.@\-]+$/.test(id)) return false; | ||
| if (id.includes('\\')) return false; | ||
| try { | ||
| const decoded = decodeURIComponent(id); |
There was a problem hiding this comment.
🔴 Bug: This traversal guard only protects the handful of routes that call isValidContextGraphId(). The /:id/* context-graph endpoints still decodeURIComponent() the path segment and pass it straight to agent methods without validation, and most other request-body routes still go through @origintrail-official/dkg-core's validateContextGraphId(), which still accepts ../.... That leaves an easy bypass for the fix. Move the ./.. segment check into the shared validator (or validate every decoded contextGraphId) and add a regression test against one of the path-param routes.
Made-with: Cursor
|
Addressed the traversal-bypass review in 4547d89:
Validation:
|
| assertSafeKdfParams(kdfparams); | ||
| const salt = Buffer.from(kdfparams.salt, 'hex'); | ||
| const key = deriveKey(passphrase, salt); | ||
| const key = deriveKey(passphrase, salt, kdfparams); |
There was a problem hiding this comment.
🔴 Bug: decryptKeystore now feeds attacker-controlled kdfparams directly into scryptSync, but the new validation only checks minimums. A tampered keystore can still set very large n/r/p values and either block the process in a synchronous KDF or bubble up a raw crypto RangeError. Please cap these fields to the supported envelope here (or require the exact params this writer emits) before deriving the key.
Made-with: Cursor
|
Addressed the KDF upper-bound review in b4f5ff6:
Validation:
|
| const addParticipantMatch = path.match(/^\/api\/context-graph\/([^/]+)\/add-participant$/); | ||
| if (req.method === "POST" && addParticipantMatch) { | ||
| const contextGraphId = decodeURIComponent(addParticipantMatch[1]); | ||
| if (!validateRequiredContextGraphId(contextGraphId, res)) return; |
There was a problem hiding this comment.
🔴 Bug: This guard never sees malformed percent-encoding, because decodeURIComponent(addParticipantMatch[1]) can throw before validation runs. The daemon's top-level error handler maps that URIError to 500, so these new path-param routes are still crashable with a bad % sequence instead of returning 400. Use the existing safeDecodeURIComponent(...) helper (or wrap the decode in try/catch) before validateRequiredContextGraphId, and apply the same fix to the other {id} handlers added in this PR.
| if (!Number.isInteger(kdfparams.r) || kdfparams.r < MIN_SCRYPT_R) { | ||
| throw new Error('KDF parameters below minimum: scrypt r too low'); | ||
| } | ||
| if (kdfparams.r > MAX_SCRYPT_R) { |
There was a problem hiding this comment.
🔴 Bug: Rejecting any r > 8 / p > 1 makes decryptKeystore incompatible with keystores that are stronger than the one we currently emit. A valid scrypt keystore from another tool, or a future version that raises the work factor, will now hard-fail before decryption even though it is safer than today's defaults. Keep the minimum floor checks, but use a resource bound (maxmem / estimated cost) instead of pinning the reader to exactly our current r and p values.
Made-with: Cursor
|
Addressed the latest review in ee87bb3:
Validation:
|
| return Number.isInteger(value) && value > 0 && Number.isInteger(Math.log2(value)); | ||
| } | ||
|
|
||
| function assertSafeKdfParams(kdfparams: EncryptedKeystore['crypto']['kdfparams']): void { |
There was a problem hiding this comment.
🟡 Issue: decryption now rejects n < 2**15, but encryptKeystore() still uses the mutable SCRYPT_N without the same guard. Because _setScryptN() is still exported, tests/scripts can generate keystores that this module will no longer decrypt. Enforce the floor on the write path as well (or reject low values in _setScryptN) so the API stays self-consistent.
There was a problem hiding this comment.
Symmetry already holds because _setScryptN itself enforces the floor (L51): it throws Unsupported scrypt N for keystore encryption for n < MIN_SCRYPT_N, so SCRYPT_N cannot be set below 2^15 from any caller — tests, scripts, or otherwise. The full write-path values map onto the read-path constants:
| param | write path | read-path floor |
|---|---|---|
n |
SCRYPT_N ≥ 2^15 |
MIN_SCRYPT_N = 2^15 |
r |
SCRYPT_R = 8 (const) |
MIN_SCRYPT_R = 8 |
p |
SCRYPT_P = 1 (const) |
MIN_SCRYPT_P = 1 |
| salt | 32 bytes | MIN_SALT_BYTES = 16 |
| dklen | DKLEN = 32 (const) |
exact match required |
Every keystore the writer can emit decrypts under the reader's floor. Closing as not-a-bug; the cross-check is enforced by _setScryptN's own validation, not implicit.
| if (!id || id.length === 0) return { valid: false, reason: 'Context graph ID cannot be empty' }; | ||
| if (id.length > 256) return { valid: false, reason: 'Context graph ID exceeds 256 characters' }; | ||
| if (!/^[\w:/.@\-]+$/.test(id)) return { valid: false, reason: 'Context graph ID contains disallowed characters (allowed: alphanumeric, _, :, /, ., @, -)' }; | ||
| let decoded: string; |
There was a problem hiding this comment.
🟡 Issue: this extra decodeURIComponent() is effectively dead for direct callers of validateContextGraphId() because the whitelist above still rejects % first. That means the new malformed-encoding/traversal branch only runs after a caller has already decoded once, and the added core test passes for the wrong reason. Either allow % through the whitelist and let this block validate it, or drop the second decode and keep percent-decoding checks strictly at the HTTP boundary.
There was a problem hiding this comment.
Resolved by commit c8879702 (latest on this branch) which dropped the decodeURIComponent block entirely. The validator now relies purely on the whitelist (which rejects % so percent-decoding can't matter) plus the literal segment check. Percent-decoding stays at the HTTP boundary in safeDecodeURIComponent, matching the second of the two options you suggested.
Made-with: Cursor
|
Addressed the final self-consistency/percent-decoding feedback in c887970:
Validation:
|
| if (!id || id.length === 0) return { valid: false, reason: 'Context graph ID cannot be empty' }; | ||
| if (id.length > 256) return { valid: false, reason: 'Context graph ID exceeds 256 characters' }; | ||
| if (!/^[\w:/.@\-]+$/.test(id)) return { valid: false, reason: 'Context graph ID contains disallowed characters (allowed: alphanumeric, _, :, /, ., @, -)' }; | ||
| if (id.split('/').some((segment) => segment === '.' || segment === '..')) { |
There was a problem hiding this comment.
🔴 Bug: This only rejects literal . / .. segments. isValidContextGraphId('legit-cg/%2e%2e/other-cg') still returns true, so the new packages/cli/test/http-utils.test.ts case fails and any body/query caller that doesn't go through safeDecodeURIComponent can still bypass the traversal guard by percent-encoding the segment. Decode once before splitting, or explicitly reject %2e/%2e%2e case-insensitively in the shared validator.
There was a problem hiding this comment.
Re-verified empirically against the head of this branch (commit c8879702):
"legit-cg/%2e%2e/other-cg" => rejected: disallowed characters (% not in whitelist)
"legit-cg/%2E%2E/other-cg" => rejected: disallowed characters
"%2e%2e%2f%2e%2e%2fetc%2fpasswd" => rejected: disallowed characters
"legit/../other" => rejected: path traversal segments
The whitelist regex ^[\w:/.@\-]+$ does NOT include %, so any %2e (or %2E, %2f, etc.) is rejected by the character check before the segment-traversal check ever runs. The example in the comment can't bypass the validator. Closing as not-a-bug; happy to add %2e/%2E to the test corpus as a regression anchor if reviewers prefer.
1 participant
Summary
Test plan
pnpm --filter @origintrail-official/dkg buildpnpm exec vitest run packages/cli/test/http-utils.test.ts packages/cli/test/keystore.test.ts packages/cli/test/daemon-keystore-extra.test.tsNote: I attempted the HTTP behavior test filter for CLI-16, but that suite requires the shared Hardhat context file and failed locally before test execution because the context file was absent.
Made with Cursor