Uplift removal of js protocol from reps bundle linkification regex

VERIFIED FIXED in Firefox 60

Status

defect
VERIFIED FIXED
a year ago
6 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

({sec-audit})

Trunk
Firefox 61

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox59 wontfix, firefox60 verified, firefox61 verified)

Details

(Whiteboard: [adv-main60-])

Attachments

(1 attachment, 1 obsolete attachment)

1.23 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
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
> bugzilla@adblockplus.org) 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.
(Assignee)

Comment 1

a year ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #8965110 - Flags: review?(nchevobbe)
(Assignee)

Comment 2

a year ago
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
Attachment #8965110 - Flags: approval-mozilla-beta?
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 ?
Attachment #8965110 - Flags: review?(nchevobbe) → review+
(Assignee)

Comment 4

a year ago
(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.
(Assignee)

Comment 5

a year ago
Well, more specifically, whether there was a newline at the end of the file.
(Assignee)

Comment 6

a year ago
Posted patch PatchSplinter Review
Approval Request Comment in comment 2. carrying over r+
Attachment #8965110 - Attachment is obsolete: true
Attachment #8965110 - Flags: approval-mozilla-beta?
Attachment #8965413 - Flags: review+
Attachment #8965413 - Flags: approval-mozilla-beta?
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?
Attachment #8965413 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 8

a year ago
(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...
https://hg.mozilla.org/releases/mozilla-beta/rev/4b1c94bc56dd
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main60-]

Updated

11 months ago
Product: Firefox → DevTools
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.