1387355 - EventSource: ignore IDs with U+0000

Status: RESOLVED FIXED

(Core :: DOM: Networking, enhancement, P3)

Description

[Anne (:annevk)]

Change to standard: https://github.com/whatwg/html/pull/2849

Proposed tests (still need review): https://github.com/w3c/web-platform-tests/pull/6584

It seems good to fix this as this is the only way for someone to smuggle a U+0000 into a header value.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 2

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Is this important? Or a good-first-bug maybe?

Comment 3

[Anne (:annevk)]

It seems bad to violate HTTP invariants, but we managed thus far, so a bit of both?

Comment 4

[Andrea Marchesini [:baku]]

annevk gave a good answer, but it also seems that we support it: we pass the WPT. maybe already fixed?

Comment 5

[Anne (:annevk)]

Hmm yeah, https://hg.mozilla.org/mozilla-central/rev/c9f228c158d7 looks like it might be it. Simply ignoring 0x00 seems a little dangerous though.

Comment 6

[Anne (:annevk)]

I realized that bug 1453318 would obviate the need for that check, though it would also require changing the passing condition of the test mentioned in comment 0 (it'd simply error).

Comment 7

[Kershaw Chang [:kershaw]]

(In reply to Anne (:annevk) from comment #5)
> Hmm yeah, https://hg.mozilla.org/mozilla-central/rev/c9f228c158d7 looks like
> it might be it. Simply ignoring 0x00 seems a little dangerous though.

Maybe we can close this bug, since this is already fixed.
What do yoy think, Anne?

Comment 8

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #7)
> (In reply to Anne (:annevk) from comment #5)
> > Hmm yeah, https://hg.mozilla.org/mozilla-central/rev/c9f228c158d7 looks like
> > it might be it. Simply ignoring 0x00 seems a little dangerous though.
> 
> Maybe we can close this bug, since this is already fixed.
> What do yoy think, Anne?

I just found that our current code [1] does have a problem.
We current ignore the char if it is \0. If we get "id: 1\02", the parser set the id to "12".
According to the spec, we should ignore this value.

So, clear the ni and assign this to myself.


[1] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/dom/base/EventSource.cpp#1761-1763

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Bug 1387355 - Ignore the field value if it contains NULL character

The original code only skip the NULL character, but according to the spec we should ignore the whole field.

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Bug 1387355 - Ignore the field value if it contains NULL character

The original code only skip the NULL character, but according to the spec we should ignore the whole field.

Comment 11

[Anne (:annevk)]

Kershaw, Olli, https://github.com/web-platform-tests/wpt/pull/14095 has a PR to modify the test to demonstrate the issue in Firefox. Other browsers (Chrome, Safari) pass these tests.

(Also, my earlier comments were a bit confused as this doesn't directly impact the HTTP layer in Firefox (and per code inspection that already has guards for 0x00 and friends), this is only about parsing the text/event-stream format.)

Comment 12

[Kershaw Chang [:kershaw]]

Not work on this actively.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Farooq, perhaps you'd be interested in this :)

Comment 14

[Farooq AR]

Sure. Make this mine :)

Comment 15

[Farooq AR]

Attachment: Bug 1387355 - EventSource: ignore IDs with U+0000. r=smaug

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2aa24254daa1
EventSource: ignore IDs with U+0000. r=smaug

Comment 17

[Dorel Luca [:dluca]]

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