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

Allow wp-login.php query variables to be filtered #35

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jazzsequence
Copy link
Contributor

We have a situation where we need to use unique urls for things that are currently hard-coded as query variables (specifically the assertion URL and the XML metadata urls, but the same could apply to any of the URL query variables).

This PR adds filters to each of the url query variables where they are being used so additional developers can customize the query variables (say to distinguish between dev/prod environments, which is our scenario).

Also tweaked usage of get_site_url where it was being used (specifically with reference to wp-login.php with these query variables applied) to pass the relative path to the actual get_site_url function.

@pitbulk
Copy link
Contributor

pitbulk commented Mar 14, 2017

I dont know if that PR affects the security of the plugin, I may review.

I think there are alternatives to distinguish between dev/prod environments, like use specific SP entity ID (at advanced settings section of the SAML settings) for each environment, rather than create different SAML endpoints.

@jazzsequence
Copy link
Contributor Author

Using a different SP ID is something we're going to do as well, but we've been told that the endpoints need to be unique also.

I can add sanitization so the result of the apply_filters is run through esc_html or something to make sure someone doesn't embed malicious code in there.

@pitbulk
Copy link
Contributor

pitbulk commented Mar 14, 2017

We've been told that the endpoints need to be unique also.

I don't understand what motivate to have different endpoints, but that seems a very custom behavior that rest of the users of the SAML plugin really don't need.

BTW, right now you are able to set different ACS endpoint (using the alternative acs parameter)

@jazzsequence
Copy link
Contributor Author

Using the alternative ACS parameter changes the script that's run, though, doesn't it? We don't want to change how logins are processed, just the URL that's configured in the IdP.

I understand that this is pretty edge case, and that's fine, but I would rather not fork the plugin just to add in some filter hooks if possible.

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