-
Notifications
You must be signed in to change notification settings - Fork 50
Add suppport for configuring http compliance violations #124
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
Conversation
Reviewer's GuideIntroduces a new HTTP compliance violation configuration by defining an enum, extending server config to parse and expose a CSV-based property, applying custom Jetty violations in the server builder, and updating tests to cover defaults and explicit mappings. Sequence diagram for applying custom HTTP compliance violations during server startupsequenceDiagram
participant "HttpServer"
participant "HttpServerConfig"
participant "HttpComplianceViolation"
participant "Jetty HttpConfiguration"
"HttpServer"->>"HttpServerConfig": getHttpComplianceViolations()
alt Violations configured
"HttpServerConfig"->>"HttpComplianceViolation": map to Jetty Violation
"HttpServer"->>"Jetty HttpConfiguration": setHttpCompliance(customViolations)
else No violations
"HttpServer"->>"Jetty HttpConfiguration": setHttpCompliance(default)
end
Class diagram for new and updated HTTP compliance configuration typesclassDiagram
class HttpServerConfig {
- String secureRandomAlgorithm
- List<String> includedCipherSuites
- UriCompliance uriCompliance
- List<HttpComplianceViolation> httpComplianceViolations
+ HttpServerConfig setHttpComplianceViolations(String httpComplianceViolations)
+ List<HttpComplianceViolation> getHttpComplianceViolations()
}
class HttpComplianceViolation {
<<enum>>
+ getHttpComplianceViolation()
- HttpCompliance.Violation httpComplianceViolation
}
HttpServerConfig --> "*" HttpComplianceViolation
Class diagram for HttpServer applying custom HTTP compliance violationsclassDiagram
class HttpServer {
+ HttpServer(HttpServerInfo httpServerInfo, ...)
}
class HttpServerConfig {
+ List<HttpComplianceViolation> getHttpComplianceViolations()
}
class HttpComplianceViolation {
+ getHttpComplianceViolation()
}
HttpServer --> HttpServerConfig
HttpServerConfig --> "*" HttpComplianceViolation
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Fix the error message passed to requireNonNull in setHttpComplianceViolations so it references
httpComplianceViolations
instead ofincludedCipherSuites
. - Remove the unnecessary
/**/
placeholder and trailing comma at the end of the HttpComplianceViolation enum to clean up the declaration. - Consider adding validation or clearer error messaging in setHttpComplianceViolations for unknown or misspelled violation names to help users debug configuration errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Fix the error message passed to requireNonNull in setHttpComplianceViolations so it references `httpComplianceViolations` instead of `includedCipherSuites`.
- Remove the unnecessary `/**/` placeholder and trailing comma at the end of the HttpComplianceViolation enum to clean up the declaration.
- Consider adding validation or clearer error messaging in setHttpComplianceViolations for unknown or misspelled violation names to help users debug configuration errors.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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, just one question about the "customViolations" string
http-server/src/main/java/com/facebook/airlift/http/server/HttpServer.java
Show resolved
Hide resolved
5577d04
to
1860692
Compare
addressed comments from the ai reviewer. |
The jetty 12 upgrade includes stricter enforcement of http compliance violations. Users may want to allow certain violations while their clients adapt to the new requirements.
1860692
to
337e2f8
Compare
The jetty 12 upgrade includes stricter enforcement of http compliance violations. Users may want to allow certain violations while their clients adapt to the new requirements.