1451502 - Uplift removal of js protocol from reps bundle linkification regex

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[:Gijs (he/him)]

Separate bug to make sure we track correctly for beta.

(In reply to :Gijs (he/him) from bug 1447969 comment #23)
> (In reply to Wladimir Palant (for Adblock Plus info Cc
> [e-mail]) from bug 1447969 comment #21)
> > However, the console in the
> > Web Developer tools will still linkify javascript: URLs, e.g. if the page
> > does console.log("javascript:alert('oops')"). These links open in a new tab
> > and the code executes in an unprivileged context (about:blank), but this
> > still shouldn't be.
> 
> Right... Nicolas, we could uplift a 1-line fix to the reps bundle for this,
> or uplift the reps bundle change, or do nothing. Given there isn't a
> concrete security impact for 60 anymore, I could live with the last option -
> but on the other hand, this will be an ESR release and it may be nice to
> have that change for 60 anyway. Up to you...


(In reply to Nicolas Chevobbe [:nchevobbe] from bug 1447969 comment #24)
> It would be nice to uplift the 1-line fix in the Reps bundle IMO

So let's do that. This doesn't really fix anything anymore, but we might as well do things properly for 60 given it's an esr. Marking sec-audit but hiding because it implies things about the other bug.

Comment 1

[:Gijs (he/him)]

Attachment: Patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 8965110 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: confusion because we show links for javascript: URLs that then don't work, or open in new tabs, potential for more subtle security issues based on what the JS does in the new tab, or something.
[Is this code covered by automated tests?]: generally yes, not for this specific change.
[Has the fix been verified in Nightly?]: sort of. Not this specific change, but the reps bundle change encompassed a slightly larger change. If you like, this is a minimal patch for beta....
[Needs manual test from QE? If yes, steps to reproduce]: Eh, someone could check after this lands that if a webpage executes:

console.log("javascript:alert('oops')")

the logged message in the devtools console does not contain a link anymore.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: trivial change to a regular expression (I know, I know, I know, but really though...)
[String changes made/needed]: no

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8965110 [details] [diff] [review]
Patch

Review of attachment 8965110 [details] [diff] [review]:
-----------------------------------------------------------------

Seems okay to me. Just one small question about the unrelated change.

::: devtools/client/shared/components/reps/reps.js
@@ +6372,5 @@
>  module.exports = __WEBPACK_EXTERNAL_MODULE_56__;
>  
>  /***/ })
>  /******/ ]);
> +});

what changed here ? line ending ?

Comment 4

[:Gijs (he/him)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Comment on attachment 8965110 [details] [diff] [review]
> Patch
> 
> Review of attachment 8965110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems okay to me. Just one small question about the unrelated change.
> 
> ::: devtools/client/shared/components/reps/reps.js
> @@ +6372,5 @@
> >  module.exports = __WEBPACK_EXTERNAL_MODULE_56__;
> >  
> >  /***/ })
> >  /******/ ]);
> > +});
> 
> what changed here ? line ending ?

Ugh, yes, thank my editor. I'll fix this.

Comment 5

[:Gijs (he/him)]

Well, more specifically, whether there was a newline at the end of the file.

Comment 6

[:Gijs (he/him)]

Attachment: Patch

Approval Request Comment in comment 2. carrying over r+

Comment 7

[Julien Cristau [:jcristau]]

Comment on attachment 8965413 [details] [diff] [review]
Patch

seems fine.  Should this be marked as fixed in 61 then if it's already part of an existing reps change?

Comment 8

[:Gijs (he/him)]

(In reply to Julien Cristau [:jcristau] from comment #7)
> Comment on attachment 8965413 [details] [diff] [review]
> Patch
> 
> seems fine.  Should this be marked as fixed in 61 then if it's already part
> of an existing reps change?

Yes...

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b1c94bc56dd

Comment 10

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue on old beta build 60.0b6. Verified that 'console.log("javascript:alert('oops')") ' contains no link in Web Console using 60.0b11 and Latest Nightly 61.0a1 across platforms (Windows 10 64bit, macOS 10.13.4 and Ubuntu 16.04 32bit).