fix(security): close user & agent enumeration oracles — uniform 404 + email-request parity + MCP owner-disclosure removal (#186)#1455
Merged
Conversation
…186) Tier 1: collapse the four agent-access dependency helpers to a uniform 404 for both non-existent and inaccessible agents. Evaluate existence and access booleans before branching so query-count (hence timing) is equal, and run the connector-scope check before the existence lookup so a connector key gets a uniform 403 across all non-bound names. Tier 2: POST /api/auth/email/request now returns a byte-identical body + status for whitelisted, non-whitelisted, and rate-limited emails, and dispatches the code email fire-and-forget so the whitelisted path's latency matches the immediate-return paths. Over-limit is WARN-logged server-side instead of a 429 (fail-loud for ops, no client-visible membership oracle). Closes the differential-response enumeration oracles from UnderDefense pentest 3.3.3 at the highest-leverage surfaces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tier 3 — collapse ad-hoc 404-then-403 sequences that bypass the dependency helpers: - avatar.py generate/regenerate/delete → OwnedAgentByName (uniform 404; now also connector-scope gated, intended hardening). - nevermined.py _require_read/_require_write_access → uniform 404. - event_subscriptions.py create → collapse source-agent 400-vs-403 into a single uniform 403 for cross-agent subscriptions. - schedules.py webhook generate/status/revoke → AuthorizedAgent so the schedule-404 is only reachable by an authorized accessor. Tier 4 — close the open-GET authz hole + 404-vs-200 existence oracle on agent_config.py capabilities/timeout/public-channel-model/guardrails GETs (raw agent_name + get_current_user, no access check) by binding them to AuthorizedAgentByName. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tier 5 (Invariant #13). chat.ts checkAgentAccess: collapse the 'not found' vs 'owned by X (not shared)' branches into a single uniform 'Agent X not found or not accessible' reason and REMOVE the owner-username interpolation — a user-scoped caller could otherwise learn who owns any agent (the most serious item the review found: a disclosure, not just a status differential). Consumer classifiers adjusted for the backend 403->404 flip: - reports.ts: treat a 404 on the dep-gated report endpoint as not_authorized (alongside 403). - messages.ts: match the dep's exact 'Agent not found' detail as not_authorized before the generic recipient-404 branch, so an agent-access denial isn't misclassified as recipient_not_found. - nevermined.ts: document that an inaccessible agent now reports configured:false (intentional, enumeration-safe). agents.ts needs no change — its agent-scoped permission denials name caller/target only (no owner leak) and are already uniform w.r.t. existence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- New tests/unit/test_186_enumeration_uniformity.py: real-DB (db_harness) assertions that all four agent-access deps return a byte-identical 404 for non-existent vs inaccessible agents (incl. admin-on-nonexistent and owner positive control); real-handler email-request body/no-429 uniformity; and a static guard that the four Tier-4 agent_config GETs now bind get_authorized_agent_by_name. 16 tests, all green. - test_access_control.py: flip the 15 dep-routed owner/read asserts from 403 to 404; admin-gated asserts stay 403. - test_28_voip / test_brain_orb: dep-override denial tests updated to the 404 contract. - test_public_links.py: owner skip-guards tolerate 404. - test_email_auth.py: tighten the unknown-email assert to ==200 (dead 429 branch removed) and assert no expires_in_seconds leaks. Verified in a py3.12 venv: new file 16/16, test_28+test_brain_orb 104/104. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- architecture.md Invariant #8: add the self-uniform (never 404-then-403) rule and the uniform-404 dependency behavior + MCP mirror. - requirements/auth.md §2.1: email-request identical body/status/timing. - requirements/security.md §20.7: new subsection recording #186. - feature-flows/email-authentication.md: revision-history row for the request-code change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- feature-flows.md: Recent Updates row for #186. - agent-permissions.md: uniform-404 dependency behavior, MCP checkAgentAccess uniform reason + owner-username removal, refreshed error-handling table + self-uniform contract note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…unt read path (#186) Close the sibling-path gap for the nevermined router's own module-level access helpers (they predate the dependency helpers): - _require_read_access now evaluates existence AND access unconditionally (no short-circuit) so the query count — hence timing — is identical for the non-existent and the existing-but-inaccessible case, matching _require_write_access and the dependencies.py helpers (#186 equal-query-count discipline). - Add real-DB tests asserting both helpers raise a byte-identical uniform 404 for a missing vs inaccessible agent, admin-on-nonexistent still 404s, and the write helper keeps owner-only enforcement (shared reader reads but cannot write) — guards the sibling codepath the dependency-helper tests don't reach. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Resolve by running |
…#186) Two order-dependent CI failures on this branch, both test/analysis-level (no product behaviour change to the enumeration fix): 1. backend-unit-test regression diff (seed 99999): test_186's _agent_exists probe resolved a truthy leaked Mock. A sibling unit test leaves services/services.docker_service as Mocks in sys.modules; _no_docker's dotted-string monkeypatch landed on a different auto-mock than the one `from services.docker_service import get_agent_by_name` resolves, so a non-existent agent read as existing and the admin-nonexistent 404 never fired. Capture the real modules at collection and pin them into sys.modules (monkeypatch.setitem, auto-restored) before stubbing the probe. Verified green across all three CI seeds. 2. CodeQL: 18 new py/path-injection alerts in avatar.py. Moving the inline db.get_agent_owner() check into the OwnedAgentByName dependency removed the barrier CodeQL saw on agent_name before AVATAR_DIR / f"{agent_name}...". Add _safe_avatar_component(): an explicit single-path-component basename guard at the top of generate/regenerate/delete — defense-in-depth (traversal is not reachable given OwnedAgentByName + charset-restricted names, but the constraint now lives in a callee CodeQL can't trace) with a uniform 404 that preserves the #186 no-existence-oracle contract. +10 barrier unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The first attempt (_safe_avatar_component, an os.path.basename guard in a helper) did not clear the 18 py/path-injection alerts: basename is not a CodeQL-modeled sanitizer and the guard sat in a callee, so CodeQL saw tainted-in/tainted-out — the same cross-function-barrier blind spot that hiding the check in OwnedAgentByName created. Replace it with _avatar_path(agent_name, suffix): os.path.normpath(join(base, …)) then require the result stay under AVATAR_DIR (the CodeQL SafeAccessCheck barrier documented in the py/path-injection help). A barrier guard severs taint flow even from inside a helper, since the guarded return only runs when contained. Route the 11 path sites in the four flagged functions (generate/regenerate/delete + _generate_emotions_background) through it; the unflagged GET handlers are left as-is to keep this scoped to #186. Produces byte-identical paths for real (charset- restricted) agent names; uniform 404 on escape preserves the no-existence-oracle contract. Barrier tests updated to assert containment/escape. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Prior barrier used `full != base and not full.startswith(base + os.sep)`; the
`base + os.sep` expression and compound condition didn't match CodeQL's
SafeAccessCheck guard model, so the 18 py/path-injection alerts persisted. Align
_avatar_path with the exact pattern from the py/path-injection query help:
fullpath = os.path.normpath(os.path.join(base, name))
if not fullpath.startswith(base):
raise ...
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vybe
approved these changes
Jul 6, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
Validated via /validate-pr: conventional commits, Fixes #186 (P2 type-bug, security/pentest), uniform-404 applied consistently across dependency family + router sweep + MCP layer, CodeQL path-injection alerts driven to zero, real-DB regression suite (test_186_enumeration_uniformity.py), no allow/deny boundary moved except the intentional Tier-4 hardening. Follow-ups noted (channel-panel 403 UX sweep, _avatar_path startswith(base+sep) tightening) — non-blocking. Dev-merge conflict in feature-flows.md resolved keep-both.
vybe
enabled auto-merge (squash)
July 6, 2026 08:46
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
4 participants
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.
Fixes #186
Closes the two enumeration oracles from UnderDefense pentest 3.3.3 (Low, CVSS 2.0): user (email) enumeration on
POST /api/auth/email/requestand agent enumeration across 30+ endpoints — plus an MCP-layer owner-username disclosure the review surfaced as the most serious item.Design decision
Uniform 404 (house convention, per the Session-Tab pattern) rather than the issue's literal "403 for both" — both close the oracle equally; 404 also hides that access control even applies. Documented as the self-uniform rule in architecture Invariant #8: a handler must never return an existence-404 followed by an access-403 in the same function. Access-first inline handlers that already 403 uniformly stay 403.
The fix — 5 tiers
dependencies.py): all four (get_authorized_agent/get_owned_agent/…_by_name) return a uniform 404 for both non-existent and inaccessible/unowned agents. Existence + access are evaluated before branching (equal query-count → equal timing), and_enforce_connector_scoperuns first. Covers the bulk of the 30+ endpoints.routers/auth.py): identical generic body + status for whitelisted / non-whitelisted / rate-limited; over-limit returns the generic 200 (no 429 differential, suppression WARN-logged); verification email dispatched fire-and-forget so latency no longer splits fast/slow.avatar.py(→OwnedAgentByName),nevermined.py(uniform-404 helpers, equal-query-count read path),event_subscriptions.py(source-agent 400/403 → uniform 403),schedules.pywebhook endpoints (→AuthorizedAgent).agent_config.pyGETs (capabilities/timeout/public-channel-model/guardrails): wereDepends(get_current_user)only — authz hole + 404-vs-200 oracle. NowAuthorizedAgentByName./capabilitiesis named in the issue.chat.ts checkAgentAccessreturns one uniform reason and removes the owner-username interpolation (genuine disclosure). Consumer classifiers adjusted for the 403→404 flip:reports.ts/messages.tstreat the dep 404 as not-authorized;nevermined.tsreportsconfigured:falsefor an inaccessible agent (enumeration-safe).Intended hardening called out
_enforce_connector_scope(owner_op=True)— connector keys that could generate avatars get 403.nevermined.tssemantic change: an inaccessible agent reports "not configured" (indistinguishable from genuinely-unconfigured — by design).DELETE /{agent_name}intentionally stays 403 (already self-uniform; folding to 404 risks system-agent semantics).Docs
architecture.mdInvariant security: implement safe tar extraction with symlink/hardlink validation #8 — self-uniform rule + MCP feat: SMARTS trading pipeline with Telegram notifications and Miro visualization #13 noterequirements/auth.md §2.1(email parity) +requirements/security.md §20.7(finding record)feature-flows/email-authentication.md+feature-flows/agent-permissions.md+ index Recent-Updates rowTests
tests/unit/test_186_enumeration_uniformity.py— real-DB byte-identical 404 across all four deps (Authorized/Owned × name/agent_name), nevermined sibling helpers (uniform 404, admin-on-nonexistent, owner-only write), email body/no-429, Tier-4 wiring guard. 23 passed locally.test_access_control.py,test_public_links.py,test_email_auth.py, and dep-override unit tests (test_28_voip,test_brain_orb— 104 passed). Admin-gated asserts verified to stay 403.Verification
test_access_control/test_email_auth/test_public_links) require a live stack — to run via/verify-local+ sibling-stack curl probes + the MCPtools/list/callToolprobe (MCP TS layer is not covered byverify-local) before flipping ready-for-review.Draft — validated via
/validate-pr(all green). Held for the second-reviewer pass (SOC 2 separation of duties).