1937905 - A document should be added to DOM indicating network errors due to invalid header values

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[[:ats]]

Steps to reproduce:

Visit a webpage which returns a header containing a null value (\x00).

Actual results:

• Firefox returns a network error (NS_ERROR_DOM_INVALID_HEADER_VALUE) as per spec.
• No document is added to DOM indicating error.

Expected results:

An error document (eg: about:error) should be added to DOM for better user experience. This also causes tests for headers with null values to fail.

This is a follow-up bug from https://bugzilla.mozilla.org/show_bug.cgi?id=1891465 & https://github.com/web-platform-tests/wpt/pull/48855.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::Networking' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Kershaw Chang [:kershaw]]

Hi Sean,

I think you might be interested in this bug as well.

Thanks.

Comment 3

[Sean Kim [:sekim]]

Attachment: Bug 1937905 - Part 1: Add a neterror page for DOM indicating network errors due to invalid header values r=#necko

Comment 4

[Sean Kim [:sekim]]

Hello Kershaw,

Do you have any suggestions for the error message?

Thank you

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Sean Kim from comment #4)

Hello Kershaw,

Do you have any suggestions for the error message?

Thank you

Maybe something like:

A header received from the server contains prohibited null characters (0x00). 

I would suggest you to also ask in #content_design.
Thanks.

Comment 6

[Sean Kim [:sekim]]

Attachment: Bug 1937905 - Part 2: Add a test for a neterror page due to invalid header values r=#necko

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e37c1dbf5004
Part 1: Add a neterror page for DOM indicating network errors due to invalid header values r=fluent-reviewers,bolsson
https://hg.mozilla.org/integration/autoland/rev/f7983c22a75f
Part 2: Add a test for a neterror page due to invalid header values r=necko-reviewers,kershaw

Comment 8

[amarc]

https://hg.mozilla.org/mozilla-central/rev/e37c1dbf5004
https://hg.mozilla.org/mozilla-central/rev/f7983c22a75f

Comment 9

[Sean Kim [:sekim]]

Attachment: Bug 1937905 - DOM's appstrings.properties should include entity for invalidHeaderValue r=flod

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3b5deda2492
DOM's appstrings.properties should include entity for invalidHeaderValue r=flod DONTBUILD

Comment 11

[Sandor Molnar[:smolnar]]

Backed out for causing lint failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/ca1baad7a3999872b8055a17de5b89ebffd96aa8

Push with failures

Failure log -> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/locales/en-US/chrome/overrides/appstrings.properties:49:1 | Duplicate string with ID: invalidHeaderValue (l10n)

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/e3b5deda2492

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ad61b4ea16d
DOM's appstrings.properties should include entity for blockedByCORP and invalidHeaderValue r=flod

Comment 14

[Francesco Lodolo [:flod]]

Making significant changes to a patch after review is generally not OK.

Recording here for a follow-up, since there is already a needinfo open.

+blockedByCORP=Firefox didn’t load this page because it looks like the security configuration doesn’t match the previous page.

DOM is supposed to be browser-agnostic, so having "Firefox" is a string is incorrect (although there is at least one in the file).

The browser didn’t load this page because it looks like the security configuration doesn’t match the previous page.

Even the string in /browser shouldn't have Firefox hard-coded (linters would have caught that for .ftl), it should be a placeholder replaced at run-time. This makes it impossible for other browsers to expose their own name instead of Firefox.

Note that the fix will require a new string ID.

Comment 15

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/6ad61b4ea16d

Comment 16

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 17

[Sean Kim [:sekim]]

(In reply to Francesco Lodolo [:flod] from comment #14)

Making significant changes to a patch after review is generally not OK.

Recording here for a follow-up, since there is already a needinfo open.

+blockedByCORP=Firefox didn’t load this page because it looks like the security configuration doesn’t match the previous page.

DOM is supposed to be browser-agnostic, so having "Firefox" is a string is incorrect (although there is at least one in the file).

The browser didn’t load this page because it looks like the security configuration doesn’t match the previous page.

Even the string in /browser shouldn't have Firefox hard-coded (linters would have caught that for .ftl), it should be a placeholder replaced at run-time. This makes it impossible for other browsers to expose their own name instead of Firefox.

Note that the fix will require a new string ID.

Sorry about that, I assumed that it was fine to do so since I copied the lines of code from appstring.properties in browser/. I just filed a new bug -- https://bugzilla.mozilla.org/show_bug.cgi?id=1952234 -- we should back this patch out and only add invalidHeaderValue. Bug 1952244 should handle a browser-agnostic message for blockedByCORP.

Comment 18

[Atila Butkovits]

Backed out as requested: https://hg.mozilla.org/integration/autoland/rev/23ff917a28f60182ae32440b618ffe2a787e20b3

Comment 19

[Cosmin Sabou [:CosminS]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/23ff917a28f6

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f3369435b8b
DOM's appstrings.properties should include entity for invalidHeaderValue r=flod

Comment 21

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/2f3369435b8b