fix: security hardening — iframe sandbox, CSP, webhook auth, path traversal - #36
Closed
joshuajerin wants to merge 4 commits into
Closed
fix: security hardening — iframe sandbox, CSP, webhook auth, path traversal#36joshuajerin wants to merge 4 commits into
joshuajerin wants to merge 4 commits into
Conversation
The iframe running Expo Snack previews had no sandbox attribute and granted geolocation/camera/microphone permissions, allowing AI-generated code full access to the parent origin's storage, cookies, and navigation. Add sandbox with minimal permissions (scripts, same-origin, forms, popups) and remove unnecessary device access grants.
1. CSP enforcement (vercel.json): Promote Content-Security-Policy from report-only to enforced. Add frame-src for Expo Snack preview, connect-src for PostHog, and wasm-unsafe-eval for Snack SDK. 2. Google webhook verification (webhooks.py, config.py, watch_manager.py): Add GOOGLE_WEBHOOK_TOKEN config. Gmail endpoint verifies ?token= query param; Calendar endpoint verifies X-Goog-Channel-Token header. Watch creation now includes the token so Google sends it back on notifications. 3. Sandbox path traversal fix (sandbox_files.py): Replace weak regex sanitization with posixpath.normpath + prefix validation + shlex.quote. Paths must resolve under /home/user to prevent ../ traversal.
📝 WalkthroughWalkthroughAdds a new Google webhook token setting and enforces its use for Pub/Sub/Gmail and Calendar webhooks, adds server-side token verification, tightens sandbox path sanitization, changes iframe sandboxing attributes, and updates CSP from report-only to enforced. Tests and subscription setup updated to require the token. Changes
Sequence Diagram(s)sequenceDiagram
participant Google as Google (Pub/Sub / Calendar / Gmail)
participant Server as App Server (webhooks endpoint)
participant WatchSvc as WatchManager / PubSub setup
participant Queue as Work Queue / Processor
Note over WatchSvc,Google: Create watch / Pub/Sub subscription (includes token)
WatchSvc->>Google: POST /watches (body includes token)
Google-->>WatchSvc: 200 OK
Note over Google,Server: Incoming webhook push
Google->>Server: POST /api/webhooks/gmail?token=... or POST /api/webhooks/calendar with X-Goog-Channel-Token
Server->>Server: _verify_google_webhook_token(token)
alt token valid
Server->>Queue: enqueue processing job
Server-->>Google: 200 OK
else token invalid
Server-->>Google: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (3)
core-web/vercel.json (1)
8-8: Narrowconnect-srcto explicit PostHog hosts.The CSP currently allows
https://*.posthog.com, but the code uses onlyhttps://us.i.posthog.comby default (with optional env override viaVITE_POSTHOG_HOST). Tightening this to the specific expected host(s) reduces outbound surface in case of XSS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-web/vercel.json` at line 8, Update the CSP string in the "value" entry so connect-src no longer contains the wildcard "https://*.posthog.com"; instead list the specific PostHog host(s) your app uses (e.g., the default "https://us.i.posthog.com" and any alternate host you support via VITE_POSTHOG_HOST) so the connect-src directive explicitly allows only those exact origins; locate and edit the connect-src portion of the policy string in the current value to replace "https://*.posthog.com" with the exact host(s) you intend to permit.core-api/api/config.py (1)
187-189: Add fail-fast validation for webhook token configuration.
google_webhook_tokendefaults to empty, but webhook handlers now reject requests when it is unset. Consider validating this at startup (e.g., when Google webhook features are enabled) to avoid silent runtime breakage.Suggested guard
class Settings(BaseSettings): @@ google_webhook_token: str = "" @@ `@model_validator`(mode="after") def validate_token_encryption_settings(self): @@ return self + + `@model_validator`(mode="after") + def validate_google_webhook_settings(self): + if self.google_pubsub_topic and not self.google_webhook_token: + raise ValueError( + "GOOGLE_WEBHOOK_TOKEN must be set when GOOGLE_PUBSUB_TOPIC is configured" + ) + return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/api/config.py` around lines 187 - 189, The config currently sets google_webhook_token = "" which leads to runtime rejections; add a fail-fast validation that checks google_webhook_token at startup (for example in the Config class __post_init__ or in the existing application startup/init function that enables Google webhook features) and raise a clear exception or exit the process if the token is empty when webhook handling is enabled; reference the google_webhook_token setting and the startup/init code path that enables Google webhook features so the app fails fast with an explicit error message rather than silently breaking at request time.core-api/api/routers/webhooks.py (1)
63-63: UseAnnotatedfor FastAPI dependency parameters to satisfy tooling.Switching dependency params to
Annotated[...]follows modern FastAPI best practices (0.95.0+) and clears Sonar violations. This also improves editor support and type checking.Proposed refactor
-from typing import Optional +from typing import Optional, Annotated @@ -async def gmail_webhook(request: Request, token: Optional[str] = Query(None)): +async def gmail_webhook( + request: Request, + token: Annotated[Optional[str], Query()] = None +): @@ - x_goog_message_number: Optional[str] = Header(None), - x_goog_channel_token: Optional[str] = Header(None), + x_goog_message_number: Annotated[Optional[str], Header()] = None, + x_goog_channel_token: Annotated[Optional[str], Header()] = None, ):Also applies to: 177-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/api/routers/webhooks.py` at line 63, The gmail_webhook dependency parameter should use typing.Annotated to satisfy FastAPI tooling: change the signature in gmail_webhook from token: Optional[str] = Query(None) to token: Annotated[Optional[str], Query(None)] and similarly update the other dependency parameters referenced around lines 177-178 to use Annotated[...] with their Query/Depends markers; also add the necessary Annotated import (e.g., from typing import Annotated) alongside existing Optional imports so type checkers and Sonar stop flagging these parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core-api/api/routers/webhooks.py`:
- Around line 89-92: The webhook handlers currently return a JSON body with
"Unauthorized" while sending HTTP 200; update the logic around
_verify_google_webhook_token to return proper HTTP auth status codes (raise
fastapi.HTTPException(status_code=401, detail="Unauthorized") or return a
Response with status_code=401/403) instead of returning {"status":"error",...} —
apply this change in the Gmail webhook verification branch where
_verify_google_webhook_token(token) is checked and the other identical block
later in the file so invalid tokens produce a 401/403 HTTP response and not a
200 OK.
- Around line 63-64: The Gmail webhook now expects a query parameter token but
the Pub/Sub subscription setup still constructs the push endpoint without it,
causing rejected notifications; update the setup script to include the token
when building the push endpoint (append ?token=<YOUR_VERIFICATION_TOKEN> or
proper URL-encoding) so the generated push_endpoint matches what the
gmail_webhook handler expects, or alternatively relax gmail_webhook's validation
to accept Pub/Sub calls without the token; locate the gmail_webhook function and
the push_endpoint variable in the setup_pubsub_subscription script and make the
token inclusion/validation consistent between them.
In `@core-api/api/services/agents/sandbox_files.py`:
- Around line 6-8: Remove the unused import `re` from the top of
sandbox_files.py to satisfy the linter (Ruff F401); locate the import line that
currently reads "import posixpath, import re, import shlex" (or separate
imports) and delete the `re` entry so only the used modules (e.g., posixpath and
shlex) remain imported.
In `@core-api/api/services/syncs/watch_manager.py`:
- Around line 265-267: Both code paths that optionally attach
settings.google_webhook_token to request_body (the conditional around
request_body['token']) must fail fast when the token is unset because the
webhook endpoint now requires a valid token; update the logic so that wherever
you currently have "if settings.google_webhook_token: request_body['token'] =
settings.google_webhook_token" you instead check for a truthy
settings.google_webhook_token and if missing log an error and raise/return an
explicit failure (e.g., raise an exception or return an error result) so the
Calendar watch creation is aborted rather than creating an unusable
subscription; apply this change to the two places that build request_body (the
branch that currently sets request_body['token'] and the second occurrence
further down).
---
Nitpick comments:
In `@core-api/api/config.py`:
- Around line 187-189: The config currently sets google_webhook_token = "" which
leads to runtime rejections; add a fail-fast validation that checks
google_webhook_token at startup (for example in the Config class __post_init__
or in the existing application startup/init function that enables Google webhook
features) and raise a clear exception or exit the process if the token is empty
when webhook handling is enabled; reference the google_webhook_token setting and
the startup/init code path that enables Google webhook features so the app fails
fast with an explicit error message rather than silently breaking at request
time.
In `@core-api/api/routers/webhooks.py`:
- Line 63: The gmail_webhook dependency parameter should use typing.Annotated to
satisfy FastAPI tooling: change the signature in gmail_webhook from token:
Optional[str] = Query(None) to token: Annotated[Optional[str], Query(None)] and
similarly update the other dependency parameters referenced around lines 177-178
to use Annotated[...] with their Query/Depends markers; also add the necessary
Annotated import (e.g., from typing import Annotated) alongside existing
Optional imports so type checkers and Sonar stop flagging these parameters.
In `@core-web/vercel.json`:
- Line 8: Update the CSP string in the "value" entry so connect-src no longer
contains the wildcard "https://*.posthog.com"; instead list the specific PostHog
host(s) your app uses (e.g., the default "https://us.i.posthog.com" and any
alternate host you support via VITE_POSTHOG_HOST) so the connect-src directive
explicitly allows only those exact origins; locate and edit the connect-src
portion of the policy string in the current value to replace
"https://*.posthog.com" with the exact host(s) you intend to permit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e8a899b-0cad-4692-8974-445ff6db69f3
📒 Files selected for processing (6)
core-api/api/config.pycore-api/api/routers/webhooks.pycore-api/api/services/agents/sandbox_files.pycore-api/api/services/syncs/watch_manager.pycore-web/src/components/ai-app-builder/BuilderPreview.tsxcore-web/vercel.json
- Remove unused `re` import (ruff F401 lint failure) - Return HTTP 401 instead of 200 on invalid webhook tokens - Fail fast in watch creation when GOOGLE_WEBHOOK_TOKEN is unset - Include token in Pub/Sub setup script push endpoint - Add google_webhook_token to test mocks to fix 2 failing tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core-api/api/routers/webhooks.py (1)
62-63: SonarCloud: Cognitive complexity and type hint style.Static analysis flags:
- Cognitive complexity 17 > 15 — the nested try/except and inline queue fallback contribute. Consider extracting the queue-enqueue logic into a helper.
- Use
Annotatedtype hints — FastAPI recommendsAnnotated[str, Query(...)]overQuery(...)as default value for better tooling support.- Document 401 in
responses— addresponses={401: {"description": "Invalid or missing token"}}to the decorator.These are style/documentation improvements, not blockers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/api/routers/webhooks.py` around lines 62 - 63, The gmail_webhook endpoint has high cognitive complexity due to nested try/except and inline queue fallback — extract the queue-enqueue and fallback logic into a separate helper (e.g., enqueue_gmail_webhook or enqueue_webhook_event) and move the try/except blocks into that helper so gmail_webhook becomes a simple orchestrator; change the token parameter annotation to use Annotated[str, Query(None)] instead of Query(...) for FastAPI typing; and update the `@router.post` decorator on gmail_webhook to include responses={401: {"description": "Invalid or missing token"}} to document the 401 case.core-api/setup_pubsub_subscription.py (1)
54-54: Consider masking the token in printed output.The full endpoint URL (including the secret token) is printed at Line 54 and Line 100. While this is a manual setup script, printing secrets can lead to accidental exposure in terminal history or CI logs if someone repurposes this script.
Suggested redaction
- print(f"🌐 Push Endpoint: {push_endpoint}") + # Redact token in output + safe_endpoint = f"{webhook_url}/api/webhooks/gmail?token=****" + print(f"🌐 Push Endpoint: {safe_endpoint}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/setup_pubsub_subscription.py` at line 54, The script currently prints the full push_endpoint (variable push_endpoint) which includes a secret token; update both print sites that output push_endpoint to redact the token before printing by parsing the URL/string and replacing the token value with a masked string (e.g., show only last 4 chars or "<redacted>") so logs never contain the secret; locate usages of push_endpoint (the print at Line 54 and the later print around Line 100) and replace direct printing with a masked representation generated by a short helper (e.g., mask_token_from_url(push_endpoint)) or inline parsing to preserve host/path but hide the token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core-api/api/routers/webhooks.py`:
- Around line 87-91: Move the token verification out of the broad try/except so
raised HTTPException is not swallowed: for both webhook handlers (the Gmail
handler that calls _verify_google_webhook_token and the calendar_webhook
handler), perform the token check and raise HTTPException(status_code=401)
before entering the try block that catches Exception; this ensures the
authentication error propagates as a 401 instead of being caught and converted
into a 200 error response.
---
Nitpick comments:
In `@core-api/api/routers/webhooks.py`:
- Around line 62-63: The gmail_webhook endpoint has high cognitive complexity
due to nested try/except and inline queue fallback — extract the queue-enqueue
and fallback logic into a separate helper (e.g., enqueue_gmail_webhook or
enqueue_webhook_event) and move the try/except blocks into that helper so
gmail_webhook becomes a simple orchestrator; change the token parameter
annotation to use Annotated[str, Query(None)] instead of Query(...) for FastAPI
typing; and update the `@router.post` decorator on gmail_webhook to include
responses={401: {"description": "Invalid or missing token"}} to document the 401
case.
In `@core-api/setup_pubsub_subscription.py`:
- Line 54: The script currently prints the full push_endpoint (variable
push_endpoint) which includes a secret token; update both print sites that
output push_endpoint to redact the token before printing by parsing the
URL/string and replacing the token value with a masked string (e.g., show only
last 4 chars or "<redacted>") so logs never contain the secret; locate usages of
push_endpoint (the print at Line 54 and the later print around Line 100) and
replace direct printing with a masked representation generated by a short helper
(e.g., mask_token_from_url(push_endpoint)) or inline parsing to preserve
host/path but hide the token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 032c36f7-4353-4b6c-853b-456b7d0702f3
📒 Files selected for processing (5)
core-api/api/routers/webhooks.pycore-api/api/services/agents/sandbox_files.pycore-api/api/services/syncs/watch_manager.pycore-api/setup_pubsub_subscription.pycore-api/tests/unit/test_watch_manager_permanent_failures.py
✅ Files skipped from review due to trivial changes (1)
- core-api/tests/unit/test_watch_manager_permanent_failures.py
🚧 Files skipped from review as they are similar to previous changes (1)
- core-api/api/services/syncs/watch_manager.py
… 401 The outer try/except catches all Exceptions (including HTTPException) and returns 200 OK. Auth verification must happen before this block so the 401 actually reaches the client.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core-api/api/routers/webhooks.py (2)
92-168: Consider decomposinggmail_webhookto reduce complexity.The handler on Line 92-168 still bundles parsing, validation, queueing, fallback, and error handling in one function. Extracting these into helpers would improve readability and should clear the cognitive complexity gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/api/routers/webhooks.py` around lines 92 - 168, The gmail_webhook handler is doing parsing, validation, queueing and inline processing in one large block; split it into small helpers: extract JSON parsing/validation into a function parse_pubsub_message(request) that returns (email_address, history_id) or raises, move the QStash enqueue logic into enqueue_gmail_sync(connection_lookup_client, email_address, history_id) (use queue_client and get_service_role_client inside), and keep inline processing via a separate function process_gmail_notification_thread(email_address, history_id) that calls asyncio.to_thread(process_gmail_notification,...). Replace the big body in gmail_webhook with sequential calls to parse_pubsub_message, enqueue_gmail_sync (fallback to process_gmail_notification_thread on False/exception), and centralize error logging/returns so behavior remains identical while reducing cognitive complexity.
63-63: UseAnnotatedfor FastAPI dependency parameters.Lines 63, 174-178 use legacy
= Query(None)/= Header(None)style. Switching toAnnotated[...]improves typing clarity and aligns with current FastAPI conventions.♻️ Proposed refactor
-from typing import Optional +from typing import Annotated, Optional @@ -async def gmail_webhook(request: Request, token: Optional[str] = Query(None)): +async def gmail_webhook( + request: Request, + token: Annotated[Optional[str], Query()] = None, +): @@ -async def calendar_webhook( - request: Request, - x_goog_channel_id: Optional[str] = Header(None), - x_goog_resource_id: Optional[str] = Header(None), - x_goog_resource_state: Optional[str] = Header(None), - x_goog_message_number: Optional[str] = Header(None), - x_goog_channel_token: Optional[str] = Header(None), +async def calendar_webhook( + request: Request, + x_goog_channel_id: Annotated[Optional[str], Header()] = None, + x_goog_resource_id: Annotated[Optional[str], Header()] = None, + x_goog_resource_state: Annotated[Optional[str], Header()] = None, + x_goog_message_number: Annotated[Optional[str], Header()] = None, + x_goog_channel_token: Annotated[Optional[str], Header()] = None,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/api/routers/webhooks.py` at line 63, Replace legacy dependency parameter syntax with Annotated-based typing: import Annotated and update the gmail_webhook signature (and other handlers around lines 174-178) to declare Query and Header params as Annotated[...] instead of using "= Query(None)" or "= Header(None)". For example, change parameters like token: Optional[str] = Query(None) to token: Annotated[Optional[str], Query(None)] and similarly convert header parameters (e.g., any params using Header(None)) to Annotated[Optional[str], Header(None)] so the function signatures (gmail_webhook and the mentioned header-handling endpoints) use modern FastAPI typing conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core-api/api/routers/webhooks.py`:
- Around line 62-63: The route decorators for gmail_webhook and calendar_webhook
(the `@router.post`(...) endpoints named gmail_webhook and calendar_webhook) are
missing a 401 response declaration; update their `@router.post` decorators to
include responses={"401": {"description": "Unauthorized"}} (or equivalent
OpenAPI schema) so the OpenAPI docs reflect the HTTPException(status_code=401)
raised in those handlers and generated clients see the 401 response.
---
Nitpick comments:
In `@core-api/api/routers/webhooks.py`:
- Around line 92-168: The gmail_webhook handler is doing parsing, validation,
queueing and inline processing in one large block; split it into small helpers:
extract JSON parsing/validation into a function parse_pubsub_message(request)
that returns (email_address, history_id) or raises, move the QStash enqueue
logic into enqueue_gmail_sync(connection_lookup_client, email_address,
history_id) (use queue_client and get_service_role_client inside), and keep
inline processing via a separate function
process_gmail_notification_thread(email_address, history_id) that calls
asyncio.to_thread(process_gmail_notification,...). Replace the big body in
gmail_webhook with sequential calls to parse_pubsub_message, enqueue_gmail_sync
(fallback to process_gmail_notification_thread on False/exception), and
centralize error logging/returns so behavior remains identical while reducing
cognitive complexity.
- Line 63: Replace legacy dependency parameter syntax with Annotated-based
typing: import Annotated and update the gmail_webhook signature (and other
handlers around lines 174-178) to declare Query and Header params as
Annotated[...] instead of using "= Query(None)" or "= Header(None)". For
example, change parameters like token: Optional[str] = Query(None) to token:
Annotated[Optional[str], Query(None)] and similarly convert header parameters
(e.g., any params using Header(None)) to Annotated[Optional[str], Header(None)]
so the function signatures (gmail_webhook and the mentioned header-handling
endpoints) use modern FastAPI typing conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29d71247-7774-4e52-8d15-401f15c9a47a
📒 Files selected for processing (1)
core-api/api/routers/webhooks.py
5 tasks
Contributor
Author
|
@benwxng why'd u close it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sandboxattribute to AI builder preview iframe, removing unrestricted camera/mic/geo accessContent-Security-Policyfrom report-only to enforced, with correctframe-srcfor Expo SnackGOOGLE_WEBHOOK_TOKENconfig; Gmail verifies?token=param, Calendar verifiesX-Goog-Channel-Tokenheader; watch creation includes the tokenposixpath.normpath+ prefix validation +shlex.quoteto prevent../traversal in E2B sandbox file browserTest plan
GOOGLE_WEBHOOK_TOKENenv var and verify Gmail/Calendar webhooks accept valid token and reject missing/invalid ones../— should reject with ValueError/home/user— should work as beforeSummary by CodeRabbit
Security Improvements
Chores
Tests