Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix check for URI length to prevent incorrect HTTP 414 errors #2046

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

brettp
Copy link
Contributor

@brettp brettp commented Feb 9, 2025

This fixes a bug where the full stream's length, including POST data, is checked instead of just the URI. This results in false 414 URI Too Long errors. The fix is to check only the URI part of the first line of the stream.

@yhirose
Copy link
Owner

yhirose commented Feb 9, 2025

@brettp could you please add at least one unit text in test/test.cc?

httplib.h Outdated
Comment on lines 7165 to 7170
auto req_line = line_reader.ptr();

// Extract URI from the request line
std::string method, uri, version;
std::istringstream iss(req_line);
iss >> method >> uri >> version;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is effectively what parse_request_line() does. Shouldn't the check be moved there?

@brettp
Copy link
Contributor Author

brettp commented Feb 11, 2025

@yhirose - Test added for request lines larger than max URI length but with a URI itself that is shorter.

@falbrechtskirchinger - I remove the extra parsing by checking the length of req.target after parse_request_line() is called.

@yhirose yhirose merged commit a268d65 into yhirose:master Feb 11, 2025
4 checks passed
@yhirose
Copy link
Owner

yhirose commented Feb 11, 2025

@brettp thanks for your fine contribution!

@brettp brettp deleted the fix-http-414 branch February 11, 2025 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants