Fix IndexDefect in httpclient and fix parsing of multiline headers
#19262
Conversation
|
Also, we might want to check that |
ghost
marked this pull request as draft
|
Currently fixed by exporting |
lib/pure/httpclient.nim
Outdated
| var leadingSpaces = skipWhitespace(line, linei) | ||
| if leadingSpaces == 0: | ||
| var le = parseUntil(line, name, ':', linei) | ||
| if le <= 0 or le >= line.len: httpError("invalid headers") |
There was a problem hiding this comment.
le >= line.len makes httpclient a little more stricter regarding random lines it finds in the response - basically, if the response is like:
Header-One: value
Header-Two: value
somerandomname
httpclient will error out on somerandomname. Chromium and Firefox don't error out on that and just skip the line - should we follow them or throw an error?
lib/pure/httpclient.nim
Outdated
| # that should be appended to the previous header, see bug #19261 | ||
| # Also, if there was no header before this, we just ignore the line | ||
| elif prevHeader != "": | ||
| result.headers.table[result.headers.toCaseInsensitive(prevHeader)][^1].add line.strip() |
There was a problem hiding this comment.
We know that prevHeader is already in the table because we check if it's empty or not.
ghost
changed the title
IndexDefect in httpclient and fix parsing of multiline headers
|
This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions. |
ghost
marked this pull request as ready for review
|
I'll see if I can either try to test this PR more properly to make it ready to merge, or make a smaller PR just to fix the IndexDefect without changing anything else. I hope we can get this into 2.0, sorry for not doing it before |
|
Going to open a new PR instead |
ghost
closed this
ghost
mentioned this pull request
Continuation of #19262 Fixes #19261 The parsing code is still too lenient (e.g. it will happily parse header names with spaces in them, which is outright invalid by the spec), but I didn't want to touch it beyond the simple changes to make sure that `std/httpclient` won't throw `IndexDefect`s like it does now on those cases: - Multiline header values - No colon after the header name - No value after the header name + colon One question remains - should I keep `toCaseInsensitive` exported in `httpcore` or just copy-paste the implementation? --------- Co-authored-by: Andreas Rumpf <[email protected]>
Follow the part of the HTTP spec regarding headers - this solves the actual issue in #19261 as well as makes
httpclientfollow the spec more closely. I've chosen not to enforce the spec to the level Firefox does (regarding disallowing spaces after the header name but before the value) as I've wanted to limit this change just to fix the bug that I encountered.Also, I don't know how I should make the tests for this.