Skip to content

fix(cli): harden context graph ids and keystores #296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
branarakic wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix(cli): harden context graph ids and keystores #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac5aa3f
fix(cli): harden context graph ids and keystores
branarakic Apr 25, 2026
4547d89
fix(cli): share context graph traversal validation
branarakic Apr 25, 2026
b4f5ff6
fix(cli): cap keystore KDF parameters
branarakic Apr 25, 2026
ee87bb3
fix(cli): handle malformed context graph paths
branarakic Apr 25, 2026
c887970
fix(cli): keep keystore writer and reader aligned
branarakic Apr 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions packages/cli/src/daemon/http-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,10 +576,8 @@ export function shouldBypassRateLimitForLoopbackTraffic(ip: string, pathname: st
}

export function isValidContextGraphId(id: string): boolean {
if (!id || typeof id !== "string") return false;
if (id.length > 256) return false;
// Allow URNs, DIDs, simple slug-like identifiers, and URIs
return /^[\w:/.@\-]+$/.test(id);
if (typeof id !== "string") return false;
return validateContextGraphId(id).valid;
}

export function shortId(peerId: string): string {
Expand Down
44 changes: 33 additions & 11 deletions packages/cli/src/daemon/routes/context-graph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi
// POST /api/context-graph/{id}/add-participant
const addParticipantMatch = path.match(/^\/api\/context-graph\/([^/]+)\/add-participant$/);
if (req.method === "POST" && addParticipantMatch) {
const contextGraphId = decodeURIComponent(addParticipantMatch[1]);
const contextGraphId = safeDecodeURIComponent(addParticipantMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

const body = await readBody(req);
const { agentAddress } = JSON.parse(body);
if (!agentAddress || typeof agentAddress !== 'string') {
Expand All @@ -687,7 +689,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi
// POST /api/context-graph/{id}/remove-participant
const removeParticipantMatch = path.match(/^\/api\/context-graph\/([^/]+)\/remove-participant$/);
if (req.method === "POST" && removeParticipantMatch) {
const contextGraphId = decodeURIComponent(removeParticipantMatch[1]);
const contextGraphId = safeDecodeURIComponent(removeParticipantMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
const body = await readBody(req);
const { agentAddress } = JSON.parse(body);
if (!agentAddress || typeof agentAddress !== 'string') {
Expand All @@ -705,7 +709,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi
// GET /api/context-graph/{id}/participants
const listParticipantsMatch = path.match(/^\/api\/context-graph\/([^/]+)\/participants$/);
if (req.method === "GET" && listParticipantsMatch) {
const contextGraphId = decodeURIComponent(listParticipantsMatch[1]);
const contextGraphId = safeDecodeURIComponent(listParticipantsMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
try {
const agents = await agent.getContextGraphAllowedAgents(contextGraphId);
return jsonResponse(res, 200, { contextGraphId, allowedAgents: agents });
Expand All @@ -720,7 +726,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi
// Otherwise, forward via P2P to all connected peers so the curator receives it.
const requestJoinMatch = path.match(/^\/api\/context-graph\/([^/]+)\/request-join$/);
if (req.method === "POST" && requestJoinMatch) {
const contextGraphId = decodeURIComponent(requestJoinMatch[1]);
const contextGraphId = safeDecodeURIComponent(requestJoinMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
const body = await readBody(req);
try {
const { agentAddress, signature, timestamp, agentName } = JSON.parse(body);
Expand Down Expand Up @@ -749,7 +757,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi
// GET /api/context-graph/{id}/join-requests — list pending join requests (curator view)
const joinRequestsMatch = path.match(/^\/api\/context-graph\/([^/]+)\/join-requests$/);
if (req.method === "GET" && joinRequestsMatch) {
const contextGraphId = decodeURIComponent(joinRequestsMatch[1]);
const contextGraphId = safeDecodeURIComponent(joinRequestsMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
try {
const requests = await agent.listPendingJoinRequests(contextGraphId);
return jsonResponse(res, 200, { contextGraphId, requests });
Expand All @@ -762,7 +772,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi
// POST /api/context-graph/{id}/approve-join — approve a pending request
const approveJoinMatch = path.match(/^\/api\/context-graph\/([^/]+)\/approve-join$/);
if (req.method === "POST" && approveJoinMatch) {
const contextGraphId = decodeURIComponent(approveJoinMatch[1]);
const contextGraphId = safeDecodeURIComponent(approveJoinMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
const body = await readBody(req);
try {
const { agentAddress } = JSON.parse(body);
Expand All @@ -778,7 +790,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi
// POST /api/context-graph/{id}/reject-join — reject a pending request
const rejectJoinMatch = path.match(/^\/api\/context-graph\/([^/]+)\/reject-join$/);
if (req.method === "POST" && rejectJoinMatch) {
const contextGraphId = decodeURIComponent(rejectJoinMatch[1]);
const contextGraphId = safeDecodeURIComponent(rejectJoinMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
const body = await readBody(req);
try {
const { agentAddress } = JSON.parse(body);
Expand All @@ -794,7 +808,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi
// POST /api/context-graph/{id}/sign-join — sign a join request and forward to curator via P2P
const signJoinMatch = path.match(/^\/api\/context-graph\/([^/]+)\/sign-join$/);
if (req.method === "POST" && signJoinMatch) {
const contextGraphId = decodeURIComponent(signJoinMatch[1]);
const contextGraphId = safeDecodeURIComponent(signJoinMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
try {
const callerAddress = agent.resolveAgentAddress(
extractBearerToken(req.headers.authorization),
Expand Down Expand Up @@ -837,7 +853,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi

const manifestPublishMatch = path.match(/^\/api\/context-graph\/([^/]+)\/manifest\/publish$/);
if (req.method === 'POST' && manifestPublishMatch) {
const contextGraphId = decodeURIComponent(manifestPublishMatch[1]);
const contextGraphId = safeDecodeURIComponent(manifestPublishMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
let body: any = {};
try { body = JSON.parse(await readBody(req, SMALL_BODY_BYTES) || '{}'); }
catch { return jsonResponse(res, 400, { error: 'Invalid JSON body' }); }
Expand Down Expand Up @@ -936,7 +954,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi

const manifestPlanInstallMatch = path.match(/^\/api\/context-graph\/([^/]+)\/manifest\/plan-install$/);
if (req.method === 'POST' && manifestPlanInstallMatch) {
const contextGraphId = decodeURIComponent(manifestPlanInstallMatch[1]);
const contextGraphId = safeDecodeURIComponent(manifestPlanInstallMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
let body: any = {};
try { body = JSON.parse(await readBody(req, SMALL_BODY_BYTES) || '{}'); }
catch { return jsonResponse(res, 400, { error: 'Invalid JSON body' }); }
Expand Down Expand Up @@ -1029,7 +1049,9 @@ export async function handleContextGraphRoutes(ctx: RequestContext): Promise<voi

const manifestInstallMatch = path.match(/^\/api\/context-graph\/([^/]+)\/manifest\/install$/);
if (req.method === 'POST' && manifestInstallMatch) {
const contextGraphId = decodeURIComponent(manifestInstallMatch[1]);
const contextGraphId = safeDecodeURIComponent(manifestInstallMatch[1], res);
if (contextGraphId === null) return;
if (!validateRequiredContextGraphId(contextGraphId, res)) return;
let body: any = {};
try { body = JSON.parse(await readBody(req, SMALL_BODY_BYTES) || '{}'); }
catch { return jsonResponse(res, 400, { error: 'Invalid JSON body' }); }
Expand Down
65 changes: 58 additions & 7 deletions packages/cli/src/keystore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,60 @@ let SCRYPT_N = 2 ** 18;
const SCRYPT_R = 8;
const SCRYPT_P = 1;
const DKLEN = 32;
const MIN_SCRYPT_N = 2 ** 15;
const MIN_SCRYPT_R = 8;
const MIN_SCRYPT_P = 1;
const MAX_SCRYPT_MEMORY_BYTES = 256 * 1024 * 1024;
const MAX_SCRYPT_P = 16;
const MIN_SALT_BYTES = 16;

/** @internal Allow tests to use lighter scrypt params to avoid memory limits */
export function _setScryptN(n: number) { SCRYPT_N = n; }
export function _setScryptN(n: number) {
const estimatedMemoryBytes = 128 * n * SCRYPT_R;
if (!Number.isSafeInteger(n) || !isPowerOfTwo(n) || n < MIN_SCRYPT_N || estimatedMemoryBytes > MAX_SCRYPT_MEMORY_BYTES) {
throw new Error('Unsupported scrypt N for keystore encryption');
}
SCRYPT_N = n;
}

function deriveKey(passphrase: string, salt: Buffer): Buffer {
function isPowerOfTwo(value: number): boolean {
return Number.isInteger(value) && value > 0 && Number.isInteger(Math.log2(value));
}

function assertSafeKdfParams(kdfparams: EncryptedKeystore['crypto']['kdfparams']): void {
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Copy link
Copy Markdown
Contributor Author

@branarakic branarakic May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (!Number.isSafeInteger(kdfparams.n) || !isPowerOfTwo(kdfparams.n) || kdfparams.n < MIN_SCRYPT_N) {
throw new Error('KDF parameters below minimum: scrypt N too low');
}
if (!Number.isSafeInteger(kdfparams.r) || kdfparams.r < MIN_SCRYPT_R) {
throw new Error('KDF parameters below minimum: scrypt r too low');
}
if (!Number.isSafeInteger(kdfparams.p) || kdfparams.p < MIN_SCRYPT_P) {
throw new Error('KDF parameters below minimum: scrypt p too low');
}
const estimatedMemoryBytes = 128 * kdfparams.n * kdfparams.r;
if (!Number.isSafeInteger(estimatedMemoryBytes) || estimatedMemoryBytes > MAX_SCRYPT_MEMORY_BYTES) {
throw new Error('Unsupported keystore KDF parameters: scrypt memory cost too high');
}
if (kdfparams.p > MAX_SCRYPT_P) {
throw new Error('Unsupported keystore KDF parameters: scrypt p too high');
}
if (kdfparams.dklen !== DKLEN) {
throw new Error(`Invalid dklen: dklen must be ${DKLEN}`);
}
if (!/^[0-9a-fA-F]+$/.test(kdfparams.salt) || kdfparams.salt.length % 2 !== 0 || kdfparams.salt.length < MIN_SALT_BYTES * 2) {
throw new Error(`KDF parameters below minimum: salt too short (minimum ${MIN_SALT_BYTES} bytes)`);
}
}

function deriveKey(
passphrase: string,
salt: Buffer,
params: Pick<EncryptedKeystore['crypto']['kdfparams'], 'n' | 'r' | 'p' | 'dklen'>,
): Buffer {
return scryptSync(passphrase, salt, DKLEN, {
N: SCRYPT_N,
r: SCRYPT_R,
p: SCRYPT_P,
N: params.n,
r: params.r,
p: params.p,
maxmem: 256 * 1024 * 1024,
});
}
Expand All @@ -56,7 +101,12 @@ export async function encryptKeystore(
passphrase: string,
): Promise<EncryptedKeystore> {
const salt = randomBytes(32);
const key = deriveKey(passphrase, salt);
const key = deriveKey(passphrase, salt, {
n: SCRYPT_N,
r: SCRYPT_R,
p: SCRYPT_P,
dklen: DKLEN,
});
const iv = randomBytes(12);

const cipher = createCipheriv('aes-256-gcm', key, iv);
Expand Down Expand Up @@ -96,8 +146,9 @@ export async function decryptKeystore(
}

const { kdfparams } = keystore.crypto;
assertSafeKdfParams(kdfparams);
const salt = Buffer.from(kdfparams.salt, 'hex');
const key = deriveKey(passphrase, salt);
const key = deriveKey(passphrase, salt, kdfparams);
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.


const iv = Buffer.from(keystore.crypto.iv, 'hex');
const tag = Buffer.from(keystore.crypto.tag, 'hex');
Expand Down
23 changes: 23 additions & 0 deletions packages/cli/test/daemon-http-behavior-extra.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@
// Should be 4xx (404 "peer not found" or 400 "invalid peerId").
// PROD-BUG candidate: if this returns 500, it's CLI-7 dup #72 #85.
expect(res.status).toBeGreaterThanOrEqual(400);
expect(res.status).toBeLessThan(500);

Check failure on line 485 in packages/cli/test/daemon-http-behavior-extra.test.ts

View workflow job for this annotation

GitHub Actions / Bura: cli

test/daemon-http-behavior-extra.test.ts > CLI-7 — SPARQL endpoint 4xx matrix > rejects /api/query-remote to an invalid peer with 4xx, NOT 500 

AssertionError: expected 500 to be less than 500
 ❯ test/daemon-http-behavior-extra.test.ts:485:24
});

it('returns 409 on duplicate context-graph create', async () => {
Expand Down Expand Up @@ -562,7 +562,7 @@
// RED ON PURPOSE: current code lets the throw bubble to the top-level
// catch and emits 500 with the raw agent error. Spec/issue #158
// mandates 404 for not-found. See CLI-9.
expect(res.status).not.toBe(500);

Check failure on line 565 in packages/cli/test/daemon-http-behavior-extra.test.ts

View workflow job for this annotation

GitHub Actions / Bura: cli

test/daemon-http-behavior-extra.test.ts > CLI-9 — /api/verify & /api/ccl error-code mapping > /api/verify on a non-existent verifiedMemoryId returns 4xx (ideally 404), NOT 500 (PROD-BUG) 

AssertionError: expected 500 not to be 500 // Object.is equality
 ❯ test/daemon-http-behavior-extra.test.ts:565:28
expect(res.status).toBeGreaterThanOrEqual(400);
expect(res.status).toBeLessThan(500);
});
Expand Down Expand Up @@ -634,6 +634,29 @@
expect(body.error).toMatch(/context graph id|invalid/i);
});
}

it('rejects encoded traversal in context-graph path-param routes', async () => {
const d = daemon!;
const encoded = encodeURIComponent('../etc/passwd');
const res = await fetch(urlFor(d, `/api/context-graph/${encoded}/participants`), {
method: 'GET',
headers: authHeaders(d),
});
expect(res.status).toBe(400);
const body = await res.json().catch(() => ({}));
expect(body.error).toMatch(/contextGraphId|context graph id|invalid/i);
});

it('rejects malformed percent-encoding in context-graph path-param routes', async () => {
const d = daemon!;
const res = await fetch(urlFor(d, '/api/context-graph/%E0%A4%A/participants'), {
method: 'GET',
headers: authHeaders(d),
});
expect(res.status).toBe(400);
const body = await res.json().catch(() => ({}));
expect(body.error).toMatch(/percent-encoding|malformed/i);
});
});

// ---------------------------------------------------------------------------
Expand Down
40 changes: 18 additions & 22 deletions packages/cli/test/daemon-keystore-extra.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,31 +80,15 @@ describe('CLI-1 — scrypt KDF parameter floor (PROD-BUG: not enforced)', () =>
_setScryptN(SAFE_N);
const ks = await encryptKeystore(PRIVKEY, PASSPHRASE);

// Re-encrypt using a toy N so we can faithfully construct a forged
// "weak" keystore (same ciphertext+IV+tag would not decrypt if we just
// mutated kdfparams because the derived key would differ).
_setScryptN(WEAK_N);
const weakKs = await encryptKeystore(PRIVKEY, PASSPHRASE);
_setScryptN(SAFE_N);

// Sanity: this really is a weak keystore — the advertised N is the one we
// encrypted with.
expect(weakKs.crypto.kdfparams.n).toBe(WEAK_N);
await expect(() => _setScryptN(WEAK_N)).toThrow(/Unsupported scrypt N/);

// Sanity: the "strong" keystore is rejected if we lie about its N
// (tampered kdfparams → wrong key → GCM auth failure).
// Sanity: the "strong" keystore is rejected if we lie about its N.
// The loader should fail at KDF validation before attempting GCM.
await expect(
decryptKeystore(withKdfParams(ks, { n: WEAK_N }), PASSPHRASE),
).rejects.toThrow(/Decryption failed/);

// PROD-BUG: the below call SHOULD throw "KDF parameters below minimum"
// (or any rejection tied to the cost floor). Instead it returns the
// plaintext — which means any attacker who can write a keystore file
// can force an O(1)-to-brute-force KDF. See issue #11.
//
// This assertion stays RED until `decryptKeystore` enforces N >= 2**15,
// r >= 8, p >= 1. Leaving red-on-purpose.
await expect(decryptKeystore(weakKs, PASSPHRASE)).rejects.toThrow(
).rejects.toThrow(/KDF parameters below minimum|scrypt N too low|weak keystore/i);

await expect(decryptKeystore(withKdfParams(ks, { n: WEAK_N }), PASSPHRASE)).rejects.toThrow(
/KDF parameters below minimum|scrypt cost too low|weak keystore/i,
);
});
Expand Down Expand Up @@ -145,6 +129,18 @@ describe('CLI-1 — scrypt KDF parameter floor (PROD-BUG: not enforced)', () =>
);
});

it('refuses KDF parameters above the supported envelope before calling scrypt', async () => {
_setScryptN(SAFE_N);
const ks = await encryptKeystore(PRIVKEY, PASSPHRASE);

await expect(decryptKeystore(withKdfParams(ks, { n: 2 ** 30 }), PASSPHRASE))
.rejects.toThrow(/memory cost too high|unsupported keystore/i);
await expect(decryptKeystore(withKdfParams(ks, { r: 65 }), PASSPHRASE))
.rejects.toThrow(/memory cost too high|unsupported keystore/i);
await expect(decryptKeystore(withKdfParams(ks, { p: 64 }), PASSPHRASE))
.rejects.toThrow(/scrypt p too high|unsupported keystore/i);
});

it('refuses to decrypt a keystore with a short salt (<16 bytes)', async () => {
_setScryptN(SAFE_N);
const ks = await encryptKeystore(PRIVKEY, PASSPHRASE);
Expand Down
28 changes: 28 additions & 0 deletions packages/cli/test/http-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { describe, expect, it } from 'vitest';
import { isValidContextGraphId } from '../src/daemon/http-utils.js';

describe('isValidContextGraphId', () => {
it('rejects traversal path segments', () => {
for (const id of [
'../etc/passwd',
'../../root',
'./../_private',
'legit-cg/../../other-cg',
'legit-cg/%2e%2e/other-cg',
]) {
expect(isValidContextGraphId(id)).toBe(false);
}
});

it('keeps existing slug, DID, URN, and URL-style identifiers valid', () => {
for (const id of [
'devnet-test',
'did:dkg:context-graph:devnet-test',
'urn:dkg:project:smart-contracts',
'https://example.org/context-graphs/devnet-test',
'agent@example.org/context.graph-v1',
]) {
expect(isValidContextGraphId(id)).toBe(true);
}
});
});
2 changes: 1 addition & 1 deletion packages/cli/test/keystore.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
} from '../src/keystore.js';

beforeAll(() => {
_setScryptN(2 ** 14);
_setScryptN(2 ** 15);
});

const TEST_KEY = 'aabbccdd11223344aabbccdd11223344aabbccdd11223344aabbccdd11223344';
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ export function validateContextGraphId(id: string): { valid: boolean; reason?: s
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 === '..')) {
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Copy link
Copy Markdown
Contributor Author

@branarakic branarakic May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return { valid: false, reason: 'Context graph ID cannot contain path traversal segments' };
}
return { valid: true };
}

Expand Down
5 changes: 5 additions & 0 deletions packages/core/test/constants.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ describe('validateContextGraphId', () => {
expect(validateContextGraphId('a'.repeat(257)).valid).toBe(false);
expect(validateContextGraphId('a'.repeat(256)).valid).toBe(true);
});

it('rejects literal traversal path segments', () => {
expect(validateContextGraphId('../etc/passwd').valid).toBe(false);
expect(validateContextGraphId('legit-cg/../../other-cg').valid).toBe(false);
});
});

describe('validateAssertionName', () => {
Expand Down
Loading