1635490 - Reddit Enhancement Suite Neverending Reddit Broken on Nightly

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Vash63]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

1. Install Reddit Enhancement Suite
2. Browsed Reddit on an account with "New Reddit" disabled to get the classic view
3. Scrolled down to bottom of page

This problem started on either the 3rd or 4th of May nightly builds and has been tested with both Linux and macOS based systems with the same results. Older versions of Firefox still work as expected showing this is not a regression in the extension itself (though it has been reported to them also - https://www.reddit.com/r/RESissues/comments/gdu62a/could_not_load_the_next_page_page_loaded_was_not/ )

Actual results:

Neverending reddit fails with "Could not load the next page: Page loaded was not for current user"

Expected results:

Next page should have loaded

Comment 1

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[westons]

This just started happening to me as well.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:77.0) Gecko/20100101 Firefox/77.0

Comment 3

[Qoi]

mozregression shows as: Bug 1629436 - requests with webExtension loading principal are not 3rd party - tests, r=robwu

Comment 4

[Tom Schuster [:evilpie]]

Source:
https://github.com/honestbleeps/Reddit-Enhancement-Suite/blob/4a6d5b61c19741909d8adbfa4ea1396f3e5d06ba/lib/modules/neverEndingReddit.js#L334

Comment 5

[Ben]

https://bugzilla.mozilla.org/show_bug.cgi?id=1322113

That is a previous bug that has caused very similar impact, it was resolved in Private Browsing but not standard browsing mode.

Comment 6

[Tom Schuster [:evilpie]]

Ah. I think this might not be related to the main change in this bug at all. The code doing this fetch is running in the content script and not in the background script. So I assumed this change would be transparent to content scripts. However it seems like there was a new check for triggeringPrincipal->AddonPolicy() added in NS_IsSameSiteForeign. That could explain the failures for something running in the content-script, as AddonPolicy is only non-null for principals of the background script. We should try reverting that change.

Comment 7

[Tom Schuster [:evilpie]]

(I can't reproduce this bug myself and I am a RES user)

Comment 8

[Ben]

The bug impacts probably less <1% FF users we have. Generally we see when a user does config changes to the browser they are more likely to hit the issue.

Comment 9

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

Set release status flags based on info from the regressing bug 1629436

Comment 10

[Andrea Marchesini [:baku]]

Do you have thoughts about this?

Comment 11

[Rob Wu [:robwu]]

As of bug 1604212, cookies use SameSite=Lax by default on Nightly. This means that cookies are no longer included by default in requests from different sites. This improves the privacy of users and the security of websites, at the expense of breaking applications (web sites and extensions) that rely on authenticated cross-site requests.

In bug 1629436, we decided to allow extensions to be excepted from this policy if they have the extension permissions, but only for requests from extension pages.

Content scripts are intentionally excepted from this policy bypass, to prevent content scripts from inadvertently allowing web pages to bypass the SameSite policies through extensions. Extensions that want to make requests that bypasses security/content policies are encouraged to use their background page instead. That would be needed anyway if we decide to proceed with bug 1578405. Note that Chrome already disallows cross-origin requests from content scripts, so extensions that want to be cross-browser-compatible already need to use background pages to make request on behalf of the content script.

Comment 12

[Ben]

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

As of bug 1604212, cookies use SameSite=Lax by default on Nightly. This means that cookies are no longer included by default in requests from different sites. This improves the privacy of users and the security of websites, at the expense of breaking applications (web sites and extensions) that rely on authenticated cross-site requests.

In bug 1629436, we decided to allow extensions to be excepted from this policy if they have the extension permissions, but only for requests from extension pages.

Content scripts are intentionally excepted from this policy bypass, to prevent content scripts from inadvertently allowing web pages to bypass the SameSite policies through extensions. Extensions that want to make requests that bypasses security/content policies are encouraged to use their background page instead. That would be needed anyway if we decide to proceed with bug 1578405. Note that Chrome already disallows cross-origin requests from content scripts, so extensions that want to be cross-browser-compatible already need to use background pages to make request on behalf of the content script.

We have no issues with the same code running on Chromium based browsers. Are you saying for this to work within FF we need to shift requests from content to background scripts?

Comment 13

[creesch.r]

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

Okay, means that this raises a rather big privacy concern, if I understand this correctly and they now need to use the background page. Firefox does not support the split permission meaning that there is no good way for extensions to differentiate between requests from private and non private pages.

Comment 14

[Tom Schuster [:evilpie]]

Aren't these request same-origin anyway? I think we should first figure out what actually caused the regression, than maybe look into Chrome's behavior. We should not get too far off target here.

Comment 15

[Rob Wu [:robwu]]

Attachment: fetch-same-origin-content-script.zip

Network requests from content scripts that are same-site (NOTE: same-site, not necessarily same-origin!) relative to the document's URL SHOULD still include samesite=lax cookies. This does apparently NOT happen.

STR:

1. Load the attached extension.
   (the extension will create a session cookie with SameSite=Lax at httpbin.org)
2. Open https://httpbin.org/dummy
3. Look at the page (modified by the extension).

Expected:

fetch to 'http://httpbin.org/cookies' received the following cookies: { "testcookie": "test from ..." }
This is a same-origin request, SameSite cookies should be included.

Actual:

fetch to 'http://httpbin.org/cookies' received the following cookies: {}
This is a same-origin request, SameSite cookies should be included.

Note: The expected behavior occurs if you open the DevTools console for the page from step 3 and run runTest()
Note: The actual behavior (of not including cookies) is expected when you visit a third-party site such as example.com.

Comment 16

[Rob Wu [:robwu]]

I originally thought that the extension was making a cross-origin request, but after looking at the source, this turns out to not be the case. Then I created a small test case (comment 15), and reproduced the problem. The patch is now on Beta, so we should fix this bug and uplift the fix.

Comment 17

[Rob Wu [:robwu]]

The content script uses an ExpandedPrincipal [ContentPrincipal of page + ContentPrincipal of extension]. When facing an expanded principal, the
SameSite/third-party checks should check if any of the principals of the expanded principal is SameSite/first-party relative to the requested URL, and if so, treat it as same site / first-party.

:baku Do you have room to fix this regression?

Comment 18

[Rob Wu [:robwu]]

I made a mistake in my test extension (the opened tab is httpbin.com and the content script requests httpbin.org) and cannot reproduce.

I then tried to follow the originally reported steps, but cannot reproduce.

1. Install RES 5.18.14
2. Create a new Reddit account and log in.
3. Go to https://old.reddit.com/r/all
4. Scroll to bottom - works.
5. Open a thread, scroll to bottom - works.

Could you explain what I need to do in order to reproduce the problem?

Comment 19

[Joe Hildebrand [:jhildebrand] (UTC-6)]

On MacOS 10.15.4, current Nightly:

1. Start browser with new profile mkdir /tmp/reddit; /Applications/Firefox\ Nightly.app/Contents/MacOS/firefox --profile /tmp/reddit
2. Navigate to https://old.reddit.com/
3. Click "Sign up". Generate email address w/ Firefox Relay, enter it.
4. Click next.
5. Select first suggested username.
6. Use a securely-generate password as suggested in the drop down under the password field
7. I'm not a robot. prove it. Next.
8. Find the good stuff? Select several. Submit
9. click on link in email. paste it into correct ffx instance, in a new tab.
10. Install https://addons.mozilla.org/en-US/firefox/addon/reddit-enhancement-suite/
11. New tab, go to https://old.reddit.com/
12. Scroll past bottom of the page

Expected: more stories fill in
Actual: "Could not load the next page: Page loaded was not for current user"

Comment 20

[Andrea Marchesini [:baku]]

Attachment: Bug 1635490 - Implement ExpandedPrincipal::IsThirdPartyURI() to consider any sub principal's URI, r?robwu

Comment 21

[Andrea Marchesini [:baku]]

Attachment: Bug 1635490 - Implement ExpandedPrincipal::IsThirdPartyURI() to consider any sub principal's URI - tests, r?robwu

Comment 22

[Rob Wu [:robwu]]

I am able to reproduce, with the test case from comment 15, with Nightly 78.0a1 buildID 20200506093716

There was a typo in the STR, and when :baku notified me of it, I failed to reproduce because I used an older Nightly build.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77d3d2cd452d
Implement ExpandedPrincipal::IsThirdPartyURI() to consider any sub principal's URI, r=robwu
https://hg.mozilla.org/integration/autoland/rev/ad16a3b7b0b5
Implement ExpandedPrincipal::IsThirdPartyURI() to consider any sub principal's URI - tests, r=robwu

Comment 24

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/77d3d2cd452d
https://hg.mozilla.org/mozilla-central/rev/ad16a3b7b0b5

Comment 25

[Andrea Marchesini [:baku]]

Comment on attachment 9146359 [details]
Bug 1635490 - Implement ExpandedPrincipal::IsThirdPartyURI() to consider any sub principal's URI, r?robwu

Beta/Release Uplift Approval Request

• User impact if declined: This patch fixed a regression introduced by 1629436. Bug 1629436 is landed in 77 and we need to have this patch to 77 too.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): There is a good test coverage.
• String changes made/needed:

Comment 26

[Pascal Chevrel:pascalc]

Comment on attachment 9146359 [details]
Bug 1635490 - Implement ExpandedPrincipal::IsThirdPartyURI() to consider any sub principal's URI, r?robwu

Fix for a P1 77 regression, uplift approved for beta 6, thanks.

Comment 27

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d6ad2a9399e7
https://hg.mozilla.org/releases/mozilla-beta/rev/9da87bb136d5