Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.

Managed query string parameters in the authorize endpoint. #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Managed query string parameters in the authorize endpoint. #28

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/AuthService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,10 @@ export class AuthService<TIDToken = JWTIDToken> {
client_id: clientId,
post_logout_redirect_uri: redirectUri
}
const url = `${logoutEndpoint || `${provider}/logout`}?${toUrlEncoded(query)}`
const logoutUrl = `${logoutEndpoint || `${provider}/logout`}`;
// Check if the url already contains at least one parameter
const separator = new RegExp('^.*\?.+=.+$').test(logoutUrl) ? '&' : '?';
Copy link

@sidm1983 sidm1983 Feb 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, thank you for this code update. I actually just requested an enhancement for this without realising that this PR existed.

I think we should make a very small tweak to the regex here. Instead of '^.*\?.+=.+$', which will still use a '?' character for a query string parameter with no value, it should be changed to '^.*\?.+=.*$'. (Note the '*' instead of a '+' character before the '$' character.)

This will allow us to also add query string parameters with null/empty values (e.g. ?key=)

Suggested change
const separator = new RegExp('^.*\?.+=.+$').test(logoutUrl) ? '&' : '?';
const separator = new RegExp('^.*\?.+=.*$').test(logoutUrl) ? '&' : '?';

const url = logoutUrl + separator + toUrlEncoded(query);
window.location.replace(url)
return true;
} else {
Expand Down Expand Up @@ -189,7 +192,10 @@ export class AuthService<TIDToken = JWTIDToken> {
codeChallengeMethod: 'S256'
}
// Responds with a 302 redirect
const url = `${authorizeEndpoint || `${provider}/authorize`}?${toUrlEncoded(query)}`
const authorizeUrl = `${authorizeEndpoint || `${provider}/authorize`}`;
// Check if the url already contains at least one parameter
const separator = new RegExp('^.*\?.+=.+$').test(authorizeUrl) ? '&' : '?';
Copy link

@sidm1983 sidm1983 Feb 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
const separator = new RegExp('^.*\?.+=.+$').test(authorizeUrl) ? '&' : '?';
const separator = new RegExp('^.*\?.+=.*$').test(authorizeUrl) ? '&' : '?';

const url = authorizeUrl + separator + toUrlEncoded(query);
window.location.replace(url)
return true
}
Expand Down