1847918 - HTTPS-only mode is ignored when reopening closed tab

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[6k64x4ma]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/117.0

Steps to reproduce:

1. Set dom.security.https_only_mode to true.
2. Go to http://http.badssl.com/ in the new tab. Secure Site Not Available page appears.
3. Close the tab (ctrl + w).
4. Reopen the tab (ctrl + shift + t).

Actual results:

http.badssl.com is loaded.

Expected results:

Loading HTTP site should be blocked.

Regression window
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=29a59c4a2f6ab17b86054f5d8c0d18893b0da18a&tochange=b242365f7a2f1101b3edf116b24ba5a2bac9bc7b

Comment 1

[Malte Jürgens [:maltejur]]

I can not reproduce this on my end. I tested the latest Nightly, and mozilla-central builds from 2023-06-10 (before the commits you linked) and 2023-06-12 (after the commits you linked). I will attach a screen recording of my attempt.

Just to confirm, did you click the "Continue to HTTP Site" button? Because if you did, what you describe would be the expected behavior.

Comment 2

[Malte Jürgens [:maltejur]]

Attachment: Kooha-2023-08-09-13-51-34.webm

Comment 3

[6k64x4ma]

I didn't click the button.

On the latest Nightly with a new profile, I followed the exact same steps as you did in the screen recording, but http.badssl.com is loaded.

Comment 4

[Malte Jürgens [:maltejur]]

I can reproduce the problem now. I tested the wrong date because I misread it, it should be 2023-07-12, not 2023-06-12, and it seems I did something wrong when testing it with Nightly originally, my bad.

Comment 5

[Malte Jürgens [:maltejur]]

Thanks a lot for the report. This seems to be the code causing the page to load here.

Comment 6

[Malte Jürgens [:maltejur]]

The same issue is also happening with history navigation, so for example:

1. Set dom.security.https_only_mode to true
2. Open http://http.badssl.com
3. In the same tab, open https://google.com
4. Press the back button

Result: http://http.badssl.com is loaded via HTTP.
Expected: http://http.badssl.com should be prevented from loading.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1839612

Comment 8

[Malte Jürgens [:maltejur]]

Attachment: Bug 1847918 - Limit `nsDocShellLoadState::IsExemptFromHTTPSOnlyMode` to HTTPS-First r?freddyb

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Tom, IIUC, Freddy is on PTO at the moment. Do you have cycles to review this given that 117 goes to RC next week?

Comment 10

[Tom Ritter [:tjr]]

I'm sorry, with Freddy and Christoph out I think we're going to have to wait - I don't feel comfortable reviewing this.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Not likely to make Fx117 before it goes to RC in that case. We'll keep it on the radar for a dot release at least.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1839612

Comment 13

[Pascal Chevrel:pascalc]

Malte, are you working on this one for a potential uplift or should it ride the trains?

Comment 14

[Malte Jürgens [:maltejur]]

I probably want to get this uplifted, but it has to be reviewed first.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0dd2b360ae24
Limit `nsDocShellLoadState::IsExemptFromHTTPSOnlyMode` to HTTPS-First r=freddyb

Comment 16

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/0dd2b360ae24

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:mjurgens, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox118 to wontfix.

For more information, please visit BugBot documentation.

Comment 18

[Malte Jürgens [:maltejur]]

Attachment: Bug 1847918 - Limit `nsDocShellLoadState::IsExemptFromHTTPSOnlyMode` to HTTPS-First

nsDocShellLoadState::IsExemptFromHTTPSOnlyMode is currently only used by HTTPS-First. It is used for fixing upgrade-downgrade loops and when loading history entries, as when we already know if HTTPS-First succeeded there or not, we have no need for trying to upgrade again and can disable HTTPS-First. With the changes introduced by Bug 1839612, nsDocShellLoadState::IsExemptFromHTTPSOnlyMode also applies to HTTPS-Only, which is a problem because disabling HTTPS-Only for history entries will result in them potentially being loaded insecurely without the user setting an exception. As a solution this patch just applies nsILoadInfo::HTTPS_ONLY_EXEMPT_NEXT_LOAD, the flag being set when nsDocShellLoadState::IsExemptFromHTTPSOnlyMode is set, when HTTPS-First is enabled, and renames both flags to reflect that behavior.

Original Revision: https://phabricator.services.mozilla.com/D185829

Comment 19

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: none
• Code covered by automated testing: yes
• Risk associated with taking this patch: Medium
• Is Android affected?: yes
• Steps to reproduce for manual QE testing: -
• User impact if declined: In the case of the user loading a HTTPS-Only error page from history, the error page will be skipped and Firefox will load the insecure site without asking the user.
• Fix verified in Nightly: no
• Needs manual QE test: no
• Explanation of risk level: The fix is limiting a certain flag to only have an effect when HTTPS-First, not HTTPS-Only is enabled. That flag is spread out arround the HTTPS-Only code, so if I missed a occurence of the flag being used in some edgecase, that coud possibly result in another HTTPS-Only or HTTPS-First regression.

Comment 20

[Pascal Chevrel:pascalc]

Comment on attachment 9352858 [details]
Bug 1847918 - Limit nsDocShellLoadState::IsExemptFromHTTPSOnlyMode to HTTPS-First

That's a S3 and a large patch with some risk of causing regressions, I think we should just let it ride the train as we are uilding our last beta tomorrow, we probably wouldn't have time left to find regressions before we ship 118, thanks.