-
Notifications
You must be signed in to change notification settings - Fork 0
π‘οΈ Sentinel: [HIGH] Fix missing Vary header in CORS responses #333
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
base: main
Are you sure you want to change the base?
Changes from all commits
21ef9c1
b8d4faa
3136d0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -255,6 +255,7 @@ pub async fn cors_middleware( | |
| { | ||
| if let Ok(header_value) = HeaderValue::from_str(&origin) { | ||
| response.headers_mut().insert(header::ACCESS_CONTROL_ALLOW_ORIGIN, header_value); | ||
| response.headers_mut().append(header::VARY, HeaderValue::from_static("origin")); | ||
| } | ||
|
|
||
| if state.cors_config.allow_credentials { | ||
|
|
@@ -300,6 +301,7 @@ fn handle_preflight( | |
| // Set allowed origin | ||
| if let Ok(header_value) = HeaderValue::from_str(&origin) { | ||
| response.headers_mut().insert(header::ACCESS_CONTROL_ALLOW_ORIGIN, header_value); | ||
| response.headers_mut().append(header::VARY, HeaderValue::from_static("origin")); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| // Set allowed methods | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -149,6 +149,7 @@ pub async fn cors_middleware(config: CorsConfig, request: Request, next: Next) - | |||||
| { | ||||||
| if let Ok(header_value) = HeaderValue::from_str(&origin) { | ||||||
| response.headers_mut().insert(header::ACCESS_CONTROL_ALLOW_ORIGIN, header_value); | ||||||
| response.headers_mut().append(header::VARY, HeaderValue::from_static("origin")); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Vary: Origin header should be added to all responses (including rejections) to prevent cache poisoning where a CORS-less response is served to an allowed origin. Also, using the canonical "Origin" casing is recommended.
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| if config.allow_credentials { | ||||||
|
|
@@ -194,6 +195,7 @@ fn handle_preflight( | |||||
| // Set allowed origin | ||||||
| if let Ok(header_value) = HeaderValue::from_str(&origin) { | ||||||
| response.headers_mut().insert(header::ACCESS_CONTROL_ALLOW_ORIGIN, header_value); | ||||||
| response.headers_mut().append(header::VARY, HeaderValue::from_static("origin")); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| } | ||||||
|
|
||||||
| // Set allowed methods | ||||||
|
|
||||||
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.
The Vary: Origin header should be added to all responses where the Origin header is inspected, including those where the origin is rejected. If a 'denied' response is cached without this header, it may be served to subsequent requests from allowed origins, causing CORS failures. Since the middleware dynamically determines the Access-Control-Allow-Origin header, Vary: Origin is essential for all cacheable responses. Consider moving this logic outside the conditional block to ensure it's always applied when the middleware is active. Also, using the canonical casing "Origin" is recommended. Finally, ensure that integration tests are updated to verify the presence of this header.