Closed Bug 366192 Opened 13 years ago Closed 2 years ago

Disallow htppfoobor schemes when only http/https are valid

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: mano, Assigned: mb, Mentored)

Details

Attachments

(1 file)

Should also "/^https?:/" be replaced with "/^https?:\/\//" for checks applied on urls?
Flags: needinfo?(past)
Marco, I don't know if you have all the historical context here, but could you mentor this bug?
Flags: needinfo?(past) → needinfo?(mak77)
Attachment #8923502 - Flags: review?(past) → review?(mak77)
Assignee: nobody → moritzbrunner
Mentor: mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
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?
Flags: needinfo?(standard8)
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?
(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.
Flags: needinfo?(standard8) → needinfo?(ianb)
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"?
(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 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.
Attachment #8923502 - Flags: review?(mak77) → review+
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)
Flags: needinfo?(ianb)
Moritz, would you mind splitting the patch and posting the screenshots part as a pull request to the github repo?
I split it up, the push request in Github is https://github.com/mozilla-services/screenshots/pull/3720.
Thank you! Landing...
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/bbbd75cd12a9
Fixed http/https regex checks. r=mak
https://hg.mozilla.org/mozilla-central/rev/bbbd75cd12a9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.