366192 - Disallow htppfoobor schemes when only http/https are valid

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Asaf Romano (gone)]

See bug 365570 comment 8.

Comment 1

[Moritz Brunner [:mb]]

Attachment: Bug 366192 - Fixed http/https regex checks.

Review commit: https://reviewboard.mozilla.org/r/194638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194638/

Comment 2

[Moritz Brunner [:mb]]

Should also "/^https?:/" be replaced with "/^https?:\/\//" for checks applied on urls?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Marco, I don't know if you have all the historical context here, but could you mentor this bug?

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8923502 [details]
Bug 366192 - Fixed http/https regex checks.

https://reviewboard.mozilla.org/r/194638/#review199674

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I don't know if we can update the screenshots extension in m-c or if there's a separate process via github for that. Mark?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8923502 [details]
Bug 366192 - Fixed http/https regex checks.

https://reviewboard.mozilla.org/r/194638/#review199770

::: browser/extensions/screenshots/webextension/build/shot.js:391
(Diff revision 1)
>  
>    get urlDisplay() {
>      if (!this.url) {
>        return null;
>      }
> -    if (this.url.search(/^https?/i) != -1) {
> +    if (this.url.search(/^https?:\/\//i) != -1) {

I don't see the point of a search here, while touching this code, what about using test() instead?

Comment 7

[Mark Banner (:standard8)]

(In reply to Marco Bonardo [::mak] from comment #5)
> I don't know if we can update the screenshots extension in m-c or if there's
> a separate process via github for that. Mark?

In theory, they should detect the landing in m-c and merge it across. I don't know about their test infrastructure though. Going to forward this to Ian as he seems to have been doing most of the exports from github to m-c.

Comment 8

[Moritz Brunner [:mb]]

Comment on attachment 8923502 [details]
Bug 366192 - Fixed http/https regex checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194638/diff/1-2/

Comment 9

[Moritz Brunner [:mb]]

Comment on attachment 8923502 [details]
Bug 366192 - Fixed http/https regex checks.

https://reviewboard.mozilla.org/r/194638/#review199770

> I don't see the point of a search here, while touching this code, what about using test() instead?

I noticed that sometimes checks for http/https are case-insensitive and sometimes not, wouldn't it be better to always perform case-insensitive tests so that "http" gets handled the same way as "hTtp"?

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Moritz Brunner [:mb] from comment #9)
> I noticed that sometimes checks for http/https are case-insensitive and
> sometimes not, wouldn't it be better to always perform case-insensitive
> tests so that "http" gets handled the same way as "hTtp"?

Hopefully most of the cases are using a normalized url, that came our an nsIURI or an URL object, and in such a case it's not important to be case-insensitive. I don't think we should use a ci comparison unless the code requires it (e.g. it is working on a raw string that came from an unverified input)

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8923502 [details]
Bug 366192 - Fixed http/https regex checks.

https://reviewboard.mozilla.org/r/194638/#review200978

Tha patch looks good, but we must wait for the needinfo to know if we can land it as-is.

Comment 12

[Ian Bicking (:ianbicking)]

Sorry I missed this... we export our code from a GitHub repository into Firefox, so ideally the patch would come through that GitHub repository: https://github.com/mozilla-services/screenshots (if changes land in Firefox we usually manually turn them into changes into the parent repository)

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Moritz, would you mind splitting the patch and posting the screenshots part as a pull request to the github repo?

Comment 14

[Moritz Brunner [:mb]]

Comment on attachment 8923502 [details]
Bug 366192 - Fixed http/https regex checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194638/diff/2-3/

Comment 15

[Moritz Brunner [:mb]]

I split it up, the push request in Github is https://github.com/mozilla-services/screenshots/pull/3720.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thank you! Landing...

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bbbd75cd12a9
Fixed http/https regex checks. r=mak

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bbbd75cd12a9