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:

import unittest
from urllib.parse import urljoin
from pydantic import AnyHttpUrl

class TestModifyUrlPath(unittest.TestCase):
    def test_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"),
        ]

        for base_url, expected in test_cases:
            any_http_url = AnyHttpUrl(base_url)
            with self.subTest(base_url=any_http_url):
                result = AnyHttpUrl(urljoin(str(any_http_url), "/authorize"))
                self.assertEqual(result, AnyHttpUrl(expected))

    def test_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"),
        ]

        for base_url, expected in test_cases:
            any_http_url = AnyHttpUrl(base_url)
            with self.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 🙏