feat(admin): DNS rebinding + opaque handoff URL + WS origin + cookie path#301
feat(admin): DNS rebinding + opaque handoff URL + WS origin + cookie path#301elijahr wants to merge 13 commits into
Conversation
Prevents in-memory handoff/exchange token dict leakage between tests. Uses getattr fallback so the fixture survives the upcoming _exchange_tokens -> _handoff_tokens rename.
ASGI middleware that validates the Host header against a bare-hostname allowlist [127.0.0.1, localhost, ::1]. Handles bracketed IPv6 hosts correctly (Starlette's default split on ":" mangles [::1]:port). Not yet wired into create_admin_app() — Task 3 does that.
create_admin_app() now reads the bound port from get_env("PORT", "8765")
(same alias the CLI uses, resolving to SPELLBOOK_MCP_PORT) and stores it
on app.state.bound_port. Also computes the allowed origins list for use
by the Origin middleware (Task 5) and the WS handler (Task 11).
HostValidatorMiddleware is wired with the bare-hostname allowlist.
After this commit, the full tests/admin/ suite will be RED until Task 6
injects a default Host header via conftest. Targeted runs remain green:
- tests/admin/test_host_validator.py (Task 2)
- New test_admin_app_rejects_bad_host and
test_admin_app_state_has_bound_port_and_origins (this task)
ASGI middleware that requires a same-origin Origin header on state-changing methods (POST/PUT/PATCH/DELETE). Skips GET/HEAD/OPTIONS as safe methods. Requests presenting a valid Bearer token (matched by secrets.compare_digest against load_token()) skip the Origin check, enabling the CLI's bearer-authed POST /handoff to work without an Origin header. load_token is resolved at request time (not at middleware construction) so monkeypatch on spellbook.admin.auth.load_token works in tests. Not yet wired into create_admin_app() — Task 5 does that.
Adds OriginCheckMiddleware with allowed_origins sourced from app.state.allowed_origins (populated in Task 3 from the bound port). Middleware execution order: HostValidator outermost (rejects bad Host), OriginCheck inner (rejects cross-origin state-changing requests). Full tests/admin/ suite remains RED until Task 6 injects default Host and Origin headers via conftest. Targeted runs green.
TestClient fixtures now construct with default headers Host: 127.0.0.1:8765 Origin: http://127.0.0.1:8765 so existing tests don't need per-call header changes after Task 3/5 wired the new HostValidator + OriginCheck middlewares. Tests that explicitly override Host or Origin still take precedence (httpx.Client header merging rules). Restores the tests/admin/ suite from the planned RED window introduced by Tasks 3+5. Residual failures (if any) are scheduled for resolution by Task 8 (legacy /exchange and /callback endpoint deletion).
… instances
5 test files instantiate TestClient locally instead of using conftest's
client / unauthenticated_client fixtures, so Task 6's conftest header
defaults did not reach them. Apply the same headers={Host, Origin}
kwarg at each local construction.
Mechanical change only; no test logic touched.
Pure rename in preparation for endpoint replacement in the next commit
(POST /exchange + GET /callback -> POST /handoff + GET /handoff/{id}).
- _exchange_tokens dict -> _handoff_tokens
- create_exchange_token() -> create_handoff_token()
- validate_exchange_token() -> validate_handoff_token()
- routes/auth.py imports and call sites updated
- tests/admin/test_auth.py: TestExchangeToken class and 4 unit tests
renamed to match
No behavior change. URL paths unchanged.
H2 fix: eliminate token-in-URL leakage via browser history, Referer, and
process argv.
OLD: POST /exchange (JSON body) -> exchange_token; GET /callback?auth=...
NEW: POST /handoff (Bearer auth, empty body) -> { login_url };
GET /handoff/{opaque-id} -> Set-Cookie + 302 /admin/
The opaque ID is a single-use server-side lookup key, not a credential.
Tokens never appear in any URL. Same TTL (60s), same single-use
semantics, same in-memory store as the previous flow.
Old endpoints removed; tests updated. CLI update in next commit.
Module docstring refreshed.
CLI no longer constructs URLs containing tokens. Instead: 1. POST /admin/api/auth/handoff with Authorization: Bearer <mcp-token> 2. Read login_url from JSON response (server-built absolute URL) 3. webbrowser.open(login_url) The opaque handoff ID in the URL is a single-use server-side lookup key, not a credential -- safe to appear in browser history. Closes the H2 leg of the H2 fix (server-side flow was added in the prior commit; this commit makes the CLI actually use it).
Previously the spellbook_admin_session cookie was set with Path=/ (default), so browsers sent it to /mcp and /health too — broader than necessary. Add path="/admin" to all set_cookie and delete_cookie calls operating on this cookie. Pre-PR cookies with Path=/ will continue to be sent by browsers during the natural 24h expiry window (broader path matches narrower); no user action required. M2 from the security audit, folded into this PR because we are already touching cookie-minting code.
Defense-in-depth against cross-origin WebSocket hijack. The ws handler
now reads ws.headers.get("origin") and rejects (close code 1008,
Policy Violation) any upgrade whose Origin is not in
ws.app.state.allowed_origins (set in create_admin_app from the bound
port).
No bearer exemption on WS — ticket alone is insufficient for the
origin check; ticket is checked after origin.
Browsers always send Origin on WS upgrade; non-browser clients that
omit Origin are rejected.
…re order test
Code review findings:
1. RFC 7235 mandates the Bearer auth scheme token be case-insensitive.
OriginCheckMiddleware and POST /handoff both matched the scheme
case-sensitively. Switch to .lower().startswith("bearer ") and
.strip() the extracted token to also tolerate trailing whitespace.
2. Add a middleware-execution-order regression test that pins
HostValidator-before-OriginCheck via response body fingerprinting
(bad Host + bad Origin -> Host rejects, body says "Invalid host
header" not "Forbidden: invalid Origin").
No behavior change for the bundled CLI, which sends capital "Bearer ".
|
✅ Momus review posted — verdict REQUEST_CHANGES, 1 finding
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a more secure 'handoff' authentication flow for the admin interface, replacing the previous token exchange mechanism. It introduces HostValidatorMiddleware and OriginCheckMiddleware to defend against DNS rebinding and CSRF attacks, scopes session cookies to the /admin path, and adds origin validation to WebSockets. Feedback focuses on several Repository Style Guide violations regarding the mandatory use of the tripwire framework for mocking instead of monkeypatch or hand-rolled stubs. Other suggestions include updating docstrings to match the new 'handoff' terminology, optimizing origin lookups with a frozenset, and moving a function-level import to the top level.
| monkeypatch.setattr(admin_auth, "load_token", lambda: token) | ||
| monkeypatch.setattr( | ||
| "spellbook.admin.routes.auth.load_token", lambda: token | ||
| ) |
There was a problem hiding this comment.
This test uses monkeypatch.setattr to replace functions, which is strictly forbidden by the Repository Style Guide (lines 65-74, 101-109). All mocking must use the tripwire framework. Rewrite this using tripwire's three-step flow: register a mock (e.g., m = tripwire.mock("spellbook.admin.auth:load_token")), execute within a with tripwire: sandbox, and assert the interaction afterwards.
References
- tripwire is the ONLY acceptable framework for mocking. monkeypatch.setattr is forbidden for replacing functions. (link)
| monkeypatch.setattr( | ||
| "spellbook.admin.auth.time.time", lambda: real_time() + 61 | ||
| ) |
There was a problem hiding this comment.
Using monkeypatch.setattr to mock time.time is forbidden. Use tripwire.mock("spellbook.admin.auth:time.time") instead, following the register-sandbox-assert flow.
References
- monkeypatch.setattr is forbidden for mocking functions; tripwire must be used. (link)
| class _UrlopenRecorder: | ||
| """Records the Request object passed to urlopen and returns a fixed response.""" | ||
|
|
||
| def __init__(self, response: _FakeResponse): | ||
| self.response = response | ||
| self.requests: list[urllib.request.Request] = [] | ||
|
|
||
| def __call__(self, req, *args, **kwargs): | ||
| # urlopen may be called with either a Request or a url string; | ||
| # in CLI we always pass a Request. | ||
| self.requests.append(req) | ||
| return self.response |
There was a problem hiding this comment.
The use of hand-rolled stub classes like _UrlopenRecorder and _FakeResponse is forbidden by the Repository Style Guide (line 72). Additionally, mocking urllib.request.urlopen via monkeypatch is prohibited. Please use the tripwire.http domain plugin to mock and assert HTTP interactions.
References
- Hand-rolled stub classes and fake objects are forbidden. tripwire.http should be used for HTTP mocking. (link)
|
|
||
| def create_exchange_token() -> str: | ||
| def create_handoff_token() -> str: | ||
| """Create a one-time exchange token valid for 60 seconds.""" |
There was a problem hiding this comment.
The docstring still refers to an 'exchange token'. It should be updated to 'handoff token' to match the new naming convention used in the function name and implementation.
| """Create a one-time exchange token valid for 60 seconds.""" | |
| """Create a one-time handoff token valid for 60 seconds.""" |
| """Validate and consume an exchange token (single use).""" | ||
| _cleanup_expired(_exchange_tokens) | ||
| expiry = _exchange_tokens.pop(token, None) | ||
| _cleanup_expired(_handoff_tokens) |
There was a problem hiding this comment.
The docstring still refers to an 'exchange token'. It should be updated to 'handoff token'.
| _cleanup_expired(_handoff_tokens) | |
| """Validate and consume a handoff token (single use).""" |
|
|
||
| def __init__(self, app: ASGIApp, allowed_origins: list[str]) -> None: | ||
| self.app = app | ||
| self.allowed_origins = list(allowed_origins) |
There was a problem hiding this comment.
Consider using a frozenset for allowed_origins to ensure O(1) lookup time and maintain consistency with the HostValidatorMiddleware implementation.
| self.allowed_origins = list(allowed_origins) | |
| self.allowed_origins = frozenset(allowed_origins) |
| # Strip the extracted token to tolerate trailing whitespace. | ||
| if auth_header.lower().startswith("bearer "): | ||
| provided = auth_header[7:].strip() | ||
| from spellbook.admin import auth as admin_auth |
There was a problem hiding this comment.
The Repository Style Guide (line 25) prefers top-level imports over function-level imports unless there is a circular dependency. Since auth.py does not appear to import middleware.py, this import should be moved to the top of the file.
References
- Prefer top-level imports over function-level imports unless there is a circular dependency. (link)
![axiomantic-momus[bot]](https://avatars.githubusercontent.com/u/230757868?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR adds DNS rebinding defense, opaque handoff URLs, WS origin checks, and cookie path scoping to the admin web UI. The security hardening itself is well-designed with thorough test coverage. However, the daemon manager change from spellbook.mcp to spellbook.mcp.server breaks server startup in both execution branches — spellbook/mcp/server.py has no __main__ block and never calls mcp.run(). This is a critical regression that would prevent the MCP server from starting, which blocks this PR.
Severity tally: 1 Critical.
Critical (blocking)
- BOT-A1 (
spellbook/daemon/manager.py:94): Server startup broken:python -m spellbook.mcp.servernever callsmcp.run()
Noteworthy
- Test coverage is exceptional — thorough ESCAPE-documented tests for DNS rebinding, origin checks on all state-changing methods, handoff token single-use/replay/expiry/case-insensitivity, WS origin rejection, cookie path scoping, and middleware ordering.
Verdict: REQUEST_CHANGES.
Commands
- Comment
/ai-reviewor mention @axiomantic-momus[bot] to request a re-review of the latest changes. - Reply to a finding with
won't fix,by design, ornot a bugto decline it. - Reply with
instead, ...to propose an alternative fix.
Cost: $0.19 - 363,784 in / 27,186 out tokens - deepseek/deepseek-v4-pro
Powered by Momus running deepseek/deepseek-v4-pro via openrouter.ai.
Note: inline comments were demoted to body because some line citations were not on diff hunks.
1 participant
What does this PR do?
Hardens the admin web UI auth/transport surface against the four findings from the recent security audit: DNS rebinding (C1), token-in-URL exposure on the auth callback (H2), missing Origin check on WebSocket upgrades, and overly broad session cookie scope (M2).
Related issue
Checklist
Detail
Fixes
HostValidatorMiddlewarerejects requests whoseHostheader is not an exact match for the boundhost:port(default loopback only). Cross-origin browser POSTs are additionally blocked by anOriginCheckMiddlewareOrigin allowlist. Bearer-token API clients are exempt from Origin (they cannot be victims of CSRF).Originmatching the allowlist; mismatched origins are rejected at the handshake.Path=/admin, so it is no longer sent on unrelated routes served by the same host.Tests
Migration / risk notes
Path=/; they will simply not be sent on/admin/*and will expire on their existing 24h TTL. No forced logout, no data loss.localhost/127.0.0.1continue to work. Clients reaching the admin server via a non-loopback hostname must add that hostname to the configured host allowlist.