fix(tests/redirection): check HTTPS redirects properly#495
Open
fix(tests/redirection): check HTTPS redirects properly#495
Conversation
…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).
caugner
changed the title
caugner
changed the title
LeoMcA
requested changes
Apr 17, 2026
| // 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 && |
Member
There was a problem hiding this comment.
Could we add a test for the "route is 1" condition here?
| 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 = []; |
Member
There was a problem hiding this comment.
Is this possible? Wouldn't a site returning a http response have at least one value in this array with a 200 status?
| // use the http redirect chain | ||
| retrievals.responses.httpRedirects = httpSession.redirectHistory; | ||
| retrievals.responses.httpsRedirects = httpSession.redirectHistory; | ||
| retrievals.responses.httpsRedirects = httpsSession.redirectHistory; |
Member
There was a problem hiding this comment.
Can we add a test for this change specifically?
| assert.isFalse(res.pass); | ||
| }); | ||
|
|
||
| it("uses https redirects as fallback for destination and route when http redirects are empty", function () { |
Member
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
httpsRedirectsrevealed three latent bugs in the redirection tests:Wrong chain for pass/fail when
httpRedirectsis empty — thehttpRedirects OR httpsRedirectsfallback was used for both display and all pass/fail checks. With an emptyhttpRedirects, the off-host and initial-redirection checks silently evaluated against anhttps:URL forroute[0], always falling through to a pass.Vacuous truth in
allRedirectsPreloaded—[].every(...)returnstrue, so an emptyhttpRedirectscould produceRedirectionAllRedirectsPreloadedwhen 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