SAML-Toolkits / wordpress-saml

OneLogin SAML plugin for Wordpress
MIT License
65 stars 73 forks source link

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

Open jazzsequence opened 7 years ago

jazzsequence commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.