1454242 - Login on https://testrail.stage.mozaws.net/ fails after same-site cookies set by cross site requests have been forbidden

Status: RESOLVED FIXED

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

Description

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

Bisection with Nightly on Windows confirms that the patches for bug 1452496 broke the login on https://testrail.stage.mozaws.net/

Steps to reproduce:
1. Open e.g. https://testrail.stage.mozaws.net/index.php?/plans/view/8760
2. Login with your LDAP and second factor.

Actual result:
File not found

An error occurred during a connection to testrail.stage.mozaws.net.

    Check the file name for capitalization or other typing errors.
    Check to see if the file was moved, renamed or deleted.

Expected result:
Testrail's own login form to login with LDAP shown.

needinfo for Miles as the op for testrail and Christoph as developer of bug 1452496 which has a pending beta uplift.
CC Julien and Ryan because of the pending uplift.
CC Andrei, Cristian and Rares in case SV might also work with testrail on Nightly.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1454242_same_site_cross_origin_context.patch

Valentin,

within Bug 1452496 we started to not allow same-site cookies being set from within a cross origin context. Unfortunately I used NS_IsSameSite() instead of simply consulting the third party util to determine whether a same-site cookie is allowed to be set. NS_IsSameSite uses the triggeringPrincipal for top-level loads [1] to determine whether samesite cookies in lax mode should be send. In this case we not only checked for sending same-site cookies, but also for setting, which is different and wrong.

In more detail, the URI https://testrail.stage.mozaws.net/index.php?/plans/view/8760 wants to set samesite cookie in 'lax' mode; when setting the cookie on the top-level page we used the triggeringPrincipal https://auth.mozilla.auth0.com/ which did not allow the same site cookie to be set. Simply using the thirdparty util within CanSetCookie() does the right check and causes the problem from comment 0 work correctly again.

[1] https://hg.mozilla.org/mozilla-central/rev/453b91e427d6#l1.25

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=c323023261ae432179816adfa36e57f2a7327dae

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8968078 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context.patch

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

Glad we caught it early.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1454242_same_site_cross_origin_context_tests.patch

Mark, since we need to uplift that change I suppose it's wise having an automated test for that as well. Please see a description of the problem in comment 1 - thanks!

Comment 5

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8968114 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context_tests.patch

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

Good catch!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1fdf8c83da
Setting samesite cookie should not rely on NS_IsSameSiteForeign. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e2ab3b7a26
Test samesite cookie on top-level page from cross-origin context. r=mgoodwin

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8968078 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1452496
[User impact if declined]: does not allow same-site cookies to be set if top-level page was triggered by cross origin page
[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]: no
[List of other uplifts needed for the feature/fix]: Bug 1452496
[Is the change risky?]: no
[Why is the change risky/not risky?]: only affects same-site cookies
[String changes made/needed]: no

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8968114 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context_tests.patch

Approval Request Comment
see comment 7

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8968078 [details] [diff] [review]
bug_1454242_same_site_cross_origin_context.patch

same-site cookies followup, approved for 60.0b13

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a8a5ba53d104
https://hg.mozilla.org/releases/mozilla-beta/rev/7cb7bafe04e4

Comment 11

[Brian Pitts]

We have several admin panels using lua-resty-openidc (Balrog, Normandy, probably others) that are affected by this.

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3c1fdf8c83da
https://hg.mozilla.org/mozilla-central/rev/b5e2ab3b7a26