Skip to content

Preserve empty issuer/resource paths on AuthSettings#2987

Merged
Kludex merged 1 commit into
mainfrom
auth-settings-preserve-empty-path
Jun 26, 2026
Merged

Preserve empty issuer/resource paths on AuthSettings#2987
Kludex merged 1 commit into
mainfrom
auth-settings-preserve-empty-path

Conversation

@Kludex

@Kludex Kludex commented Jun 26, 2026

Copy link
Copy Markdown
Member

Summary

AuthSettings.issuer_url and resource_server_url are typed AnyHttpUrl, which normalizes a path-less URL with a trailing slash before the model's config can apply. As a result the authorization server advertised issuer as https://as.example.com/ instead of https://as.example.com in its metadata — inconsistent with the exact string comparison RFC 8414 / RFC 9207 require for issuer matching.

This applies url_preserve_empty_path=True to AuthSettings, the same fix #2925 made for the metadata models (OAuthMetadata, ProtectedResourceMetadata, OAuthClientMetadata). A string issuer_url/resource_server_url now keeps its canonical form end to end (config → build_metadata → served metadata).

Why

Surfaced while testing a real OAuth flow end to end: a client that discovers the issuer over the wire and uses it for issuer comparison (or as a token audience) saw .../ while the deployer's own constant had no slash, breaking the exact-match comparison the RFCs mandate.

Behavior change

  • Passing a string issuer/resource keeps the empty path (no trailing slash).
  • Passing an already-built AnyHttpUrl object still normalizes at construction (unchanged) — pass a string to get the preserved form.

Documented in docs/migration.md alongside the existing trailing-slash entry.

AI Disclaimer

This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.

AuthSettings.issuer_url and resource_server_url are typed AnyHttpUrl, which normalized a
path-less URL with a trailing slash before the model's config could apply. The authorization
server therefore advertised issuer as https://as.example.com/ instead of
https://as.example.com, inconsistent with the exact string comparison RFC 8414/9207 require.

Apply url_preserve_empty_path=True to AuthSettings (matching #2925 for the metadata models)
so a string issuer_url/resource_server_url keeps its canonical form end to end.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

Re-trigger cubic

@Kludex Kludex merged commit f41a519 into main Jun 26, 2026
34 checks passed
@Kludex Kludex deleted the auth-settings-preserve-empty-path branch June 26, 2026 09:41
Comment on lines +51 to +65
def test_auth_settings_preserves_path_less_issuer():
"""A path-less issuer passed as a string keeps its canonical form (no trailing slash)."""
settings = AuthSettings(
issuer_url="https://as.example.com", # type: ignore[arg-type]
resource_server_url="https://rs.example.com", # type: ignore[arg-type]
)
assert str(settings.issuer_url) == "https://as.example.com"
assert str(settings.resource_server_url) == "https://rs.example.com"


def test_build_metadata_serves_issuer_without_trailing_slash():
"""The served issuer matches the configured one exactly (RFC 8414/9207 string comparison)."""
settings = AuthSettings(
issuer_url="https://as.example.com", # type: ignore[arg-type]
resource_server_url="https://rs.example.com", # type: ignore[arg-type]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 These two new tests add four new # type: ignore[arg-type] comments, which AGENTS.md explicitly asks to avoid ("Avoid adding new # pragma: no cover, # type: ignore, or # noqa comments"). The same string-validation path can be exercised without the ignores via AuthSettings.model_validate({"issuer_url": "https://as.example.com", "resource_server_url": "https://rs.example.com"}), matching the pattern already used in tests/client/test_auth.py for OAuthMetadata.

Extended reasoning...

What the issue is

The two tests added in this PR (test_auth_settings_preserves_path_less_issuer and test_build_metadata_serves_issuer_without_trailing_slash) construct AuthSettings by passing plain strings to the AnyHttpUrl-typed issuer_url and resource_server_url fields, suppressing the resulting type errors with four new # type: ignore[arg-type] comments (lines 54–55 and 64–65). AGENTS.md (Code Quality / Coverage section, ~line 100) explicitly says: "Avoid adding new # pragma: no cover, # type: ignore, or # noqa comments", and even provides the audit command maintainers run before pushing — git diff origin/main... | grep -E 'type: ignore' — which would flag exactly these four lines. These are also the only # type: ignore[arg-type] occurrences introduced into the tests tree by this PR.

Why the tests are written this way (and why the ignores are still avoidable)

The tests genuinely need string input: passing an already-built AnyHttpUrl("https://as.example.com") object would normalize the URL to https://as.example.com/ at construction time, before url_preserve_empty_path=True ever applies, defeating the entire purpose of the test. So the string input is correct — but the constructor-with-ignore pattern is not the only way to get it. AuthSettings.model_validate({...}) accepts a plain dict and runs the exact same Pydantic string-validation path, with no static type error and therefore no ignore needed.

Existing precedent in the codebase

The repo already uses exactly this pattern for the same scenario: tests/client/test_auth.py (around lines 2724–2734) uses OAuthMetadata.model_validate({...}) with string URL values precisely to test the url_preserve_empty_path behavior introduced in PR #2925, with a comment explaining it matches the wire path. Following that precedent here keeps the new tests consistent with the existing test for the sibling fix.

Step-by-step proof that the alternative is equivalent

  1. Today: AuthSettings(issuer_url="https://as.example.com", ...) — Pydantic coerces the string through AnyHttpUrl validation inside the model, where url_preserve_empty_path=True from model_config applies, so str(settings.issuer_url) == "https://as.example.com". The type checker complains because the declared type is AnyHttpUrl, hence the ignores.
  2. Alternative: AuthSettings.model_validate({"issuer_url": "https://as.example.com", "resource_server_url": "https://rs.example.com"}) — the dict values go through the identical field validation (string → AnyHttpUrl with the model's config), producing the same settings object and the same assertions passing. model_validate accepts Any-typed input, so no type: ignore is needed.
  3. The rest of both tests (the assertions and the build_metadata call) is unchanged.

How to fix

Replace the keyword-argument construction with AuthSettings.model_validate({...}) in both tests and drop the four # type: ignore[arg-type] comments. This is a minor, non-blocking convention issue — the tests are otherwise correct and valuable — so it's filed as a nit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants