feat: add max_header_len & validate_handshake options#94
Open
tjasko wants to merge 1 commit intoopenresty:masterfrom
Open
feat: add max_header_len & validate_handshake options#94tjasko wants to merge 1 commit intoopenresty:masterfrom
max_header_len & validate_handshake options#94tjasko wants to merge 1 commit intoopenresty:masterfrom
Conversation
tjasko
force-pushed
the
feat/ws-upgrade-validation
branch
from
884133b to
9710364
Compare
- Added `max_header_len` option to limit the maximum allowed header size during the WebSocket upgrade process. - Added `validate_handshake` option to enforce that the WebSocket handshake response must return HTTP 101. - Improved HTTP response parsing by checking the status line and extracting response headers properly. - Added new test cases
tjasko
force-pushed
the
feat/ws-upgrade-validation
branch
from
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.
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.
Introducing new functionality to add two new options to
client:new(). Changes have been implemented in a way to retain backward compatibility.max_header_lenoption to limit the maximum allowed header size during the WebSocket upgrade process.validate_handshakeoption to enforce that the WebSocket handshake response must return HTTP 101.Not all existing tests are passing, but that's unrelated to this improvement. I had to re-generate a new certificate to get Nginx happy, this likely should be fixed in another pull request & add the below to the
Makefile:$ openssl req -new -key test.key -out test.csr -subj "/C=US/ST=California/L=San Francisco/O=OpenResty/OU=OpenResty/CN=test.com/emailAddress=agentzh@gmail.com" $ openssl x509 -req -days 365 -in test.csr -signkey test.key -out test.crtThanks for considering this change!