Closed Bug 1635490 Opened 6 months ago Closed 6 months ago

Reddit Enhancement Suite Neverending Reddit Broken on Nightly

Categories

(WebExtensions :: Untriaged, defect, P1)

78 Branch
defect

Tracking

(firefox-esr68 unaffected, firefox76 unaffected, firefox77 fixed, firefox78 fixed)

RESOLVED FIXED
mozilla78
Tracking Status
firefox-esr68 --- unaffected
firefox76 --- unaffected
firefox77 --- fixed
firefox78 --- fixed

People

(Reporter: Vash63, Assigned: baku)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

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

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

Product: Firefox → WebExtensions

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

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

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.

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.

Flags: needinfo?(amarchesini)

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

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.

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

Do you have thoughts about this?

Flags: needinfo?(amarchesini) → needinfo?(rob)

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.

Flags: needinfo?(rob)

(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?

(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.

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.

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.

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true

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?

Flags: needinfo?(amarchesini)

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"

Assignee: nobody → amarchesini

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.

Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(Vash63)
Severity: normal → S3
Priority: -- → P1
See Also: → 1637670
Pushed by amarchesini@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

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:
Attachment #9146359 - Flags: approval-mozilla-beta?

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.

Attachment #9146359 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.