-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
http: explicitly blocking CONNECT for HTTP/1.1 #9637
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a question. Thank you!
/wait-any
@@ -29,9 +29,9 @@ constexpr const char* runtime_features[] = { | |||
"envoy.reloadable_features.buffer_filter_populate_content_length", | |||
"envoy.reloadable_features.outlier_detection_support_for_grpc_status", | |||
"envoy.reloadable_features.connection_header_sanitization", | |||
"envoy.reloadable_features.strict_header_validation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to delete this? Don't we still need this since it exists in the code? Or should we clean it up in the code also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Also removing a spurious "envoy.reloadable_features.strict_header_validation" as it was enabled twice." :-P
Github truncated but the file was
"envoy.reloadable_features.test_feature_true",
"envoy.reloadable_features.strict_header_validation",
"envoy.reloadable_features.buffer_filter_populate_content_length",
"envoy.reloadable_features.outlier_detection_support_for_grpc_status",
"envoy.reloadable_features.connection_header_sanitization",
"envoy.reloadable_features.strict_header_validation",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops sorry I should read more. :)
if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would just do this at the top of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
This is continued clean up from red emu, unifying HTTP/1 and HTTP/2 behavior.
Guarded by envoy.reloadable_features.strict_method_validation
Also removing a spurious "envoy.reloadable_features.strict_header_validation" as it was enabled twice.
Risk Level: Medium (someone might depend on current broken behavior)
Testing: new integration tests
Docs Changes: n/a
Release Notes: inline