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

[msal-browser] Fix encoding issues #1852

Merged
merged 6 commits into from Jun 30, 2020
Merged

Conversation

pkanher617
Copy link
Contributor

@pkanher617 pkanher617 commented Jun 29, 2020

This PR fixes a couple issues

  • Cookies were not encoded before being saved, which resulted in cookie key:value errors when setting b64 values with trailing = characters.
  • State was being double encoded in some cases on the server-side, adding an additional decodeURIComponent should fix those issues.

@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package labels Jun 29, 2020
@pkanher617 pkanher617 changed the title Fix url encoding msal browser [msal-browser] Fix encoding issues Jun 29, 2020
Copy link
Contributor

@jmckennon jmckennon left a comment

Choose a reason for hiding this comment

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

Looks good, but if state is (arbitrarily?) being double-encoded on the backend... shouldn't we look to get that fixed rather than double decode as well? Seems odd. What if they end up triple encoding something someday?

@tnorling
Copy link
Collaborator

Looks good, but if state is (arbitrarily?) being double-encoded on the backend... shouldn't we look to get that fixed rather than double decode as well? Seems odd. What if they end up triple encoding something someday?

Agree with this. Should we maybe check for a certain property to know we're done decoding?

@jasonnutter
Copy link
Contributor

Looks good, but if state is (arbitrarily?) being double-encoded on the backend... shouldn't we look to get that fixed rather than double decode as well? Seems odd. What if they end up triple encoding something someday?

Yes, we should get this fixed on the server, but for now, I think it is fine to assume things will at most be encoded twice.

@pkanher617 pkanher617 changed the base branch from bug-bash-samples to dev June 29, 2020 22:04
@coveralls
Copy link

coveralls commented Jun 29, 2020

Coverage Status

Coverage decreased (-0.06%) to 79.218% when pulling 3b880f3 on fix-url-encoding-msal-browser into 532a9a4 on dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package msal-common Related to msal-common package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants