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

Parse folded HTTP field values according to RFC 7230 #53227

Closed
brianquinlan opened this issue Aug 15, 2023 · 6 comments
Closed

Parse folded HTTP field values according to RFC 7230 #53227

brianquinlan opened this issue Aug 15, 2023 · 6 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-request This tracks requests for feedback on breaking changes cherry-pick-approved Label for approved cherrypick request library-io

Comments

@brianquinlan
Copy link
Contributor

brianquinlan commented Aug 15, 2023

Change Intent

The intent of this change is to correctly parse folded header field values contained (through HttpHeaders) in HttpRequest and HttpClientResponse.

For example, an HTTP request like:

POST /test HTTP/1.1
Header-A: cat\r
 food\r
Header-B: cat \r <- note space before carriage return
 food\r

Would be parsed like:

// CURRENT BEHAVIOR: 
assert(headers['header-a'] == 'catfood');
assert(headers['header-b'] == 'cat food');
// PROPOSED BEHAVIOR:
assert(headers['header-a'] == 'cat food');
assert(headers['header-b'] == 'cat  food');

Justification

RFC 7230 Section 3.2.4 says:

...replace each received obs-fold with one or more SP octets
prior to interpreting the field value or forwarding the
message downstream.

The amount of interior whitespace in an HTTP header value is not significant according to RFC 2616 2.2:

All linear white space, including folding, has the same semantics as SP

Impact

Header folding was deprecated in RFC 7230 (published in 2014) so this is change is unlikely to have a significant impact.

Code that (against the standard) relies on particular whitespace in folded header values will be broken.

Mitigation

I think that this is unlikely to have significant impact.

@brianquinlan brianquinlan added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io breaking-change-request This tracks requests for feedback on breaking changes labels Aug 15, 2023
@brianquinlan brianquinlan changed the title Parse HTTP header folding according to RFC 7230 Parse folded HTTP field values according to RFC 7230 Aug 15, 2023
@brianquinlan
Copy link
Contributor Author

@itsjustkevin I think that this is ready for sign off.

@itsjustkevin
Copy link
Contributor

@vsmenon @Hixie @grouma another breaking change request, hot off the presses!

@Hixie
Copy link
Contributor

Hixie commented Aug 28, 2023

SGTM, but didn't we just make a change to remove trailing whitespace? Should we also fold inner whitespace since it's not semantically meaningful? Anyway, either way is fine by me.

@grouma
Copy link
Member

grouma commented Aug 29, 2023

SGTM.

@brianquinlan
Copy link
Contributor Author

SGTM, but didn't we just make a change to remove trailing whitespace? Should we also fold inner whitespace since it's not semantically meaningful? Anyway, either way is fine by me.

My intuition, unsupported by any data, is that folding internal whitespace is likely to break things because developers aren't aware that folding is allowed.

@vsmenon
Copy link
Member

vsmenon commented Sep 11, 2023

lgtm

@itsjustkevin itsjustkevin added the cherry-pick-approved Label for approved cherrypick request label Sep 11, 2023
copybara-service bot pushed a commit that referenced this issue Sep 11, 2023
Bug: #53227
Bug: #53185
Change-Id: Ibbdbdf9c8f2875e8f687244982810fffee20e69c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/320920
Commit-Queue: Brian Quinlan <bquinlan@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-request This tracks requests for feedback on breaking changes cherry-pick-approved Label for approved cherrypick request library-io
Projects
None yet
Development

No branches or pull requests

6 participants
@Hixie @grouma @vsmenon @brianquinlan @itsjustkevin and others