Conversation
tjasko
force-pushed
the
feat/ws-upgrade-validation
branch
2 times, most recently
from
March 15, 2025 03:10
9710364 to
2db11ae
Compare
zhuizhuhaomeng
approved these changes
Jun 14, 2025
| m, err = re_match(header, [[^\s*HTTP/1\.1\s+]], "jo") | ||
| if not m then | ||
| -- Validate HTTP status line. | ||
| local status_line_end = header:find("\r?\n") |
Contributor
There was a problem hiding this comment.
use nginx.re to get the status code directly.
Author
There was a problem hiding this comment.
Thanks for the review. Are you stating that you wish to revert to the previous re_match() behavior & not validate the HTTP status line per HTTP spec?
Contributor
There was a problem hiding this comment.
The original implementation is more efficient and already meets the requirements, so we can keep it as is.
| ^error: "response headers too large \(limit: 1024 bytes\)" | ||
| --- no_error_log | ||
| [error] | ||
| [warn] No newline at end of file |
Contributor
There was a problem hiding this comment.
require a newline at the end of file.
zhuizhuhaomeng
requested changes
Jun 27, 2025
| m, err = re_match(header, [[^\s*HTTP/1\.1\s+]], "jo") | ||
| if not m then | ||
| -- Validate HTTP status line. | ||
| local status_line_end = header:find("\r?\n") |
Contributor
There was a problem hiding this comment.
The original implementation is more efficient and already meets the requirements, so we can keep it as is.
Add two options to client:new(), both off by default so existing callers are unaffected. max_header_len bounds the handshake response headers, addressing the "FIXME: check for too big response headers" in connect(). Without it a server can stream headers until the client runs out of memory. The reader is given max_header_len + 1 so an over-long response is detected rather than silently truncated, and the length check tolerates the nil the reader returns on a socket error. capture_error_body reads the response body when the upgrade is refused and appends it to the error. The 101 check itself now lives upstream, which reports only the status code; the body usually carries the reason the upgrade was refused. It is opt-in because reading it means waiting on the socket again, and because the default error text stays exactly as upstream has it.
tjasko
force-pushed
the
feat/ws-upgrade-validation
branch
from
September 12, 2026 20:55
183c9a2 to
b02684a
Compare
tjasko
changed the title
max_header_len & validate_handshake optionsmax_header_len & capture_error_body options
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
2 participants
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.
Adds two options to
client:new(), both off by default so existing callers are unaffected.max_header_lenbounds the size of the handshake response headers, addressing the-- FIXME: check for too big response headersinconnect(). Without it, a server can stream headers until the client runs out of memory. The reader is givenmax_header_len + 1so an over-long response is detected rather than silently truncated.capture_error_bodyreads the response body when the upgrade is refused and appends it to the returned error.Rebased onto the current master
This branch has been rebased. The 101-status rejection it originally proposed as
validate_handshakelanded independently in 2606072, so that part is dropped; the check is now unconditional, which is better than the opt-in this PR proposed.What survived is the body.
2606072reports only the status code, and the body is usually where the server explains why it refused the upgrade (an auth failure, a proxy error page).capture_error_bodyappends it to the existing message rather than replacing it:It is opt-in for two reasons: reading the body means waiting on the socket again, and the default error text stays byte-for-byte as
2606072left it, sot/handshake.tpasses untouched.The option was named
validate_handshakein the first revision of this PR. Since validation is now unconditional, that name described something it no longer controls, hence the rename.Review feedback
Both review comments are addressed. The status line is matched with a single
ngx.re.matchthat also captures the code, which is what2606072settled on, andt/cs.tends with a newline again.Tests
t/handshake.tpasses unchanged, confirming no regression in the upstream 101 behavior.t/cs.t, one per option.t/cs.tfailures are identical on a baremastercheckout and are environmental: they assert onngx.DEBUGlog lines that a non-debug nginx build never emits.Unrelated: the test certificate
t/cert/test.keyis a 1024-bit RSA key, which modern OpenSSL rejects withee key too small, aborting the suite before it reaches the later tests. That is pre-existing and not touched here, but it does block runningt/cs.tlocally. Worth its own PR, along with aMakefiletarget:$ openssl req -x509 -newkey rsa:2048 -nodes -keyout test.key -out test.crt -days 3650 \ -subj "/C=US/ST=California/L=San Francisco/O=OpenResty/OU=OpenResty/CN=test.com/emailAddress=agentzh@gmail.com"Thanks for considering this change!