Repository navigation
fix(dev): default adk.web.cors.origins to the server's own loopback o… #1674
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
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,8 +16,10 @@ | |
|
|
||
| package com.google.adk.web.config; | ||
|
|
||
| import java.util.List; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.context.annotation.Bean; | ||
| import org.springframework.context.annotation.Configuration; | ||
| import org.springframework.web.cors.CorsConfiguration; | ||
|
|
@@ -30,35 +32,44 @@ | |
| * application. This class defines beans for configuring CORS settings based on properties defined | ||
| * in {@link AdkWebCorsProperties}. | ||
| * | ||
| * <p>CORS allows the application to handle requests from different origins, enabling secure | ||
| * communication between the frontend and backend services. | ||
| * | ||
| * <p>Beans provided: | ||
| * | ||
| * <ul> | ||
| * <li>{@link CorsConfigurationSource}: Configures CORS settings such as allowed origins, methods, | ||
| * headers, credentials, and max age. | ||
| * <li>{@link CorsFilter}: Applies the CORS configuration to incoming requests. | ||
| * </ul> | ||
| * <p>When no explicit origin allowlist is configured, this defaults to the server's own loopback | ||
| * origins (localhost / 127.0.0.1 / [::1] on the configured port) instead of allowing every origin, | ||
| * since allowing "*" by default lets any website a developer's browser visits call this server | ||
| * while it is running. Set 'adk.web.cors.origins' explicitly to widen this. | ||
| */ | ||
| @Configuration | ||
| public class AdkWebCorsConfig { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(AdkWebCorsConfig.class); | ||
|
|
||
| @Value("${server.port:8000}") | ||
| private int serverPort; | ||
|
Comment on lines
+45
to
+46
Contributor
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. |
||
|
|
||
| @Bean | ||
| public CorsConfigurationSource corsConfigurationSource(AdkWebCorsProperties corsProperties) { | ||
| if (corsProperties.origins().contains("*")) { | ||
| List<String> origins = corsProperties.origins(); | ||
|
|
||
| if (origins.isEmpty()) { | ||
| origins = | ||
| List.of( | ||
| "http://localhost:" + serverPort, | ||
| "http://127.0.0.1:" + serverPort, | ||
| "http://[::1]:" + serverPort); | ||
| logger.info( | ||
| "No 'adk.web.cors.origins' configured; defaulting to this server's own loopback" | ||
| + " origins on port {}. Set 'adk.web.cors.origins' explicitly to allow other" | ||
| + " origins.", | ||
| serverPort); | ||
| } else if (origins.contains("*")) { | ||
| logger.warn( | ||
| "CORS is configured to allow all origins (\"*\"), which is insecure and intended for" | ||
| + " local development only. This also applies to the /run_live WebSocket endpoint." | ||
| + " Set 'adk.web.cors.origins' to an explicit allowlist to restrict which origins" | ||
| + " may call the server."); | ||
| "CORS is configured to allow all origins (\"*\"), which is insecure and lets any" | ||
| + " website call this server, including the /run_live WebSocket endpoint. Only" | ||
| + " set this if you understand the risk."); | ||
| } | ||
|
|
||
| CorsConfiguration configuration = new CorsConfiguration(); | ||
|
|
||
| configuration.setAllowedOrigins(corsProperties.origins()); | ||
| configuration.setAllowedOrigins(origins); | ||
|
Comment on lines
+50
to
+72
Contributor
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. Instead of hardcoding the server port by injecting This approach has several advantages:
List<String> origins = corsProperties.origins();
CorsConfiguration configuration = new CorsConfiguration();
if (origins.isEmpty()) {
configuration.setAllowedOriginPatterns(
List.of(
"http://localhost:*",
"http://127.0.0.1:*",
"http://[::1]:*"));
logger.info(
"No 'adk.web.cors.origins' configured; defaulting to loopback origins on any port"
+ " (localhost / 127.0.0.1 / [::1]). Set 'adk.web.cors.origins' explicitly to"
+ " allow other origins.");
} else {
configuration.setAllowedOrigins(origins);
if (origins.contains("*")) {
logger.warn(
"CORS is configured to allow all origins (\"*\"), which is insecure and lets any"
+ " website call this server, including the /run_live WebSocket endpoint. Only"
+ " set this if you understand the risk.");
}
} |
||
| configuration.setAllowedMethods(corsProperties.methods()); | ||
| configuration.setAllowedHeaders(corsProperties.headers()); | ||
| configuration.setAllowCredentials(corsProperties.allowCredentials()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,7 +34,10 @@ public record AdkWebCorsProperties( | |||||||||||||
|
|
||||||||||||||
| public AdkWebCorsProperties { | ||||||||||||||
| mapping = mapping != null ? mapping : "/**"; | ||||||||||||||
| origins = origins != null && !origins.isEmpty() ? origins : List.of("*"); | ||||||||||||||
| // Leave unset origins empty rather than defaulting to "*": AdkWebCorsConfig computes a | ||||||||||||||
| // same-machine loopback default from the server's own port when this is empty, so every | ||||||||||||||
| // origin on the internet is not allowed to call the server out of the box. | ||||||||||||||
|
Comment on lines
+37
to
+39
Contributor
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. Update the comment to reflect that the default loopback origins are allowed on any port using origin patterns, rather than being bound to the server's own port.
Suggested change
|
||||||||||||||
| origins = origins != null ? origins : List.of(); | ||||||||||||||
| methods = | ||||||||||||||
| methods != null && !methods.isEmpty() | ||||||||||||||
| ? 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.
Since we no longer need to inject the server port when using origin patterns, this import can be removed.