Skip to content

LCORE-405: configurable CORS settings #406

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 17, 2025
Merged

LCORE-405: configurable CORS settings #406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba6e216
LCORE-405: configurable CORS settings
tisnik Aug 17, 2025
ee17b5d
Updated unit and integration tests
tisnik Aug 17, 2025
8f3970b
Updated configuration accordingly
tisnik Aug 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified docs/config.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
9 changes: 9 additions & 0 deletions docs/config.puml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ class "AuthenticationConfiguration" as src.models.config.AuthenticationConfigura
skip_tls_verification : bool
check_authentication_model() -> Self
}
class "CORSConfiguration" as src.models.config.CORSConfiguration {
allow_credentials : bool
allow_headers : list[str]
allow_methods : list[str]
allow_origins : list[str]
check_cors_configuration() -> Self
}
class "Configuration" as src.models.config.Configuration {
authentication
customization : Optional[Customization]
Expand Down Expand Up @@ -78,6 +85,7 @@ class "ServiceConfiguration" as src.models.config.ServiceConfiguration {
access_log : bool
auth_enabled : bool
color_log : bool
cors
host : str
port : int
tls_config
Expand All @@ -98,6 +106,7 @@ class "UserDataCollection" as src.models.config.UserDataCollection {
check_storage_location_is_set_when_needed() -> Self
}
src.models.config.AuthenticationConfiguration --* src.models.config.Configuration : authentication
src.models.config.CORSConfiguration --* src.models.config.ServiceConfiguration : cors
src.models.config.DatabaseConfiguration --* src.models.config.Configuration : database
src.models.config.InferenceConfiguration --* src.models.config.Configuration : inference
src.models.config.JwtConfiguration --* src.models.config.JwkConfiguration : jwt_configuration
Expand Down
10 changes: 6 additions & 4 deletions src/app/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,14 @@
],
)

cors = configuration.service_configuration.cors

app.add_middleware(
CORSMiddleware,
allow_origins=["*"],
allow_credentials=True,
allow_methods=["*"],
allow_headers=["*"],
allow_origins=cors.allow_origins,
allow_credentials=cors.allow_credentials,
allow_methods=cors.allow_methods,
allow_headers=cors.allow_headers,
Comment on lines +43 to +50
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Guard against invalid CORS at runtime (credentials + '*')

Given current defaults, this will configure CORSMiddleware with allow_credentials=True and wildcard origins, which browsers will reject. Either rely on config-model validation (preferred, see suggested change in config.py) or defensively disable credentials when wildcards are present.

Apply this diff to add a runtime guard:

-cors = configuration.service_configuration.cors
+cors = configuration.service_configuration.cors
+allow_credentials = cors.allow_credentials
+if cors.allow_credentials and "*" in cors.allow_origins:
+    logger.warning(
+        "CORS misconfiguration detected: allow_credentials=True with wildcard origins. "
+        "Overriding to allow_credentials=False to comply with browsers and CORS spec."
+    )
+    allow_credentials = False
 
 app.add_middleware(
     CORSMiddleware,
     allow_origins=cors.allow_origins,
-    allow_credentials=cors.allow_credentials,
+    allow_credentials=allow_credentials,
     allow_methods=cors.allow_methods,
     allow_headers=cors.allow_headers,
 )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cors = configuration.service_configuration.cors
app.add_middleware(
CORSMiddleware,
allow_origins=["*"],
allow_credentials=True,
allow_methods=["*"],
allow_headers=["*"],
allow_origins=cors.allow_origins,
allow_credentials=cors.allow_credentials,
allow_methods=cors.allow_methods,
allow_headers=cors.allow_headers,
cors = configuration.service_configuration.cors
allow_credentials = cors.allow_credentials
if cors.allow_credentials and "*" in cors.allow_origins:
logger.warning(
"CORS misconfiguration detected: allow_credentials=True with wildcard origins. "
"Overriding to allow_credentials=False to comply with browsers and CORS spec."
)
allow_credentials = False
app.add_middleware(
CORSMiddleware,
allow_origins=cors.allow_origins,
allow_credentials=allow_credentials,
allow_methods=cors.allow_methods,
allow_headers=cors.allow_headers,
)
🤖 Prompt for AI Agents 
In src/app/main.py around lines 43 to 50, the CORS middleware is being
constructed with allow_credentials=True while allow_origins may be the wildcard
('*'), which browsers reject; add a runtime guard that checks if
cors.allow_credentials is truthy and cors.allow_origins contains a wildcard
(exact '*' or pattern that means any origin) and if so set
allow_credentials_used=False (or override to False) and emit a warning log
explaining credentials were disabled due to wildcard origins; then pass the
possibly-overridden allow_credentials value into CORSMiddleware so credentials
are not enabled when origins are wildcarded (alternative: you may instead
raise/abort early if you prefer strict validation, but implement the defensive
override as described).

)


Expand Down
17 changes: 17 additions & 0 deletions src/models/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,22 @@ def check_tls_configuration(self) -> Self:
return self


class CORSConfiguration(BaseModel):
"""CORS configuration."""

allow_origins: list[str] = [
"*"
] # not AnyHttpUrl: we need to support "*" that is not valid URL
allow_credentials: bool = True
allow_methods: list[str] = ["*"]
allow_headers: list[str] = ["*"]

@model_validator(mode="after")
def check_cors_configuration(self) -> Self:
"""Check CORS configuration."""
return self

Comment on lines +27 to +41
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Block invalid/insecure combo: '*' origins with allow_credentials=True

Browsers reject credentialed requests when Access-Control-Allow-Origin is ''. Shipping defaults as allow_origins [''] and allow_credentials True is both insecure and functionally broken.

Apply this diff to use a safe default and enforce validation:

 class CORSConfiguration(BaseModel):
     """CORS configuration."""
 
-    allow_origins: list[str] = [
-        "*"
-    ]  # not AnyHttpUrl: we need to support "*" that is not valid URL
-    allow_credentials: bool = True
+    allow_origins: list[str] = [
+        "*"
+    ]  # not AnyHttpUrl: we need to support "*" that is not valid URL
+    allow_credentials: bool = False
     allow_methods: list[str] = ["*"]
     allow_headers: list[str] = ["*"]
 
     @model_validator(mode="after")
     def check_cors_configuration(self) -> Self:
-        """Check CORS configuration."""
-        return self
+        """Check CORS configuration."""
+        # Credentials are not allowed with wildcard origins per CORS/Fetch spec.
+        if self.allow_credentials and "*" in self.allow_origins:
+            raise ValueError(
+                "Invalid CORS config: allow_credentials cannot be true when allow_origins contains '*'. "
+                "Use explicit origins or disable credentials."
+            )
+        return self
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class CORSConfiguration(BaseModel):
"""CORS configuration."""
allow_origins: list[str] = [
"*"
] # not AnyHttpUrl: we need to support "*" that is not valid URL
allow_credentials: bool = True
allow_methods: list[str] = ["*"]
allow_headers: list[str] = ["*"]
@model_validator(mode="after")
def check_cors_configuration(self) -> Self:
"""Check CORS configuration."""
return self
class CORSConfiguration(BaseModel):
"""CORS configuration."""
allow_origins: list[str] = [
"*"
] # not AnyHttpUrl: we need to support "*" that is not valid URL
allow_credentials: bool = False
allow_methods: list[str] = ["*"]
allow_headers: list[str] = ["*"]
@model_validator(mode="after")
def check_cors_configuration(self) -> Self:
"""Check CORS configuration."""
# Credentials are not allowed with wildcard origins per CORS/Fetch spec.
if self.allow_credentials and "*" in self.allow_origins:
raise ValueError(
"Invalid CORS config: allow_credentials cannot be true when allow_origins contains '*'. "
"Use explicit origins or disable credentials."
)
return self
🤖 Prompt for AI Agents 
In src/models/config.py around lines 27 to 41, the model currently defaults
allow_origins to ["*"] while allow_credentials is True which is invalid and
insecure; change the default to a safe value (e.g., an empty list or a specific
origin like "http://localhost") and update the model_validator to enforce that
if allow_credentials is True then allow_origins must not contain "*" (raise a
ValueError with a clear message), or alternatively if "*" is present and
allow_credentials is True, set allow_credentials to False or require the
developer to provide explicit origins; ensure the validator runs after
initialization and returns Self when valid.


class SQLiteDatabaseConfiguration(BaseModel):
"""SQLite database configuration."""

Expand Down Expand Up @@ -106,6 +122,7 @@ class ServiceConfiguration(BaseModel):
color_log: bool = True
access_log: bool = True
tls_config: TLSConfiguration = TLSConfiguration()
cors: CORSConfiguration = CORSConfiguration()

@model_validator(mode="after")
def check_service_configuration(self) -> Self:
Expand Down
14 changes: 14 additions & 0 deletions tests/configuration/lightspeed-stack.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,20 @@ service:
workers: 1
color_log: true
access_log: true
cors:
allow_origins:
- foo_origin
- bar_origin
- baz_origin
allow_credentials: false
allow_methods:
- foo_method
- bar_method
- baz_method
allow_headers:
- foo_header
- bar_header
- baz_header
llama_stack:
# Uses a remote llama-stack service
# The instance would have already been started with a llama-stack-run.yaml file
Expand Down
7 changes: 7 additions & 0 deletions tests/integration/test_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ def test_loading_proper_configuration(configuration_filename: str) -> None:
assert svc_config.color_log is True
assert svc_config.access_log is True

# check 'service.cors' section
cors_config = cfg.service_configuration.cors
assert cors_config.allow_origins == ["foo_origin", "bar_origin", "baz_origin"]
assert cors_config.allow_credentials is False
assert cors_config.allow_methods == ["foo_method", "bar_method", "baz_method"]
assert cors_config.allow_headers == ["foo_header", "bar_header", "baz_header"]

Comment on lines +50 to +56
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Guard against import-time access to configuration in app startup.

Per src/configuration.py (lines 62-67), accessing service_configuration asserts that configuration is loaded. If src/app/main.py reads configuration.service_configuration.cors at module import time (as stated in the PR summary), startup can fail unless configuration is loaded beforehand. Consider deferring the CORS read to runtime (e.g., during app factory, lifespan event) or ensuring configuration is loaded before main is imported.

[offer_architecture_advice]
Run this script to confirm if main.py accesses CORS at import time and whether configuration is loaded before that anywhere:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Searching for module-level access to configuration.service_configuration.cors:"
rg -n -C3 -P 'configuration\.service_configuration\.cors' --type=py

echo
echo "Searching for calls that load configuration (order review):"
rg -n -C3 -P '\bload_configuration\s*\(' --type=py

Length of output: 4482

Defer import-time CORS setup in src/app/main.py

The middleware is pulling in configuration.service_configuration.cors at module import time (line 43) but there’s no prior call to load_configuration() in that file. If someone does import src.app.main without first loading your YAML config, startup will crash.

• Location to fix:
– src/app/main.py:43
python cors = configuration.service_configuration.cors
• Suggested refactor:
– Move CORS retrieval and middleware registration into a function that runs after load_configuration()—for example, inside an app_factory() or an @app.on_event("startup") handler.
– Or, if you truly must keep it at module level, call configuration.load_configuration(...) before accessing service_configuration in that module.

🤖 Prompt for AI Agents 
In src/app/main.py around line 43, the module currently accesses
configuration.service_configuration.cors at import time which can crash if
load_configuration() hasn't been called; move the CORS retrieval and middleware
registration out of module scope into a function that runs after configuration
is loaded (e.g., an app_factory() that calls
configuration.load_configuration(...) before reading service_configuration and
then registers the CORS middleware, or register the middleware inside an
@app.on_event("startup") handler that reads
configuration.service_configuration.cors), alternatively if you must keep it at
module level call configuration.load_configuration(...) first before accessing
service_configuration.

# check 'llama_stack' section
ls_config = cfg.llama_stack_configuration
assert ls_config.use_as_library_client is False
Expand Down
52 changes: 51 additions & 1 deletion tests/unit/models/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
ServiceConfiguration,
UserDataCollection,
TLSConfiguration,
CORSConfiguration,
ModelContextProtocolServer,
InferenceConfiguration,
)
Expand Down Expand Up @@ -214,6 +215,31 @@ def test_user_data_collection_transcripts_disabled() -> None:
UserDataCollection(transcripts_enabled=True, transcripts_storage=None)


def test_cors_default_configuration() -> None:
"""Test the CORS configuration."""
cfg = CORSConfiguration()
assert cfg is not None
assert cfg.allow_origins == ["*"]
assert cfg.allow_credentials is True
assert cfg.allow_methods == ["*"]
assert cfg.allow_headers == ["*"]


def test_cors_custom_configuration() -> None:
"""Test the CORS configuration."""
cfg = CORSConfiguration(
allow_origins=["foo_origin", "bar_origin", "baz_origin"],
allow_credentials=False,
allow_methods=["foo_method", "bar_method", "baz_method"],
allow_headers=["foo_header", "bar_header", "baz_header"],
)
assert cfg is not None
assert cfg.allow_origins == ["foo_origin", "bar_origin", "baz_origin"]
assert cfg.allow_credentials is False
assert cfg.allow_methods == ["foo_method", "bar_method", "baz_method"]
assert cfg.allow_headers == ["foo_header", "bar_header", "baz_header"]


def test_tls_configuration() -> None:
"""Test the TLS configuration."""
cfg = TLSConfiguration(
Expand Down Expand Up @@ -437,7 +463,13 @@ def test_dump_configuration(tmp_path) -> None:
tls_certificate_path=Path("tests/configuration/server.crt"),
tls_key_path=Path("tests/configuration/server.key"),
tls_key_password=Path("tests/configuration/password"),
)
),
cors=CORSConfiguration(
allow_origins=["foo_origin", "bar_origin", "baz_origin"],
allow_credentials=False,
allow_methods=["foo_method", "bar_method", "baz_method"],
allow_headers=["foo_header", "bar_header", "baz_header"],
),
),
llama_stack=LlamaStackConfiguration(
use_as_library_client=True,
Expand Down Expand Up @@ -488,6 +520,24 @@ def test_dump_configuration(tmp_path) -> None:
"tls_key_password": "tests/configuration/password",
"tls_key_path": "tests/configuration/server.key",
},
"cors": {
"allow_credentials": False,
"allow_headers": [
"foo_header",
"bar_header",
"baz_header",
],
"allow_methods": [
"foo_method",
"bar_method",
"baz_method",
],
"allow_origins": [
"foo_origin",
"bar_origin",
"baz_origin",
],
},
},
"llama_stack": {
"url": None,
Expand Down