Repository navigation
Allow redirect URL to be different than app origin - #230
Conversation
Closes keycloak#189 Signed-off-by: Jon Koops <jonkoops@gmail.com>
|
@markusheiden @ikkentim Since this issue affects your applications, could you please test these changes, and leave a review for me? |
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression introduced in commit 82924d0 that prevented redirect URLs from being on a different domain than the application origin. The fix restores the original behavior by renaming an internal variable to avoid confusion and correcting the hash fragment handling to preserve non-OAuth parameters.
- Renames internal
redirectUriproperty tonewUrlto distinguish it from the configured redirect URI stored in OAuth state - Fixes hash fragment handling to preserve non-OAuth parameters instead of clearing them entirely
- Adds comprehensive regression test with a simulated redirect service middleware
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/keycloak.js |
Renames callback property from redirectUri to newUrl and fixes fragment mode to preserve non-OAuth params, making it consistent with query mode behavior |
test/tests/login-with-redirect.spec.ts |
Adds regression test simulating authentication flow through an intermediate redirect service on a different domain |
test/support/common.ts |
Adds REDIRECT_SERVICE_URL constant for the redirect service hostname used in tests |
test/app/app.ts |
Implements middleware to simulate a redirect service that forwards OAuth callbacks back to the application origin |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jonkoops Your fix works for me. Thanks for fixing! |
|
@keycloak/core-clients could I get a review please? |
|
|
||
| // After login, the user should be authenticated. | ||
| expect(await executor.initializeAdapter(initOptions)).toBe(true) | ||
| expect(await executor.isAuthenticated()).toBe(true) |
There was a problem hiding this comment.
Maybe check that the final url is the app one:
| expect(await executor.isAuthenticated()).toBe(true) | |
| expect(await executor.isAuthenticated()).toBe(true) | |
| expect(page.url()).toBe(new URL(appUrl.origin).toString()) |
There was a problem hiding this comment.
Currently, navigation happens in the background implicitly. So there should be no need to do this. That said, I kind of want to refactor this, so that any navigation is handled explicitly. However, that is a topic separate from this PR.
This fixes a regression that was introduced by 82924d0, which has now been partially reverted, restoring the original behavior. A regression test has also been added to ensure correctness in the future.
Closes #189