Closed Bug 1454914 Opened 6 years ago Closed 6 years ago

Edit button does not have its badge when you first use Firefox Screenshots

Categories

(Core :: DOM: Security, defect, P1)

x86_64
All
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + verified
firefox61 --- verified

People

(Reporter: cbadescu, Assigned: ckerschb)

References

Details

(Keywords: regression, Whiteboard: [domsecurity-active])

Attachments

(3 files, 3 obsolete files)

[Affected versions]:
- Nightly 61.0a1, Build ID: 20180417225505

[Affected Platforms]:
- All Windows
- All Mac
- All Linux

[Steps to reproduce]:
1. Open the Firefox browser and navigate to any website.
2. Click the "Page actions" menu and select "Take a Screenshot" option.
3. Skip the onboarding tour.
4. Click the "Save visible" button and select the "Save" button.
5. Observe the "Edit" (Pencil) button.

[Expected result]:
- The button has a blue circle that has a star in it.

[Actual result]:
- The button has no badge.

[Regression]:
- It seems that this is a Nightly regression. Using mozregression tools I have found a regression window. Here are the results:
Last good revision: ae45e56c3c71ae23084e9ef549c3d58880add8a1
First bad revision: 87b552f9c09b68a255eeb84c5c554384b0c1231a
Pushlog: https://goo.gl/pYTBYX

From the pushlog it seems that bug 1452496 has caused this issue. 

[Notes]:
- This issue is also reproducible on an older Nightly 61.0a1 build ID 20180108220132 with latest Firefox Screenshots v32.0.0dev installed from here( https://screenshots.dev.mozaws.net/homepage/install-test-local.html ) and on the latest Beta 60.0b13 with latest Firefox Screenshots v30.1.0.
- This issue is not reproducible on the latest Firefox Beta 60.0b12 with Firefox Screenshots v30.1.0.
- This issue is not reproducible on the latest Firefox Release 59.0.2 with Firefox Screenshots v25.0.0.
- Attached is a screen recording with this issue.
Cristoph, can you please take a look at this?
Flags: needinfo?(ckerschb)
(In reply to Cristina Badescu from comment #1)
> Cristoph, can you please take a look at this?

Christina, I just checked out the latest code from mozilla-central and I couldn't reproduce the issue. Personally, I highly doubt that same-site cookies cause that problem because there are no cookies involved in saving a screenshot. Anyway, Francois just landed a pref for same-site cookies [1] within Bug 1452699. If it's still repdroducable with that pref flipped than we know for sure it's not caused by same-site cookies.

CC'ing a bunch of folks who have an eye on same-site cookies. I'll keep that bug on my radar, but again. I highly doubt that same-site cookies cause that problem.

[1] pref("network.cookie.same-site.enabled",    true); // Honor the SameSite cookie attribute
Flags: needinfo?(ckerschb)
It looks to me like screenshot uses 'samesite: "lax"' for cookies ( https://github.com/mozilla-services/screenshots/search?utf8=%E2%9C%93&q=samesite&type= ), but it also looks like they might use other cookie features, and github's search is famously useless, so maybe Jared or Ian can clarify what Screenshots' cookie requirements are like and why this would potentially break if we're properly enforcing cookie restrictions?

(FWIW, the fact that this is also breaking on beta, to me, makes it more likely that this is actually a regression from the samesite cookie changes.)
Flags: needinfo?(jhirsch)
Flags: needinfo?(ianb)
Christoph, with the "network.cookie.same-site.enabled" pref set to "false" I can no longer reproduce this issue on latest Nightly 61.0a1 (Build ID: 20180417225505) with Firefox Screenshots 30.1.0.
(In reply to :Gijs (he/him) from comment #3)
> (FWIW, the fact that this is also breaking on beta, to me, makes it more
> likely that this is actually a regression from the samesite cookie changes.)

I guess you are right. So here is the problem - we are about to set a cookie, something like:
  abtests=eyJ; path=/; expires=Fri, 18 May 2018 13:46:48 GMT; samesite=lax; secure; httponly


But we are treating that cookie as foreign, because of the loadingPrincipal, see:
  channelURI: https://screenshots.firefox.com/event
  hostURI: https://screenshots.firefox.com/event
  isForeign: yes
  contentType: 20
  isInThirdPartyContext: no
  loadingPrincipal: moz-extension://f6d9f303-c8bd-4f98-8b78-adac23f071b6/_generated_background_page.html

Gijs, do we need to exempt moz-extension: somehow?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> (In reply to :Gijs (he/him) from comment #3)
> > (FWIW, the fact that this is also breaking on beta, to me, makes it more
> > likely that this is actually a regression from the samesite cookie changes.)
> 
> I guess you are right. So here is the problem - we are about to set a
> cookie, something like:
>   abtests=eyJ; path=/; expires=Fri, 18 May 2018 13:46:48 GMT; samesite=lax;
> secure; httponly
> 
> 
> But we are treating that cookie as foreign, because of the loadingPrincipal,
> see:
>   channelURI: https://screenshots.firefox.com/event
>   hostURI: https://screenshots.firefox.com/event
>   isForeign: yes
>   contentType: 20
>   isInThirdPartyContext: no
>   loadingPrincipal:
> moz-extension://f6d9f303-c8bd-4f98-8b78-adac23f071b6/
> _generated_background_page.html
> 
> Gijs, do we need to exempt moz-extension: somehow?

I guess we should treat it the same as system/chrome, yeah...

Can we verify how samesite cookies work in Chromium/Blink and their add-on support?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs (he/him) from comment #6)
> I guess we should treat it the same as system/chrome, yeah...
> 
> Can we verify how samesite cookies work in Chromium/Blink and their add-on
> support?

Francois, can you look into that? I'll try to write a patch to exempts extensions!
Flags: needinfo?(francois)
Even if we fix the moz-extension problem there is still another problem here. In particular, same-site cookies are not allowed to be set within a cross-site context, but that also happens here, see:

  channelURI: https://screenshotscdn.firefoxusercontent.com/images/6493f375-2fec-4773-a301-ece676678069.png
  hostURI: https://screenshotscdn.firefoxusercontent.com/images/6493f375-2fec-4773-a301-ece676678069.png
  contentType: 38
  isInThirdPartyContext: no
  loadingPrincipal: https://screenshots.firefox.com/KdANgkOnyLwF7Y4U/www.mozilla.org
  triggeringPrincipal: https://screenshots.firefox.com/KdANgkOnyLwF7Y4U/www.mozilla.org
  addonPrincipal: no


LogFailure {
  aCookieString: abtests=eyJ; path=/; expires=Fri, 18 May 2018 15:04:12 GMT; samesite=lax; secure; httponly

In other words, https://screenshotscdn.firefoxusercontent.com tries to set a same site in the context of https://screenshots.firefox.com, which is not allowed by the spec.
Gijs, obviously the right fix would live somewhere in the thirdpartytuil code, which I don't really wanna touch for this bug, because of the other side effects we are not aware of.

Would that patch be potentially acceptable? Basically guarding the entry points into the same site cookie checks?
Attachment #8968940 - Flags: feedback?(gijskruitbosch+bugs)
Also, ni? Dan because of the urgency.

Dan, please see previous comment!
Flags: needinfo?(dveditz)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> In other words, https://screenshotscdn.firefoxusercontent.com tries to set a
> same site in the context of https://screenshots.firefox.com, which is not
> allowed by the spec.

Hopefully Ian or Jared can comment on this.
Comment on attachment 8968940 [details] [diff] [review]
bug_1454914_same_site_cookies_webextensions.patch

Review of attachment 8968940 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think this would be OK for uplift. We can pursue the correct fix (updating ThirdPartyUtil) for 61/62.

(Please get review from a peer, ie not me. :-) )

::: netwerk/cookie/nsCookieService.cpp
@@ +3491,5 @@
> +    nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
> +    bool triggeredByAddon = loadInfo &&
> +      BasePrincipal::Cast(loadInfo->TriggeringPrincipal())->AddonPolicy();
> +
> +    if (!triggeredByAddon) {  

Nit: trailing whitespace
Attachment #8968940 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Comment on attachment 8968940 [details] [diff] [review]
bug_1454914_same_site_cookies_webextensions.patch

Review of attachment 8968940 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsNetUtil.cpp
@@ +2145,5 @@
>      return false;
>    }
> +
> +  // Do not treat loads triggered by web extensions as foreign
> +  if (BasePrincipal::Cast(loadInfo->TriggeringPrincipal())->AddonPolicy()) {

This should be something like `principal->AddonAllowsLoad(channelURI)`, here and below. That would have the dual effects of making this work for add-on expanded principals, and restricting the exemption to sites the add-on has permissions for.

Also, it would probably make more sense to just add this check to IsThirdPartyChannel...
Valentin, Kmag, it seems we should exempt web extensions from same-site cookie restrictions.

Would you be fine on signing off on that or are we missing something here?
Attachment #8968940 - Attachment is obsolete: true
Attachment #8968969 - Flags: review?(valentin.gosu)
Attachment #8968969 - Flags: review?(kmaglione+bmo)
I guess I have seen comment 13 too late before I uploaded the other patch. Anyway, I verified that this patch treats web extension triggered loads not as foreign with regards to our same-site cookie implementation.

Valentin, kmag, acceptable?
Attachment #8968969 - Attachment is obsolete: true
Attachment #8968969 - Flags: review?(valentin.gosu)
Attachment #8968969 - Flags: review?(kmaglione+bmo)
Attachment #8968992 - Flags: review?(valentin.gosu)
Attachment #8968992 - Flags: review?(kmaglione+bmo)
Comment on attachment 8968992 [details] [diff] [review]
bug_1454914_same_site_cookies_webextensions.patch

Review of attachment 8968992 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thanks.

::: netwerk/base/nsNetUtil.cpp
@@ +2148,5 @@
> +  // Do not treat loads triggered by web extensions as foreign
> +  nsCOMPtr<nsIURI> channelURI;
> +  NS_GetFinalChannelURI(aChannel, getter_AddRefs(channelURI));
> +  if (BasePrincipal::Cast(loadInfo->TriggeringPrincipal())->
> +      AddonAllowsLoad(channelURI, true)) {

No need for `, true`

::: netwerk/cookie/nsCookieService.cpp
@@ +3493,5 @@
> +    NS_GetFinalChannelURI(aChannel, getter_AddRefs(channelURI));
> +    nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
> +    bool addonAllowsLoad = loadInfo &&
> +      BasePrincipal::Cast(loadInfo->TriggeringPrincipal())->
> +      AddonAllowsLoad(channelURI, true);

No need for `, true`
Attachment #8968992 - Flags: review?(kmaglione+bmo) → review+
Replying to Comment 3:

We set cookies like: cookies.set("cookieName", value, {signed: true, sameSite: "lax", maxAge: COOKIE_EXPIRE_TIME});

We use that same pattern for all cookies. We use this library, which has some defaults: https://www.npmjs.com/package/cookies – specifically it also turns on path=/; secure; httpOnly

It appears we set an A/B test-related cookie from the content domain, but that's not important (I've opened https://github.com/mozilla-services/screenshots/issues/4346 to remove it).

The add-on attempts to set a cookie by making a request from the extension background page (with a moz-extension://uuid origin). But that has never worked if you have "Don't Accept Third-Party Cookies", so we additionally use an HTTP authentication header for our API calls, and make a later attempt to set the cookie via a normal content page. So Screenshots should be resilient to some cookie problems, but it's not as well-tested a path.
Flags: needinfo?(jhirsch)
Flags: needinfo?(ianb)
Comment on attachment 8968992 [details] [diff] [review]
bug_1454914_same_site_cookies_webextensions.patch

Review of attachment 8968992 [details] [diff] [review]:
-----------------------------------------------------------------

This all looks fine, but we do need to add more tests to these areas.
The number of corner cases is growing, and it's hard to know where we are missing something.
Please file a bug for the tests.

::: netwerk/cookie/nsCookieService.cpp
@@ +3493,5 @@
> +    NS_GetFinalChannelURI(aChannel, getter_AddRefs(channelURI));
> +    nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
> +    bool addonAllowsLoad = loadInfo &&
> +      BasePrincipal::Cast(loadInfo->TriggeringPrincipal())->
> +      AddonAllowsLoad(channelURI, true);

nit: I would add an extra indentation level on this line.
Attachment #8968992 - Flags: review?(valentin.gosu) → review+
We clearly need to treat the moz-extension: scheme like chrome: here, just as we also give it a pass for CSP purposes. Given the timing I'm happier fixing this outside thirdpartytuil as you've done in this patch. Do you want to file another bug for "Move add-on/same-site exemption into thirdpartytuil"? Need to be careful with that one and test carefully on a Nightly.

(In reply to Ian Bicking (:ianbicking) from comment #17)
> We set cookies like: cookies.set("cookieName", value, {signed: true,
> sameSite: "lax", maxAge: COOKIE_EXPIRE_TIME});

samesite is great for auth cookies, but not for resources designed to be loaded from a 3rd party! On the other hand one of the cookies being set is called "_csrf". The primary web-app attack samesite cookies try to prevent are CSRF attacks, so how are you using this cookie?
Flags: needinfo?(dveditz)
(In reply to Daniel Veditz [:dveditz] from comment #19)
> We clearly need to treat the moz-extension: scheme like chrome: here, just
> as we also give it a pass for CSP purposes. Given the timing I'm happier
> fixing this outside thirdpartytuil as you've done in this patch. Do you want
> to file another bug for "Move add-on/same-site exemption into
> thirdpartytuil"? Need to be careful with that one and test carefully on a
> Nightly.

I just filed bug 1455157. Christoph and I are going to discuss this in Berlin next week.
Flags: needinfo?(francois)
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Component: Networking: Cookies → DOM: Security
Priority: -- → P1
Whiteboard: [domsecurity-active]
Incorporated nits - carrying over r+!
Attachment #8968992 - Attachment is obsolete: true
Attachment #8969261 - Flags: review+
:jkt, as discussed on IRC, I managed to write an automated test for web-extensions. Can you please take a look?
Attachment #8969262 - Flags: review?(jkt)
Comment on attachment 8969262 [details] [diff] [review]
bug_1454914_same_site_cookies_webextensions_tests.patch

As mentioned on slack "webRequest" and "webRequestBlocking" are not needed for this to work.

I also feel like the content script is only running because the test itself is in a mochi.test page which feels a little fragile. I would have opened a tab for this instead.

Anyway no blockers from me :) r+
Attachment #8969262 - Flags: review?(jkt) → review+
Kmag, I spent quite so much time on that test but apparently it's not working on TRY [1]. Works just fine locally. Any idea what might be wrong on the test TV. I am getting:

> TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_same_site_cookies_webextension.html | message queue is empty - got "[\"image-loaded-and-same-site-cookie-set\"]", expected "[]"


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2ab55e9a5a0b86edc3a20e4f737fe33ab0114a9
Flags: needinfo?(kmaglione+bmo)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #24)
> Kmag, I spent quite so much time on that test but apparently it's not
> working on TRY [1].

It seems to be working now (thank jkt):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e03a88affe5206931f192ac0e815239723a7014d
Flags: needinfo?(kmaglione+bmo)
Just FYI, I generally prefer these kinds of tests to be xpcshell tests, e.g.,

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_async_loading.js

I won't block on it, though. Thanks for adding a test for this.
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf15cbc8ffb
Exempt web-extensions from same-site cookie policy. r=valentin,kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/5524e587eff2
Test web extensions are exempt from samesite cookie policy.r=jkt
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #26)
> Just FYI, I generally prefer these kinds of tests to be xpcshell tests, e.g.,

Ah, thanks for letting me know. Next time I will keep that in mind. We have have a bunch of follow up items from same-site cookies anyway. Probably worth adding such a test. Given the urgency I'll leave the test as is for now.
Blocks: 1455406
https://hg.mozilla.org/mozilla-central/rev/4bf15cbc8ffb
https://hg.mozilla.org/mozilla-central/rev/5524e587eff2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8969261 [details] [diff] [review]
bug_1454914_same_site_cookies_webextensions.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1286861
[User impact if declined]: Web extensions can't access same-site cookies
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: comment 0 if needed
[List of other uplifts needed for the feature/fix]: I think the necessary dependencies of Bug 1286861 have already been uplifted
[Is the change risky?]: no
[Why is the change risky/not risky?]: only affects same site cookies
[String changes made/needed]: no
Attachment #8969261 - Flags: approval-mozilla-beta?
Comment on attachment 8969262 [details] [diff] [review]
bug_1454914_same_site_cookies_webextensions_tests.patch

Approval Request Comment
see comment 30
Attachment #8969262 - Flags: approval-mozilla-beta?
Comment on attachment 8969261 [details] [diff] [review]
bug_1454914_same_site_cookies_webextensions.patch

same-site cookies followup, approved for 60.0b15
Attachment #8969261 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8969262 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have verified that the issue is no longer reproducible on latest Nightly build 61.0a1 (Build ID: 20180419224145), on Windows 10 x64, Mac 10.13.3 and Arch Linux 4.12. When you first use the Firefox Screenshots 30.1.0, the "Edit" (pencil) button has its badge.
Flags: qe-verify+
I successfully reproduced the issue on Nightly Nightly 61.0a1 (2018-04-17) under Windows 10 (x64) using STR from Comment 0.

The issue is fixed on latest Nightly (2018-04-24) and Firefox 60.0b15. Tests were performed under macOS 10.12, Ubuntu 16.04 (x64) and Windows 10 (x64).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.