1659155 - (CVE-2021-43531) contextMenus.onClicked: info.srcUrl is the final URL (after redirects) instead of the <img src> value.

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Rob Wu [:robwu]]

Attachment: menus-srcUrl-redirect.zip

Since bug 1406253, the info.srcUrl field of the contextMenus.onClicked event returns the URL after redirects, instead of the URL of the image request.

Prior to that bug, and also in Chrome, the info.srcUrl field was the image's src element.

STR:

1. Load attached extension, which opens a tab with an image.
2. Right-click on the image and click on the extension's context menu.

Expected (Chrome 84):

• A dialog with a URL ending with "before-redirect".

Actual (Firefox 81):

• A dialog with a URL ending with "after-redirect"

I'm inclined to resolve the bug as follows:

• Update MDN to clarify that info.srcUrl may differ from the <img>'s src attribute. In Chrome, this is always the URL of the initial request. In Firefox, this is the final URL (after any redirects). EDIT: The alternative is preferred, see comment 2 and comment 3.
• Add a unit test to make sure that the behavior continues to be as documented.

The alternative is to let info.srcUrl be the initial URL again. This would result in consistent behavior between Firefox and Chrome. I guess that few users cared either way, since nobody has filed a bug about this (I only discovered this when I investigated a report about DownThemAll not working on Twitter).

Some extensions (including DownThemAll) use info.srcUrl to identify the element to which the context menu is anchored from a content script. Since there is a superior alternative (info.targetElementId + browser.menus.getTargetElement), I would recommend extension authors to use that API to really identify the target element without having to traverse the whole DOM to find the image element of interest.

Comment 1

[Rob Wu [:robwu]]

I'll wait till the next bug triage, and if the team agrees, I'll add dev-doc-needed to the bug.

Or maybe dev-doc-complete after updating the documentation myself, at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/OnClickData

Comment 2

[Shane Caraveo (:mixedpuppy)]

I'm a bit on the fence here.

Writing cross browser compatible code would be nice, thus srcUrl being consistent with (probably) the rest of the browsers implementing this api is preferable. As well, while targetElementId is superior for the purpose, it's not cross browser.

Having the final url in srcUrl is probably useful for some, but I'm not sure it outweighs the compatibility.

In retrospect we should have kept srcUrl compatible, and introduced finalSrcUrl. It feels like doing that at this point just muddies the water.

I think we'd probably be better off if this were compatible at least in V3, and it probably makes sense to have it compatible in V2 to lower the V2->V3 transition.

Comment 3

[Rob Wu [:robwu]]

During the triage we decided that info.srcUrl should be the original URL, before redirects. This is because the final URL (after redirects) is normally not exposed to the web platform, and the original URL is usually expected. It's also more compatible with Chrome.

The doubts about final URL vs current URL is not limited to extensions, there is a similar discussion going on in bug 1644802. That bug and this bug may be able to share the same solution.

FYI, the property is internally called .mediaURL, and used by video, audio and images. For video and audio, it's already the pre-redirect URL. For images, it's somehow the final URL: https://searchfox.org/mozilla-central/rev/d54210d490ef335b13fc1fcac817525120c8c46b/browser/actors/ContextMenuChild.jsm#1005-1022

Comment 4

[Daniel Veditz [:dveditz]]

The reason it's the post-redirect URL was also web extensions :-( This was done in bug 1406253 (and causing other problems, see bug 1719203)

Comment 5

[Daniel Veditz [:dveditz]]

In the case of a redirected image this reveals information to an extension that the Same-origin policy forbids unless it has WebRequest access to the hosts in order to follow the redirect chain.

Comment 6

[Rob Wu [:robwu]]

Depending on the implementation, fixing bug 1719203 would resolve this bug and also bug 1644802

Comment 7

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #6)

Depending on the implementation, fixing bug 1719203 would resolve this bug and also bug 1644802

There's a patch up in 1719203 now, is more work needed here or not?

Comment 8

[Rob Wu [:robwu]]

Yes, more work is needed.

The extension framework implementation reads the URL from contextData.srcUrl :

• https://searchfox.org/mozilla-central/rev/235982cdb78d0dc3ab4e48326b084ce087302cab/browser/components/extensions/parent/ext-menus.js#694
• https://searchfox.org/mozilla-central/rev/235982cdb78d0dc3ab4e48326b084ce087302cab/browser/components/extensions/parent/ext-menus.js#958

srcUrl is currently set to mediaURL at https://searchfox.org/mozilla-central/rev/235982cdb78d0dc3ab4e48326b084ce087302cab/browser/base/content/nsContextMenu.js#136

To fix this bug, we would have to replace srcUrl: this.mediaURL with srcUrl: this.originalMediaURL (introduced in the patch for bug 1719203), plus a unit test for this scenario. The fix is trivial, the tests are most of the work. I could take care of that once the patch for bug 1719203 lands.

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1659155 - Use pre-redirect URL as srcUrl in contextMenus API

Comment 10

[Rob Wu [:robwu]]

Once this bug gets fixed, the change should be documented in the release notes.

And the documentation of srcUrl on MDN should clarify that the URL is the pre-redirect URL, but that it was the post-redirect URL from Firefox 59 (bug 1406253) up until Firefox 95 (fixed in this bug) (or maybe even 94, if we decide to uplift this bug for consistency with the behavior in bug 1719203).

Comment 11

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

Use pre-redirect URL as srcUrl in contextMenus API r=Gijs
https://hg.mozilla.org/integration/autoland/rev/010f3ed252635ba277844b24ed7b073592d5e8df
https://hg.mozilla.org/mozilla-central/rev/010f3ed25263

Comment 12

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

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 13

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

The patch landed in nightly and beta is affected.
:robwu, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 14

[Rob Wu [:robwu]]

Comment on attachment 9244718 [details]
Bug 1659155 - Use pre-redirect URL as srcUrl in contextMenus API

Beta/Release Uplift Approval Request

• User impact if declined: It would be nice if the two related secbugs were fixed in the same release (this bug and bug 1719203). Otherwise there is inconsistent behavior.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Why potentially risky: This changes the behavior of the extension API in some cases.
  Why not risky: The new behavior is consistent with user expectations in other situations (bug 1719203), and identical to the behavior of the same extension API in Chromium.
• String changes made/needed:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9244718 [details]
Bug 1659155 - Use pre-redirect URL as srcUrl in contextMenus API

Approved for 94.0b5.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Should we take this and bug 1719203 on ESR91?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8852d521e10

Comment 18

[Rob Wu [:robwu]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

Should we take this and bug 1719203 on ESR91?

I'd put this question in the other bug (there is already a question from the reporter, but nobody was needinfo'd).

The change to the extension API is a behavioral change, which has some (small) risk of affecting extensions.

The change to the context menu implementation (bug 1719203) affects user behavior.

The main question is whether we consider the sec issue severe enough to warrant uplifting to ESR. I'm 50/50 divided on this one, I'd be fine with uplifting, but also fine if we did not. I'll put a needinfo on the other bug.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Sounds like this is wontfix for ESR91 based on the last comments in bug 1719203.

Comment 20

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 21

[Richard Bloor]

Could I get some clarification here:

• the documentation for srcUrl is rather cryptic, what image is it referring to?
• what is the difference between the pre-redirect URL and post-redirect URL?

Comment 22

[Rob Wu [:robwu]]

It's the image (e.g. <img> or a background image) under the pointer where the context menu has been opened.

The "pre-redirect" URL is the initial URL, e.g. the value of an <img>'s src attribute (or the value of the url() CSS function in case of background-image).

After a redirect (e.g. HTTP 302 by a server), the final URL (post any redirects) is the ultimate redirection target, likely different from the initial URL.
/

Comment 23

[Richard Bloor]

Documentation changes are ready to review:

• menus.OnClickedData.srcUrl returns <img src> #10214
• menus.OnClickedData.srcUrl returned post-redirect URL #13259

Comment 24

[Rob Wu [:robwu]]

Posted reviews.