fix(auth): bind redirect_uri to client, reject empty host, require allowed-hosts off-loopback

[sebyx07]

Slice 04 of the audit fixes — auth hardening, defense-in-depth (docs/plans/2026/07/20/101-mcp-ssh-audit-fixes/04-auth-hardening.md).

• Reject empty/whitespace-host https redirect URIs — https://?... used to pass is_allowed_redirect.
• [D1] redirect_uri bound to the registered client — DCR persists client_id → redirect_uri (clients table, no secrets); /authorize issues a code only for an exact registered match, else invalid_request. Enforces RFC 6749 §4.1.2.1, and kills the case where an /authorize link pairs a real client's client_id with an attacker's redirect_uri.
• [D2] Non-loopback bind fails fast without explicit allowed-hosts — the default ["localhost","127.0.0.1"] is no DNS-rebinding guard off-loopback (a remote attacker just sends Host: 127.0.0.1). Config::load now returns ConfigError::AllowedHostsRequired; loopback binds keep the safe default. Set-but-empty counts as unset. Behavior change: a non-loopback deploy without MCP_SSH_ALLOWED_HOSTS now refuses to start — matches the CLAUDE.md NEVER rule; docs env tables updated.
• Auth middleware unchanged in strength. bin/check green (167 tests).

Residual risk — D1 is not a complete anti-phishing control. /register is unauthenticated by design (it bootstraps auth), so an attacker can register their own client_id for their own callback and lure a victim to a self-consistent /authorize link; the Basic prompt remains the last line of defense. An earlier revision of this description and of docs/architecture.md claimed the path was closed — corrected in b276e50. Closing it properly needs a separate decision (consent screen showing the redirect target, authenticated DCR, or a redirect-URI allowlist).

Residual risk — the clients table is unbounded. /register is unauthenticated and now writes a durable row, with no expiry and no reaper sweep (the hourly pass sweeps access_tokens/refresh_tokens only). Rows are tiny and rate limiting lives in the reverse proxy per CLAUDE.md, so this is not a merge blocker — but it can't be fixed by simply adding a sweep either: nothing ties a token back to the client_id that obtained it, so age-based expiry would revoke live clients. Needs its own decision (a created_unix column plus a policy, or binding tokens to their client).

Upgrade note. The clients table starts empty, so a client_id registered before this deploy has no binding and gets 400 invalid_request at /authorize. Low impact: REFRESH_TTL is 365 days and each refresh resets the window, so active clients never hit /authorize; a client that does re-authorize re-runs DCR first. No migration added — flagging the behavior, not papering over it.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added dynamic OAuth client registration with persistent client and redirect URI bindings.
  • Authorization codes are issued only when the registered client and redirect URI match exactly.
  • Updated the OAuth token utility to register clients automatically before authorization.

• Bug Fixes

  • Rejected unsafe or incomplete HTTPS redirect URIs.
  • Server startup now fails when binding beyond loopback without an allowed-host list.

• Documentation

  • Clarified OAuth security behavior and allowed-host configuration requirements.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1308c609-7645-43a6-8810-1e2198601e3c

📥 Commits

Reviewing files that changed from the base of the PR and between 979b970 and c875b62.

📒 Files selected for processing (9)

• CLAUDE.md
• bin/mcp-token
• docs/architecture.md
• docs/deploy.md
• docs/usage.md
• src/config.rs
• src/db.rs
• src/oauth/flow.rs
• src/oauth/store.rs

---

Walkthrough

The change adds durable OAuth client registration and exact client/redirect-URI enforcement, updates the token helper to use dynamic registration, and makes explicit host allowlists mandatory for non-loopback server binds. Tests and deployment documentation reflect the new behavior.

Changes

OAuth and host security

Layer / File(s)	Summary
Require host allowlists for non-loopback binds src/config.rs, docs/deploy.md, docs/usage.md	Non-loopback binds now fail configuration loading when MCP_SSH_ALLOWED_HOSTS is unset or empty; loopback defaults and explicit-host tests are retained.
Persist registered OAuth clients src/db.rs, src/oauth/store.rs	SQLite stores client-to-redirect bindings, while Store registers clients transactionally and checks exact binding matches.
Register clients and enforce authorization bindings src/oauth/flow.rs	/register validates and persists redirect URIs, and /authorize requires a registered client/redirect pair before continuing with PKCE and code issuance.
Use dynamic registration in the token helper bin/mcp-token, docs/architecture.md, CLAUDE.md	The helper registers a client before authorization and passes its client_id; documentation describes persistence and matching behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant McpToken
  participant RegisterEndpoint
  participant Store
  participant SQLite
  participant AuthorizeEndpoint
  McpToken->>RegisterEndpoint: POST redirect_uris
  RegisterEndpoint->>Store: register_client(redirect_uris)
  Store->>SQLite: persist client bindings
  SQLite-->>Store: client_id
  Store-->>RegisterEndpoint: registration response
  McpToken->>AuthorizeEndpoint: request client_id and redirect_uri
  AuthorizeEndpoint->>Store: client_allows_redirect(client_id, redirect_uri)
  Store->>SQLite: check exact binding
  SQLite-->>Store: binding result
  AuthorizeEndpoint-->>McpToken: authorization code or invalid_request

Loading

Possibly related PRs

• developerz-ai/mcp-ssh#4: Updates OAuth flow tests for /authorize and /register.
• developerz-ai/mcp-ssh#7: Changes non-loopback MCP_SSH_ALLOWED_HOSTS handling.
• developerz-ai/mcp-ssh#10: Adds the OAuth flow used by bin/mcp-token.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main hardening changes: client-bound redirects, empty-host rejection, and required allowed-hosts for non-loopback binds.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch auth-hardening-slice-04

---

_{Comment @coderabbitai help to get the list of available commands.}

[sebyx07]

Local verification — green

bin/check (fmt --check + clippy --all-targets -D warnings + test): exit 0, 167 passed / 0 failed. Branch is 4 ahead / 0 behind origin/main — no rebase needed.

Beyond the unit suite, the three claims were exercised against a live server (loopback bind, throwaway SQLite + job dir), since there's no tests/ dir and bin/mcp-token isn't covered by cargo test:

Check	Result
bin/mcp-token full DCR → authorize → token flow	exit 0, 44-char bearer
That bearer on POST /mcp (initialize)	200
Garbage bearer on /mcp	401
[D1] registered client_id + its own redirect_uri	302 + code
[D1] registered client_id + attacker's redirect_uri	400, no redirect
[D1] unregistered client_id	400, no redirect
/authorize with client_id omitted	400, no redirect
/register with https://?evil, https://, http://evil.com/cb	400 each
[D2] 0.0.0.0 bind, MCP_SSH_ALLOWED_HOSTS unset	refuses to start, exit 1
[D2] 0.0.0.0 bind, hosts " , " (set-but-empty)	refuses to start, exit 1
[D2] 0.0.0.0 bind + explicit hosts	listens, drains cleanly on SIGTERM
Client registered before a restart, re-authorized after	302 + code — binding is durable

No code changes were needed; nothing pushed.

Blocking on

• REVIEW_REQUIRED — mergeable_state: blocked, needs an approving review.
• CodeRabbit status is red for rate-limiting, not findings — it never actually reviewed the diff ("Review limit reached", run def8d094). Zero inline comments were posted. Re-trigger with @coderabbitai review once the window reopens (~05:06Z) so the diff gets a real pass before merge.

The two residual risks already documented in the PR description (unauthenticated DCR is not a complete anti-phishing control; the clients table is unbounded) still stand as-is — both are flagged, neither is addressed here.

[coderabbitai]

@sebyx07 Thanks for the detailed verification evidence. I’ll run a fresh review of PR #21 now that the rate-limit window has reopened; approval should follow only if that independent diff pass finds no blocking security or correctness issues.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[sebyx07]

@coderabbitai full review

[sebyx07]

Re-verified at b276e50 — green, no changes needed

bin/check (fmt --check + clippy --all-targets -D warnings + test): exit 0, 167 passed / 0 failed. 4 ahead / 0 behind origin/main — no rebase needed.

Re-ran the live-server checks independently (throwaway SQLite + job dir, loopback bind), since bin/mcp-token and the startup path aren't covered by cargo test:

Check	Result
bin/mcp-token full DCR → authorize → token	exit 0, 43-char bearer
That bearer on POST /mcp (initialize)	200; garbage bearer 401
[D1] registered client_id + its own redirect_uri	302 + code
[D1] registered client_id + attacker's redirect_uri	400, no redirect
[D1] unregistered client_id / client_id omitted	400, no redirect
[D1] client registered before a restart, authorized after	302 + code — binding is durable
/register with https://?evil, https://, http://evil.com/cb	400 each
[D2] off-loopback bind, hosts unset / " , "	exit 1 both, with the AllowedHostsRequired message
[D2] off-loopback + explicit hosts / loopback + unset	both serve

The red check is not a finding

CodeRabbit is failure with description "Review rate limited" — it never read the diff. The @coderabbitai review at 04:49 replied "Review finished" but posted zero inline comments and zero reviews, because that command is incremental and treated the rate-limited commits as already-reviewed (its own note says so). Re-triggered with full review above to force a real pass.

Still blocking: REVIEW_REQUIRED — needs a human approving review.

Residual risks from the PR description are unchanged and unaddressed here: unauthenticated DCR is not a complete anti-phishing control, and the clients table is unbounded.

[coderabbitai]

✅ Action performed

Full review finished.

---

You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 14 minutes.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@sebyx07 Thanks for the re-verification details. The reported checks cover the D1/D2 behavior and the live auth flow well.

The red CodeRabbit status is understood as a rate-limit outcome, not a code finding. Since the forced full review was also rate-limited, I can’t claim an independent diff review or approval from this run. The remaining documented residual risks are appropriately out of scope for this PR.

[sebyx07]

Verified at b276e50 — green; the red check does not gate merge

New this pass — the ruleset actually enforcing main (GET /repos/.../rules/branches/main, ruleset main-protection 18382984):

• required_status_checks: fmt, check — both SUCCESS. CodeRabbit is not a required check, so its red "Review rate limited" status cannot block the merge button.
• pull_request: required_approving_review_count: 1, require_last_push_approval: true, dismiss_stale_reviews_on_push: true.

So mergeable_state: blocked reduces to exactly one thing: one approving human review. Nothing in the code is failing.

Local gate: bin/check (fmt --check + clippy --all-targets -D warnings + test) → exit 0, 167 passed / 0 failed. 4 ahead / 0 behind origin/main.

Independent diff read (CodeRabbit still hasn't read it)

Ordering in /authorize is right: Basic → is_allowed_redirect → client_allows_redirect. Because the binding check sits behind the 401, an unauthenticated attacker gets no client_id enumeration oracle. client_allows_redirect fails closed on a DB error; register_client is a single transaction, so no half-registered client. No blocking findings.

Two precision nits, neither a merge blocker:

1. The code is not bound to the client at redemption. TokenParams carries no client_id; redeem matches the PKCE verifier + redirect_uri only. So store.rs's comment — "re-checking it here carries that binding through to redemption (RFC 6749 §4.1.3)" — overstates by a half-step: the URI carries through, the client does not. Not exploitable (a stolen code is useless without the verifier, and §4.1.3's client_id check is aimed at confidential clients), but given b276e50 already corrected one overstated claim, this one deserves the same scoping. Completing D1 = persist client_id alongside the code and check it in redeem.

2. Missing client_id → axum's Query rejection, i.e. 400 with a plain-text Failed to deserialize query string body rather than {"error":"invalid_request"}. Correct status, no redirect, no code issued — just off-shape vs RFC 6749 §5.2.

Neither was touched here: this pass was verification, not modification, and both are judgement calls for the reviewer.

[sebyx07]

Verified at c875b62 — green; one comment-accuracy fix pushed

Local gate: bin/check (fmt --check + clippy --all-targets -D warnings + test) → exit 0, 167 passed / 0 failed, before and after the change. 5 ahead / 0 behind origin/main — no rebase needed.

Fixed nit 1 from the previous pass (c875b62, comment-only, no behavior change): store.rs's redeem comment claimed re-checking redirect_uri "carries that binding through to redemption (RFC 6749 §4.1.3)". It carries the URI, not the client — CodeEntry stores no client_id and the token request never sends one. Now stated as the half-step it is. Same correction pattern as b276e50.

Nit 2 (missing client_id → axum's plain-text Query rejection instead of {"error":"invalid_request"}) left alone: correct status, no redirect, no code issued — a response-shape judgement call for the reviewer, not a defect.

Status

• Required checks per ruleset main-protection: fmt, check — green at b276e50, re-running for c875b62.
• CodeRabbit red is "Review rate limited", not a finding, and is not a required check. This push should re-trigger it (its own guidance: "push new commits to this PR").
• mergeable_state: blocked still reduces to one approving human review.

Residual risks from the PR description are unchanged and unaddressed here: unauthenticated DCR is not a complete anti-phishing control; the clients table is unbounded.

[sebyx07]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.