Skip to content
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.

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

Add 'session_state' to redirect query removal #67

Conversation

dreamlibrarian
Copy link
Contributor

AzureAD sends code, state, and session_state query parameters along with the redirect, which need to be scrubbed for this function to work with AzureAD oauth.

@dreamlibrarian
Copy link
Contributor Author

I suppose the side question is 'why allow any query parameters in the redirect url at all', but I assume there's a reason.

AzureAD sends code, state, and session_state query parameters along with the redirect, which need to be scrubbed for this function to work.
@dreamlibrarian dreamlibrarian force-pushed the tsmith/automatic-redirect-url-azuread-support branch from d92fdef to 85c5e97 Compare November 19, 2021 23:03
Copy link
Member

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

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

Why allow any query parameters in the redirect url at all?

I don't remember exactly why I did it this way, but I think I chose to keep query parameters because the OAuth spec allows it and I didn't want be overly prescriptive. One possible use is to capture information about the origin page when redirecting someone to authenticate so that you can send them back to the right place after the auth flow is complete.

Removing session_state looks good though, as it's an optional parameter in the spec that I missed.

@bluekeyes
Copy link
Member

Looks like this managed to cause a panic in Policy Bot somehow, so let me see if I can fix that and if not, I'll force merge this.

@bluekeyes bluekeyes merged commit 7642efc into palantir:develop Nov 19, 2021
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.

2 participants