1709150 - Consider linter rule to make `https` the default for all kind of tests we add

Status: RESOLVED FIXED

(Core :: DOM: Security, task, P1)

Description

[Christoph Kerschbaumer [:ckerschb]]

It seems that https is becoming the default on the web, while it's critical to maintain http tests for all sorts of reasons, I think we should make the default for our tests to rely on https.

We should add some kind of linter rule that warns if someone wants to check-in a new tests using http, because in most test scenarios the scheme should not really matter and so using https becomes the better default.

Additionally we are considering to ship some form of https-first mode soon which also would ease that transition period.

Comment 1

[Tomer Yavor]

Attachment: WIP: Bug 1709150 - Consider linter rule to make the default for all kind of tests we add. r=freddyb

Comment 2

[Tomer Yavor]

Attachment: Bug 1709150 - Consider linter rule to make the default for all kind of tests we add. r=freddyb

Comment 3

[Tomer Yavor]

Attachment: Bug 1709150 - Consider linter rule to make https the default for all kind of tests we add. r=freddyb

Comment 4

[Mark Banner (:standard8)]

For the record, I've checked the licenses of the dependencies for @microsoft/eslint-plugin-sdl and found they are MIT, Apache 2.0, ISC & BSD-2-Clause. These are all acceptable for tests.

Comment 5

[Mark Banner (:standard8)]

Whilst I understand the reasoning for doing this, I'm slightly concerned about with the current suggestion of blanket enabling it as a warning - it introduces some 15,000 new warnings across the tree. For comparison, we currently have around 3,700 warnings for existing rules.

Do we have a plan for resolving all of these warnings? Generally my assumption with enabling rules via warnings is that we will obviously start to catch new instances, but that there is also a plan/intention to work through the existing warnings and drive them to zero, not simply via "natural" methods.

My concern is that the additional numbers of affected files and amount of warnings could mean that these start being ignored and as a result potentially missing the more important warnings - especially where these overlap with other warnings in the same file. This might prove not to be true, but I do think it is a risk. (For reference, this is the treeherder log from the automated analysis run for the patch)

Even if we have a plan, I think we need a different roll-out method - I think we should focus on enabling the rule in areas as they pass, rather than doing a blanket enable. I realise this is potentially a lot of work, but I think if we want it doing (as comment 0 implies) I think the amount of work here warrants a dedicated effort.

I've cc'ed Gijs and Mossop in case they have comments as they are the owners for the main part of the code, although this does affect all areas.

Comment 6

[Tomer Yavor]

Even if we have a plan, I think we need a different roll-out method - I think we should focus on enabling the rule in areas as they pass, rather than doing a blanket enable. I realise this is potentially a lot of work, but I think if we want it doing (as comment 0 implies) I think the amount of work here warrants a dedicated effort.

That's sounds good. I am on it. The current state of exempted tests and components can be tracked here: https://docs.google.com/spreadsheets/d/1qLxA8iXRLSxlJ4Wiru-dFpCwFuj9Hs4UQWOPECkE-mo/edit?usp=sharing.
But soon I will provide also a new patch for that linter.

Do we have a plan for resolving all of these warnings?

Yes, but we probably won't be able to address all of them because some uses of HTTP might be necessary. Also we would like to focus more on new test files instead of those which are already existing that's why we should consider to disable the linter for specific tests and not only for components and areas.

Comment 7

[Tomer Yavor]

Attachment: WIP: Bug 1709150 - TEST

Comment 8

[Tomer Yavor]

Attachment: WIP: Bug 1709150 - TEST. r=lyavor

Comment 9

[Tomer Yavor]

Attachment: WIP: Bug 1709150 - next try

Comment 10

[Tomer Yavor]

Attachment: WIP: Bug 1709150 - Consider linter rule to make https the default for all kind of tests we add. r=freddyb

Comment 11

[Tomer Yavor]

Attachment: Bug 1709150 - Consider linter rule to make https the default for all kind of tests we add. r=standard8

Comment 12

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

The following patch is waiting for review from reviewers who are inactive or resigned from the review:

ID	Title	Author	Reviewer Status
D149394	Bug 1709150 - Consider linter rule to make https the default for all kind of tests we add. r=standard8	lyavor	mossop: Resigned from review
			Gijs: Back Aug 24, 2022

:lyavor, could you please find another reviewer?

For more information, please visit auto_nag documentation.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c9abae1a94e
Consider linter rule to make https the default for all kind of tests we add. r=Standard8,Gijs

Comment 14

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/8c9abae1a94e

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: Port Bug 1709150 - Linter rule to make `https` the default for all kind of tests we add. r=#thunderbird-reviewers

Comment 16

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7f275b3f0db9
Port Bug 1709150 - Linter rule to make `https` the default for all kind of tests we add. r=mkmelin