Skip to content

Commit

Permalink
Wait for URL update on redirect
Browse files Browse the repository at this point in the history
Updating the URL through the window object is an asynchronous operation,
but it has a synchronous signature. This enforces that the code waits on
the appropriate event before proceeding, preventing a race condition
resulting in the behavior observed in
#2891.
  • Loading branch information
NSeydoux committed Oct 2, 2023
1 parent 5818a79 commit 0fde58d
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 11 deletions.
8 changes: 6 additions & 2 deletions packages/browser/src/ClientAuthentication.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -436,8 +436,12 @@ describe("ClientAuthentication", () => {
});

it("clears the current IRI from OAuth query parameters in the auth code flow", async () => {
// eslint-disable-next-line no-restricted-globals
history.replaceState = jest.fn();
window.history.replaceState = jest.fn();
window.addEventListener = jest
.fn()
.mockImplementation((_event: unknown, callback: unknown) => {
(callback as () => void)();
});
const clientAuthn = getClientAuthentication();
const url =
"https://coolapp.com/redirect?state=someState&code=someAuthCode&iss=someIssuer";
Expand Down
23 changes: 14 additions & 9 deletions packages/browser/src/ClientAuthentication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
this.boundLogout = redirectInfo.getLogoutUrl;

// Strip the oauth params:
this.cleanUrlAfterRedirect(url);
await this.cleanUrlAfterRedirect(url);

return {
isLoggedIn: redirectInfo.isLoggedIn,
Expand All @@ -119,7 +119,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
};
} catch (err) {
// Strip the oauth params:
this.cleanUrlAfterRedirect(url);
await this.cleanUrlAfterRedirect(url);

// FIXME: EVENTS.ERROR should be errorCode, errorDescription
//
Expand All @@ -132,7 +132,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
}
};

private cleanUrlAfterRedirect(url: string): void {
private async cleanUrlAfterRedirect(url: string): Promise<void> {
const cleanedUpUrl = new URL(url);
cleanedUpUrl.searchParams.delete("state");
// For auth code flow
Expand All @@ -145,11 +145,16 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
cleanedUpUrl.searchParams.delete("error_description");
cleanedUpUrl.searchParams.delete("iss");

// Remove OAuth-specific query params (since the login flow finishes with
// the browser being redirected back with OAuth2 query params (e.g. for
// 'code' and 'state'), and so if the user simply refreshes this page our
// authentication library will be called again with what are now invalid
// query parameters!).
window.history.replaceState(null, "", cleanedUpUrl.toString());
return new Promise<void>((resolve) => {
window.addEventListener("popstate", () => {
resolve();
});
// Remove OAuth-specific query params (since the login flow finishes with
// the browser being redirected back with OAuth2 query params (e.g. for
// 'code' and 'state'), and so if the user simply refreshes this page our
// authentication library will be called again with what are now invalid
// query parameters!).
window.history.replaceState(null, "", cleanedUpUrl.toString());
});
}
}

0 comments on commit 0fde58d

Please sign in to comment.