Repository navigation
Conversation
A handler or caller could tell an invalid pattern from a disallowed origin only through the path.ErrBadPattern wrap, which no doc promised.
An invalid pattern rejected only cross origin requests that reached it before a match, so a misconfigured server failed for some clients and worked for others. Accept now checks every pattern first and rejects every handshake, whatever its origin.
An invalid pattern now rejects every handshake, including same origin requests, so 403 Forbidden told those clients their origin was not allowed. The fault is the server's configuration.
Accept does not check the patterns when InsecureSkipVerify is set, so "every handshake" was false in that mode.
An invalid pattern now rejects same origin requests too. The comment on the pattern check also restated the loop instead of saying why it runs before the Origin checks.
The OriginPatterns doc says InsecureSkipVerify skips the pattern check, and no test set both. The badOriginPattern row also no longer sets an Origin, which no longer affects the result, so the Accept level tests check a request without one.
"Every handshake" already covers every origin.
This comment has been minimized.
This comment has been minimized.
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 3 | Last posted: Round 3, 9 findings (2 P3, 6 P4, 1 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledgedRound logRound 1Netero first pass: no findings, 2 Notes. Full panel (round 1), security-sensitive origin matching so multi-domain size: ging-go, kurapika, ryosuke, killua, pariston, mafuuu, bisky, hisoka, gon, leorio, chopper, mafu-san, wildcard zoro (random from distilled set). 1 P3, 1 P4, 1 Nit posted; 3 dropped; 2 Notes in body; 7 out of scope. Gon's P2 on the loop comment downgraded to P3 (comment is incomplete, not wrong). Reviewed against 98a34ba..fd4a902. Round 2Churn guard PROCEED: CRF-3, CRF-6, CRF-7 author fixed. Restructure round (authenticateOrigin rewritten in fa62356), full panel: netero (no findings), ging-go, kurapika, ryosuke, killua, pariston, mafuuu, bisky, hisoka, gon, leorio, chopper, mafu-san, wildcard razor (random from distilled set). 1 P3, 2 P4 posted; 1 Note dropped; 1 out of scope. CRF-16 settled at P3 between Gon P2 and Leorio P4 because invalidPatternAfterMatch fails if a break is added, so the omission costs a confusing test failure, not a shipped bug. CRF-17 lowered from P2 to P4: the PR text labels the figure as an earlier separate check, and the measured shipped cost (about 0.5 to 1 µs per handshake, Killua and Mafu-san) does not change the decision. CRF-18 lowered from P3 to P4 to match CRF-7, the same claim in the doc. Reviewed against 98a34ba..fa62356. Round 3Churn guard PROCEED: CRF-16, CRF-17, CRF-18 addressed by 5e45c9c, which drops the up-front check and changes the PR goal to the 403 to 500 status change only. CRF-2 and CRF-8 dropped because their premise (log and rejection on every handshake) no longer holds. Restructure round (PR title and goal changed), full panel: netero (no findings), ging-go, ryosuke, kurapika, pariston, mafuuu, bisky, gon, leorio, chopper, mafu-san, wildcard knov (random from distilled set). CRF-17 re-raised by Mafu-san at P2 on the rewritten figure; kept at its inventory severity P4 because the decision it supports holds with either number (Mafu-san states this). 3 new P4 posted, 1 re-raise reply, 2 Notes in body, 1 Note dropped. Knov's note that raw path.Match gives the same result as Accept is contradicted by Mafuuu's and Gon's invalid UTF-8 case; the empirical case wins. Reviewed against 98a34ba..5e45c9c. 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 checks every OriginPatterns entry before the Origin checks and answers an invalid entry with 500 on every handshake. Findings: 1 P3, 1 P4, 1 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
accept.go:35: theOriginPatternsdoc does not say that a request without anOriginheader is authorized, so a reader can assume the patterns restrict non-browser clients.accept.go:39: unchanged lines of the same doc block use "i.e" for an example, repeat thepath.Matchlink, and add a "ramifications" sentence that says nothing the CSRF sentence does not.accept.go:251: theauthenticateOriginparameteroriginHostsholds patterns, not hosts, while both loops name the elementhostPattern.accept.go:290: inverted ranges such as[z-a]passpath.Matchwithout an error and match nothing, so the new check does not report them.accept_test.go:197:closeRacepairswg.Add(2)with manualwg.Done(), so a panic beforeDoneblockswg.Waitforever;sync.WaitGroup.Goremoves this.accept_test.go:699: thetc := tccopies here and at line 830 are unnecessary undergo 1.26.accept_test.go:339:badOriginPatternsays "bad" for the condition the new tests call "invalid".
Notes:
- With an invalid pattern, the
log.Printfataccept.go:134now writes one line per handshake, at the client request rate. - The release notes need the behavior change paragraph from the PR description, because same origin clients that connect on v1.8.15 fail after this change.
🤖 This review was automatically generated with Coder Agents.
The doc, the error text and the Test_authenticateOrigin cases call the pattern invalid, not bad.
A request that fails the protocol checks gets its own 4xx before the pattern check runs, so "every handshake" was too broad.
authenticateOrigin matched every pattern against "" to check it, then matched the patterns again against the origin. path.Match reports a malformed pattern whatever the name, so one loop now checks and matches each pattern and keeps the result.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
This round reviews fa62356, which merges pattern validation into the matching loop, and the doc and test name fixes. Findings: 1 P3, 2 P4.
The three findings from the previous review are fixed: the comment on the separate match(p, "") check (fa62356 removes that check), the "every handshake" doc wording (3b4d4c1), and the test name (6950739).
Out of scope (needs a ticket or explicit acceptance by a human):
accept.go:284: the unauthorized-origin error saysrequest Origin %qbut printsu.Host(other.com), not the header the client sent (https://other.com). The diff moved this line without changing its text.
The out-of-scope items and notes from the previous review still apply.
🤖 This review was automatically generated with Coder Agents.
Checking every pattern on each handshake added work to same origin requests and requests without Origin, and a caller can check its patterns with path.Match before use. Accept now reports an invalid pattern only when it tries it, as before, still with 500, and the doc says how to find invalid patterns up front.
mafredri
changed the title
|
/coder-agents-review
|
There was a problem hiding this comment.
This round reviews 5e45c9c, which drops the up-front pattern check and keeps in-order matching, so the PR now changes only the status for an invalid OriginPatterns entry from 403 to 500 and documents the rule. Findings: 3 P4, plus one re-raised P4 on the decision log cost figure.
The three findings from the previous review are addressed: 5e45c9c removes the loop comment and the shipped loop, and the title and body no longer say "every handshake". The rewritten cost figure has the same problem as before; see the reply on that thread.
Notes:
accept.go:50:path.Matchon the raw pattern rejects a few patterns thatAcceptaccepts, such as"[\xff]", becausematchlowercases first andstrings.ToLowerreplaces invalid UTF-8 with U+FFFD. The suggested check never misses a pattern thatAcceptanswers with 500.accept.go:136: on v1.8.15, an unmatched cross origin request that reaches an invalid entry gets 403; after this PR it gets 500. The client choosesOrigin, so any cross-site page can produce these 500s. The release notes should mention the change for operators with 5xx alerts.
The out-of-scope items from the previous reviews still apply.
🤖 This review was automatically generated with Coder Agents.
| // 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". | ||
| // Accept tries the patterns in order. When it tries an invalid pattern, |
There was a problem hiding this comment.
P4 [CRF-21] The doc says Accept "tries the patterns in order" but not that it stops at the first match, so it does not say which requests never reach an invalid pattern. (Mafuuu)
With OriginPatterns: []string{"good.com", "["}, every handshake from good.com succeeds and only other cross origin handshakes get 500 (invalidPatternAfterMatch pins this). The sentence also reads as "every pattern is tried". An operator who tests only from good.com sees no error and ships the invalid entry. Suggested: "Accept tries the patterns in order and stops at the first match."
🤖
| success: true, | ||
| }, | ||
| { | ||
| name: "invalidPatternAfterMatch", |
There was a problem hiding this comment.
P4 [CRF-22] No test puts an invalid pattern before a matching pattern, so the "in order" rule in the new doc is not tested. (Bisky)
invalidPatternAfterMatch covers {"other.com", "["}, and badOriginPattern uses "[" alone. Changing authenticateOrigin to skip invalid patterns and return the error only when no later pattern matches still passes go test -run 'TestAccept|Test_authenticateOrigin'. Under that change, {"[", "other.com"} accepts https://other.com. Add a row invalidPatternBeforeMatch with origin https://other.com, host example.com, patterns {"[", "other.com"}, and success: false.
🤖
| defaultBody := err.Error() | ||
| status, defaultBody := http.StatusForbidden, err.Error() | ||
| if errors.Is(err, path.ErrBadPattern) { | ||
| log.Printf("websocket: %v", err) |
There was a problem hiding this comment.
P4 [CRF-23] The decision log says "The log.Printf of the pattern error stays." with no reason, unlike the other entry. (Leorio)
A maintainer who reads the squashed commit cannot tell why the log was kept. A likely reason: the default body is only "Internal Server Error", so for a handler that drops the error from Accept and sets no ErrorHandler, the log line is the only place an operator sees which entry is invalid. State the actual reason.
🤖
An invalid
OriginPatternsentry, such as"[", is a server misconfiguration, butAcceptrejected the request with 403 Forbidden, which tells the client that its origin is not allowed.Acceptnow rejects it with 500 Internal Server Error and an error that wrapspath.ErrBadPattern. The default body is "Internal Server Error". As before,Accepttries the patterns in order and reports an invalid pattern only when it tries it, so same origin requests, requests withoutOrigin, and origins matched by an earlier pattern are accepted. TheOriginPatternsdoc now says this and suggests checking patterns withpath.Matchto find invalid ones up front.Test_authenticateOrigincovers an invalid pattern with noOrigin, with a same origin request, and after a matching pattern. Another test checks thatInsecureSkipVerifyskips the patterns.Depends on #588, which makes
Acceptreturn the pattern error instead of "Forbidden".Decision log
path.Matchbefore use.log.Printfof the pattern error stays.