fix: don't write POST body to a request that will be hijacked for stdin - #198
Open
mliem2k wants to merge 1 commit into
Open
fix: don't write POST body to a request that will be hijacked for stdin#198mliem2k wants to merge 1 commit into
mliem2k wants to merge 1 commit into
Conversation
Modem#dial always JSON-stringifies its options object and buildRequest() unconditionally req.write()s it for POST requests. For attach()/exec-start() calls with hijack:true + openStdin:true, that write happens before the HTTP connection upgrades to a raw duplex stream, so the serialized options end up delivered to the remote container's stdin as if a user had typed them. buildRequest() already skips req.end() in this case; extend the same !context.openStdin guard to the req.write()/pipe() call above it, since attach/exec-start never need a body (their parameters are query-string based). Skipping the write also means the request headers are no longer implicitly flushed by that write() call, so the connection would otherwise never reach the server to upgrade. Add an explicit req.flushHeaders() for the openStdin case to send the headers without a body.
3 tasks
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.
Summary
Modem#dialalways JSON-stringifies its options object for POST requests andbuildRequest()unconditionallyreq.write()s it, with no guard for theopenStdin/hijackcase. Forcontainer.attach()/exec.start()calls, that write happens before the HTTP connection upgrades to a raw duplex socket, so the serialized options object gets delivered straight into the remote container's stdin.buildRequest()already skipsreq.end()in this case; this PR extends the same!context.openStdinguard to thereq.write()/.pipe()call above it, and adds an explicitreq.flushHeaders()for that branch, since the write being removed was also implicitly what flushed the request headers to the socket (without it, an openStdin request would hang instead of upgrading).Found via a downstream MCSManager bug report where users saw the literal attach-options JSON auto-typed into their container's terminal on every start: MCSManager/MCSManager#2136
Test plan