Enforce Connection: close for close-delimited responses in HTTP/1.1#13
Closed
blueyetisoftware wants to merge 1 commit intocosock:mainfrom
Closed
Enforce Connection: close for close-delimited responses in HTTP/1.1#13blueyetisoftware wants to merge 1 commit intocosock:mainfrom
blueyetisoftware wants to merge 1 commit intocosock:mainfrom
Conversation
- Add get_connection() method to retrieve Connection header - Modify body_type() to check for protocol violation when close-delimited response lacks Connection: close header in HTTP/1.1 - Prevents hangs on keep-alive connections with malformed servers
Contributor
|
This looks good to me, I've added someone from ST to also review before merging to ensure this will get updated in the hub's libs
Would you be able to provide any details about how you've done this testing to enable others to repeat it? |
Contributor
Author
|
Based on conversation with @cjswedes on the ST forums, I think I am going to pullback on this PR. I misread the spec. The server SHOULD send a 'close', not MUST as I indicated above. My change would make the code non-compliant and overly strict. |
Contributor
Author
|
Closing. Probably not appropriate for pure spec compliance. May be useful for a 'strict' mode at some point in the future. Thanks for the review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses a potential hang issue in the Luncheon HTTP library when parsing responses that lack
Content-LengthorTransfer-Encodingheaders on HTTP/1.1 keep-alive connections. The issue arises because the library correctly follows RFC 9112 by reading the response body until the connection closes, but non-compliant servers may not close keep-alive connections, leading to indefinite waits.To resolve this, we enforce RFC 9112 Section 9.6, which states that servers MUST send
Connection: closefor any HTTP/1.1 message immediately followed by a connection close. Close-delimited responses (without explicit length indicators) inherently require the connection to close, so we now reject such responses ifConnection: closeis not present, preventing hangs on malformed servers.This change improves robustness for embedded systems like SmartThings hubs while maintaining spec compliance for well-behaved servers.
Changes
HttpMessage:get_connection()method: Retrieves and normalizes theConnectionheader value (lowercased), returningnilif absent.HttpMessage:body_type():closeorchunked) without early returns.Connectionis not explicitlyclose, return an error instead of proceeding.Testing
Connection: closeand close-delimiting work as before.Connection: close(or withkeep-alive) now fail fast with a clear error message.Related Issues
Checklist