Repository navigation
Conversation
ff05d69 to
f154711
Compare
Accept's rejection responses had no test that checked the status, headers and body together. Pinning them for every rejection before the 101 lets a change to how the response is written prove that the default output stays the same.
Accept always wrote a plain text http.Error response when it rejected a handshake, so servers with a structured error format had to write a second response, which net/http appends to Accept's body and logs as a superfluous WriteHeader call. ErrorHandler lets the caller write that response instead, with the same signature as gorilla's Upgrader.Error; nil keeps the previous output. Refs #545
f154711 to
0eca8fe
Compare
This comment has been minimized.
This comment has been minimized.
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 4 | Last posted: Round 4, 19 findings (6 P3, 9 P4, 4 Nit), COMMENT. Review Finding inventoryFinding inventory: coder/websocket PR #588Findings
Contested and acknowledgedCRF-14 (P4, accept.go:91) - Name ErrorHandler implies all Accept errors
Round logRound 1Full panel (round 1). Netero: 1 P3, 1 Note, no P0. Panel: ging-go, ryosuke, kurapika, pariston, mafuuu, bisky, gon, leorio, hisoka, chopper, mafu-san; wildcard meruem (random from distilled set). Kurapika added for the origin rejection path, which put the panel in the 9-12 target. Posted 5 P3, 3 P4, 2 Nit. Gon's comment-wording P2s rated Nit (style, code correct) or dropped. Mafu-san's P2 on rejection table coverage set to P3: both missing branches go through the tested call site at accept.go:136 in unchanged authenticateOrigin; the gap is the default-response pin and the claim. PR description items (CRF-16, CRF-17) have no file location and are posted as a body process observation. Orchestrator verified CRF-3 (1/30 failures of TestAcceptErrorHandler filtered run) and CRF-21 (net/http server.go:1172 excludes 101 from 1xx handling). Reviewed against e1a21e5..0eca8fe. Round 2Fix round: no files added or renamed, no exported signature changed, no function rewritten, +47 effective additions since R1. Churn guard PROCEED: 9 addressed, 1 contested (CRF-14). CRF-11 reopened because 56eccc2 passes the pattern error to ErrorHandler, removing the drop premise. Netero: 1 Note (CRF-26). Panel: gon (resumed R1 agent for CRF-14 dispute check-in), bisky, pariston, meruem, leorio, hisoka, chopper, ryosuke, mafu-san (raised findings the new commits touch), ging-go, kurapika (changed Go files and origin path); wildcard kite. All verified fixes held (CRF-1, 3, 4, 5, 6, 7, 8, 10, 12). CRF-14 closed 1/1. New: 2 P4 (CRF-29, CRF-30), 1 Nit (CRF-27); CRF-11 re-raised and posted at P4 (inaccurate test comment, no runtime effect). Gon's CRF-27 P2 rated Nit (wording of a correct doc). Gon's CRF-28 P4 moved to Note (body): keep-argument is that an unexplained header token leaves readers unsure whether the leak fix depends on it; it stays a note because Round 3Fix round: 4 commits since R2 change docs, one error string, and one test comment; no files added, no exported signatures changed. Churn guard PROCEED: CRF-11, CRF-27, CRF-29, CRF-30 addressed. Netero: no findings, all four fixes verified. Panel: gon, leorio (raised the fixed findings), kurapika (origin path), ging-go, ryosuke (Go files); wildcard kite. All four fixes verified. New: 1 P4 (CRF-33, convergent Ryosuke and Kite), 1 Note (body). Reviewed against e1a21e5..f7f3599. Round 4Restructure round: the PR description adds a new behavior change to OriginPatterns validation (61a7faf, 618a580, e6ff172) beyond the ErrorHandler goal. Churn guard PROCEED: CRF-33 addressed. Netero: no findings, 1 Note. Full panel (multi-domain, security-sensitive origin code, target 9-12): ging-go, kurapika, ryosuke, killua (specialists); pariston, mafuuu (core); bisky, hisoka, chopper, gon, leorio, knov; wildcard zoro. Eight reviewers independently verified that path.Match(p, "") reports every syntax error that matching against a target reports (up to 8.5M fuzz executions), so the discarded match error is safe. New: 1 P3 (CRF-36), 2 P4, 1 Nit, 1 OOS. Reviewed against e1a21e5..e6ff172. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds AcceptOptions.ErrorHandler, which writes the HTTP response when Accept rejects a handshake request before the 101 response. 5 P3, 3 P4, 2 Nit.
PR description: "For four rejections, the raw responses match master byte for byte" names neither the four rejections nor the command, so a reviewer cannot repeat it; a real-server comparison of all 12 rejections a server can produce matched base e1a21e5 byte for byte excluding Date, so the sentence can say that. The PR body says Fixes #545 and commit 0eca8fe says Refs #545; use one keyword in both.
Out of scope (needs a ticket or explicit acceptance by a human):
accept.go:175: afterhj.Hijack()fails,Acceptcallshttp.ErrorafterWriteHeader(101). net/http treats 101 as the final status (net/http/server.goexcludesStatusSwitchingProtocolsfrom 1xx handling), so the 500 is a superfluousWriteHeaderand the client receives a 101 with no usable connection.accept.go:143: "http.ResponseWriter does not implement http.Hijacker" does not name the usual causes (an HTTP/2 request, or middleware that wraps theResponseWriterwithoutUnwrap), so an operator has to find the cause without help.accept.go:91: rejection reasons have no stable identity (status 400 covers four reasons), so a handler that maps reasons to RFC 9457typevalues must match error strings. The decision log defers sentinel errors without a ticket.accept.go:207:verifyClientRequestnames the statuserrCode, whileErrorHandlerandwriteRejectionname the same valuestatus.accept.go:385,accept_test.go:566, and other unchanged lines:go fix -diff .reports astrings.SplitSeqrewrite and deadtc := tccopies.
Notes:
accept_test.go:211:newHandshakeRequestbuilds the same request that severalTestAcceptcases build inline; the inline copies predate this PR.accept_test.go:229: in commit 7bd7405, thewantHeadercomment refers to a handler that is added in 0eca8fe.
🤖 This review was automatically generated with Coder Agents.
The doc comment on writeRejection only existed to explain that body is the fallback text for a nil ErrorHandler. The name defaultBody says that at each call site, so the comment is removed.
The ErrorHandler doc did not say whether err is safe to send to the client. For noHijacker, err names a server-side interface, so a handler that writes err.Error() into the response would expose server details.
No test failed if Accept called ErrorHandler after a failed hijack, when the 101 status line is already written. The new case uses a hijacker that returns an error and asserts the handler never runs.
The http.Client transport goroutines in the server subtest exited after TestMain counted goroutines, so filtered runs such as -run TestAccept failed the leak check intermittently. A raw TCP connection with Connection: close leaves no client goroutines behind.
Every test handler discarded the request argument, so passing nil or a copy to ErrorHandler left the tests passing. The table test now asserts the handler gets the same *http.Request that was passed to Accept.
TestAcceptRejectionDefault repeated each row's header and body in a map keyed by row name, and a new row without a map entry failed with a header mismatch that did not name the missing key. The header and body now come from the row, with wantDefaultBody for the one row whose default body is not wantErr.
acceptRejectionTests is documented as covering every rejection, but the Origin parse failure and the Origin without a host had no row, so their responses were not pinned.
The previous wording said err is not meant for the client, but Accept's default response sends err.Error() for every rejection except noHijacker. The doc now leaves the choice of what to send to the handler.
The earlier rewrite dropped the reason for testing over a real connection along with the part that restated the writesNothing row. Only the restated part needed to go.
Accept replaced an invalid OriginPatterns error with "Forbidden", so an ErrorHandler could not tell a misconfigured server from a disallowed origin. The real error now reaches ErrorHandler and Accept's caller, and the default response body stays "Forbidden".
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The first sentence says ErrorHandler writes the response, so "so ErrorHandler decides what to send to the client" added nothing to "err may contain server details".
Accept now returns this error and passes it to ErrorHandler, and "failed to parse path pattern" did not say which option held the pattern.
ErrorHandler now receives the pattern error, so only the default body keeps it from the client.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/coder-agents-review
|
There was a problem hiding this comment.
Re-review at e6ff172. 1 P3, 2 P4, 1 Nit.
The round-3 finding is fixed by removing the order dependence: authenticateOrigin checks every pattern before the Origin checks, and the new Test_authenticateOrigin rows fail without that check. Reviewers fuzzed path.Match on go1.26.5 (up to 8.5M executions) and found no pattern where match(p, "") and match(p, target) disagree on ErrBadPattern, so discarding the error at accept.go:282 drops nothing.
Out of scope (needs a ticket or explicit acceptance by a human):
accept.go:282: an IPv6 literal pattern such ashttps://[::1]:8080is a validpath.Matchpattern in which[::1]is a character class, so it never matches the originhttps://[::1]:8080and the new check does not report it.
Notes:
accept.go:137: with a bad entry,log.Printfnow writes one line per handshake, not one per cross-origin request that reached the entry.accept_test.go:321: thebadOriginPatternrow still setsOrigin: https://other.com, which no longer affects the result. Withoutmodify, both table tests would check the every-handshake rejection at theAcceptlevel; today onlyTest_authenticateOriginchecks same-origin and no-Originrequests.
accept.go:36
P4 [CRF-37] "The request host is always authorized." is false after 61a7faf: with an invalid entry, a same-origin request is rejected with 500. (Leorio)
The pattern check now runs before the strings.EqualFold(r.Host, u.Host) return, and invalidPatternSameOrigin asserts the error, so the doc says both "always authorized" and "reject every handshake". A developer who reads only the first lines expects same-origin clients to keep working with a bad entry. Change it to The request host is authorized without a matching pattern.
🤖
🤖 This review was automatically generated with Coder Agents.
| } | ||
|
|
||
| func authenticateOrigin(r *http.Request, originHosts []string) error { | ||
| // Check every pattern first, so an invalid one rejects every request, |
There was a problem hiding this comment.
P3 [CRF-36] 61a7faf and 618a580 change origin validation for every server, with or without ErrorHandler, inside a PR that the squash merge will title feat: add AcceptOptions.ErrorHandler. (Zoro, Pariston)
A deployed server with one invalid OriginPatterns entry serves same-origin requests, requests without Origin (most non-browser clients), and origins matched by an earlier entry today; after upgrading, every handshake gets 500. Master is squash merged with one title per PR (ci: ... (#594), fix: ... (#552)), so this change is listed under a new optional field, and reverting it later also reverts ErrorHandler. ErrorHandler needs only 56eccc2; move 61a7faf, 618a580 and e6ff172 to a separate fix: PR, or state the outage-on-upgrade in the squash commit message and release notes.
🤖
| // https://golang.org/pkg/path/#Match). By default, it is matched | ||
| // against the request origin host. If the pattern contains a URI | ||
| // scheme ("://"), it will be matched against "scheme://host". | ||
| // Unless InsecureSkipVerify is set, an invalid pattern makes Accept reject |
There was a problem hiding this comment.
P4 [CRF-38] No test covers the "Unless InsecureSkipVerify is set" exception that e6ff172 adds to the exported doc. (Bisky)
The only test with InsecureSkipVerify: true (conn_test.go:454) sets no patterns, so a change that validates patterns once in cloneWithDefaults would reject these handshakes with 500 and every test would pass. Add a test that calls Accept with AcceptOptions{InsecureSkipVerify: true, OriginPatterns: []string{"["}}, a cross-origin newHandshakeRequest(), and a hijackable recorder, and asserts success.
🤖
| } | ||
|
|
||
| func authenticateOrigin(r *http.Request, originHosts []string) error { | ||
| // Check every pattern first, so an invalid one rejects every request, |
There was a problem hiding this comment.
Nit [CRF-39] "Check every pattern first" restates the loop below it; the fact a reader cannot see is that the placement before the Origin checks is deliberate. (Gon)
Replace both lines with // Before the Origin checks, so an invalid pattern rejects every request.
🤖
The ErrorHandler doc started sentences with the argument names status and err, and said Accept chose the status. Test comments said a body hides an error, goroutines outlive a server and a test pins a response.
The doc explained the arguments the signature already shows and said what Accept returns, which is Accept's behavior. It now says what the field is for and what a handler can rely on.
1d52f8e to
1718326
Compare
It repeated when ErrorHandler runs, which the field doc states, and told callers not to write after an error, which follows from Accept writing the response.
mafredri
changed the title
Acceptwrites a plain texthttp.Errorresponse when it rejects a handshake request. A server that needs a structured error body, such as RFC 9457 JSON (#545), cannot replace that response. A second write afterAcceptreturns only appends to the plain text body: the status andtext/plaincontent type stayAccept's, and net/http logs a superfluousWriteHeadercall.AcceptOptions.ErrorHandler func(w http.ResponseWriter, r *http.Request, status int, err error)writes that response instead. It receives the status of the rejection and the errorAcceptreturns, before wrapping.w.Header()already holds the headersAcceptset, such asSec-WebSocket-Version: 13for an unsupported version. It is called for every rejection before the 101 response: request validation, origin, and aResponseWriterwithouthttp.Hijacker. It is not called when hijacking fails, becauseAccepthas already written 101 Switching Protocols. A handler that writes nothing leaves net/http's default 200 OK with an empty body.With a nil
ErrorHandler, the response is the same as before.For an invalid
OriginPatternsentry,Acceptnow returns an error that names the entry and wrapspath.ErrBadPattern, instead of "Forbidden", and the handler receives it. The client still gets 403 Forbidden.Tests cover the handler for all 13 rejections. A separate test checks the default status, headers and body for all 13, which are unchanged from master. Only the error
Acceptreturns and logs for an invalid pattern differs.Fixes #545
Decision log
r, so one handler shared across requests can use it, for exampler.Context(). Allow customize how HTTP errors are rendered whenwebsocket.Acceptfails to upgrade #545 discussed capturing the request in a closure instead, which does not work for shared options.websocket.Acceptfails to upgrade #545, Support disabling error writing via AcceptOptions #152).OriginPatternsentry and withouthttp.Hijacker, the handler receives the descriptive error while the nil default body is "Forbidden" or "Not Implemented".recoveraround the handler: a panic behaves like any other handler panic in net/http.AcceptOptionsstub so shared code compiles.