Publish the OAuth identifiers the operator configured, not a normalised copy of them - #157
Merged
Merged
Conversation
An issuer written without a path was published with a trailing slash. That is not a spelling difference: RFC 8414 requires the issuer a client reads back from an authorization server to be identical to the value it started from, and says the response MUST NOT be used otherwise. An authorization server whose issuer has no path returns it without one forever, so the document described a server no client could validate. RFC 9728 states the same requirement for the resource identifier, which stayed invisible only because the deployed resource carries a path. Two causes had to go together, and either alone leaves the output unchanged: this server converted the strings before handing them over, and the SDK's metadata models are themselves typed with the normalising URL type. The second is fixed upstream and ships in the next major SDK, which this server has not taken yet — so the same flag is set on the same models here, where it becomes a no-op once that upgrade lands. Measured against the live deployment: the configured issuer has no slash, the published one had it, and the authorization server reports the former. Two existing tests had encoded the normalised form; their subjects were the public route and the list separators, not the spelling.
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
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.
The discovery document published an authorization server identifier the
operator had not configured. Measured against the live deployment: the
configured value carries no trailing slash, the published one did, and the
authorization server itself reports the configured form.
Why this is a defect rather than a spelling difference
RFC 8414 §3.3, verbatim:
The comparison is against the string the client started from — the one this
document handed it. An authorization server whose issuer has no path returns
it without a slash forever, so a conformant client must refuse the metadata it
just fetched. RFC 9728 §3.3 places the same requirement on
resource; thathalf stayed invisible because the deployed resource carries a path, and the
normalisation only rewrites identifiers that have none.
Two causes, and why neither fix works alone
Both were measured, not reasoned about:
https://as.example/https://as.example/https://as.example/https://as.exampleThis server converted the strings to the normalising URL type before handing
them over, and the SDK's metadata models are themselves typed with it. The
second is not ours: upstream fixed it in modelcontextprotocol/python-sdk#2925
(closing #2883) with a single
url_preserve_empty_pathflag, and it ships inSDK 2.0 — a major upgrade this server has not taken. Rather than hand-write a
document whose whole value is that clients already parse it, the same flag is
set on the same models at startup. It is skipped where the SDK already carries
it, so it retires itself when that upgrade lands.
Verification
without a terminating slash) and assert the served document reproduces each,
plus the multi-server ordering case and
resourcemodel flag turns 5 tests red — the same 5 either way, which is the point
warning, pinned by a test, since publishing verbatim means nothing
downstream would catch a typo
public route and the list separators; the expectations were written around
what the code did (
AUTH_SERVER + "/"), and are corrected with a pointer tothe reason.