success_url should be forced to be https

[jdevalk]

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]

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]

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

[georgestephanis]

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:

application-passwords/class.application-passwords.php

Lines 441 to 455 in 6a1e9ef

	<h1><?php esc_html_e( 'Authorize Application' ); ?></h1>
	<div class="card js-auth-app-card">
	<h2 class="title"><?php esc_html_e( 'An application would like to connect to your account.' ); ?></h2>
	<?php if ( $app_name ) : ?>
	<p>
	<?php
	// translators: application name.
	printf( esc_html__( 'Would you like to give the application identifying itself as %1$s access to your account? You should only do this if you trust the app in question.' ), '<strong>' . esc_html( $app_name ) . '</strong>' );
	?>
	</p>
	<?php else : ?>
	<p><?php esc_html_e( 'Would you like to give this application access to your account? You should only do this if you trust the app in question.' ); ?></p>
	<?php endif; ?>
	<form action="<?php echo esc_url( admin_url( 'admin-post.php' ) ); ?>" method="post">

-- 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]

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]

@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]

A basic wp_die form of this was implemented in WordPress/wordpress-develop@bd57332.