1901325 - SolarWinds Papertrail dashboard does not update when selecting a different organization from the dropdown menu in Nightly

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[Bianca Danforth [:bdanforth]]

Attachment: Solarwinds_Papertrail_dashboard_organization_selection_dropdown.png

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

Steps to reproduce

1. Have access to at least two different SolarWinds Papertrail organizations (Mozilla has 4 that I'm aware of).
2. Log into Solarwinds Papertrail.
3. Navigate to https://papertrailapp.com/
4. Take note of what the current dashboard looks like.
5. In the top right corner, click the current organization name (Mozilla Marketing in the attached screenshot) and select a different organization from the dropdown.

Expected results

• The organization listed in the top right corner changes to the newly selected, different organization.
• The dashboard for the newly selected organization is displayed.

Actual results

• The organization listed in the top right corner does not change.
• The dashboard for the previously selected organization is displayed.

Notes

• Two other teammates were also able to reproduce this in Nightly 128.0a1.
• I do not see this bug in release Firefox 126.0.1.

Comment 1

[Sven Marnach :sven]

The regression happened in this range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=29f2fb6a6189c4d600b770246fb072374f095e63&tochange=11097fcc4f76cc50b606c9e42981d320aeed176a

In the bad versions, the console shows errors like this one:

Cookie “last_customer_id” has been rejected for invalid characters in the attributes.

Comment 2

[Yarik Kurmyza [:yarik] (he/him) (UTC+1)]

Attachment: image.png

I had experienced similar issue when trying to access "Events" section on papertrails. It is located on a sub-domain and Nightly keeps redirecting between auth endpoints.
Sven might be right, as I also see "Cookie "user_credentials" has been rejected for invalid characters in the attributes." messages

That cookie value looks like user_credentials: 1111::222 - it contains :: which might be triggering the error above?

v129 - not working, v126 - working

UPD: Papertrail server is also likely doing something strange, I've contacted their support.
They send empty cookies in response:

< HTTP/1.1 302 Found
< Date: Thu, 13 Jun 2024 08:17:48 GMT
< Content-Type: text/html; charset=utf-8
< Content-Length: 101
< Status: 302 Found
< Cache-Control: no-cache
< Strict-Transport-Security: max-age=31536000
< Location: https://my.papertrailapp.com/events
< X-Frame-Options: SAMEORIGIN
< Set-Cookie: user_credentials=2222111103cf081d9623bd7d5f2368b735%3A%3A35805491; path=/; expires=Fri, 13-Sep-2024 08:17:48 GMT; secure; HttpOnly
< Set-Cookie: ; secure
< Set-Cookie: last_customer_id=; path=/; expires=Thu, 01-Jan-1970 00:00:00 GMT; secure
< Set-Cookie: ; secure
< Set-Cookie: last_customer_id=AAIlDw%3D%3D--e949a92222b61206b; domain=.papertrailapp.com; path=/; expires=Fri, 13-Sep-2024 08:17:48 GMT; secure; HttpOnly
< Set-Cookie: _papertrail_session=AAAAAmQ3ZDVmMjM2OGI3MzUGOwZUSSIYdXNlcl9jcmVkZW50aWFsc19pZAY7BlRpBDNZIgI%3D--090af763674ee6fa80464509df1cd8e34af03a09; path=/; secure; HttpOnly
<

Comment 3

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

Not sure if it's valid to have Set-Cookie: ; secure in the headers, but I've reproduced this with a local test case.
Bug 1892748 started rejecting cookie headers that contain control characters like \n, but our cookie header merging introduces \n when it sees multiple cookie headers, thus causing the bug.

Comment 4

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

Removing the two ; secure lines allows the cookie to be set.

Comment 5

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

Attachment: Bug 1901325 - Backed out changeset 11097fcc4f76 r=pbz

This patch causes Firefox to mistakenly reject cookies if the following cookie
header appears to be a continuation of the previous one.
This is because when cookie headers get merged the are separated by a \n
character which is considered invalid since bug 1892748.

Comment 6

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

Attachment: Bug 1901325 - add test that ;secure Set-Cookie header doesn't affect previous cookie's value r=pbz

The Ignore name- and value-less Set-Cookie: ; bar test was also failing
before, but due to bug 1848226 it wasn't removed in bug 1892748.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2571b574b2c
Backed out changeset 11097fcc4f76 r=pbz,cookie-reviewers
https://hg.mozilla.org/integration/autoland/rev/a60507c6dfe7
add test that ;secure Set-Cookie header doesn't affect previous cookie's value r=edgul

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46732 for changes under testing/web-platform/tests

Comment 9

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

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

Comment 10

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

Attachment: Bug 1901325 - Backed out changeset 11097fcc4f76 r=pbz

This patch causes Firefox to mistakenly reject cookies if the following cookie
header appears to be a continuation of the previous one.
This is because when cookie headers get merged the are separated by a \n
character which is considered invalid since bug 1892748.

Original Revision: https://phabricator.services.mozilla.com/D213547

Comment 11

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

Attachment: Bug 1901325 - add test that ;secure Set-Cookie header doesn't affect previous cookie's value r=pbz

The Ignore name- and value-less Set-Cookie: ; bar test was also failing
before, but due to bug 1848226 it wasn't removed in bug 1892748.

Original Revision: https://phabricator.services.mozilla.com/D213551

Comment 12

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

Attachment: server.js

STR: run node server.js then open http://localhost:3000

Open devtools, and check that:

1. No cookie rejected error messages are present
2. That the cookies are present in the Storage > Cookies section of devtools.

Delete the cookies after finishing the test case

Comment 13

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Login issues on Solarwinds or potential sites misbehaving
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: See https://bugzilla.mozilla.org/show_bug.cgi?id=1901325#c12
• Risk associated with taking this patch: Low risk.
• Explanation of risk level: This merely backs out the regressing patch.
• String changes made/needed: none
• Is Android affected?: yes

Comment 14

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

Attachment: Bug 1901325 - Backed out changeset 11097fcc4f76 r=pbz

This patch causes Firefox to mistakenly reject cookies if the following cookie
header appears to be a continuation of the previous one.
This is because when cookie headers get merged the are separated by a \n
character which is considered invalid since bug 1892748.

Original Revision: https://phabricator.services.mozilla.com/D213547

Comment 15

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

Attachment: Bug 1901325 - add test that ;secure Set-Cookie header doesn't affect previous cookie's value r=pbz

The Ignore name- and value-less Set-Cookie: ; bar test was also failing
before, but due to bug 1848226 it wasn't removed in bug 1892748.

Original Revision: https://phabricator.services.mozilla.com/D213551

Comment 16

[Phabricator Automation]

release Uplift Approval Request

• User impact if declined: Login issues on Solarwinds or potential sites misbehaving
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: See https://bugzilla.mozilla.org/show_bug.cgi?id=1901325#c12
• Risk associated with taking this patch: Low
• Explanation of risk level: This merely backs out the regressing patch.
• String changes made/needed: None
• Is Android affected?: yes

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/f666ca68aabf
https://hg.mozilla.org/releases/mozilla-beta/rev/27aa6b5ac3a4

Comment 18

[Aron Cseh]

https://hg.mozilla.org/mozilla-central/rev/b2571b574b2c
https://hg.mozilla.org/mozilla-central/rev/a60507c6dfe7

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 20

[Ciprian Georgiu, Desktop QA]

I've reproduced the issue using the STR from comment 12, using an affected Firefox Beta 128.0b2 build.

The issue is verified as fixed on latest Nightly 129.0a1 and Beta 128.0b3 running macOS 14, Ubuntu 20.04 and Win 11.

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/46254373281c
https://hg.mozilla.org/releases/mozilla-release/rev/e2a9707b7337

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/201d5f176950

Comment 24

[Emma Zühlcke [:emz]]

It looks like solarwinds fixed the issue on their end. Here are new STR to verify the issue:

1. With a fresh profile go to https://complete-horn-glass.glitch.me/test-bug-1901325-get The site may take a few seconds to launch. After that it should show cookies:undefined
2. Navigate to https://complete-horn-glass.glitch.me/test-bug-1901325-set The site shows "Cookie has been set"
3. Navigate back to https://complete-horn-glass.glitch.me/test-bug-1901325-get and check the site output

Expected result:
The site prints cookies:cookieA=valueA; cookieC=valueC; cookieD=valueD

Actual result:
The site prints cookies:undefined

Comment 25

[Adina Petridean QA [:apetridean]]

Verified as fixed on the Android side with Firefox 127.0.1 using the following devices: Samsung A32 (Android 13), Samsung Galaxy S23 Ultra (Android 14), Google Pixel 7 Pro (Android 14), and Motorola G9 Plus (Android 11).

Comment 26

[Peter Magyari (Desktop QA)]

Verified fixed using Firefox 127.0.1 (20240617164919) on MacOS 14, Windows 10 and Ubuntu 24.04.