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 SSO URL #114

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

Fix SSO URL #114

wants to merge 1 commit into from

Conversation

mohanjith
Copy link

esc_js makes GET variable inaccessible hence causing SSO URL to 403

@splaquet
Copy link

sweet, thank you! works for me 🙂

@r-a-y
Copy link
Contributor

r-a-y commented Oct 10, 2020

Experienced this as well. PR fixes this problem. Looks like commit 71d024c#diff-510cbcbe891412c4645530eee499b128727b3eebc7845b33c31c2734afac4268L333 broke this.

r-a-y added a commit to r-a-y/Mercator that referenced this pull request Oct 20, 2020
@tomjn
Copy link
Contributor

tomjn commented Aug 22, 2023

Are there any examples of values that should be passed through but get mangled?

@r-a-y
Copy link
Contributor

r-a-y commented Aug 22, 2023

Are there any examples of values that should be passed through but get mangled?

This particular PR fixes the usage of esc_js() in the SSO URL. Using esc_js() will HTML-encode ampersands. So:

http://example.com/wp-admin/admin-ajax.php?action=mercator-sso-login&host=http%3A%2F%2Fexample.com&back=http%3A%2F%2Fexample.com%2Fsample-page&site=5

Would become:

http://example.com/wp-admin/admin-ajax.php?action=mercator-sso-login&host=http%3A%2F%2Fexample.com&back=http%3A%2F%2Fexample.com%2Fsample-page&site=5

This redirect URL when used with window.location in JS fails because of the HTML-encoded ampersand.

Commit 71d024c#diff-510cbcbe891412c4645530eee499b128727b3eebc7845b33c31c2734afac4268L333 was an oversight. Using esc_url_raw() fixes this issue.

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.

4 participants