⚡ Bolt: Optimize security headers middleware with caching#205
⚡ Bolt: Optimize security headers middleware with caching#205EffortlessSteven wants to merge 1 commit intomainfrom
Conversation
Pre-computes and caches `HeaderValue` instances in `SecurityHeadersConfig` to avoid parsing strings on every request. This improves middleware performance by eliminating repeated validation and allocation of header values. - Adds `CachedSecurityHeaders` struct to store pre-parsed headers - Adds `cached_headers` field to `SecurityHeadersConfig` - Initializes cache in `from_sources` and `Default` - Updates `apply_headers` to use the cache fast path
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
WalkthroughImplements a header value caching optimization for security headers middleware. Introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
Summary of ChangesHello @EffortlessSteven, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant performance improvement to the security headers middleware. By pre-parsing and caching the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively optimizes the security headers middleware by introducing a caching mechanism for HeaderValue instances. This change correctly addresses the performance overhead of repeatedly parsing header strings on every request, leading to improved CPU efficiency. The implementation correctly handles initialization in both Default and from_sources methods, and includes a robust fallback to the original parsing logic if cached headers are not available. The addition of #[serde(skip)] for cached_headers is also appropriate.
| if let Some(val) = &cached.content_security_policy { | ||
| response.headers_mut().insert("Content-Security-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.x_frame_options { | ||
| response.headers_mut().insert("X-Frame-Options", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.x_content_type_options { | ||
| response.headers_mut().insert("X-Content-Type-Options", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.x_xss_protection { | ||
| response.headers_mut().insert("X-XSS-Protection", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.strict_transport_security { | ||
| response.headers_mut().insert("Strict-Transport-Security", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.referrer_policy { | ||
| response.headers_mut().insert("Referrer-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.permissions_policy { | ||
| response.headers_mut().insert("Permissions-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.cross_origin_embedder_policy { | ||
| response.headers_mut().insert("Cross-Origin-Embedder-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.cross_origin_opener_policy { | ||
| response.headers_mut().insert("Cross-Origin-Opener-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.cross_origin_resource_policy { | ||
| response.headers_mut().insert("Cross-Origin-Resource-Policy", val.clone()); | ||
| } |
There was a problem hiding this comment.
The apply_headers method contains a significant amount of repetitive code for inserting each header. This pattern can be refactored into a helper function to improve readability and reduce duplication. A similar helper could also be beneficial for the init_cache method to abstract away the HeaderValue::from_str calls, especially for Option<String> versus String fields, making the code more concise and less prone to copy-paste errors.
insert_header_if_some(response, "Content-Security-Policy", &cached.content_security_policy);
insert_header_if_some(response, "X-Frame-Options", &cached.x_frame_options);
insert_header_if_some(response, "X-Content-Type-Options", &cached.x_content_type_options);
insert_header_if_some(response, "X-XSS-Protection", &cached.x_xss_protection);
insert_header_if_some(response, "Strict-Transport-Security", &cached.strict_transport_security);
insert_header_if_some(response, "Referrer-Policy", &cached.referrer_policy);
insert_header_if_some(response, "Permissions-Policy", &cached.permissions_policy);
insert_header_if_some(response, "Cross-Origin-Embedder-Policy", &cached.cross_origin_embedder_policy);
insert_header_if_some(response, "Cross-Origin-Opener-Policy", &cached.cross_origin_opener_policy);
insert_header_if_some(response, "Cross-Origin-Resource-Policy", &cached.cross_origin_resource_policy);
return;
}
// Helper function to insert a header if its value is Some
fn insert_header_if_some(
response: &mut Response,
header_name: &str,
header_value: &Option<HeaderValue>,
) {
if let Some(val) = header_value {
response.headers_mut().insert(header_name, val.clone());
}
}
// Fallback to parsing (slow path, should only happen if not initialized)There was a problem hiding this comment.
Pull request overview
This PR optimizes the security headers middleware by pre-computing and caching parsed HeaderValue instances at configuration initialization time, eliminating the need to parse header values on every HTTP request. The optimization uses an Arc<CachedSecurityHeaders> structure to share the cached values efficiently across requests, while maintaining a fallback path for compatibility.
Changes:
- Added
CachedSecurityHeadersstruct to hold pre-parsedHeaderValueinstances for all security headers - Added
cached_headersfield toSecurityHeadersConfigwith#[serde(skip)]to prevent serialization - Modified
Default::default()andfrom_sources()to callinit_cache()after construction - Implemented fast path in
apply_headers()that uses cached values when available - Added
.Jules/bolt.mddocumentation to record this optimization pattern
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/app-http/src/middleware/security_headers.rs | Adds header value caching infrastructure and fast path for applying headers using cached values |
| .Jules/bolt.md | Documents the learning about pre-computing HeaderValue instances for static configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let cached = CachedSecurityHeaders { | ||
| content_security_policy: self | ||
| .content_security_policy | ||
| .as_ref() | ||
| .and_then(|s| HeaderValue::from_str(s).ok()), | ||
| x_frame_options: HeaderValue::from_str(&self.x_frame_options).ok(), | ||
| x_content_type_options: HeaderValue::from_str(&self.x_content_type_options).ok(), | ||
| x_xss_protection: HeaderValue::from_str(&self.x_xss_protection).ok(), | ||
| strict_transport_security: self | ||
| .strict_transport_security | ||
| .as_ref() | ||
| .and_then(|s| HeaderValue::from_str(s).ok()), | ||
| referrer_policy: HeaderValue::from_str(&self.referrer_policy).ok(), | ||
| permissions_policy: self | ||
| .permissions_policy | ||
| .as_ref() | ||
| .and_then(|s| HeaderValue::from_str(s).ok()), | ||
| cross_origin_embedder_policy: self | ||
| .cross_origin_embedder_policy | ||
| .as_ref() | ||
| .and_then(|s| HeaderValue::from_str(s).ok()), | ||
| cross_origin_opener_policy: self | ||
| .cross_origin_opener_policy | ||
| .as_ref() | ||
| .and_then(|s| HeaderValue::from_str(s).ok()), | ||
| cross_origin_resource_policy: HeaderValue::from_str(&self.cross_origin_resource_policy) | ||
| .ok(), | ||
| }; |
There was a problem hiding this comment.
The init_cache() method silently ignores parse errors using .ok(). If any of the configured header values are invalid and fail to parse, they will be silently skipped without any warning or error. This could lead to a security misconfiguration going unnoticed.
Consider logging a warning when header parsing fails during initialization, so operators are aware that their security headers configuration has invalid values. For example:
- Log when
HeaderValue::from_str()returns an error - Include which header failed and what the invalid value was
| response.headers_mut().insert("Content-Security-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.x_frame_options { | ||
| response.headers_mut().insert("X-Frame-Options", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.x_content_type_options { | ||
| response.headers_mut().insert("X-Content-Type-Options", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.x_xss_protection { | ||
| response.headers_mut().insert("X-XSS-Protection", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.strict_transport_security { | ||
| response.headers_mut().insert("Strict-Transport-Security", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.referrer_policy { | ||
| response.headers_mut().insert("Referrer-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.permissions_policy { | ||
| response.headers_mut().insert("Permissions-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.cross_origin_embedder_policy { | ||
| response.headers_mut().insert("Cross-Origin-Embedder-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.cross_origin_opener_policy { | ||
| response.headers_mut().insert("Cross-Origin-Opener-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.cross_origin_resource_policy { | ||
| response.headers_mut().insert("Cross-Origin-Resource-Policy", val.clone()); |
There was a problem hiding this comment.
The header names are being inserted as string literals instead of using HeaderName constants. For consistency with the rest of the codebase (see cors.rs which uses header::ACCESS_CONTROL_ALLOW_ORIGIN), these should use HeaderName constants or HeaderName::from_static(). This would also avoid the runtime parsing overhead of converting strings to header names on every request.
While most of these security headers don't have constants in the standard http crate, you should use HeaderName::from_static() for all of them to be consistent with how other parts of the codebase handle custom headers (e.g., request_id.rs uses HeaderName::from_static("x-request-id")).
| if let Some(val) = &cached.content_security_policy { | ||
| response.headers_mut().insert("Content-Security-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.x_frame_options { | ||
| response.headers_mut().insert("X-Frame-Options", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.x_content_type_options { | ||
| response.headers_mut().insert("X-Content-Type-Options", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.x_xss_protection { | ||
| response.headers_mut().insert("X-XSS-Protection", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.strict_transport_security { | ||
| response.headers_mut().insert("Strict-Transport-Security", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.referrer_policy { | ||
| response.headers_mut().insert("Referrer-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.permissions_policy { | ||
| response.headers_mut().insert("Permissions-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.cross_origin_embedder_policy { | ||
| response.headers_mut().insert("Cross-Origin-Embedder-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.cross_origin_opener_policy { | ||
| response.headers_mut().insert("Cross-Origin-Opener-Policy", val.clone()); | ||
| } | ||
| if let Some(val) = &cached.cross_origin_resource_policy { | ||
| response.headers_mut().insert("Cross-Origin-Resource-Policy", val.clone()); |
There was a problem hiding this comment.
The cached HeaderValues are being cloned on every request with val.clone(). Since HeaderValue implements Clone cheaply (it uses Arc internally for the value data), this is acceptable but not optimal. However, to truly minimize overhead, consider storing the header name-value pairs together in the cache so you can insert them without any cloning at all. Alternatively, document that the clone is cheap due to Arc to clarify this is intentional.
This PR optimizes the security headers middleware by caching the parsed
HeaderValueinstances. Previously, the middleware parsed the configuration strings intoHeaderValueon every request, which involves validation overhead. By parsing them once during initialization and storing them in anArc<CachedSecurityHeaders>, we reduce the CPU cost of applying security headers. The original parsing logic is retained as a fallback.PR created automatically by Jules for task 8874080653256103478 started by @EffortlessSteven