1648036 - `sourceURL` in abuse report has the wrong value when site uses client-side (javascript) navigation

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[William Durand [:willdurand]]

STR

1. It's probably best to try in a -dev env, so let's configure your FF profile for addons-dev.allizom.org. Create/update the following prefs:

• extensions.webapi.testing set to true
• xpinstall.signatures.dev-root set to true
• extensions.abuseReport.amoDetailsURL set to https://services.addons-dev.allizom.org/api/v4/addons/addon/
• extensions.abuseReport.url set to https://services.addons-dev.allizom.org/api/v4/abuse/report/addon/

2. Go to: https://addons-dev.allizom.org/en-US/firefox/addon/search_by_image/?src=recommended_fallback

3. Click on another extension on this page (for example in "Other popular extensions")

4. Install this extension (not the "search by image" one)

5. Open the Browser Toolbox > Network panel

6. Report this extension using the button on AMO

7. Observe the request being sent by AbuseReport in the network panel

What happened?

The source URL in the payload (addon_install_source_url) is:

https://addons-dev.allizom.org/en-US/firefox/addon/search_by_image/?src=recommended_fallback

Expected

The source URL in the payload (addon_install_source_url) is the URL of the current page on AMO, not the one pasted above.

Anything else?

This happens because once we load a page on AMO, all buttons/links use "client-side navigation" using a JavaScript router and probably the History API. We can see here that we use this.window.document.nodePrincipal.URI, which retains the URI of the very first page loaded. Its value isn't updated.

We probably need to rely on something like this.window.location instead.

The issue is probably present in InstallTrigger too.

Comment 1

[Andreas Wagner [:TheOne] [use NI]]

Shane and Stuart, could we prioritize this?

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Andreas Wagner [:TheOne] [use NI] from comment #1)

Shane and Stuart, could we prioritize this?

If I recall correctly, Will did also want to take this one and attach a patch. If Will can't work on a patch for this anymore, I'm ok to take it.

Comment 3

[William Durand [:willdurand]]

If I recall correctly, Will did also want to take this one and attach a patch.

That's correct.

Comment 4

[William Durand [:willdurand]]

Attachment: Bug 1648036 - Use `window.location` to determine the `sourceURL`/`sourceHost` when installing an add-on. r=rpl

I made changes to amWebAPI and amInstallTrigger but left
amContentHandler unchanged.

It's not clear to me how we are using this handler. Some existing test
cases expect sourceURL to be undefined so I am not sure we can
retrieve a URL from aRequest.URI for instance.

Comment 5

[Shane Caraveo (:mixedpuppy)]

looks like this is moving forward.

Comment 6

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

The severity field is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cec17cfddb34
Use `window.location` to determine the `sourceURL`/`sourceHost` when installing an add-on. r=rpl

Comment 8

[Atila Butkovits]

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

Comment 9

[William Durand [:willdurand]]

Comment on attachment 9162200 [details]
Bug 1648036 - Use window.location to determine the sourceURL/sourceHost when installing an add-on. r=rpl

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Without this patch, the abuse reports submitted by the users would not always include the correct url of the website from where the extension installation has been triggered. This would likely prevent AMO admins to correctly handle abuse reports and/or make it harder to detect malicious activities.

Given that we only noticed the issue recently and that the ESR version stays around for a longer time, the AMO admins would like to make sure that we send the correct URL for as many abuse reports as possible, which is why an uplift to ESR would be helpful.

• User impact if declined: Users who report abuse reports might not see any action taken by AMO admins because admins wouldn't have accurate data.
• Fix Landed on Version: 80
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch includes two small changes to the AddonManager internals to fix the value collected and stored in the addon db. Both changes are covered by tests.
• String or UUID changes made by this patch:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9162200 [details]
Bug 1648036 - Use window.location to determine the sourceURL/sourceHost when installing an add-on. r=rpl

Approved for 78.2esr. Thanks for the test coverage.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/743b5298c4db