Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ASGI_App as "ASGI App\n(with CORSMiddleware)"
participant MiddlewareSetup as "Middleware setup"
participant Settings as "Settings\n(CORS_ORIGINS)"
Note over MiddlewareSetup,Settings: Startup
MiddlewareSetup->>Settings: read CORS_ORIGINS (string)
Settings-->>MiddlewareSetup: "http://localhost:3000,..."
MiddlewareSetup->>MiddlewareSetup: parse & validate -> list(origins)
MiddlewareSetup-->>ASGI_App: configure CORSMiddleware(origins list)
Client->>ASGI_App: Send request (with Origin header)
ASGI_App->>ASGI_App: CORSMiddleware checks Origin against origins list
ASGI_App-->>Client: respond (allowed or blocked)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@engine/src/api/middleware/__init__.py`:
- Around line 9-17: Move the misplaced docstring inside the setup_middleware
function to be the first statement in the function body (immediately after the
def line) so it becomes the function's actual docstring; specifically, relocate
the string """Set up all middleware for the application.""" above the settings =
get_settings() and cors_origins assignment in setup_middleware to ensure tools
and introspection see it correctly.
- Line 5: The relative import in engine/src/api/middleware/__init__.py is wrong:
change the import of get_settings from ".config.settings" to use a
parent-relative import ("..config.settings") so it resolves to
api/config/settings.py, and move the misplaced docstring so it immediately
follows the function definition (not after the cors_origins assignment);
specifically update the import of get_settings and reposition the docstring that
documents the function (the docstring should be placed directly below the
function signature at line 9) to restore correct module resolution and proper
docstring association.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/src/api/middleware/__init__.py`:
- Around line 3-6: The import block in engine/src/api/middleware/__init__.py is
misordered and uses an incorrect relative path for the settings import; change
the import from ".config.settings" to "..config.settings" and reorder the
imports to follow isort rules (third-party imports first, then local package
imports) so that FastAPI and CORSMiddleware imports come before the local
imports for get_settings and LoggingMiddleware; ensure the symbols FastAPI,
CORSMiddleware, get_settings, and LoggingMiddleware are preserved exactly.
|
@MayurK-cmd are you planning on resolving the comments? |
|
Yea I will give me 1-2 days |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
engine/src/api/middleware/__init__.py (1)
3-6: Import block still not isort-compliant — CI remains broken.The relative import fix (
..config.settings) is correctly applied, but the missing blank line between the third-party (fastapi) and first-party (..config,.logging_middleware) import groups is still present, which is what the CI linter was failing on.🔧 Proposed fix
from fastapi import FastAPI from fastapi.middleware.cors import CORSMiddleware -from ..config.settings import get_settings + +from ..config.settings import get_settings + from .logging_middleware import LoggingMiddleware🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/middleware/__init__.py` around lines 3 - 6, The import block in __init__.py violates isort grouping: add a blank line between the third-party imports (FastAPI, CORSMiddleware) and the local/first-party imports (get_settings from ..config.settings and LoggingMiddleware from .logging_middleware) so imports are grouped as third-party then a blank line then first-party; ensure the four identifiers FastAPI, CORSMiddleware, get_settings, and LoggingMiddleware remain and are separated by the blank line to satisfy the linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/api/middleware/__init__.py`:
- Line 18: Remove the trailing whitespace on the blank line after the list
comprehension in engine/src/api/middleware/__init__.py (the empty line following
the comprehension in the module scope); simply delete the extra spaces so the
blank line contains no whitespace and re-run formatting/linting (ruff/black) to
ensure CI no longer fails.
- Around line 13-22: The CORS setup currently builds cors_origins from
settings.CORS_ORIGINS and then calls app.add_middleware(CORSMiddleware,
allow_origins=cors_origins, allow_credentials=True) without guarding against the
wildcard "*"; add a startup-time validation that inspects settings.CORS_ORIGINS
(and the computed cors_origins list) and raises an explicit error (or exits) if
it contains "*" while allow_credentials is True, or alternatively convert "*" to
a safe explicit list or set allow_credentials=False; implement this check near
where cors_origins is computed so the app fails fast and logs a clear message
referencing cors_origins, settings.CORS_ORIGINS, app.add_middleware and
CORSMiddleware.
---
Duplicate comments:
In `@engine/src/api/middleware/__init__.py`:
- Around line 3-6: The import block in __init__.py violates isort grouping: add
a blank line between the third-party imports (FastAPI, CORSMiddleware) and the
local/first-party imports (get_settings from ..config.settings and
LoggingMiddleware from .logging_middleware) so imports are grouped as
third-party then a blank line then first-party; ensure the four identifiers
FastAPI, CORSMiddleware, get_settings, and LoggingMiddleware remain and are
separated by the blank line to satisfy the linter.
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
@MayurK-cmd resolve coderabbit comments, fix merge conflicts and push a single squashed commit: feat (engine): make CORS origins configurable via environment variable
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/src/api/middleware/__init__.py (1)
5-31:⚠️ Potential issue | 🟠 MajorFail fast when
CORS_ORIGINSresolves to an empty list.If the env var is empty or comma-only,
cors_originsbecomes[]and the service will silently reject all cross‑origin requests. Add a guard so misconfigurations fail loudly at startup.🛡️ Proposed fix
cors_origins = [ origin.strip() for origin in settings.CORS_ORIGINS.split(",") if origin.strip() ] + if not cors_origins: + raise ValueError( + "CORS_ORIGINS resolved to an empty list. " + "Set at least one allowed origin." + ) if "*" in cors_origins: raise ValueError( "CORS_ORIGINS cannot be set to '*' when allow_credentials=True. " "Please specify explicit origins instead." )Based on learnings: Skyflo.ai pull requests must be reviewed as top-tier, enterprise-grade software. Enforce best-in-class open-source standards across infrastructure, security, performance, developer ergonomics, and user experience. Assume real production impact and long-term ownership by default. No partial correctness. No cosmetic approvals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/middleware/__init__.py` around lines 5 - 31, The code currently builds cors_origins in setup_middleware but doesn't error when it resolves to an empty list; add a guard after the cors_origins comprehension in setup_middleware that raises a clear ValueError if cors_origins is empty (e.g. "CORS_ORIGINS resolved to empty list; please provide at least one origin"), so misconfigured or comma-only env values fail at startup; keep the existing wildcard check ("*" in cors_origins) intact but ensure the empty-list check runs before or alongside it to enforce the fail-fast behavior (refer to get_settings, settings.CORS_ORIGINS and the cors_origins local variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@engine/src/api/middleware/__init__.py`:
- Around line 5-31: The code currently builds cors_origins in setup_middleware
but doesn't error when it resolves to an empty list; add a guard after the
cors_origins comprehension in setup_middleware that raises a clear ValueError if
cors_origins is empty (e.g. "CORS_ORIGINS resolved to empty list; please provide
at least one origin"), so misconfigured or comma-only env values fail at
startup; keep the existing wildcard check ("*" in cors_origins) intact but
ensure the empty-list check runs before or alongside it to enforce the fail-fast
behavior (refer to get_settings, settings.CORS_ORIGINS and the cors_origins
local variable).
|
Pushed the new changes, Please check it |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/api/config/settings.py`:
- Line 17: Add a Pydantic `@field_validator` for the CORS_ORIGINS setting to
validate each comma-separated origin at startup: in the Settings (BaseSettings)
class add a validator for the "CORS_ORIGINS" field that splits the string on
commas, strips whitespace, skips or allows a single "*" if intended, and for
each origin uses a URL parse (e.g., urllib.parse.urlparse) to ensure it has a
scheme of "http" or "https" and no path/params/fragment (i.e., netloc present
and path == ""), raising a ValueError with an actionable message if any origin
is malformed so misconfigurations fail fast.
In `@engine/src/api/middleware/__init__.py`:
- Around line 13-17: The list comprehension that builds cors_origins calls
origin.strip() twice; change it to compute the stripped value once using the
walrus operator (e.g., use "if (s := origin.strip())" and add s to the list) so
the code only strips each origin once; update the comprehension that references
settings.CORS_ORIGINS and the cors_origins variable accordingly to use the
walrus-bound value.
- Around line 34-36: The CORS setup uses allow_headers=["*"] while
allow_credentials=True which violates the CORS spec; update the CORS middleware
configuration (the allow_credentials and allow_headers settings in
engine/src/api/middleware/__init__.py) to replace the wildcard with an explicit
list of allowed headers (for example:
["Authorization","Content-Type","Accept","Origin","User-Agent","X-Requested-With","Access-Control-Request-Method","Access-Control-Request-Headers"])
so credentialed requests use explicit header names instead of "*".
|
|
||
| LOG_LEVEL: str = "INFO" | ||
|
|
||
| CORS_ORIGINS: str = "http://localhost:3000,http://localhost:3001" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a @field_validator to validate each origin's format at startup.
The current str field accepts any value. Misconfigured origins such as https://app.example.com/path (path component included) or app.example.com (missing scheme) will pass all existing runtime guards in the middleware (non-empty, non-wildcard), yet never match any browser Origin header — causing silent CORS failures in production with no server-side diagnostic.
A startup-time validator catches this class of misconfiguration with an actionable message:
🛡️ Proposed fix
from typing import Optional
from pydantic import Field
+from pydantic import field_validator
from pydantic_settings import BaseSettings
class Settings(BaseSettings):
...
CORS_ORIGINS: str = "http://localhost:3000,http://localhost:3001"
+
+ `@field_validator`("CORS_ORIGINS")
+ `@classmethod`
+ def validate_cors_origins(cls, v: str) -> str:
+ from urllib.parse import urlparse
+ for raw in v.split(","):
+ origin = raw.strip()
+ if not origin:
+ continue
+ parsed = urlparse(origin)
+ if parsed.scheme not in ("http", "https"):
+ raise ValueError(
+ f"CORS origin '{origin}' must use http or https scheme."
+ )
+ if not parsed.netloc:
+ raise ValueError(
+ f"CORS origin '{origin}' is missing a valid host."
+ )
+ if parsed.path and parsed.path != "/":
+ raise ValueError(
+ f"CORS origin '{origin}' must not include a path component."
+ )
+ return v📝 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.
| CORS_ORIGINS: str = "http://localhost:3000,http://localhost:3001" | |
| from typing import Optional | |
| from pydantic import Field | |
| from pydantic import field_validator | |
| from pydantic_settings import BaseSettings | |
| class Settings(BaseSettings): | |
| ... | |
| CORS_ORIGINS: str = "http://localhost:3000,http://localhost:3001" | |
| `@field_validator`("CORS_ORIGINS") | |
| `@classmethod` | |
| def validate_cors_origins(cls, v: str) -> str: | |
| from urllib.parse import urlparse | |
| for raw in v.split(","): | |
| origin = raw.strip() | |
| if not origin: | |
| continue | |
| parsed = urlparse(origin) | |
| if parsed.scheme not in ("http", "https"): | |
| raise ValueError( | |
| f"CORS origin '{origin}' must use http or https scheme." | |
| ) | |
| if not parsed.netloc: | |
| raise ValueError( | |
| f"CORS origin '{origin}' is missing a valid host." | |
| ) | |
| if parsed.path and parsed.path != "/": | |
| raise ValueError( | |
| f"CORS origin '{origin}' must not include a path component." | |
| ) | |
| return v |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/api/config/settings.py` at line 17, Add a Pydantic
`@field_validator` for the CORS_ORIGINS setting to validate each comma-separated
origin at startup: in the Settings (BaseSettings) class add a validator for the
"CORS_ORIGINS" field that splits the string on commas, strips whitespace, skips
or allows a single "*" if intended, and for each origin uses a URL parse (e.g.,
urllib.parse.urlparse) to ensure it has a scheme of "http" or "https" and no
path/params/fragment (i.e., netloc present and path == ""), raising a ValueError
with an actionable message if any origin is malformed so misconfigurations fail
fast.
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
@MayurK-cmd please resolve coderabbit comments and resolve merge conflicts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
engine/src/api/config/settings.py (1)
17-18:⚠️ Potential issue | 🟠 MajorFail fast on malformed
CORS_ORIGINSvalues at settings load.Line 17 stores
CORS_ORIGINSas an unvalidated raw string. Misconfigured values (e.g., missing scheme, path/query/fragment, slash-suffixed origins) can pass startup but never match browserOrigin, causing silent production CORS failures.🛠️ Proposed fix
class Settings(BaseSettings): @@ def __init__(self, **kwargs): super().__init__(**kwargs) + self.CORS_ORIGINS = self._validate_and_normalize_cors_origins( + self.CORS_ORIGINS + ) @@ def _get_checkpointer_url(self) -> str: url = self.POSTGRES_DATABASE_URL @@ return url + + def _validate_and_normalize_cors_origins(self, raw_value: str) -> str: + from urllib.parse import urlparse + + normalized: list[str] = [] + for raw_origin in raw_value.split(","): + origin = raw_origin.strip() + if not origin: + continue + + parsed = urlparse(origin) + if parsed.scheme not in {"http", "https"} or not parsed.netloc: + raise ValueError( + f"Invalid CORS origin '{origin}'. Use http(s)://host[:port]." + ) + if parsed.path not in ("", "/") or parsed.params or parsed.query or parsed.fragment: + raise ValueError( + f"Invalid CORS origin '{origin}'. Paths/query/fragment are not allowed." + ) + + normalized.append(f"{parsed.scheme}://{parsed.netloc}") + + if not normalized: + raise ValueError("CORS_ORIGINS must contain at least one valid origin.") + + return ",".join(dict.fromkeys(normalized))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/config/settings.py` around lines 17 - 18, CORS_ORIGINS is stored as an unvalidated raw string; change settings to parse and validate it at load time (split the string into origins, ensure each parses to a valid origin with scheme http/https, hostname and optional port, and contains no path/query/fragment or trailing slash) and fail fast by raising an error if any entry is malformed so the process cannot start with bad values; implement this validation where CORS_ORIGINS is defined (or in the Settings model/constructor/validator that produces CORS_ORIGINS) and replace the raw string with a validated list/tuple of origins so downstream code receives only correct origin values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@engine/src/api/config/settings.py`:
- Around line 17-18: CORS_ORIGINS is stored as an unvalidated raw string; change
settings to parse and validate it at load time (split the string into origins,
ensure each parses to a valid origin with scheme http/https, hostname and
optional port, and contains no path/query/fragment or trailing slash) and fail
fast by raising an error if any entry is malformed so the process cannot start
with bad values; implement this validation where CORS_ORIGINS is defined (or in
the Settings model/constructor/validator that produces CORS_ORIGINS) and replace
the raw string with a validated list/tuple of origins so downstream code
receives only correct origin values.
|
@KaranJagtiani, I have fixed all the CodeRabbit issues. Will you resolve the conflicts? |
|
@MayurK-cmd Conflicts are standard. Rebase on |
|
I am on it |
|
@MayurK-cmd There are pending changes blocking this PR. |
conflicts fixed :wq
|
@KaranJagtiani really sorry i was trying to understand the conflicts it took me some time |
2 participants
Description
This PR makes the Engine CORS configuration configurable via environment variables instead of hardcoded localhost values.
CORS allowed origins are now read from a
CORS_ORIGINSsetting, allowing production and staging frontend domains to be configured without code changes. Default localhost origins are preserved to ensure no regression in local development.Related Issue(s)
Fixes #95
Type of Change
Testing
Manual testing performed:
CORS_ORIGINSand verified requests fromhttp://localhost:3000andhttp://localhost:3001are allowed.CORS_ORIGINS=https://app.skyflo.ai,https://staging.skyflo.aiand restarted the Engine.Checklist
Before Requesting Review
Code Quality
printstatements orconsole.logcallspackage-lock.json(we useyarnonly for the UI)Screenshots (if applicable)
N/A