1911839 - Https-first: Investigate whether commented out code adding upgrade exception is necessary or can be removed

Status: RESOLVED FIXED

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

Description

[Manuel Bucher [:manuel]]

After review, before landing one code path that added an exception was commented out. The test coverage is very good, and it has been in Stable for some time, so probably not much or nothing breaks, but should be investigated if we behave incorrect in some scenarios.

Diff that commented the code out: https://phabricator.services.mozilla.com/D193672?vs=872230&id=872764#toc.

Either remove the code block that is commented out or remove the /* and */ in /dom/security/nsHTTPSOnlyUtils.cpp#412,423

// https-first needs to account for breaking upgrade-downgrade endless
// loops at this point for meta and js redirects because this function
// is called before we
// check the redirect limit in HttpBaseChannel. If we encounter
// a same-origin server side downgrade from e.g https://example.com
// to http://example.com then we simply not annotating the loadinfo
// and returning false from within this function. Please note that
// the handling for https-only mode is different from https-first mode,
// because https-only mode results in an exception page in case
// we encounter and endless upgrade downgrade loop.
/*
bool isUpgradeDowngradeEndlessLoop = IsUpgradeDowngradeEndlessLoop(
    aURI, aURI, aLoadInfo,
    {UpgradeDowngradeEndlessLoopOptions::EnforceForHTTPSFirstMode});
if (isUpgradeDowngradeEndlessLoop) {
  if (mozilla::StaticPrefs::
          dom_security_https_first_add_exception_on_failiure()) {
    nsHTTPSOnlyUtils::AddHTTPSFirstExceptionForSession(aURI, aLoadInfo);
  }
  return false;
}
*/

Comment 1

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

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

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Freddy, can you help find an owner for this issue? Or, Manuel, if you intend to work on it, can you take ownership? Thanks!

Comment 3

[Manuel Bucher [:manuel]]

I took a look now. There exist a test cases describing exactly the scenario and they are passing.

https://searchfox.org/mozilla-central/rev/396a6123691f7ab3ffb449dcbe95304af6f9df3c/dom/security/test/https-first/test_break_endless_upgrade_downgrade_loop.html#25-26

{ query: "downgrade_redirect_meta", result: "http" },
{ query: "downgrade_redirect_js", result: "http" },

The code is not necessary. I verified that the test case working as intended.

Comment 4

[Manuel Bucher [:manuel]]

Attachment: Bug 1911839 - Remove commented out https-first exception mechanism that is no longer required r=maltejur

Commented out in Bug 1747230, but could have been removed instead.

Comment 5

[Frederik Braun [:freddy]]

Thank you, Manuel.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37e963769f63
Remove commented out https-first exception mechanism that is no longer required r=freddyb

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/37e963769f63

Comment 8

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

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

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

For more information, please visit BugBot documentation.