fix(security): validate agent_id character set on registration - #16
Conversation
Block characters dangerous in URLs and HTTP headers: - Newlines: signing string injection via (request-target) - Slashes: path traversal in URL routing - Spaces, null bytes, shell metacharacters, angle brackets (XSS) Allowed: [a-zA-Z0-9._\-:] — covers all real patterns in use (auth.backend, clearauth-gm, decisive_redux, did-web:domain.com) Also fix auto-generated agent_id: was `agent://agent-<uuid>` which contains `://` and would fail its own validation. Changed to `agent-<uuid>`. DID:web shadow agents call storage.createAgent() directly and are unaffected — system-generated IDs are trusted at that layer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Code Review: fix(security): validate agent_id character set on registrationOverall this is a well-scoped, high-value security fix. The allowlist regex approach is the right call, and the PR description is excellent - clear threat model, severity ratings, and explicit callout of what is not protected (DID:web shadow agents). A few observations below. The core fix is correctThe old default ID format (agent://agent-UUID) contained two forward slashes in the scheme prefix, which fail the new regex. Stripping the scheme prefix to agent-UUID is the minimal correct fix. Issues and suggestions1. No maximum length enforcement (Medium) The regex enforces a minimum of 1 character via +, but there is no upper bound. An agent_id used in URL paths, HTTP headers, and database keys should have a reasonable ceiling to prevent abuse and avoid silent truncation at the storage layer. Suggested addition after the regex check (line 42): 2. Dead code path: double getAgent call (Low - pre-existing) Lines 44-48 guard against an already-existing agent and throw if found. Lines 124-130 then call storage.getAgent(agent_id) again and attempt to preserve a previous registration_status. Because the function already threw at line 47, existingAgent at line 125 will always be null, making lines 126-128 unreachable. This is pre-existing and not introduced by this PR. If re-registration is an intended code path, the early throw at line 47 should be removed and the flow consolidated at lines 124-130. 3. DID:web bypass documented but not guarded (Low) The PR description notes that DID:web shadow agents call storage.createAgent() directly and bypass register() entirely. If DID:web agent IDs arrive from an untrusted source (e.g. a DNS response or HTTP header), those IDs would reach storage unvalidated. Consider adding the same regex check inside storage.createAgent() as a defense-in-depth measure. 4. Missing tests for the new validation (Medium) No test files appear in the diff. For a security-focused change, tests covering the regex would prevent regressions. Suggested cases:
5. %2F clarification (Trivial) The PR description lists encoded slash %2F as a blocked character. It is blocked, but because % itself is not in the allowlist - not because %2F is matched as a unit. A small wording update would make this clearer. Summary
The security intent is solid and the fix is correct. Recommend adding a length cap and at minimum a few unit tests before merging. |
Greptile SummaryFixes auto-generated Changes:
Note: DID:web shadow agents that call Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| src/services/agent.service.js | Fixed auto-generated agent_id format to pass validation regex by removing agent:// prefix |
Last reviewed commit: 90818cf
…alidation - Strip agent:// prefix from all test agent_id values (34 occurrences) to match the new validation regex that forbids slashes in agent IDs - Update inbox.service validateEnvelope to accept bare agent IDs (matching ^[a-zA-Z0-9._-:]+$) in addition to agent:// URIs and did:seed: DIDs for backward compatibility This fixes CI test failures caused by the agent_id character validation introduced in the previous commit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CI Fixes PushedTwo issues were found and fixed: 1. Test helper used
|
PR Review: fix(security): validate agent_id character set on registrationGood security fix overall. The core intent — blocking dangerous characters in agent IDs — is sound and the identified vulnerabilities (newline injection in signing strings, path traversal) are real. A few issues worth addressing before merge. Bugs / Security Gaps1. The const validId = (id) =>
id.startsWith('agent://') || // no secondary check; still injectable
id.startsWith('did:seed:') || // same problem
/^[a-zA-Z0-9._\-:]+$/.test(id);Consider validating the suffix after the scheme, or rejecting 2. Same gap applies to the
3.
Missing Test CoverageThe test changes update fixtures to the new bare-ID format, but there are no negative tests for the validation logic itself. The following scenarios should be covered: // Should all return 400
await registerAgent('agent://injected'); // old format now invalid
await registerAgent('bad\nagent'); // newline injection
await registerAgent('../traversal'); // path traversal
await registerAgent('has spaces'); // spaces
await registerAgent('angle<brackets>'); // XSSWithout these, a future refactor could silently break the security property with no signal. Inconsistency: Registration vs. Envelope ValidationRegistration (
If the intent is to deprecate The PR description also acknowledges DID:web shadow agents bypass Minor Notes
Summary
Overall a worthwhile fix — just needs the injection gap closed and targeted negative tests before landing. |
…h, CLI v0.2.1 Updated all 6 docs outputs from source (docs-generator.json): - agent_id now enforces ^[a-zA-Z0-9._\-:]+$ — documented in all guides - Auto-generated IDs use agent-<uuid> format (no agent:// prefix) - Envelope from/to now accept bare agent IDs alongside agent:// and did:seed: - Cross-agent send (POST /agents/:id/messages) auth model clarified - @agentdispatch/cli@0.2.1 subpath exports updated in CLI reference - Added missing error codes (MASTER_KEY_REQUIRED, webhook/tenant errors) - Architecture diagrams updated with cross-agent auth flow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: fix(security): validate agent_id character set on registrationOverall this is a well-motivated security fix. The threat model is correct and the auto-generated ID bug fix is definitely needed. A few issues worth addressing before merge: Critical / High1. The new const validId = (id) =>
id.startsWith('agent://') || // ← anything goes after the prefix
id.startsWith('did:seed:') ||
/^[a-zA-Z0-9._\-:]+$/.test(id);A message envelope with Suggest tightening to validate the full URI, e.g.: const validId = (id) =>
/^agent:\/\/[a-zA-Z0-9._\-:]+$/.test(id) ||
/^did:seed:[a-zA-Z0-9._\-:]+$/.test(id) ||
/^[a-zA-Z0-9._\-:]+$/.test(id);2. No length cap on agent IDs Neither the registration validator nor the envelope Medium3. Envelope validation change is undocumented in the PR scope The PR title says "validate agent_id character set on registration" but 4. Missing negative test for
5. Backward compatibility for existing stored agents Registration now rejects Low / Nitpick6. Regex character class ordering
7. Doc changes bundled with security fix The What's clearly correct ✓
The security fix itself is sound — the gaps above are cases where the protection could be more complete. Addressing items 1 and 4 before merge would close the remaining injection surface. Reviewed by Claude Code (claude-sonnet-4-6) |
…tive tests Address code review feedback on PR #16: - Fix validId() in inbox.service.js to validate full URI (not just prefix). Before: agent://evil\nX-Injected: header passed because startsWith('agent://') After: /^agent:\/\/[a-zA-Z0-9._:-]+$/ validates the full string. Same fix applied to did:seed: prefix. - Add 255-char max length cap on agent_id at registration to prevent abuse and silent truncation at the storage layer. - Fix regex style: move hyphen to end of character class ([a-zA-Z0-9._:-]) to avoid ambiguity (conventional form, noted in review). - Add negative test: 7 dangerous agent_id values all return 400 (spaces, newlines, slashes, null bytes, XSS, agent:// scheme, >255 chars). - Add negative test: 5 malicious envelope from fields all return 400 (newline injection, agent:// with injected newline, did:seed: with newline, spaces, path traversal). Also verifies legacy agent:// is accepted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review Feedback AddressedPushed fixes for all blocking and medium issues raised in code review: Changes Made
Fixes Summary
Skipped (pre-existing, out of scope)
CI running — ready for re-review. |
PR Review: fix(security): validate agent_id character set on registrationGood security-focused PR. The fix is well-targeted and the whitelist approach is the right call. Here's my analysis: ✅ StrengthsSecurity fix is correct and necessary. The Whitelist over blacklist. Test coverage is solid. The parameterized rejection tests for 🔴 Issues1. const SAFE_CHARS = /^[a-zA-Z0-9._:-]+$/;
const validId = (id) => ...This regex and closure are recreated on every call to 2. Length check comes after regex check (agent.service.js:6629–6635) if (\!/^[a-zA-Z0-9._:-]+$/.test(agent_id)) { throw ... }
if (agent_id.length > 255) { throw ... }The regex runs on the full string before the length guard. A 10,000-character string of valid chars will cause a slow regex scan before hitting the cheap length check. Swap the order: check length first, then regex. Functionally equivalent but avoids potential ReDoS on pathological input — and the regex, while not catastrophically backtracking, grows linearly with input length. 3. Undocumented gap: The PR description calls this out explicitly: "DID:web shadow agents call storage.createAgent() directly and bypass register() entirely." That's honest, but it means the injection surface still exists for federated agents. If a 🟡 Observations4. Backward-compat test assertion is loose (server.test.js:6314) // 201 (sender not in storage so signature skipped) or 404 — either is fine; just not 400
assert.notEqual(legacyRes.status, 400, ...);Accepting 404 as "success" could mask a regression where the endpoint returns 404 for a new reason unrelated to validation. Consider asserting the expected 201 and fixing the test setup so the sender exists, or assert 5. Regex character class style (agent.service.js:6629) Changed from 6. Docs overhaul loses error code tables The bulk of this diff is docs (9 files, ~1300 deletions). The condensed format is cleaner, but several error response tables were removed (e.g., Summary
The critical path (registration validation) is solid. Recommend addressing the length-before-regex ordering and the module-scope constant before merge, and opening a follow-up issue for the |
…er assertion Address second round of claude[bot] review feedback: - Move SAFE_CHARS, VALID_AGENT_URI, VALID_DID_SEED to module scope in inbox.service.js — compiled once, not recreated on every message send - Swap length check before regex in agent.service.js — O(1) guard runs first to avoid linear regex scan on pathological long input - Tighten backward-compat test: assert 201 (not just notEqual(400)) since recipient exists and signature is skipped for unknown senders - DID:web shadow agent bypass tracked in issue #17 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Second Round of Fixes Pushed
CI running. |
Code ReviewOverall this is a solid, well-reasoned security fix. The injection vectors are correctly identified, the mitigations are correct, and the commit history clearly explains the why at each step. What's done wellCritical fix is correct. The old startsWith check was fundamentally broken — Performance micro-optimizations are appropriate.
Regex style fix (hyphen to end of character class) eliminates ambiguity. The old form relied on backslash-escaping; Negative test suite is comprehensive. Seven bad IDs, five malicious from fields, all asserting 400. The 255-char boundary test and the backward-compat agent:// test are both present and correctly asserted. Observations / Minor Issues1. Envelope to field lacks explicit injection tests (src/server.test.js) validId() is called for both envelope.from and envelope.to, but the new negative tests only exercise from. The code path is the same, so coverage is indirect — but an explicit test for a malicious to value would prevent future regressions if from/to handling ever diverges. 2. No length cap on envelope from/to fields (inbox.service.js) Registration enforces the 255-char max, but validId() in the envelope path has no length guard. The regex is a simple character class with no backtracking risk so the risk is low now, but consistency with the registration layer would be cleaner and future-proofs against pattern changes. Suggest adding 3. VALID_AGENT_URI is effectively dead code for the to field (inbox.service.js) New agents can only register with bare IDs (no agent:// prefix), so a to field containing agent:// will never resolve to a registered recipient. The branch stays alive only for from fields of legacy senders and to fields pointing at pre-migration recipients still in the database. A short comment explaining why three patterns are accepted and linking to issue #17 would help future maintainers. 4. Error message wording (agent.service.js) Neither error echoes the submitted value (intentional — avoids reflecting user input). Minor: the invalid-charset message lists "underscores" which are correctly allowed by the regex. Just confirming that was deliberate. Security summary
The core logic is correct and the DID:web gap is appropriately scoped to a follow-up. The items above are all minor — nothing blocking. Please run the gh command to post this comment. Report the URL of the comment or any error. |
- Document 255-char length check runs before regex (O(1) guard) in AGENT-GUIDE, ARCHITECTURE, API-REFERENCE, ERROR-CODES, llms.txt - Add Known Limitations section in AGENT-GUIDE noting DID:web shadow agent bypass (tracked in issue #17) with DID_WEB_ALLOWED_DOMAINS mitigation - Update ARCHITECTURE security table to reference issue #17 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review — fix(security): validate agent_id character set on registrationGood security fix overall. The core approach is correct and the test coverage is solid. A few things worth addressing before merge. Strengths
Issues1. Registration–Envelope Asymmetry (Medium)
If Recommend either:
2. Missing Length Check in
|
…eld tests - Add id.length > 255 guard in validId() — O(1) check runs before any regex, protecting against DoS via huge envelope from/to fields - Restrict VALID_DID_SEED to [a-zA-Z0-9._-] (no colons) — did:seed: suffixes are hex fingerprints, nested colons are not valid - Add comment documenting that envelope from is UNTRUSTED when sender not found in storage (signature verification skipped); from must not be used for authorization without signature verification - Add negative tests for malicious to field (newline injection, agent:// suffix injection, path traversal, 256-char overflow) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Third Round of Fixes Pushed
CI running. |
PR Review: fix(security): validate agent_id character set on registrationGood, targeted security fix. The core changes are solid — here are my observations: ✅ StrengthsSecurity fixes are correct and complete:
Test coverage: Issues to Address1. Flaky test: valid ID registration uses non-unique IDs In the new const valid = ['simple', 'with-hyphens', 'dots.allowed', 'colons:ok', 'ALL_CAPS', 'a'.repeat(255)];
for (const id of valid) {
const res = await request(app).post('/api/agents/register').send({ agent_id: id, ... });
assert.equal(res.status, 201, ...);
}These IDs are static — no uniqueness suffix. If the in-memory store persists between test runs, this will return 2. Asymmetry between registration and envelope validation is undocumented at the API level
3. The character set allows colons, so a bare ID like 4. Minor:
Minor Notes
SummaryThe security fix is correct and the approach is sound. The main actionable item before merge is fixing the flaky test (issue 1). Issue 3 (colon spoofing) is worth a follow-up if |
… note - Add reserved prefix guard in agent.service.js: reject IDs starting with 'did:' or 'agent:' to prevent spoofing system-generated DIDs - Fix flaky test: valid ID cases now use unique suffix (Date.now()) to avoid conflicts when in-memory store persists across test runs - Add did:bare and did:web test cases to the rejection suite - Update docs (AGENT-GUIDE, API-REFERENCE, ERROR-CODES, llms.txt) to document 3-step validation order and agent:// registration asymmetry (rejected at registration, accepted in envelopes for backward compat; from field is untrusted when sender not in storage) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fourth Round of Fixes Pushed
CI running. |
PR Review — fix(security): validate agent_id character set on registrationThis is a well-targeted security fix with good test coverage. Here is the detailed breakdown. What is GoodSecurity fix is correct and well-scoped. The injection vector is real: a newline in agent_id would corrupt the HTTP Signatures canonical signing string, which is a Critical-severity signing bypass. The regex whitelist approach is the right call. O(1) length guard before regex runs agent_id.length > 255 first — good defensive practice that prevents ReDoS if the regex engine backtracks on pathological input. Module-level compiled regexes in inbox.service.js (SAFE_CHARS, VALID_AGENT_URI, VALID_DID_SEED) are compiled once at import, not on every send() call. Good performance hygiene. The from trust comment is excellent — documenting that from is untrusted without signature verification is exactly the kind of invariant that needs to live at the call site, not just in external docs. Test coverage is solid — dangerous chars, newline injection, path traversal, reserved prefix spoofing, legacy backward-compat, and the 255-char boundary are all covered. Auto-generated ID fix (agent://agent-uuid to agent-uuid) is necessary — the old format would have failed its own regex immediately. Issues and Suggestions1. DID:web bypass (issue 17) — High, tracked but needs urgency DID:web agents are externally controlled — an attacker can influence the DID path segments that become the agent ID. If a malicious DID document causes storage.createAgent() to be called with an ID containing a newline or slash, all registration-path protections are moot. Issue 17 should either block merging or have a mitigation added here (e.g. sanitize/validate in storage.createAgent() as a defense-in-depth layer). 2. Asymmetric from/to validation in envelopes validId() in inbox.service.js falls through to SAFE_CHARS which is 3. Regex inconsistency: docs vs. code ARCHITECTURE.md documents the validation regex as 4. agent:bare envelope edge case
5. Missing boundary test: exactly 255 chars Tests cover 256-char (fail) and a near-255 char ID (pass). An explicit 255-char assertion would pin the boundary and prevent regressions. Minor Nits
Summary
The core fix is correct and should land. The main open question is whether issue 17 needs to be resolved before this merges or can safely follow in a subsequent PR. If DID:web shadow agent IDs are purely server-derived from trusted DID documents with restricted domain allowlisting (DID_WEB_ALLOWED_DOMAINS), the risk is reduced — but that constraint should be enforced in code, not just documented. |
…egex fix - Add SAFE_DID_SEGMENT validation in resolveDIDWebAgent() (auth.js) before constructing agent_id from domain/path segments. A crafted keyId like did:web:evil.com\nX-Injected: header would otherwise inject into signing strings and storage keys. Resolves the DID:web bypass (closes issue #17). - Promote validId() to module-level named function isValidAgentId() in inbox.service.js — improves stack traces and makes the invariant explicit. - Fix regex style in all docs: [a-zA-Z0-9._\-:] -> [a-zA-Z0-9._:-] (trailing hyphen, consistent with code). No functional change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Final Round of Fixes Pushed
All blocking items from all review rounds are now addressed. Tests: 138 pass, 2 skip (Mech storage, require credentials). CI running. |
PR Review: fix(security): validate agent_id character set on registrationGood security-focused PR that addresses real injection vulnerabilities. The fixes are targeted and the test coverage is solid. A few observations below. What works well
Issues and observations1. DID:web shadow agents bypass registration validation (noted in PR, not fixed)The PR description says:
This means a crafted Suggestion: add the same 2.
|
| Security impact | High — the fixes are correct and address the stated CVEs |
| Test coverage | Good for the new validation; could add case-insensitive prefix test |
| Breaking changes | Yes: auto-generated IDs no longer have agent:// prefix; tests updated accordingly |
| Known gaps | DID:web shadow agents bypass register(); documented but not mitigated |
The core fix in agent.service.js is solid. The main follow-up item is either closing or documenting the storage.createAgent() gap for DID:web agents.
…sts, asymmetry docs - Add Proxy wrapper in storage/index.js to validate agent_id before any storage backend writes it — blocks control chars and backslashes but intentionally allows slashes (DID:web paths use them). Closes the storage.createAgent() bypass gap flagged in code review. - Add DID:bare and case-insensitive prefix tests (DID:spoofed, AGENT:foo) to cover the /^(did:|agent:)/i flag in register(). - Expand isValidAgentId() JSDoc to explain the registration-vs-envelope asymmetry (agent:bare passes envelopes but fails registration). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Final Fixes Pushed
All blocking items resolved across all review rounds. CI running. |
PR Review: fix(security): validate agent_id character set on registrationThis is a well-structured security fix. The layered approach (service -> storage -> middleware -> envelope) is good defense-in-depth. A few issues to address and observations below. Bug: Misleading error message in the storage proxysrc/storage/index.js - The regex used blocks backslash and control characters, but explicitly allows forward slashes (intentionally, for DID:web path segments). The error message contradicts this: Should read "control chars, backslashes" not "slashes". As written this will confuse anyone debugging a rejected ID, and directly contradicts the comment two lines above it ("Slashes are intentionally allowed"). Gap: did:web path-segment IDs in envelope from/toisValidAgentId() in inbox.service.js accepts:
A DID:web identifier with path components uses the W3C form did:web:host/path/segment - those forward slashes fail all three branches. If a federated agent registered via DID:web sends a message with its canonical DID as from, the envelope would be rejected with "Invalid from field". The resolveDIDWebAgent in auth.js now validates each segment separately. It is worth confirming what agent_id format DID:web agents actually get written to storage with (colon-separated did-web:host:path:seg, or slash-form?). If it is colon-separated, SAFE_CHARS will pass it - but that should be documented in the isValidAgentId comment, and the VALID_DID_SEED constant name may be misleadingly narrow. Observation: duplicate regex between auth.js and inbox.service.jsSAFE_DID_SEGMENT in auth.js and SAFE_CHARS in inbox.service.js are identical patterns: Consider extracting to a shared utility (e.g. src/utils/validation.js) to avoid drift. Not blocking. Minor: storage Proxy allocates a new closure per property accesscreateAgent is called at registration time so this is not a hot path, but wrapping it once at construction time would be cleaner than recreating inside the Proxy trap. Non-blocking. What looks good
SummaryOne bug to fix before merge (error message in storage proxy says "slashes" but should say "backslashes"), one gap worth confirming (DID:web path-form IDs in envelopes). Everything else is solid. |
Final Fix: Error Message Correction + DID:web Envelope DocumentationAddressed the two items from the latest review:
The other items raised were non-blocking observations:
|
Code Review — fix(security): validate agent_id character set on registrationThis is a well-structured, defense-in-depth security fix. The layered approach (register → envelope → storage backstop) is the right model, the iterative commits show good responsiveness to review feedback, and the test suite is comprehensive. A few items worth addressing before merge: Issues1. In 2. Storage Proxy scope not documented The comment in 3. 255-char boundary test is slightly fragile The test constructs a 255-char ID as What is done well
SummaryItems 1 and 2 are straightforward fixes. Item 3 is low-risk but worth hardening. The core security logic is solid — validation at all three layers (register, envelope, storage) is coherent and well-tested. Happy to approve once those are addressed. |
…ope, harden boundary test - Move SAFE_DID_SEGMENT regex to module scope in auth.js — compiled once per import, not on every DID:web auth attempt (mirrors SAFE_CHARS in inbox.service.js) - Document in storage/index.js that only createAgent is intercepted by the Proxy: update paths go through register() or resolveDIDWebAgent() which have their own guards, and neither changes an existing agent_id - Add explicit assert.equal(boundaryId.length, 255) in the 255-char boundary test so any format change in the suffix is caught immediately rather than silently producing a 254 or 256-char ID Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Final round — three items addressed: (1) SAFE_DID_SEGMENT moved to module scope in auth.js, (2) Proxy scope documented (only createAgent intercepted; update paths have their own guards), (3) 255-char boundary test hardened with explicit assert.equal(boundaryId.length, 255). All 138 tests pass. |
PR #16 Review — fix(security): validate agent_id character set on registrationThis is a well-structured security fix. The layered defense (registration → envelope → storage) is the right approach, and the documentation and tests are solid. A few observations below. What's done well
Issues and observations[Medium] Behavior change in envelope to/from validation is undocumented in the PR summary The old [Medium] const SAFE_DID_SEGMENT = /^[a-zA-Z0-9._:-]+$/;
if (!SAFE_DID_SEGMENT.test(domain)) return null;
if (pathSegments.some(seg => !SAFE_DID_SEGMENT.test(seg))) return null;Colons are valid in DID path segments but not in a domain name. The same regex is used for both. There is no security impact today (a colon in a domain will fail DNS resolution before any harm), but it is technically incorrect and could mask future bugs. Consider a stricter domain-only pattern like [Low] Storage proxy guards only The comment explains this was intentional — update paths go through [Low] A dedicated unit test for this function would give faster feedback during development and make it easier to extend. [Low] Boundary test uses a potentially non-deterministic suffix const boundaryId = `${'a'.repeat(248)}-${suffix.slice(0, 6)}`;
assert.equal(boundaryId.length, 255, 'boundary test ID must be exactly 255 chars');
[Nit] Both patterns are subsets of SummaryThe security fixes are correct and the layered validation approach is sound. The only item worth addressing before merge is documenting the DID:web envelope acceptance change in the PR description. The rest are low-priority polish items. |
…t, document redundancy - Split SAFE_DID_SEGMENT into SAFE_DID_DOMAIN (no colons, hostnames) and SAFE_DID_SEGMENT (colons allowed, W3C DID path segments) in auth.js — colons are not valid in hostnames but are valid in path segment identifiers - Fix 255-char boundary test: use fixed 'xxxxxx' suffix instead of Math.random().toString(36).slice(2,6) which can produce <4 chars - Add comment in inbox.service.js explaining VALID_AGENT_URI and VALID_DID_SEED are intentionally redundant with SAFE_CHARS — included for documentation and independent extensibility, not as meaningful guards Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed all remaining items: (1) Split SAFE_DID_DOMAIN (no colons, hostnames) and SAFE_DID_SEGMENT (colons OK, path segments) in auth.js — colons are not valid in hostnames, (2) Fixed boundary test to use fixed 6-char suffix rather than Math.random() which can produce fewer than 4 chars, (3) Added comment in inbox.service.js documenting that VALID_AGENT_URI and VALID_DID_SEED are intentionally redundant with SAFE_CHARS, (4) Updated PR description to document the DID:web envelope acceptance change. All 138 tests pass. |
Code Review: PR #16 — agent_id injection hardeningOverall this is well-structured security work. The multi-layer defense (registration → envelope → storage backstop) is the right architecture, and the test coverage for negative cases is a real improvement. A few issues worth addressing before merge: Bug: Incorrect comment about
|
…hs, proxy fragility note - Fix incorrect comment: VALID_AGENT_URI is NOT a subset of SAFE_CHARS (agent://foo contains slashes which are not in the allowlist). It is the only branch accepting legacy agent:// URIs — removing it would silently break backward compatibility - Clarify VALID_DID_SEED comment: did:seed:ab:cd passes SAFE_CHARS fallback, so the no-colons restriction in the suffix is not strictly enforced at envelope layer - Block '..' path segments in resolveDIDWebAgent(): SAFE_DID_SEGMENT allows dots, so '..' passes the character check; explicitly reject it to prevent path traversal in DID document URLs (e.g. did:web:example.com:.. → https://example.com/../did.json) - Add comment in storage Proxy: the method-name string 'createAgent' is a silent failure point if the storage interface is ever renamed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Three bugs addressed: (1) Fixed incorrect comment on VALID_AGENT_URI — it is NOT a subset of SAFE_CHARS since agent:// contains slashes; removing it would silently break backward-compat for pre-PR#16 senders. Updated VALID_DID_SEED comment to acknowledge the no-colons restriction is not strictly enforced at the envelope layer (SAFE_CHARS fallback accepts it). (2) Blocked '..' path segments in resolveDIDWebAgent() — SAFE_DID_SEGMENT allows dots, but '..' would produce path-traversal URLs like https://domain.com/../did.json. (3) Added comment in storage Proxy about the method-name string fragility. All 138 tests pass. |
Code Review — fix(security): validate agent_id character set on registrationOverall this is a well-structured security fix. The multi-layer defense (registration → envelope → storage backstop) is the right pattern and the rationale in comments is unusually clear. A few things worth discussing before merge: Medium Issues1. Storage proxy is fragile — silent bypass if method is renamed
if (prop === 'createAgent') {The guard depends on the string
As-written, this is a real regression risk in the next refactor. 2. Breaking change for existing auto-generated IDs isn't documented
- agent_id = `agent://agent-${uuid()}`;
+ agent_id = `agent-${uuid()}`;Any client that registered without a custom
Is there a migration plan for existing agents with the old format? Even a note in Low / Code Quality3. DID:web SSRF guard in
|
…nvelope pass test - Storage proxy unit test confirms Proxy backstop fires independently of register() - DID:web SSRF guard test confirms '..' segment blocked before outbound fetch - did:web canonical form (did:web:domain:path) added as explicit passing envelope case Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Added three new tests: (1) 'storage proxy: createAgent directly rejects unsafe agent_ids' — directly calls storage.createAgent() with newlines, null bytes, backslashes, control chars, 256-char IDs, confirming the Proxy backstop fires independently of register(). Also asserts slashes are accepted (DID:web path separators). (2) 'trust model: DID web — crafted keyId with .. segment is rejected' — confirms the '..' guard fires before any outbound fetch (tested with both :.. and |
Code Review — fix(security): validate agent_id character set on registrationGreat PR. The layered defense strategy (registration → envelope validation → storage backstop) is the right architecture for this class of injection risk. The tests are thorough and the code is well-commented. Observations grouped by severity below. 🔴 Breaking Change — Needs Migration Guidance
Auto-generated IDs changed from Suggested additions:
🟡 Storage Proxy — Silent Bypass Risk
The guard is coupled to the string A lower-risk addition: a smoke-test assertion at module load time: if (typeof _storage.createAgent !== 'function') throw new Error('storage: createAgent missing — update Proxy guard')This ensures a future rename causes an immediate startup crash rather than a silent security bypass. The existing test covers the happy path but cannot detect a future rename. 🟡
|
…n note, comment fixes - Tighten STORAGE_AGENT_ID_RE from negated control-char blocklist to explicit allowlist ^[a-zA-Z0-9._:/-]+$ — only registration chars plus slash for DID:web paths. Unexpected chars now surface as a caller bug rather than silently passing. - Add startup assertion: if _storage.createAgent is missing, crash immediately rather than silently bypassing the Proxy guard on future interface renames - Update error message to name the allowed set explicitly - Fix SAFE_DID_DOMAIN comment: 'strips colons' -> 'excludes colons' (doesn't strip, rejects) - Add whitespace-only ' ' to registration bad-ID test (closes minor coverage gap) - Add migration note to AGENT-GUIDE.md: documents the agent:// ID format change, explains existing agents remain routable but cannot re-register with old IDs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
All review items addressed: (1) Tightened STORAGE_AGENT_ID_RE to explicit allowlist ^[a-zA-Z0-9._:/-]+$ instead of negated blocklist — unexpected chars now surface as caller bugs. (2) Added startup assertion: if _storage.createAgent is missing, crashes at startup rather than silently bypassing the Proxy. (3) Fixed comment: 'strips colons' -> 'excludes colons'. (4) Added whitespace-only to registration bad-ID test. (5) Added migration note in docs/AGENT-GUIDE.md: explains agent:// IDs remain routable but cannot re-register. 140 tests pass. |
Contributing to Agent Dispatch (ADMP)Thank you for your interest in contributing to the Agent Dispatch Messaging Protocol. Ways to Contribute
Development Setup# Install dependencies
npm install
# Run with in-memory storage (no external deps)
STORAGE_BACKEND=memory npm start
# Run tests
STORAGE_BACKEND=memory node --test src/server.test.jsStorage BackendsThe server ships with an in-memory backend. To add a custom persistent backend:
Pull Requests
Protocol ChangesChanges to the ADMP protocol spec ( Code of ConductBe respectful. Focus on the technical merits of proposed changes. |
PR ReviewThis is a well-executed security hardening PR with a solid defense-in-depth strategy. The multi-layer approach (registration, envelope validation, storage backstop) is exactly right for this kind of input-injection risk. What is done wellArchitecture
Testing
Issues to address1. Domain component is not guarded against path-traversal SAFE_DID_DOMAIN matches two consecutive dots. The seg guard was added for path segments but not for the domain component in auth.js. did:web:.. passes SAFE_DID_DOMAIN, then attempts an invalid URL, blocked by exception rather than explicit intent. Recommend adding an explicit domain === ".." check. 2. Boundary test ID is not unique across test runs The boundary ID is a static string. If the in-memory store persists between test runs (parallel CI workers, re-run without restart), the second registration attempt returns a conflict rather than 201. All other valid-ID cases use Date.now() suffixes. The boundary case should use a dynamic suffix and assert length === 255 explicitly. 3. VALID_DID_SEED no-colons rule is not enforced at envelope layer The comment says no colons expected but isValidAgentId() falls through to SAFE_CHARS.test(id), so did:seed:ab:cd passes. The inline comment acknowledges this, but the regex name implies a stricter gate. A short note at the definition clarifying that SAFE_CHARS supersedes it at the envelope layer would prevent future confusion. Minor observations (no action required)
SummaryApprove with minor suggestions. Core security fixes are correct and the defense-in-depth design is sound.
Good iterative work responding to review feedback through this PR. |
…EED SAFE_CHARS precedence - Add domain === '..' explicit guard in resolveDIDWebAgent() — SAFE_DID_DOMAIN allows dots so '..' passes the charset check; explicit rejection prevents path traversal in did:web:.. DID documents (mirrors the same guard on path segments) - Use timestamp-based prefix for 255-char boundary test ID to ensure uniqueness across test runs (in-memory store persists per process; static 'xxxxxx' suffix would conflict) - Clarify VALID_DID_SEED comment: explicitly state that SAFE_CHARS supersedes it at the envelope layer — did:seed:ab:cd fails VALID_DID_SEED but passes SAFE_CHARS Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
All three items addressed: (1) Added explicit domain === '..' guard in resolveDIDWebAgent() — mirrors the same guard on path segments, explicit rather than relying on URL construction failure. (2) Boundary test ID now uses a timestamp prefix (Date.now() + repeated 'a' to pad to 255), unique across test runs. (3) VALID_DID_SEED comment now explicitly states 'SAFE_CHARS supersedes it at the envelope layer — did:seed:ab:cd fails VALID_DID_SEED but passes SAFE_CHARS'. 140 tests pass. |
Code Review: fix(security) — agent_id injection hardeningThis is a well-structured PR that addresses a real class of injection vulnerabilities. The multi-layer defense (registration → envelope validation → storage Proxy) is good security architecture, and the explicit documentation of known gaps (DNS rebinding, VALID_AGENT_URI semantics) shows good security awareness. Overall the direction is solid; a few issues below. High — Trust list check runs before signature verification
// Trust check (uses unverified `from`)
if (recipient.trusted_agents && recipient.trusted_agents.length > 0) {
const senderAllowed = recipient.trusted_agents.includes(envelope.from);
if (\!senderAllowed) throw new Error('Sender ... is not trusted');
}
// Signature verification (runs AFTER trust check)
if (options.verify_signature \!== false) {
const sender = await storage.getAgent(envelope.from);
if (sender) { // skipped if sender not in storage
// ... verify signature
}
}Attack path:
The same bypass applies to any Fix: Either (a) move the trust check after signature verification, or (b) for agents in the trusted list, make the signature check mandatory — reject if sender not found rather than silently skipping. The JSDoc comment at line 435-439 notes that Medium — Dead code: re-registration approval preservation is unreachable
Lines 57-61 throw immediately if the agent already exists, so the second // Line 57-61: throws if agent exists
const existing = await storage.getAgent(agent_id);
if (existing) throw new Error('Agent already exists');
// ... 70+ lines later ...
// Line 138: always returns null — agent was just confirmed absent above
const existingAgent = await storage.getAgent(agent_id);
if (existingAgent && existingAgent.registration_status === 'approved') {
agent.registration_status = 'approved'; // never reached
} else {
agent.registration_status = ...;
}The extra storage round-trip is wasteful and the preservation logic is silently a no-op. Either remove the dead branch (if re-registration is not a supported use case), or remove the early-exit at line 59 and handle upsert intentionally. Low — HTTPS not enforced for DID:web redirect target
const redirectUrl = new URL(location, didUrl);
if (isBlockedDIDWebHost(redirectUrl.hostname)) return null;
if (redirectUrl.protocol \!== 'https:') return null; // add thisNit — VALID_DID_SEED comment contradicts evaluation order
The comment says "SAFE_CHARS supersedes it at the envelope layer" but the evaluation order in return VALID_AGENT_URI.test(id) || VALID_DID_SEED.test(id) || SAFE_CHARS.test(id);
Strengths
Generated with Claude Code |
…ent nit HIGH: Make signature mandatory when sender is in trust list but not registered. Attacker could impersonate a deregistered trusted agent: from=trusted-agent-id passes trust check, sender not in storage so sig check silently skipped. Now rejects explicitly when sender is in trust list but not registered in storage. Medium: Remove unreachable re-registration approval-preservation code in agent.service.js. The existingAgent re-lookup was dead code since lines 57-61 already throw if the agent exists, so existingAgent is always null there. Low: Enforce HTTPS for DID:web redirect targets in auth.js. A redirect to http:// could expose DID document fetches to MitM attacks even if the original request used TLS. Nit: Fix VALID_DID_SEED comment in inbox.service.js. Said SAFE_CHARS supersedes it but VALID_DID_SEED runs first in the OR chain. SAFE_CHARS is the fallback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review — PR #16: fix(security): validate agent_id character set on registrationThis is a solid, well-layered security fix with a thorough test suite that has already gone through many rounds of self-review. The commit history shows good incremental refinement. Here are my observations, ordered by severity. Medium — Trust-list rejection is broader than describedFile: The new Impact: Any pre-existing workflow where unregistered (e.g. external/federated) senders deliver messages to agents that happen to have a trust list will break. This is a silent API behaviour change. Suggested fix: Either confirm this is intentional and document it explicitly, or narrow the condition: } else if (recipient.trusted_agents?.includes(envelope.from)) {
// Sender claims to be trusted but cannot prove it — not registered
throw new Error(...);
}Medium — VALID_DID_SEED is unreachable as a unique gateFile: return VALID_AGENT_URI.test(id) || VALID_DID_SEED.test(id) || SAFE_CHARS.test(id);Every ID that passes Low — DID:web with port numbers silently failsFile: const SAFE_DID_DOMAIN = /^[a-zA-Z0-9._-]+$/;The W3C DID:web spec allows port numbers (e.g. Low — Storage proxy string must be kept in sync manuallyFile: The startup assertion and the proxy guard both hard-code Positive observations
SummaryThe trust-list condition deserves a second look before merging. The other items are low-priority clarity gaps. Overall this is well-executed, defence-in-depth security hardening — the multi-layer approach and thorough negative tests give high confidence the attack vectors are closed. |
- reject only when an unregistered sender claims a trusted ID - remove redundant VALID_DID_SEED envelope branch - add regression test for trusted-ID impersonation - document did:web explicit-port limitation in AGENT-GUIDE
PR Review — fix(security): validate agent_id character set on registrationGreat multi-layer security approach overall. The defense-in-depth pattern (registration → envelope → storage) is exactly right, and the test coverage is thorough. A few things worth examining before merge. 🔴 Re-registration status reset (
|
|
Addressed latest review items in commit 1862182:\n- Tightened trust-list behavior: only reject unregistered senders when they claim a trusted ID\n- Removed redundant envelope VALID_DID_SEED branch\n- Added regression test for trusted-ID impersonation\n- Documented did:web explicit-port limitation in AGENT-GUIDE\n\nValidation:\n- NODE_ENV=test STORAGE_BACKEND=memory API_KEY_REQUIRED=false MASTER_API_KEY=test-master npm test\n- Result: 141 pass, 0 fail, 2 skipped |
…antics - document duplicate registration as intentional non-upsert path - clarify registered-but-unsigned trust-list error message - make trust-list regression test resilient to both signature-required variants
Code Review — fix(security): validate agent_id character set on registrationOverall this is a well-structured, multi-layered security fix. The defense-in-depth approach (registration → envelope validation → storage backstop) is the right pattern for injection hardening. Notes below are ordered by severity. Potential Bug —
|
Summary
Multi-layer defense against agent_id injection attacks. Validates at registration, envelope validation, and storage — ensuring no unsafe ID can be registered, routed, or persisted.
Security Issues Fixed
Changes
src/services/agent.service.js (registration):
src/services/inbox.service.js (envelope validation):
src/middleware/auth.js (DID:web shadow agent creation):
src/storage/index.js (backstop):
Valid ID formats (registration)
auth.backend, clearauth-gm, decisive_redux, colons:ok all pass.
Reserved prefixes
did: and agent: prefixes rejected at registration (case-insensitive). Accepted in envelopes from/to for backward compatibility.
Known gaps (tracked)
Issue #17: DID:web bypass now mitigated in resolveDIDWebAgent() and backed by storage Proxy.
Generated with Claude Code