Skip to content

Fix oauth well-known paths to retain path and query #756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jul 10, 2025
Merged

Fix oauth well-known paths to retain path and query #756

merged 6 commits into from
Jul 10, 2025

Conversation

ihrpr
Copy link
Contributor

@ihrpr ihrpr commented Jul 10, 2025

Background #733

@ihrpr ihrpr mentioned this pull request Jul 10, 2025
Closed
9 tasks
@selcuktemizsoy
Copy link

Thank you!

@ihrpr ihrpr changed the title Add tests to Fix oauth well-known paths to retain path and query Fix oauth well-known paths to retain path and query Jul 10, 2025
ochafik
ochafik requested changes Jul 10, 2025
View reviewed changes
Copy link
Contributor

@ochafik ochafik left a comment

Choose a reason for hiding this comment

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

Thanks @ihrpr !

src/client/auth.ts Outdated
@@ -261,7 +261,10 @@ export async function discoverOAuthProtectedResourceMetadata(
if (opts?.resourceMetadataUrl) {
url = new URL(opts?.resourceMetadataUrl);
} else {
url = new URL("/.well-known/oauth-protected-resource", serverUrl);
const issuer = new URL(serverUrl);
const wellKnownPath = buildWellKnownPath('oauth-protected-resource', issuer.pathname);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a fallback to the non-path-suffixed well-known path, as we do for the AS (see discoverOAuthMetadata), otherwise this will be a breaking change.

RFC9728 does mention the path suffix, but also states "By default, the well-known URI string used is /.well-known/oauth-protected-resource".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm Okay with the breaking changes if it means it's spec compliant. But yeah ""By default, the well-known URI string used is /.well-known/oauth-protected-resource". - this is still compliant, let me add fallback

src/client/auth.ts Outdated
@@ -361,8 +364,9 @@ export async function discoverOAuthMetadata(
const protocolVersion = opts?.protocolVersion ?? LATEST_PROTOCOL_VERSION;

// Try path-aware discovery first (RFC 8414 compliant)
const wellKnownPath = buildWellKnownPath(issuer.pathname);
const wellKnownPath = buildWellKnownPath('oauth-authorization-server', issuer.pathname);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this whole block w/ try suffix else fallback could be factored out / reused for both cases

@ihrpr ihrpr requested a review from ochafik July 10, 2025 12:45
ochafik
ochafik approved these changes Jul 10, 2025
View reviewed changes
Copy link
Contributor

@ochafik ochafik left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @ihrpr !

@ihrpr ihrpr merged commit 442e13b into main Jul 10, 2025
5 checks passed
@ihrpr ihrpr deleted the patch-1 branch July 10, 2025 13:26
@ochafik ochafik mentioned this pull request Jul 10, 2025
Closed
9 tasks
@MTG2000
Copy link

MTG2000 commented Jul 16, 2025 •
edited
Loading

Hey @ihrpr

For the case where discoverMetadataWithFallback is called with:

serverUrl = URL (with a pathname)
wellKnownType = "oauth-authorization-server"
opts = { metadataServerUrl: URL (without the pathname) }

This line here is making the fallback attempt on the issuer URL when it should be done on the opts?.metadataServerUrl ?? issuer, right?

typescript-sdk/src/client/auth.ts

Line 573 in 16ea277

if (!opts?.metadataUrl && shouldAttemptFallback(response, issuer.pathname)) {

Might be related to this issue: #744

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

@ochafik ochafik ochafik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ihrpr @selcuktemizsoy @MTG2000 @ochafik @calclavia