fix(tests/redirection): check HTTPS redirects properly by caugner · Pull Request #495 · mdn/mdn-http-observatory

caugner · 2026-04-10T15:33:58Z

Description

Fixes an assignment in the retriever, where the redirects from the HTTP session were assigned to the HTTPS redirects.

Motivation

With both chains identical, the redirection test logic produced wrong results for any site where the HTTP and HTTPS redirect chains actually differ.

Additional details

Correcting httpsRedirects revealed three latent bugs in the redirection tests:

Wrong chain for pass/fail when httpRedirects is empty — the httpRedirects OR httpsRedirects fallback was used for both display and all pass/fail checks. With an empty httpRedirects, the off-host and initial-redirection checks silently evaluated against an https: URL for route[0], always falling through to a pass.
Vacuous truth in allRedirectsPreloaded — [].every(...) returns true, so an empty httpRedirects could produce RedirectionAllRedirectsPreloaded when the HTTP server didn't redirect at all.
HTTPS chain redirecting back to HTTP went undetected — the "not to HTTPS" check only inspected the end of the HTTP chain. If the independent HTTPS session redirected back to HTTP, it passed.

All three are fixed and covered by new tests.

Related issues and pull requests

…oute After the retriever fix, `httpRedirects` and `httpsRedirects` can differ. Unify both fields under a single `redirects` variable (HTTP preferred, HTTPS fallback) so destination and route always describe the same chain.

Two cases were not covered: - HTTPS chain redirecting back to HTTP while the HTTP chain looked fine - Empty httpRedirects with a live HTTP response producing a spurious pass (vacuous truth on .every() + wrong chain used for pass/fail checks)

Previously `redirects` was computed as httpRedirects OR httpsRedirects (fallback), and then used for both display and all pass/fail logic. This caused three bugs: - When httpRedirects was empty but an HTTP response existed, the OR-fallback ran httpsRedirects through checks designed for an HTTP chain (e.g. route[0].protocol === "http:" is always false for an https: URL), producing a spurious pass. - allRedirectsPreloaded called .every() on an empty httpRedirects array, which vacuously returns true and gave RedirectionAllRedirectsPreloaded for sites with no HTTP redirects. - The "not to HTTPS" check only inspected the end of the HTTP chain. If the independent HTTPS session redirected back to HTTP, it went undetected. Fix: separate httpRoute (used for all pass/fail checks) from displayRoute (httpRoute with httpsRoute fallback, used only for output.destination and output.route). Guard allRedirectsPreloaded with httpRoute.length > 1. Extend the "not to HTTPS" check to also test httpsRoute.at(-1).

LeoMcA · 2026-04-17T14:52:28Z

+    // Check to see if every redirection was covered by the preload list.
+    // Guard httpRoute.length > 1 to avoid vacuous truth on an empty array.
+    const allRedirectsPreloaded =
+      httpRoute.length > 1 &&


Could we add a test for the "route is 1" condition here?

Added in 2c43bbc, using a genuinely preloaded host (cloudflare.com) as a single-entry chain and asserting RedirectionMissing. The existing "redirection missing" test used mozilla.org, which isn't preloaded, so the httpRoute.length > 1 guard was never actually exercised.

LeoMcA · 2026-04-17T14:56:26Z

+  it("fails with redirection-missing when http redirects are empty but http response exists", function () {
+    // The OR fallback previously caused httpsRedirects to be used for pass/fail
+    // logic when httpRedirects is empty, which could produce a spurious pass.
+    reqs.responses.httpRedirects = [];


Is this possible? Wouldn't a site returning a http response have at least one value in this array with a 200 status?

Agree, the interceptor records every response (including the final 200) and responses.http is only set on success, so a non-null responses.http always implies a non-empty httpRedirects. Removed in fd0484e.

LeoMcA · 2026-04-17T14:57:12Z

  // use the http redirect chain
  retrievals.responses.httpRedirects = httpSession.redirectHistory;
-  retrievals.responses.httpsRedirects = httpSession.redirectHistory;
+  retrievals.responses.httpsRedirects = httpsSession.redirectHistory;


Can we add a test for this change specifically?

Added in 38b5007: the live "test retrieve mdn" case now asserts httpsRedirects[0] starts on https:. Before the fix it reused the HTTP chain, which starts on http:.

LeoMcA · 2026-04-17T15:02:11Z

    assert.isFalse(res.pass);
  });

+  it("uses https redirects as fallback for destination and route when http redirects are empty", function () {


I don't think this is possible in practice, if httpRedirects is empty, then responses.http should be undefined, and that's already covered by the first test in the file.

Agree, httpRedirects is empty only if responses.http is null. Rewrote in fd0484e to the real no-HTTP case: responses.http = null, expects RedirectionNotNeededNoHttp, and asserts destination falls back to the HTTPS chain end (the only reachable use of httpsRedirects here).

…fallback An HTTP response is only recorded together with at least one entry in `httpRedirects`, so an empty `httpRedirects` implies `responses.http` is `null`. The `displayRoute` fallback to `httpsRedirects` was therefore dead for `route` (the `else` branch always had `httpRoute === displayRoute`); only the destination fallback for the no-HTTP case is reachable. Compute `destination` inline as `httpRoute.at(-1) ?? httpsRoute.at(-1)`, assign `route` from `httpRoute` directly, and replace the test that relied on the impossible empty-`httpRedirects`-with-HTTP-response state with one covering the real no-HTTP destination fallback.

A redirect chain of length 1 means no redirection happened. Without the `httpRoute.length > 1` guard, `[singlePreloadedHost].every(...)` returns `true` and reports `RedirectionAllRedirectsPreloaded` instead of `RedirectionMissing`. Use a genuinely preloaded host (`cloudflare.com`) so the test fails if the guard is removed.

The retriever now assigns `httpsSession.redirectHistory` to `httpsRedirects`. Assert the chain starts on `https:`; a regression that reuses the HTTP session's history would start on `http:`.

caugner · 2026-07-15T13:41:51Z

+  const httpRoute = requests.responses.httpRedirects;
+  const httpsRoute = requests.responses.httpsRedirects;


@LeoMcA Should we rename http*Redirects and http*Route to avoid confusion altogether?

caugner added 6 commits April 10, 2026 17:32

fix(retriever): assign HTTPS redirects properly

91bffde

refactor(redirection): use .at(-1)

976b935

refactor(tests/redirection): rename response => httpResponse

2f8f43e

caugner changed the title ~~fix(retriever): assign HTTPS redirects properly~~ fix(retriever): test HTTPS redirects properly Apr 16, 2026

caugner changed the title ~~fix(retriever): test HTTPS redirects properly~~ fix(tests/redirection): check HTTPS redirects properly Apr 16, 2026

caugner marked this pull request as ready for review April 16, 2026 12:34

Merge branch 'main' into fix-https-redirects

e26aca6

caugner requested a review from a team as a code owner April 16, 2026 12:34

caugner requested a review from LeoMcA April 16, 2026 12:34

LeoMcA requested changes Apr 17, 2026

View reviewed changes

caugner added 4 commits July 15, 2026 10:58

Merge branch 'main' into fix-https-redirects

ed3d2cd

test(retriever): assert httpsRedirects comes from the HTTPS session

38b5007

The retriever now assigns `httpsSession.redirectHistory` to `httpsRedirects`. Assert the chain starts on `https:`; a regression that reuses the HTTP session's history would start on `http:`.

caugner commented Jul 15, 2026

View reviewed changes

caugner requested a review from LeoMcA July 15, 2026 17:20

caugner mentioned this pull request Jul 15, 2026

Redirection: distinguish HTTPS-downgrade from HTTP redirect failure #557

Open

Navigation Menu

Uh oh!

fix(tests/redirection): check HTTPS redirects properly - #495

fix(tests/redirection): check HTTPS redirects properly#495
caugner wants to merge 11 commits into
mainfrom
fix-https-redirects

caugner commented Apr 10, 2026 •

edited

Loading

Uh oh!

LeoMcA Apr 17, 2026

Uh oh!

caugner Jul 15, 2026

Uh oh!

LeoMcA Apr 17, 2026

Uh oh!

caugner Jul 15, 2026 •

edited

Loading

Uh oh!

LeoMcA Apr 17, 2026

Uh oh!

caugner Jul 15, 2026 •

edited

Loading

Uh oh!

LeoMcA Apr 17, 2026

Uh oh!

caugner Jul 15, 2026 •

edited

Loading

Uh oh!

caugner Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		const httpRoute = requests.responses.httpRedirects;
		const httpsRoute = requests.responses.httpsRedirects;

Uh oh!

Conversation

caugner commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation

Additional details

Related issues and pull requests

Uh oh!

LeoMcA Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

caugner Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

LeoMcA Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

caugner Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeoMcA Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

caugner Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeoMcA Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

caugner Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caugner Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

caugner commented Apr 10, 2026 •

edited

Loading

caugner Jul 15, 2026 •

edited

Loading

caugner Jul 15, 2026 •

edited

Loading

caugner Jul 15, 2026 •

edited

Loading