search

WordPress / application-passwords

183 stars 44 forks source link

success_url should be forced to be https #116

Open jdevalk opened 3 years ago

jdevalk commented 3 years ago

We should only redirect back to $success_url if it is HTTPS. Otherwise you're exposing the application password over plain HTTP. We might want to add an override for that for testing purposes but then we should output a clear warning on the screen that the success URL is not secure.

kasparsd commented 3 years ago

That’s a great point @jdevalk! Do you suggest we always override the return URL to be HTTPS and have a filter in place to disable the override?

jdevalk commented 3 years ago

No I just wouldn't redirect to it if it isn't https.

georgestephanis commented 3 years ago

I'd initially thought that perhaps instead of accepting a http url, we could just display the credentials to the user, so that they can enter it into the app manually -- however that seems kinda silly as most apps using this flow wouldn't have a place for the user to enter the creds if they were expecting to get them back via a redirect.

Also, if we force it to reject http:// urls, they would never get it back in the first place, so they should have caught that before they ever launched the integration in the first place. So it's kinda silly to bend over backwards to support a pattern that we can easily rule out before we launch.

It should be a simple conditional added somewhere around here:

https://github.com/WordPress/application-passwords/blob/6a1e9efec91d88ca68091dc87f488948d3a99498/class.application-passwords.php#L441-L455

-- just worth noting that if we did, we would need to do it in a way that only rejects urls matching with /^http:/i-- rather than forcing /https:/i -- as we're explicitly allowing alternate protocols for passing data back to apps:

https://developer.android.com/training/app-links https://developer.apple.com/documentation/xcode/allowing_apps_and_websites_to_link_to_your_content/defining_a_custom_url_scheme_for_your_app

TimothyBJacobs commented 3 years ago

How about introducing a wp_is_allowed_application_password_redirect_url() function that would accept a URL and return a WP_Error? By default, we'd reject http, but the function would be filterable.

kasparsd commented 3 years ago

@georgestephanis Are you suggesting we add a warning/notice to that particular admin section if the redirect URL starts with http://? Or do we also disable the "Yes, I approve of this connection." button unless the newly added filter overrides it?

TimothyBJacobs commented 3 years ago

A basic wp_die form of this was implemented in https://github.com/WordPress/wordpress-develop/pull/540/commits/bd57332c6b11368b47fc12b1a8b12eb290dfe42f.