Get 500 error if just go to wp-login.php?saml_acs

SAML-Toolkits / wordpress-saml

OneLogin SAML plugin for Wordpress

MIT License

65 stars 74 forks source link

Get 500 error if just go to wp-login.php?saml_acs #36

Closed tuanmh closed 7 years ago

tuanmh commented 7 years ago

We've been receiving 500 error notifications when Google bot browses to: wp-login.php?saml_acs using a non-POST request.

As we only support HTTP POST Binding, I believe it would be best to just skip if saml_acs is presented and the request is non-POST methods.

What do you think @pitbulk?

pitbulk commented 7 years ago

Is easier to set a restriction on the robots.txt file to that url in order to avoid that google bot index that url.. similar with onelogin endpoints.

tuanmh commented 7 years ago

While I believe a restriction on robots.txt would do the job, I think the problem should be solved at plugin level:

Everyone can go to that url not just Google bots, plus I wouldn't ask users to configure something extra (although this is not a show stopper) in order to use this plugin. It's another unnecessary step that would be easily handled by the plugin.
500 is not appropriate status code in this case. It's clearly not an internal server error not to mention that why would you intercept a request that's not your interests? The message is a false negative and would further distract people from the actual error messages.

It would be a simple fix, it's really up to you to ask for another step in setting this plugin!

pitbulk commented 7 years ago

So what should the plugin do? Present a white page and no throw an exception? We should inform the user about what happened, right?

tuanmh commented 7 years ago

So what should the plugin do? Present a white page and no throw an exception? We should inform the user about what happened, right?

It will just return login page without any error.

We should inform the user about what happened, right?

I would leave it as whatever it would show, e.g the login page as I said above. Again, since this plugin listens on POST requests with saml_acs parameter in URL, so it is not necessary to intercept, throw anything in this case. Given said that, I'm not totally against your suggestion, yes you can show something to users, as long as we don't throw 500 error! What do you think?

pitbulk commented 7 years ago

On the admin area there is a way to show messages to the user, but it seems there is no official way to prompt message to users on the front-end page.. and I'm worried about inject html to some action executed on the front-end because it can break the style of the page.

What do you think about the following approach?

Capture 500 and other saml errors and send them to a intermediary page, where the message is showed.. and a redirect to the homepage after 10 seconds (so user has time to see what went wrong).

tuanmh commented 7 years ago

On the admin area there is a way to show messages to the user, but it seems there is no official way to prompt message to users on the front-end page.. and I'm worried about inject html to some action executed on the front-end because it can break the style of the page.

What do you think about the following approach?

Capture 500 and other saml errors and send them to a intermediary page, where the message is showed.. and a redirect to the homepage after 10 seconds (so user has time to see what went wrong).

@pitbulk: I would say it's quite complicated for this issue. Let's step back, users wouldn't go to that URL unless they know or they're bots - so I think your idea would be too much for this, e.g. spending all the effort to build fancy stuff for such a small audience while it indeed just needs to suppress the error and not return 500 error.

tuanmh commented 7 years ago

@pitbulk: what do you think?

pitbulk commented 7 years ago

I think if we catch the 500 (not sending an exception) and log the error gonna be ok