test(auth): carry the shared redirect-URI case table in this repo #3
Labels
No labels
blocked
waiting-on-julian
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
jlxq0/caldav-mcp#3
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The observation
src/oauth_redirect.rsexists in five repositories —caldav-mcp,carddav-mcp,jmap-mcp,typst-mcp,m365-mcp— as five copies rather than one dependency. The RFC 8252 §7.3 loopback-port defect (#2 and its four siblings) therefore had to be fixed five times.The copies have already diverged, and nobody decided to
caldav-mcp,carddav-mcp,typst-mcp:is_allowed_redirect_uriat line 23.jmap-mcp: line 27.m365-mcp: line 53, plus a compiled-inDEFAULT_REDIRECT_URISconstant and additive env semantics —M365_MCP_OAUTH_REDIRECT_URISextends the compiled list, where the other four treat their env var as the whole allowlist.That last difference is a behavioural divergence in a security control, and it happened by drift rather than by a decision, because there was no single place for the decision to live. It also means the same operator mistake — an empty or wrong env var — fails closed in four servers and fails open-ish in the fifth.
The question, not the answer
Whether to extract this into a shared crate is a genuine trade, and this issue exists to record the decision rather than to pre-make it.
For extracting: one place for a security-relevant matcher; a defect gets fixed once; the divergence above cannot recur silently.
Against extracting: a git dependency across five repositories means five coordinated version bumps for every change, and five servers that today can be released without reference to each other stop being independent. A shared crate also makes a bad change land in five places at once, which is the same property that makes it attractive.
A middle option: keep the copies but add a CI check that fails when they disagree, so drift becomes loud without coupling the release cycles.
Remaining work in this repo
Add the two unpinned rows to
src/oauth_redirect.rs, and delete theunreachable
"::1"arm inis_loopback_hostwhile there: both call sites passUrl::host_str(), which always brackets IPv6, so the bare form cannot bereached and reads as coverage that is not there.
The alternate IPv4 spellings resolve through the
urlcrate's hostnormalisation rather than through anything this repo wrote, which is the
argument for pinning them: a crate bump could change that behaviour with
nothing going red.
Related: #2
deployed_allowlist_parsesasserts nothing about the deployment, and had already rotted in one of four #7Decision: keep the copies. Julian's constraint is that the five MCP servers stay independent of each other.
That rules out a shared crate, and it rules it out for a better reason than taste: a git dependency would mean a bad change lands in five production services at once, which is the same property that made extraction attractive. Independent release cycles are worth more than one fewer copy.
Recommendation: a shared test corpus, not a shared crate and not a byte-diff
The naive version of "keep the copies but detect drift" is a job that diffs the five
oauth_redirect.rsfiles and complains when they differ. Do not build that. It would be permanently red and therefore permanently ignored, because the files are supposed to differ —m365-mcplegitimately compiles aDEFAULT_REDIRECT_URISlist and treats its env var as additive, and that is a deliberate design difference, not drift.The divergence that actually hurt was behavioural, not textual, and no textual check would have caught it: four repos treat the env var as the whole allowlist and fail closed on a bad value; the fifth extends a compiled list and does not. Nobody decided that.
So pin the behaviour, in each repo, with no dependency between them:
A table of redirect-URI cases, identical in all five repos, each asserting an outcome rather than an implementation. Every repo carries its own copy — no repo imports anything from another, independence is untouched — and any repo whose matcher behaves differently from the others fails its own test suite. Divergence in intent then becomes a deliberate edit to that table, visible in review, with a reason attached.
The cases already exist; tonight's work produced them across four repos independently:
httpsrequest againsthttploopback entryhttprequest againsthttpsloopback entry127.1,0177.0.0.1,2130706433spellings127.0.0.1[::1]::1Four of the five repos had the
http-against-https-entry case unpinned until tonight — caldav found it, jmap, typst and m365 each confirmed it independently on their own copies. Four for four is the copy-paste, and it is exactly what this table prevents recurring.The one thing that may legitimately be byte-identical across the five is that table. A small check outside the repos — in
mantis, which depends on all of them and is depended on by none — comparing just the case table across the five is cheap and does not couple anything. That part is optional; the tests are not.What this does not fix
Nothing here stops someone writing a sixth copy, and nothing here makes the five files converge. It makes a behavioural difference between them fail loudly in the repo that has it, which is the failure mode that actually occurred. That is the whole claim.
Related: #7, which is the same "a test that cannot fail is not evidence" shape one level down.
decide: five copies of oauth_redirect.rs, and they have already divergedto test(auth): carry the shared redirect-URI case table in this repo