Skip to content

Fix OAuth redirect URL resolution - #2686

Merged
MikeEdgar merged 3 commits into
smallrye:mainfrom
arnabnandy7:fix/resolveOAuthRedirectUrl
Oct 12, 2026
Merged

MikeEdgar merged 3 commits into
smallrye:mainfrom
arnabnandy7:fix/resolveOAuthRedirectUrl

Conversation

@arnabnandy7

Copy link
Copy Markdown
Contributor

Fixes #2685

What

Resolve the configured oauth2RedirectUrl against the Swagger UI page URL using the browser URL API.

The existing sentinel value is left unchanged when no redirect URL is configured.

Why

Integrations can now supply a page-relative OAuth redirect URL. When Swagger UI is accessed through a reverse proxy that strips an external path prefix, resolving the URL against window.location.href preserves that externally visible prefix.

Absolute URLs continue to work unchanged.

Testing

mvn -pl ui/open-api-ui -Dtest=IndexCreatorTest test

@MikeEdgar MikeEdgar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@arnabnandy7, question on the changed condition. @phillip-kruger is this what you had in mind for the template change?

@phillip-kruger phillip-kruger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @arnabnandy7, yes @MikeEdgar, this is what I had in mind. Resolving against window.location.href keeps absolute paths and full URLs working as before, makes page-relative values keep a proxy prefix, and also fixes protocol-relative values (//host/x), which used to become http://host//host/x.

The "-" guard is needed: IndexHtmlCreator.replace() substitutes "-" when no redirect URL is configured, and without the guard that would turn into <page>/-.

A few small things:

  1. Comment in the template. The "-" depends on a value set in another file. A short comment would answer the question for the next reader, for example // Resolve against the page URL ("-" means not configured, see IndexHtmlCreator).
  2. Keep a test for the default case. testOauth2RedirectReplacement used to cover the unconfigured case and now only covers the configured one. Please add a test that the default still renders var oar = "-"; together with the guard.
  3. Optional: guard against a value new URL can't parse. If a configured value can't be parsed, new URL throws inside window.onload and the whole UI fails to render, where before the value was passed through as is. A try { ... } catch (e) {} that keeps the raw value would avoid that.

For Quarkus: once this is released, Quarkus can pass the relative oauth2-redirect.html instead of swaggerUiPath + "/oauth2-redirect.html" (follow-up to quarkusio/quarkus#56953), and the OAuth flow will then work behind a prefix.

@phillip-kruger phillip-kruger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The resolution logic looks good. I checked page-relative, root-relative, protocol-relative and absolute values against a prefixed page URL, and all resolve as expected. Before merging, please pick up the points from my earlier comment. The PR still has only the original commit:

  1. Add a short comment in the template explaining the "-" sentinel.
  2. Add a test for the unconfigured case (var oar = "-"; plus the guard).
  3. Optional: wrap new URL in a try/catch so a value it can't parse doesn't stop the UI from rendering.

@phillip-kruger
phillip-kruger force-pushed the fix/resolveOAuthRedirectUrl branch from 3dffe2b to 25b0bc9 Compare October 8, 2026 03:14
@MikeEdgar MikeEdgar added this to the 4.4.0 milestone Oct 8, 2026
Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
@phillip-kruger
phillip-kruger force-pushed the fix/resolveOAuthRedirectUrl branch from 5fdca21 to 90af76b Compare October 11, 2026 23:24

@phillip-kruger phillip-kruger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@MikeEdgar

Copy link
Copy Markdown
Member

@arnabnandy7 are you planning any other changes to this PR? I think we are good to go ahead and merge, but I just want to double check before doing so.

@arnabnandy7

Copy link
Copy Markdown
Contributor Author

@arnabnandy7 are you planning any other changes to this PR? I think we are good to go ahead and merge, but I just want to double check before doing so.

@MikeEdgar no I don't intend to make any other changes, unless suggested otherwise.

@MikeEdgar
MikeEdgar enabled auto-merge (squash) October 12, 2026 12:12
@sonarqubecloud

Copy link
Copy Markdown

@MikeEdgar
MikeEdgar disabled auto-merge October 12, 2026 12:28
@MikeEdgar
MikeEdgar merged commit eca7720 into smallrye:main Oct 12, 2026
20 checks passed
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.

Resolve Swagger UI OAuth redirect URL against the loaded page

3 participants