Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 33 additions & 28 deletions src/analyzer/tests/redirection.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,50 +44,55 @@ export function redirectionTest(
expectation = Expectation.RedirectionToHttps
) {
const output = new RedirectionOutput(expectation);
const response = requests.responses.http;
const httpResponse = requests.responses.http;

if (requests.responses.httpRedirects.length > 0) {
output.destination =
requests.responses.httpRedirects[
requests.responses.httpRedirects.length - 1
]?.url?.href || null;
} else if (requests.responses.httpsRedirects.length > 0) {
output.destination =
requests.responses.httpsRedirects[
requests.responses.httpsRedirects.length - 1
]?.url?.href || null;
const httpRoute = requests.responses.httpRedirects;
const httpsRoute = requests.responses.httpsRedirects;

// For display only: prefer the HTTP chain, fall back to HTTPS chain when HTTP is absent
const displayRoute = httpRoute.length > 0 ? httpRoute : httpsRoute;

const destination = displayRoute.at(-1)?.url?.href;
if (destination) {
output.destination = destination;
}
output.statusCode = response ? response.status : null;
output.statusCode = httpResponse ? httpResponse.status : null;

if (!response) {
if (!httpResponse) {
output.result = Expectation.RedirectionNotNeededNoHttp;
} else if (!response.verified) {
} else if (!httpResponse.verified) {
output.result = Expectation.RedirectionInvalidCert;
} else {
const route = requests.responses.httpRedirects;
output.route = route.map((r) => r.url.href);
output.route = displayRoute.map((r) => r.url.href);

// Check to see if every redirection was covered by the preload list
const allRedirectsPreloaded = route.every((re) =>
isHstsPreloaded(Site.fromSiteString(re.url.hostname))
);
// 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 &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

httpRoute.every((re) =>
isHstsPreloaded(Site.fromSiteString(re.url.hostname))
);
if (allRedirectsPreloaded) {
output.result = Expectation.RedirectionAllRedirectsPreloaded;
} else if (route.length === 1) {
} else if (httpRoute.length < 2) {
// No redirection, so you just stayed on the http website
output.result = Expectation.RedirectionMissing;
output.redirects = false;
} else if (route[route.length - 1]?.url.protocol !== "https:") {
// Final destination wasn't an https website
} else if (
httpRoute.at(-1)?.url.protocol !== "https:" ||
(httpsRoute.length > 0 && httpsRoute.at(-1)?.url.protocol !== "https:")
) {
// Final destination wasn't https — checked for both the HTTP chain and
// the independent HTTPS chain (catches HTTPS redirecting back to HTTP)
output.result = Expectation.RedirectionNotToHttps;
} else if (route[1]?.url.protocol === "http:") {
} else if (httpRoute[1]?.url.protocol === "http:") {
// http should never redirect to another http location -- should always go to https first
output.result = Expectation.RedirectionNotToHttpsOnInitialRedirection;
output.statusCode = route[route.length - 1]?.status || null;
output.statusCode = httpRoute.at(-1)?.status || null;
} else if (
route[0]?.url.protocol === "http:" &&
route[1]?.url.protocol === "https:" &&
route[0]?.url.hostname !== route[1]?.url.hostname
httpRoute[0]?.url.protocol === "http:" &&
httpRoute[1]?.url.protocol === "https:" &&
httpRoute[0]?.url.hostname !== httpRoute[1]?.url.hostname
) {
output.result = Expectation.RedirectionOffHostFromHttp;
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/retriever/retriever.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export async function retrieve(site, options = {}) {

// use the http redirect chain
retrievals.responses.httpRedirects = httpSession.redirectHistory;
retrievals.responses.httpsRedirects = httpSession.redirectHistory;
retrievals.responses.httpsRedirects = httpsSession.redirectHistory;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for this change specifically?


if (httpsSession.clientInstanceRecordingRedirects) {
retrievals.responses.auto = httpsSession.response;
Expand Down
71 changes: 71 additions & 0 deletions test/redirection.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,77 @@ describe("Redirections", () => {
assert.isFalse(res.pass);
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

reqs.responses.httpRedirects = [];
reqs.responses.httpsRedirects = [
{
url: new URL("https://mozilla.org/"),
status: 301,
},
{
url: new URL("https://www.mozilla.org/"),
status: 200,
},
];

const res = redirectionTest(reqs);
assert.equal(res.destination, "https://www.mozilla.org/");
assert.deepEqual(res.route, [
"https://mozilla.org/",
"https://www.mozilla.org/",
]);
});

it("fails when https redirects back to http even if http redirects look fine", function () {
// HTTP chain correctly redirects to HTTPS on the same hostname
reqs.responses.httpRedirects = [
{
url: new URL("http://mozilla.org/"),
status: 301,
},
{
url: new URL("https://mozilla.org/"),
status: 200,
},
];
// But the independent HTTPS session ends on HTTP — that should be an error
reqs.responses.httpsRedirects = [
{
url: new URL("https://mozilla.org/"),
status: 301,
},
{
url: new URL("http://mozilla.org/"),
status: 200,
},
];

const res = redirectionTest(reqs);
assert.equal(res.result, Expectation.RedirectionNotToHttps);
assert.isFalse(res.pass);
});

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 = [];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

reqs.responses.httpsRedirects = [
{
url: new URL("https://mozilla.org/"),
status: 301,
},
{
url: new URL("https://www.mozilla.org/"),
status: 200,
},
];
// responses.http is still set (from emptyRequests), so the site is reachable over HTTP

const res = redirectionTest(reqs);
assert.equal(res.result, Expectation.RedirectionMissing);
assert.isFalse(res.pass);
});

it("checks for all redirections preloaded", function () {
reqs.responses.httpRedirects = [
{
Expand Down
Loading