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

embedded:network/http/client requires headers in lower case #1218

Closed
tve opened this issue Sep 23, 2023 · 3 comments
Closed

embedded:network/http/client requires headers in lower case #1218

tve opened this issue Sep 23, 2023 · 3 comments

Comments

@tve
Copy link
Contributor

tve commented Sep 23, 2023

Build environment: Linux
Moddable SDK version: idf-v5 branch
Target device: esp32-c3

Description
Looking at https://419.ecma-international.org/#-20-http-client-class-pattern I do not see the requirement that the headers passed to the request method be all lower case. I passed ["Content-Length": ...] and this caused no onWritable callback due to the lower-case equality check in https://github.com/Moddable-OpenSource/moddable/blob/public/examples/io/tcp/httpclient/httpclient.js#L343 . Note the same issue for transfer-encoding.

Expected behavior
I expected a case-insensitive comparison or a sentence in the spec to state that the headers must be all lower-case. (I bet the latter is there somewhere and I just haven't seen it....)

@phoddie
Copy link
Collaborator

phoddie commented Sep 24, 2023

The implementation certainly expects lowercase header names on input and generates lowercase headers names on output. When used with Header as the fetch() implementation does, this happens automatically. The spec doesn't say anything about the input headers being lowercase though. The fix is easy, as I'm sure you found. Add the following before line 343:

name = name.toLowerCase();

This allows the script to control the case of the headers sent, in case it matters (it shouldn't).. Alternatively, the toLowerCase() could be done in the assignment in line 341.

I'll commit that so no one else has to trip over this.

Using Map, there is the messy case where it can contain "Content-Length", "content-length", "CONTENT-LENGTH", and more. There's no point in handling that, so I think the spec should just note it as an undefined behavior.

What do you think?

@phoddie
Copy link
Collaborator

phoddie commented Sep 24, 2023

Also.... the HTTP 1.1 specification states:

All transfer-coding names are case-insensitive...

I think that means that the chunked check needs to be updated as well (not that anyone uses "CHUNKED"):

if ("chunked" === item.value[1].toLowerCase())

@phoddie
Copy link
Collaborator

phoddie commented Sep 29, 2023

Fix committed. Closing. (If problem persists, please reopen.)

@phoddie phoddie closed this as completed Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants