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

Fix unterminated chunk code #301

Merged
merged 1 commit into from
Jun 7, 2023
Merged

Fix unterminated chunk code #301

merged 1 commit into from
Jun 7, 2023

Conversation

hannahhoward
Copy link
Collaborator

Goals

While our go integration tests showed our unterminated chunk code to indicate a premature close was working, curl and wire-shark showed a different story.

The old code was simply taking the conn and writing a "0\r\n" to it. However, it wasn't closing the conn, and because we hadn't told golang's HTTP code that we were taking over the conn, after our handler completed, go would still write the termination code: "0\r\n\r\n". So what we'd end up with is a response that ended "0\r\n0\r\n\r\n".

after "0\r\n" is written, everything before \r\n is a trailer header. Go's HTTP client attempts to parse trailer headers, so when it read the second "0\r\n" it would error cause it's an incorrectly formatted header (the source of our weird "malformed MIME header" error message we've been seeing in tests).

however, node, curl, and browser clients ignore this portion and just make sure the response terminates, so they would see the /r/n and consider it valid.

Implementation

We need to actually close the connection, which also means "Hijack"ing it so go's http server doesn't try to keep using it.

This PR does that and fixes the problem of early terminating chunked streams correctly.

Copy link
Contributor

@willscott willscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for tracking down / catching!

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2023

Codecov Report

Merging #301 (11ed6b7) into main (fe9e760) will increase coverage by 0.31%.
The diff coverage is 45.45%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
+ Coverage   72.55%   72.87%   +0.31%     
==========================================
  Files          68       68              
  Lines        6217     6252      +35     
==========================================
+ Hits         4511     4556      +45     
+ Misses       1458     1450       -8     
+ Partials      248      246       -2     
Impacted Files Coverage Δ
pkg/server/http/ipfs.go 63.08% <45.45%> (-0.85%) ⬇️

... and 7 files with indirect coverage changes

@hannahhoward
Copy link
Collaborator Author

two commits I pushed have no shown up on this PR due to: https://www.githubstatus.com/ -- some problem with PRs

@hannahhoward hannahhoward force-pushed the feat/fix-unclean-close branch 3 times, most recently from 27e39a9 to 11ed6b7 Compare June 7, 2023 17:52
@hannahhoward hannahhoward merged commit 0618cd4 into main Jun 7, 2023
21 of 23 checks passed
@hannahhoward hannahhoward deleted the feat/fix-unclean-close branch June 7, 2023 20:09
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

Successfully merging this pull request may close these issues.

None yet

3 participants