jlxq0/jmap-mcp
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases 1 Packages 1 Wiki Activity Actions

test(oauth): pin the entry scheme in loopback matching #10

Merged
jlxq0 merged 3 commits from test/loopback-guard-coverage into main 2026-08-25 05:35:37 +00:00
Conversation 0 Commits 3 Files changed 2 +53
jlxq0 commented 2026-08-25 02:28:17 +00:00
Owner
Copy link

Follow-up to #9, prompted by two findings from the m365-mcp worker. Both were
already implemented here — candidate.scheme() == "http" and the query
comparison are in the merged matcher — so instead I removed each of the six
terms of loopback_redirect_matches in turn and recorded which test died.

Before

# guard removed tests that go red
1 entry scheme is http none
2 requested scheme is http loopback_allowlist_varies_only_the_port
3 entry host is loopback none
4 host equality loopback_allowlist_varies_only_the_port
5 path equality loopback_allowlist_varies_only_the_port
6 query equality loopback_allowlist_varies_only_the_port

Guard 1 was load-bearing and untested. The port carve-out has to key off the
allowlist entry's scheme, not just the request's. Without it, an operator who
allowlists https://localhost:8443/cb has it satisfied by
http://localhost:3118/cb — verified, false → true on removal. That is the
authorization code on the wire in cleartext. No test allowlisted an https
loopback entry, so nothing caught it.

Guard 3 is unreachable, not untested. validate_redirect_uri already refuses
an http entry on a non-loopback host, so while guard 1 stands, guard 3 can never
be the deciding term. No mutation can turn it red. It stays as the second lock and
now carries a comment saying why, so the next reader does not delete it as dead.

After

# guard removed tests that go red
1 entry scheme is http loopback_https_entry_is_not_downgraded_to_cleartext
2 requested scheme is http loopback_allowlist_varies_only_the_port
3 entry host is loopback none — unreachable by construction, documented in place
4 host equality loopback_allowlist_varies_only_the_port
5 path equality loopback_allowlist_varies_only_the_port
6 query equality loopback_allowlist_varies_only_the_port

AGENTS.md

Records the URL-normalisation behaviour a hand-written host check runs into,
measured rather than reasoned about: 127.1 and 0177.0.0.1 canonicalise to
127.0.0.1 and /x/../cb to /cb, so they match; [::ffff:127.0.0.1] and
/%63b do not.

Gates

cargo fmt --all --check clean, cargo clippy --all-targets --all-features --locked -- -D warnings clean, cargo test --all-features --locked → 165 passed,
0 failed.

Tests and docs only; no behaviour change, so no release tag. Production stays on
v0.2.14.

Refs #8

Follow-up to #9, prompted by two findings from the m365-mcp worker. Both were already implemented here — `candidate.scheme() == "http"` and the query comparison are in the merged matcher — so instead I removed each of the six terms of `loopback_redirect_matches` in turn and recorded which test died. ### Before | # | guard removed | tests that go red | |---|---|---| | 1 | entry scheme is `http` | **none** | | 2 | requested scheme is `http` | `loopback_allowlist_varies_only_the_port` | | 3 | entry host is loopback | **none** | | 4 | host equality | `loopback_allowlist_varies_only_the_port` | | 5 | path equality | `loopback_allowlist_varies_only_the_port` | | 6 | query equality | `loopback_allowlist_varies_only_the_port` | **Guard 1 was load-bearing and untested.** The port carve-out has to key off the allowlist *entry's* scheme, not just the request's. Without it, an operator who allowlists `https://localhost:8443/cb` has it satisfied by `http://localhost:3118/cb` — verified, `false` → `true` on removal. That is the authorization code on the wire in cleartext. No test allowlisted an https loopback entry, so nothing caught it. **Guard 3 is unreachable, not untested.** `validate_redirect_uri` already refuses an `http` entry on a non-loopback host, so while guard 1 stands, guard 3 can never be the deciding term. No mutation can turn it red. It stays as the second lock and now carries a comment saying why, so the next reader does not delete it as dead. ### After | # | guard removed | tests that go red | |---|---|---| | 1 | entry scheme is `http` | `loopback_https_entry_is_not_downgraded_to_cleartext` | | 2 | requested scheme is `http` | `loopback_allowlist_varies_only_the_port` | | 3 | entry host is loopback | none — unreachable by construction, documented in place | | 4 | host equality | `loopback_allowlist_varies_only_the_port` | | 5 | path equality | `loopback_allowlist_varies_only_the_port` | | 6 | query equality | `loopback_allowlist_varies_only_the_port` | ### AGENTS.md Records the URL-normalisation behaviour a hand-written host check runs into, measured rather than reasoned about: `127.1` and `0177.0.0.1` canonicalise to `127.0.0.1` and `/x/../cb` to `/cb`, so they match; `[::ffff:127.0.0.1]` and `/%63b` do not. ### Gates `cargo fmt --all --check` clean, `cargo clippy --all-targets --all-features --locked -- -D warnings` clean, `cargo test --all-features --locked` → 165 passed, 0 failed. Tests and docs only; no behaviour change, so no release tag. Production stays on v0.2.14. Refs #8
test(oauth): pin the entry scheme in loopback matching
Some checks failed
CI / cargo (pull_request) Failing after 2s
Details
CI / docker (pull_request) Has been skipped
Details
53bd20a546
 
Removing each of the six terms of `loopback_redirect_matches` in turn showed
two that killed no test. One of them was load-bearing: the port carve-out keys
off the allowlist entry's scheme, and without that check an allowlisted
`https://localhost:8443/cb` is satisfied by `http://localhost:*/cb` — the
authorization code in cleartext. No test allowlisted an https loopback entry,
so nothing noticed.

The other, the entry-side loopback host check, is unreachable while the
entry-scheme check stands, because `validate_redirect_uri` already refuses an
`http` entry on a non-loopback host. It stays as the second lock and now says
so, rather than reading as redundant to whoever removes it next.

Also records in AGENTS.md which alternative host and path spellings the URL
parser canonicalizes into a match, verified rather than reasoned about.

Refs #8

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011yNTmXYzauoyQXxpfhyPxo
test(oauth): cover obfuscated loopback spellings in the downgrade case
Some checks failed
CI / cargo (pull_request) Failing after 2s
Details
CI / docker (pull_request) Has been skipped
Details
d394adfded
 
`127.1` and `0177.0.0.1` canonicalise to `127.0.0.1` in the URL parser before
the loopback host check runs, so an https entry written either way reaches the
port relaxation by the same path as the plain spelling. Assert the entry-scheme
check stops all three, so the guard is not mistaken for something the host
shape enforces.

Refs #8

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011yNTmXYzauoyQXxpfhyPxo
ci: re-trigger after runner volume expansion
All checks were successful
CI / cargo (pull_request) Successful in 1m16s
Details
CI / docker (pull_request) Successful in 1m0s
Details
60c19781af
 
Instrument only, to be dropped before merge. The runner /data PVC was 100%
full; it is now 60Gi with 38G free. No cargo job has run since the expansion,
so whether checkout works is still unverified.
jlxq0 merged commit 71c0268248 into main 2026-08-25 05:35:37 +00:00
jlxq0 deleted branch test/loopback-guard-coverage 2026-08-25 05:35:37 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
waiting-on-julian
Blocked on Julian. The ask is in the issue.

No labels
waiting-on-julian
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
jlxq0/jmap-mcp!10
No description provided.