Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EventSource] Is Last-Event-ID header value allowed to contain any-char? #689

Closed
yutakahirano opened this issue Feb 12, 2016 · 18 comments
Closed
Assignees

Comments

@yutakahirano
Copy link
Member

A Last-Event-ID header value is set to the last event ID string which can contain any chars except CR and LF. On the other hand, the Fetch spec requires that a header value consists of field-content octets. The former allows non-CRLF control octets while the latter doesn't.

@yutakahirano
Copy link
Member Author

cc: @annevk

@annevk
Copy link
Member

annevk commented Feb 12, 2016

See also whatwg/fetch#115. I'm happy to align this with whatever implementations implement. @hiroshige-g did not get back to me on that yet.

@yutakahirano
Copy link
Member Author

Chrome:

  • When the ID contains "\f", the reconnection request contains a Last-Event-ID header with the received ID.
  • When the ID contains "\0", the reconnection request contains a Last-Event-ID header with a trimmed value (e.g. If "abc\0def" was received, "abc" will be sent).

Firefox:

  • When the ID contains "\f", the reconnection request contains a Last-Event-ID header with the received ID.
  • When the ID contains "\0", the reconnection request doesn't contain a Last-Event-ID header.

@yutakahirano
Copy link
Member Author

I would like to ignore an ID value including any control characters when parsing it.

@annevk
Copy link
Member

annevk commented Mar 14, 2016

Control being anything less than 0x20 or higher than 0x7E?

@annevk
Copy link
Member

annevk commented Mar 14, 2016

Ooh, seems like the specification allows for non-ASCII values.

@yutakahirano
Copy link
Member Author

The EventSource spec allows any octet composing a valid utf-8 string other than \r and \n while the HTTP spec allows only us-ascii printable chars (VCHAR).

@annevk
Copy link
Member

annevk commented Mar 15, 2016

You preference would be that we require VCHAR in the specification instead and ignore anything outside?

@annevk
Copy link
Member

annevk commented Mar 15, 2016

@wanderview, do you know who to ask from Mozilla about this?

@yutakahirano
Copy link
Member Author

Both are acceptable, but I'm leaning towards ignoring an id field containing non-VCHARs. It looks consistent with the fact that we ignore an invalid retry field and other unrecognized fields.

@overholt
Copy link

@mcmanus can probably comment from Mozilla's standpoint.

@annevk
Copy link
Member

annevk commented Jul 18, 2017

@yutakahirano given what we decided for https://fetch.spec.whatwg.org/#concept-header-value I think UTF-8 bytes except for 0x00, 0x0A, and 0x0D is the way to go. Let me know if you disagree.

@yutakahirano
Copy link
Member Author

Does it mean Firefox's behavior in #689 (comment)?

@annevk
Copy link
Member

annevk commented Jul 18, 2017

Yeah, ignoring altogether if it doesn't meet the criteria seems best. Dropping part of the value seems more dangerous.

@yutakahirano
Copy link
Member Author

SGTM

@annevk
Copy link
Member

annevk commented Jul 19, 2017

@yutakahirano I think it would be even better if we didn't set evt.lastEventId at all if the id field contained U+0000. It does seem however that current implementation do include it and I also cannot reproduce your Chrome behavior (it does not appear to trim the value). See web-platform-tests/wpt#6584 for my test.

@annevk
Copy link
Member

annevk commented Jul 19, 2017

(It's also odd that browsers do not have the U+0000 restriction on header values here while they do have that in other APIs...)

@annevk
Copy link
Member

annevk commented Jul 19, 2017

#2849 has my proposal for how to change the HTML Standard. It would require changes by all implementations though, so feedback appreciated.

domenic pushed a commit that referenced this issue Aug 3, 2017
cdumez pushed a commit to web-platform-tests/wpt that referenced this issue Aug 16, 2017
…+0000 (#6584)

* EventSource: do not allow lastEventId creation when the id contains U+0000

See whatwg/html#689.
MXEBot pushed a commit to mirror/chromium that referenced this issue Sep 20, 2017
whatwg/html#689

Bug: 752421
Change-Id: I12d9cc6f4556011729e1f4245868b864718e1f08
Reviewed-on: https://chromium-review.googlesource.com/666366
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Takeshi Yoshino <[email protected]>
Cr-Commit-Position: refs/heads/master@{#502763}
rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this issue Nov 8, 2017
…+0000 (web-platform-tests#6584)

* EventSource: do not allow lastEventId creation when the id contains U+0000

See whatwg/html#689.
nareix pushed a commit to nareix/webrtc.third_party that referenced this issue Mar 5, 2018
whatwg/html#689

Bug: 752421
Change-Id: I12d9cc6f4556011729e1f4245868b864718e1f08
Reviewed-on: https://chromium-review.googlesource.com/666366
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Takeshi Yoshino <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#502763}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 33e02f52df5064f3e8e06eaed5c33ebf93aa6b70
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants
@overholt @annevk @yutakahirano and others