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

Fix issue where query strings in entry points mess up entryPoint URLs #92

Merged
merged 1 commit into from
Jun 8, 2015
Merged

Fix issue where query strings in entry points mess up entryPoint URLs #92

merged 1 commit into from
Jun 8, 2015

Conversation

brianhartsock
Copy link
Contributor

Noticed an issue where you a query string in an entrypoint will not be handled correctly. For example:

{
   entryPoint: 'https://exampleidp.com/path?key=value'
}

would end up causing a URL such as https://exampleidp.com/path?key=value?SAMLRequest=...

The existing tests were a little complicated for me to figure out where to throw some tests to validate behavior. I decided to write some simple tests separately to expose the issue and validate this fixes it.

Let me know if there is anything else you would like to see.

@ploer
Copy link
Contributor

ploer commented May 24, 2015

Hmm, never thought of the use case of having an entrypoint with a query parameter already in it. Thanks for the PR!

My once initial concern is that this change still uses querystring to generate the signature around line 250. It probably works, but it seems brittle to generate the signature with one codepath, but then actually use the output of a differnet codepath.

Can you update that to use url as well? (and just strike the require for querystring)

@brianhartsock
Copy link
Contributor Author

url actually uses querystring under the hood and can't format just the query string. Since you are only signing the SAML message portion of the query string and not the full URL, I think switching to sign the entire URL would break things.

I could be wrong, I haven't dug into the signing logic too much yet. Thoughts?

@brianhartsock
Copy link
Contributor Author

@ploer Following up on this thread. Thoughts on my last comment?

@ploer
Copy link
Contributor

ploer commented Jun 8, 2015

Okay, I think that makes sense. Will merge.

ploer added a commit that referenced this pull request Jun 8, 2015
…y_points

Fix issue where query strings in entry points mess up entryPoint URLs
@ploer ploer merged commit 4a91fce into node-saml:master Jun 8, 2015
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.

None yet

2 participants