Fix building auth metadata paths

[Rodriguespn]

Summary

Fixes OAuth endpoint URL construction to always join paths correctly, preventing missing or double slashes regardless of the issuer URL format. Removes unnecessary use of lstrip("/"), which caused endpoints like $MY_ISSUERauthorize instead of the expected $MY_ISSUER/authorize.

Motivation and Context

The output of /.well-known/oauth-authorization-server was incorrect when the issuer URL included a path or when using URLs like http://localhost:8000.

Expected:

"authorization_endpoint": "https://example.com/authorize"

Actual:

"authorization_endpoint": "https://example.comauthorize"

This was due to the use of lstrip("/") when joining paths, which strips the leading slash and results in malformed URLs.

Note:
There is already PR #770 attempting to address this, but it does not handle all cases (e.g., URLs like http://localhost:8000 or issuer URLs with existing paths).

How Has This Been Tested?

Tested with various issuer URLs, including:

• https://example.com
• https://example.com/
• https://example.com/auth/oidc/op/Customer
• https://example.com/auth/oidc/op/Customer
• http://localhost:8000
• http://localhost:8000/

Verified the output of:

curl http://my-mcp-server/.well-known/oauth-authorization-server

Example output:

{
"issuer": "https://example.com",
"authorization_endpoint": "https://example.com/authorize",
"token_endpoint": "https://example.com/token",
"registration_endpoint": "https://example.com/register",
"revocation_endpoint": "https://example.com/revoke"
}

All endpoints are now correctly formed, with no missing or double slashes.

Breaking Changes

No breaking changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

Related PRs

• Remove lstrip in each OAuth endpoint #770 (works partially)

@hongkunyoo@pcarleton@Kludex

[Kludex]

urllib.parse.urljoin should be enough

[Rodriguespn]

urllib.parse.urljoin should be enough

Thank you for the suggestion @Kludex. Refactored the append_path function to include urllib.parse.urljoin and reduce it to 3 lines to cover all use cases in the PR description

[Kludex]

I think we can delete this modify_url_path tho. str(issuer_url) should work.

[Rodriguespn]

I was trying to modify as little code as possible, but you're right, this can be simplified by just casting the URL to a string and merging it with the endpoint (authorize, token, etc...)
I decided to convert the merged endpoint into a AnyHttpUrl pydantic class because the OAuthMetadata class is expecting AnyHttpUrl URLs

Let me know what you think! @Kludex

[Kludex]

There's no need to strip in any of those.

Suggested change

	str(issuer_url).rstrip("/") +AUTHORIZATION_PATH
	urljoin(str(issuer_url), AUTHORIZATION_PATH)

[Rodriguespn]

Thanks for the suggestion, @Kludex!

I initially thought the same — that urljoin(str(issuer_url), "/authorize") would be enough. But it actually breaks in some cases where the base_url includes a path.

For example, when the base_url is https://example.com/auth/oidc/op/Customer/, urljoin returns https://example.com/authorize, which drops the intended path entirely. That’s because urljoin treats the /authorize as an absolute path and replaces everything after the domain.

To illustrate this, I added a test suite comparing both approaches:

importunittestfromurllib.parseimporturljoinfrompydanticimportAnyHttpUrlclassTestModifyUrlPath(unittest.TestCase):
deftest_append_authorize_to_urls_with_urljoin(self):
"""Test appending /authorize to various URL formats using urljoin"""test_cases= [
("https://example.com", "https://example.com/authorize"),
("https://example.com/", "https://example.com/authorize"),
("https://example.com/auth/oidc/op/Customer", "https://example.com/auth/oidc/op/Customer/authorize"),
("https://example.com/auth/oidc/op/Customer/", "https://example.com/auth/oidc/op/Customer/authorize"),
("http://localhost:8000", "http://localhost:8000/authorize"),
("http://localhost:8000/", "http://localhost:8000/authorize"),
]
forbase_url, expectedintest_cases:
any_http_url=AnyHttpUrl(base_url)
withself.subTest(base_url=any_http_url):
result=AnyHttpUrl(urljoin(str(any_http_url), "/authorize"))
self.assertEqual(result, AnyHttpUrl(expected))
deftest_append_authorize_to_urls_with_rstrip(self):
"""Test appending /authorize to various URL formats using rstrip"""test_cases= [
("https://example.com", "https://example.com/authorize"),
("https://example.com/", "https://example.com/authorize"),
("https://example.com/auth/oidc/op/Customer", "https://example.com/auth/oidc/op/Customer/authorize"),
("https://example.com/auth/oidc/op/Customer/", "https://example.com/auth/oidc/op/Customer/authorize"),
("http://localhost:8000", "http://localhost:8000/authorize"),
("http://localhost:8000/", "http://localhost:8000/authorize"),
]
forbase_url, expectedintest_cases:
any_http_url=AnyHttpUrl(base_url)
withself.subTest(base_url=any_http_url):
result=AnyHttpUrl(str(any_http_url).rstrip("/") +"/authorize")
self.assertEqual(result, AnyHttpUrl(expected))
if__name__=="__main__":
unittest.main()

So for consistency across all cases, I believe we need to keep the rstrip("/") approach.

Let me know what you think!

[Kludex]

Unfortunate, but it makes sense.

[Kludex]

https://www.githubstatus.com/ 😢

[Kludex]

I'll merge this once #806 lands. I need an approval from another maintainer there.

[Kludex]

I've added inline-snapshot as a test dependency. It's super useful.

[Kludex]

I've included tests.

[Rodriguespn]

I'll merge this once #806 lands. I need an approval from another maintainer there.

Thank you for all the feedback @Kludex 🙏

[agronholm]

The tests seem to be relying on global state, making test_build_metadata[with-path-param] variant fail 100% of the time locally because the snapshot from the previous invocation with different URL parameters differs from the expected metadata (AnyHttpUrl('https://auth.example.com/') != AnyHttpUrl('https://auth.example.com/v1/mcp')).

[Kludex]

• Thanks @agronholm - opened tests: use inline_snapshot.Is on parametrized test #945