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

Create a UI to force HTTPS if it's supported #17

Closed
7 of 9 tasks
kienstra opened this issue Jun 26, 2018 · 53 comments · Fixed by #36
Closed
7 of 9 tasks

Create a UI to force HTTPS if it's supported #17

kienstra opened this issue Jun 26, 2018 · 53 comments · Fixed by #36
Assignees
Labels
Milestone

Comments

@kienstra
Copy link
Contributor

kienstra commented Jun 26, 2018

Please see the "Revised ACs" comment for the ACs, instead of this comment.

Hi @westonruter and @postphotos,
What do you think about these ACs? Feel free to edit this.

User Story: As a WordPress administrator, I want a UI to ensure that my site uses HTTPS if it's supported.

  • AC1. Use the HTTPS detection logic from Issue Add reliable method for detecting HTTPS support #6
  • AC2. On the General Settings page, add a <tr> for HTTPS support (use JavaScript to move it after the home/site URLs)
  • AC3. If HTTPS is supported, but the WordPress Address (URL) and Site Address (URL) don't use HTTPS, display this notice:

possible-notice

  • AC4. On clicking "Click here," the WordPress Address (URL) and the Site Address (URL) change to HTTPS, and there's a redirect from HTTP to HTTPS
  • AC5. If HTTPS is not supported, display:

not-supported

  • AC6. Clicking "Recheck" triggers the loopback request from Add reliable method for detecting HTTPS support #6. Maybe there should be a notice at the top, like "HTTPS is still not supported" or "HTTP is supported."

  • AC7. If HTTPS is supported, and the WordPress Address (URL) and Site Address (URL) use HTTPS, display:

https-supported

@kienstra kienstra changed the title Use HTTPS detection from #6 to force HTTPS Create a UI to force HTTPS if it's supported Jun 26, 2018
@westonruter
Copy link
Collaborator

Instead of “click here” I think we should be using links with descriptive text like “re-check” or “set HTTPS”. Also, buttons may be more appropriate than links in some cases.

AC6

Instead of “not supported” and “supported” I think it should be “Support detected” and “Support not detected”.

There should probably be some link off to somewhere to get more information about HTTPS if support is not detected.

AC4

When support is detected, there should then be a checkbox presented to force the redirection from HTTP to HTTPS, in addition to changing the home/site URLs from HTTP to HTTPS. I think that this should be done without any page reloads because the user may have changed another setting on the page and it could be lost otherwise. So I think that clicking the “Check” button should do an Ajax request to check for support (even using the same request client-side that is being made via cron, which could have the added benefit of using the user's SSL certs). Then when that Ajax request returns affirmatively, then the table row with the checkbox could be shown. Upon clicking the checkbox, then JS can be used to update the home/site URL inputs to use HTTPS. Once the user then saves the changes, then the HTTPS redirection would be put in place at that point. Note that the user may end up getting prompted to re-login at this point if the admin also switched from HTTP to HTTPS.

@westonruter
Copy link
Collaborator

This is essentially implementing a UI for the constant described in wp#28521:

As per this post on make/core and its comments, we should introduce a new constant which becomes the iron-fisted ruler of HTTPS, imposing its might everywhere it can.

@kienstra
Copy link
Contributor Author

kienstra commented Jun 26, 2018

Revised ACs

Hi @westonruter,
Thanks, you make some good points. How does this look?

These ACs add your suggestions, though the AC numbers don't map to the previous ones.

AC1. Use the HTTPS detection logic from Issue #6

AC2. On the General Settings page, add a <tr> for HTTPS support after the home/site URLs (though it's shown differently below)

AC3. When support is detected, and the WordPress Address (URL) and Site Address (URL) also use HTTPS, display:
https-support-detected-wordpress-address

AC4. When support is not detected, display:
https-not-supported

AC5. Clicking "Re-check" triggers an AJAX request, not a page reload

AC6. If it then detects HTTPS support, and the WordPress Address (URL) and Site Address (URL) don't use HTTPS, display:
supports-https-not-using

AC7. This can also display on initially loading the General Settings page, if HTTPS is supported, but the WordPress Address (URL) and Site Address (URL) don't use HTTPS

AC8. On clicking the checkbox, the WordPress Address (URL) and Site Address (URL) are changed via JavaScript in the <input> fields from HTTPS to HTTP (even before clicking “Save Changes”). No request is made to change the option in the background before clicking Save Changes.

AC9. On clicking "Save Changes," the setting to redirect is saved, and the redirection is in effect

AC10. A wp-config.php can define WP_SITEURL and WP_HOME constants which will override whatever you supply in the admin. The inputs become disabled. We should make sure that this use case is accounted for. These constants are supplied for the siteurl and home options via filters. So when checking the checkbox to opt-in to HTTPS, it would probably be a good idea to use a filter to apply that change (at priority 11) instead of actually updating the underlying options.

AC11. If someone wants to turn off HTTPS support, for some reason, there should be a way to do that. So unchecking the checkbox can suspend the addition of the filter that forces the home/site URLs to be HTTPS.

@westonruter
Copy link
Collaborator

AC2. On the General Settings page, add a for HTTPS support to the bottom (there's no filter to add it higher on the page)

Use JavaScript to move it to the correct location, immediately following the home/site URLs.

AC3. When support is detected, and the WordPress Address (URL) and Site Address (URL) also use HTTPS

Don't you mean when they are not using HTTPS?

AC8. On clicking the checkbox, the WordPress Address (URL) and Site Address (URL) are changed from HTTPS to HTTP (even before clicking “Save Changes”)

Changed via JavaScript in the input fields. No request is made to change the option in the background before clicking Save Changes. Note, however, that a wp-config.php can define WP_SITEURL and WP_HOME constants which will override whatever you supply in the admin. The inputs become disabled. We should make sure that this use case is accounted for. These constants are supplied for the siteurl and home options via filters. So when checking the checkbox to opt-in to HTTPS, it would probably be a good idea to use a filter to apply that change (at priority 11) instead of actually updating the underlying options.


If someone wants to turn off HTTPS support, for some reason, there should be a way to do that. So unchecking the checkbox can suspend the addition of the filter that forces the home/site URLs to be HTTPS.

@kienstra
Copy link
Contributor Author

Hi @westonruter,
Thanks for your responses.

Use JavaScript to move it to the correct location,

Sounds good, I updated AC2 above.

(AC3) Don't you mean when they are not using HTTPS?

I was thinking of when HTTPS is supported, and the home/site URLs also use HTTPS. In that case, there would be no need for a notice, or a checkbox to force HTTPS. Maybe there would be no need for an 'HTTPS Support' <tr> at all, though.

(AC8) Changed via JavaScript in the input fields. No request is made to change the option in the background before clicking Save Changes.

Thanks, I updated AC8 with that.

Note, however, that a wp-config.php can define WP_SITEURL and WP_HOME constants which will override whatever you supply in the admin...

Thanks, I added that verbatim as AC10.

If someone wants to turn off HTTPS support, for some reason, there should be a way to do that. So unchecking the checkbox can suspend the addition of the filter that forces the home/site URLs to be HTTPS.

Thanks, I added that verbatim as AC11.

@westonruter
Copy link
Collaborator

I was thinking of when HTTPS is supported, and the home/site URLs also use HTTPS.

If the current site URL is HTTP, then the notice should be displayed for sure. If that's what you're saying then yes, agreed. If the site's URLs are currently HTTPS, then the notice maybe wouldn't need to be displayed except for being able to turn off HTTPS. So in that way, checking if home or siteurl don't start with https:// then the notice table row should always be shown.

@kienstra
Copy link
Contributor Author

Hi @westonruter,

If the current site URL is HTTP, then the notice should be displayed for sure.

Thanks, that's what I was thinking.

If the site's URLs are currently HTTPS, then the notice maybe wouldn't need to be displayed except for being able to turn off HTTPS.

That sounds good.

@postphotos
Copy link

Hi @westonruter, in #18 (comment) I described a scenario where caution should be placed in moving to full HSTS support.

Perhaps somewhere in this dialogue a prompt to describe details (why their site isn't HTTPS/HSTS ready and what they can do to fix it) could be added?

I'd also recommend @jwold help design a userflow here as well to help make this a tad easier from a usability standpoint.

@kienstra
Copy link
Contributor Author

kienstra commented Jun 27, 2018

Design

Thanks, @postphotos! If there are changes to the requirements here based on design work, it'd be good if they were done soon. I might start developing for this soon.

@kienstra
Copy link
Contributor Author

Update: waiting to develop for this, pending a design discussion on Monday.

@kevincoleman
Copy link

Would be great to also consider adding a help button/link for those who may want to learn what this does (linking out to a post or official docs).

@jwold
Copy link
Contributor

jwold commented Jul 6, 2018

We had a discussion earlier this week and went over the possible UI changes.

Following is a sketch showing our work in progress for displaying the update notice. Feedback is welcome!

updates page

Additional things we could discuss further based on the notes above:

  1. Welcome button - where would we want to put it, and what could it say?
  2. Settings page for PWA - adding an indicator next to the check and re-check button.

@westonruter
Copy link
Collaborator

Instead of “images” it should probably just say “assets”, as the issue will be regarding media (images, videos, audio) and also scripts and stylesheets. Also, it should say “at least X issues” because we won't be able to maintain a comprehensive index of the entire site's URL space and the assets that get requested.

@kienstra
Copy link
Contributor Author

kienstra commented Jul 11, 2018

Possible UI

Hi @jwold and @westonruter,
What do you think of this possible UI for "Settings" > "HTTPS Settings"? The dashboard widget above looks good, and could work along with this.

possible-ui-https-settings

This is based on the discovery document and discussion from #17.

The "Upgrade all mixed content" checkbox would add the upgrade-insecure-requests directive.

Thanks!

@jwold
Copy link
Contributor

jwold commented Jul 11, 2018

This is a great start on a UI and at a high level I think it works.

  1. When it says “you can block these below” what area below is it referring to? That’s not super clear.

  2. For the “upgrade all mixed content” and “HTTPS Support” checkboxes, are those both reversible options? For instance, if I checkmark them and save, and then come back later and uncheckmark. Is that a real life use case? I’m wondering if a radio button or a different type of button might make more sense here.

  3. Above the active mixed content and passive mixed content areas I wonder if we could have a headline. Something like… (and this is a bad example!) “It’s important for your site to support HTTPS because it’s super awesome and will change your life. Below are examples that could cause errors in your browser.

  4. The problem I’m struggling with when coming to this page is knowing what to take action on, it’s a lot to do. Now, if this page is intended for a developer, then maybe it’s fine! I’m thinking more the use case of a site owner without a lot of technical background.

@kienstra
Copy link
Contributor Author

kienstra commented Jul 11, 2018

Thanks, Great Points

Hi @jwold,
Thanks a lot for your ideas.

  1. That refers to the "Upgrade all mixed content" checkbox. You're right, the wording should be better in "you can block those..."
  2. Good question about whether these are reversible.
  • "upgrade all mixed content" is reversible
  • "redirect from HTTP to HTTPS" could be reversible
  • "Change the home/site URLS" is reversible, but on the "Settings" > "General Settings" page (it changes the value to HTTPS):

site-url

  1. Good idea to have a descriptive headline
  2. Good point...it's not clear what to do. Maybe we could have a flow of actions based on the "Scan now" button:
  • If there's no or only passive insecure content, a notice appears that recommends selecting "Change the home/site URLS..."
  • If there's active insecure content, display the insecure URLs. And recommend looking at them more or using another plugin to fix them.
    The display of URLs might be too much information for non-developers.

@westonruter
Copy link
Collaborator

Two more considerations:

  • I wonder if during an initial install we can detect if HTTPS is available and set the proper URL when first setting up a site from scratch.
  • We should show a warning on the dashboard if the site is not being served as HTTPS and direct the user to the settings screen for enabling it.

These could go into separate issues perhaps.

@kienstra
Copy link
Contributor Author

Initial install, warning in dashboard

Hi @westonruter,
Good idea to have HTTPS detection on the initial install. And to have a warning on the dashboard to direct to this settings page.

@kienstra
Copy link
Contributor Author

kienstra commented Jul 13, 2018

New Proposed UI

Hi @jwold and @westonruter,
What do you think of this new UI? It might be part of an existing "Settings" page, as it doesn't have many options now. The Dashboard > WordPress Updates page could be good, like @jwold suggested above.

This UI makes recommendations, where the previous one I proposed was more for developers.

It uses a loopback request to the homepage (with HTTPS forced), and a match for insecure URLS (using output buffering). This would happen before the page loads, like the AMP validator does for the post editor.

Here are the settings pages for when a site isn't using using HTTPS, based on what kind of mixed content it finds. The first 3 screenshots are for when the HTTPS request succeeds, and the last is for when it fails.

Thanks to 10up/sketchpress for the great templates and elements that this uses.

  1. No insecure content
    pwawp-17-no-insecure-content-jpg

  2. Only passive insecure content
    pwawp-17-only-passive-insecure-jepg

  3. Active and passive insecure content
    pwawp-17-active-and-passive-insecure-jpeg

  4. HTTPS request fails
    pwawp-17-no-https-support-jpeg

@jwold
Copy link
Contributor

jwold commented Jul 13, 2018

Great work Ryan! Got a few questions

https://v.usetapes.com/krqCZHieFT

@kienstra
Copy link
Contributor Author

@jwold, thanks for your great points. I'm working on a response now.

@kienstra
Copy link
Contributor Author

kienstra commented Jul 13, 2018

Hi @jwold,
Thanks for your great ideas. The numbers below are for the screenshot numbers above.

  1. Selecting "Yes" for "HTTPS Upgrade" is reversible. How does this new screenshot look? It uses your idea of radio buttons, and also has different text.

pwawp-17-https-settings-no-insecure-2-jpeg

  1. Here's a similar change to the one above:

pwawp-17-3-only-passive-jpeg

  1. The links for active and passive insecure content would simply go to the URL. For example:
<a href="http://example.com/foo">http://example.com/foo</a>

If it's alright, I'll reply later about where the "More details" links go to.

@kienstra
Copy link
Contributor Author

Thanks, good idea to use an expanding accordion.

@westonruter
Copy link
Collaborator

@jwold Thoughts on why Reading Settings is the best place for the HTTPS settings? I think it makes more sense adjoined to the “WordPress Address (URL)” and “Site Address (URL)” settings on the General Settings screen since it is inherently tied to the URL, and enabling HTTPS will impact the URL.

@jwold
Copy link
Contributor

jwold commented Jul 27, 2018

@westonruter that was the initial thought I had, put it next to a place that affects the site URL. In chatting with Leo and Ryan we felt that Reading represented the viewing interaction that would happen with the site. For instance, we have “discourage search engines” there. Now that I say it that sounds a tad weak.

@postphotos or @kienstra can you weigh in with what you were thinking as well?

@kienstra
Copy link
Contributor Author

kienstra commented Jul 27, 2018

Which Settings Page?

Hi @jwold,
I thought that the "Reading Settings" page would be the best settings page, but still not a great one. The HTTPS section could be pretty big to add to the main "General Settings" page.

Especially if we're listing the invalid URLs (see screenshot #3 here). Or if we have a recheck button with notices.

But HTTPS probably matches the context of the "General Settings" page better than any other page.

@westonruter
Copy link
Collaborator

Can some of the elements be hidden behind an expandable container? I'm thinking that once you set HTTPS that usually you won't really need to change the settings again. Having an expandable section under the URL settings to reveal the HTTPS settings could be a good solution. If HTTPS is not enabled the section could be expanded since users need to really enable it more than ever. But once enabled the section could be collapsed, and once there are HTTPS issues discovered then there could be a red indication with a count for the number of issues which are revealed upon expansion.

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Jul 27, 2018

My first thoughts were to have it on the "General Settings" too. As @westonruter pointed out, the site's URLs are there. You get a sense of whether you have HTTP or HTTPS by that screen.

+1 for expandable container for all the extra details, such as a list of invalid URLs. We may want to have a more intuitive UI within that container that provides more contextual information, such as what the list is, why it's there, and what to do to resolve each.

@postphotos
Copy link

Here are the UI/User flow notes from my call today with @jwold:

  1. We grouped the enable HTTPS and HSTS (see Add support for HTTP Strict Transport Security (HSTS) #18) toggles together, and grouped insecure content and its errors together.
  2. We conditionally surfaced the errors (showing a few, as well as a count of how many) when the plugin finds them.

Questions that remain:
3) @kienstra, do we need to hide the insecure content fixes (Your "item 2" here)?

screenshot 2018-07-31 at 12 28 50 pm
screenshot 2018-07-31 at 12 30 06 pm
screenshot 2018-07-31 at 12 29 55 pm
screenshot 2018-07-31 at 12 30 52 pm

Let us know if there's enough fidelity here for you @kienstra. I'm also happy to work with you on the language here.

@kienstra
Copy link
Contributor Author

kienstra commented Aug 1, 2018

Mockups, HSTS

Hi @postphotos,
Thanks a lot for your mockups.

What do you think of enabling HSTS automatically if the user has checked "HTTPS Upgrade" and "Upgrade Insecure URLs"? And not having a checkbox for it.

It looks like HSTS has a similar effect on insecure content as the upgrade-insecure-requests header (which checking "Upgrade Insecure URLs" adds).

If a script hosted on the same domain has a HTTP URL, it forces it to HTTPS. Though upgrade-insecure-requests also applies to requests outside the domain.

HSTS can cause issues if the HTTPS request somehow fails. There's no fallback:

If the security of the connection cannot be ensured (e.g. the server's TLS certificate is not trusted), show an error message and do not allow the user to access the web application.[12]

But this is something we'd probably have to handle even if the user checked an "HSTS" box, and HTTPS later failed.

We might do this:

...setting the max-age to 0 (over a https connection) will immediately expire the Strict-Transport-Security header, allowing access via http.

@kienstra
Copy link
Contributor Author

kienstra commented Aug 1, 2018

Insecure Content Checkbox

Hi @postphotos,

  1. @kienstra, do we need to hide the insecure content fixes (Your "item 2" here)?

I think it'd be good to hide the "Upgrade Insecure URLs" checkbox if this only finds passive insecure content, if we can get a good enough way to find which content is passive or active.

There wouldn't be an error in the browser anyway, only a warning in the JS console. And upgrading them could cause them to fail if the HTTPS requests fail.

@kienstra kienstra assigned kienstra and unassigned jwold Aug 1, 2018
@kienstra
Copy link
Contributor Author

kienstra commented Aug 2, 2018

Current UI From PR #36

  1. HTTPS supported, no insecure content:

https-supported-no-insecure-content

  1. HTTPS supported, only passive insecure content

https-only-passive-insecure

  1. HTTPS supported, passive and active insecure content

https-passive-and-active

@kienstra
Copy link
Contributor Author

kienstra commented Aug 2, 2018

Maybe Remove "Upgrade Insecure URLs" Checkbox

Based on some feedback in the sync today, maybe we could remove the "Upgrade Insecure URLs" checkbox. And automatically upgrade the insecure requests, only if there is active insecure content on the homepage.

Normally, the risk in upgrading insecure requests is that when a HTTP URL is upgraded to HTTPS, it could fail. And there's no fallback.

But if there are active insecure URLs, the browser already blocks them. So they're failing already, and upgrading them at least gives them a chance.

The main reason to have the checkbox, instead of upgrading all requests by default, is to prevent upgrading passive insecure URLs that might fail.

For example, the src of an <img> is upgraded from HTTP to HTTPS, and fails. The image doesn't display, but it wouldn't have caused a browser error if it wasn't upgraded.

So this wouldn't upgrade insecure requests unless there's active insecure content.

This could still display the insecure URLs if there's active insecure content, like in screenshot 3 above.

@kienstra
Copy link
Contributor Author

kienstra commented Aug 3, 2018

Current UI For Active Insecure Content

active-insecure-content

@jwold
Copy link
Contributor

jwold commented Aug 13, 2018

@kienstra just synced with Leo and we propose the following updated UI. It removes sidebars and puts all list items in a bit of a box, with alternating row colors and a button to load more. Please let us know if any further questions. Thanks!

img_e2ffbb83764e-1

@kienstra
Copy link
Contributor Author

kienstra commented Aug 13, 2018

Thanks For The UI Above

Hi @jwold,
Thanks for the updated UI, and it was great to see you last week 😄

  1. How well do you think the blue/white row colors would fit in on the General Settings page? It looks like there isn't a pattern like this in any of the Core settings pages, and they're all relatively simple. But I'm not a designer 😄

Here's the current UI:
scroll-through-insecure-urls

  1. Is the scrolling here enough? It's definitely possible to add the "Load more" button.

But I don't think there will be more than 50 or 100 insecure URLs very often. It probably wouldn't be common to show those URLs at all. They're only shown if all of the following are true:

  • The WordPress Address (URL) and Site Address (URL) are HTTP
  • The site can support HTTPS (it has a certificate)
  • The user has checked "HTTPS Upgrade"
  • There is active insecure content, like scripts or stylesheets. Not only images.

We've also been discussing the UI in #36.

  1. Could you please elaborate on "removes sidebars"?

Thanks, Joshua!

@postphotos
Copy link

postphotos commented Aug 14, 2018

Just had a mini-workshop w/ @kienstra.

  1. We toned down the striping to look a tad different to match the general settings page look-and-feel. We tried to eliminate the need for a second overflow/scroll on the page (and avoid using an unnecessary AJAX request) and have done so by changing my sketch with @jwold from earlier in the day.

Here's what we came up with:
leo-screenshot

We also moved the number count out of the copy button and into the overflow button.

  1. We discussed this and think it might not be as friendly copy. We've revised this to be: We found content on your site that wasn’t loading correctly over HTTPS. While we will try to fix these links automatically, but you might check to be sure your pages work as expected.

At this point, pending code review for #36 and closing out a few minor tasks left (AC8 from #18 was added here as a related item) I feel this issue will soon be ready for close. Thanks for all the hard work, folks! 🎉

@kienstra
Copy link
Contributor Author

Great ideas on the design here, @postphotos! This looks really good.

@kienstra
Copy link
Contributor Author

Thanks to Leo's design above, and some style rules he added, here's the current UI as applied in #36:

current-state

@jwold
Copy link
Contributor

jwold commented Aug 14, 2018

Really happy with where this landed! A few thoughts:

  1. Adding padding to top and bottom of the urls (2-4 pixels top and bottom) could make it feel a little less cramped.
  2. Love the button change showing the number of urls available. Great job.
  3. What happens when a url is too long, will it just cut off? If so that should be fine. An ellipse could be useful.
  4. No border around the whole thing actually seems to make sense here.
  5. Minor point, what if we did a border-left of 3-5 pixels to indent the entire thing. Curious what @postphotos thinks.

@kienstra
Copy link
Contributor Author

Thanks
Screenshots Applying Suggestions

Hi @jwold,
Thanks for your great suggestions.

Here are what your points 1. and 5. look like. Of course, this would be easy to change pending your suggestions:

styling-ui

  1. The URLs are truncated, with an ellipsis if they're too long:

truncated-ellipsis

@kienstra
Copy link
Contributor Author

Show more button

Hi @ThierryA,
Thanks for suggesting the "Show more" button. It's now applied with 98d3e27:

more-button

@kienstra
Copy link
Contributor Author

Copy Changes

Hi @westonruter,
Here are some copy changes, including your suggestion of using 'Upgrade to secure connection' instead of 'HTTPS Upgrade' (applied in 199f146):

copy-changes

@kienstra
Copy link
Contributor Author

kienstra commented Aug 16, 2018

Should Also Upgrade Passive Mixed Content

Passive mixed content isn't currently upgraded in #36. It's still a security risk, but not as much as scripts or stylesheets.

So it would be better to upgrade all mixed content via Upgrade-Insecure-Requests. And as @amedina mentioned, passive mixed content like HTTP images are blocked by service workers.

The blocker to this before was that there's no fallback if the upgraded URL fails. For example, if an image at http://example.com/foo.jpeg is upgraded to HTTPS and fails.

But @westonruter had a good idea of making requests for all insecure content, at the upgraded HTTPS URL, to ensure the requests don't fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants