fix: security hardening — iframe sandbox, CSP, webhook auth, path traversal - #44
Open
joshuajerin wants to merge 6 commits into
Open
fix: security hardening — iframe sandbox, CSP, webhook auth, path traversal#44joshuajerin wants to merge 6 commits into
joshuajerin wants to merge 6 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.
- 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
… 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.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded Google webhook token config and verification to webhook endpoints and Pub/Sub setup, tightened sandbox path sanitization, updated Calendar watch requests to include the token, adjusted an iframe to use a sandbox attribute, and switched CSP from report-only to enforced with updated directives. Changes
Sequence DiagramsequenceDiagram
participant Client as Webhook Sender
participant Endpoint as Gmail/Calendar Endpoint
participant Verify as Token Verification
participant Settings as Settings/Config
participant Handler as Processing Logic
Client->>Endpoint: POST request (token in query or header)
Endpoint->>Verify: call _verify_google_webhook_token(token)
Verify->>Settings: read google_webhook_token
Verify->>Verify: hmac.compare_digest(provided, stored)
alt Token valid
Verify-->>Endpoint: valid
Endpoint->>Handler: proceed with parsing & enqueue
Handler-->>Client: 200 OK
else Token invalid or missing
Verify-->>Endpoint: invalid
Endpoint-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 3
🧹 Nitpick comments (2)
core-web/vercel.json (1)
7-8:'wasm-unsafe-eval'can be removed from the parent CSP.WASM evaluation for the Snack SDK occurs only within the sandboxed iframe, not in the parent page context. The iframe has its own CSP from its origin. No direct WebAssembly usage was found in the main app source code outside the iframe context, making this directive unnecessary and a minor reduction in attack surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-web/vercel.json` around lines 7 - 8, Remove the unnecessary 'wasm-unsafe-eval' token from the Content-Security-Policy header value so the parent page's script-src no longer includes it; update the "Content-Security-Policy" entry in vercel.json (the object with "key": "Content-Security-Policy") to omit 'wasm-unsafe-eval' while preserving the rest of the policy string, since WASM runs only inside the sandboxed iframe which has its own CSP.core-api/tests/unit/test_watch_manager_permanent_failures.py (1)
84-87: Assert the watch body carries the token.These tests now provide
google_webhook_token, but they still only assert log level. Ifstart_calendar_watch_service_role()stops populatingbody["token"], both tests stay green while webhook auth breaks.🧪 Minimal assertion to add
with patch("api.services.syncs.watch_manager.logger") as logger_mock: result = start_calendar_watch_service_role( user_id="user-123", calendar_service=calendar_service, connection_id="conn-123", service_supabase=supabase, ) + watch_kwargs = calendar_service.events.return_value.watch.call_args.kwargs + assert watch_kwargs["body"]["token"] == "test-token"Apply the same assertion in both Calendar-watch tests.
Also applies to: 149-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/tests/unit/test_watch_manager_permanent_failures.py` around lines 84 - 87, The tests patch api.services.syncs.watch_manager.settings with google_webhook_token but only assert log level; update the two Calendar-watch tests in test_watch_manager_permanent_failures.py (the ones that call start_calendar_watch_service_role()) to also assert that the outgoing watch request body includes the token by checking body["token"] == "test-token" (or equivalent access pattern used in the mocked request payload) so the tests fail if start_calendar_watch_service_role() stops populating 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/services/agents/sandbox_files.py`:
- Around line 21-35: The _sanitize_sandbox_path function currently uses
posixpath.normpath which doesn't resolve symlinks; update it to call
os.path.realpath (or posixpath.realpath if path handling is POSIX-only) on the
incoming path to canonicalize symlinks, then enforce the sandbox boundary by
checking the realpath is equal to or startswith the canonical _SANDBOX_ROOT, and
finally return the shell-quoted canonical path; update the boundary check in
_sanitize_sandbox_path to use the realpath result and add a regression test that
creates a symlink inside the sandbox pointing outside and asserts list/read
operations are rejected.
In `@core-api/api/services/syncs/watch_manager.py`:
- Around line 801-804: The renewal code builds request_body without always
adding settings.google_webhook_token which causes service-role Calendar
notifications to 401 in calendar_webhook(); update the renewal logic to mirror
start_calendar_watch() by always including request_body['token'] =
settings.google_webhook_token when settings.google_webhook_token is set (do not
conditionally omit it), ensuring the token is present on the service-role
Calendar watch creation so calendar_webhook() can validate incoming
notifications; check the code paths that construct request_body and align them
with start_calendar_watch() behavior.
In `@core-api/setup_pubsub_subscription.py`:
- Around line 42-47: The push endpoint currently concatenates the raw
webhook_token into push_endpoint and prints the full URL; instead build the URL
using urllib.parse.urlencode to safely encode the token (e.g., encode {"token":
webhook_token}) when constructing push_endpoint and avoid printing the secret:
keep push_endpoint as the fully encoded URL but replace all display/print usages
that currently print push_endpoint (references in this file) with a redacted
representation (e.g., replace token value with "<redacted>" or show only a
fingerprint) so logs never contain the plain token; update the code paths that
set webhook_token and push_endpoint (symbols: webhook_token, push_endpoint) and
the three print/log calls that output the URL to use the redacted string for
display.
---
Nitpick comments:
In `@core-api/tests/unit/test_watch_manager_permanent_failures.py`:
- Around line 84-87: The tests patch api.services.syncs.watch_manager.settings
with google_webhook_token but only assert log level; update the two
Calendar-watch tests in test_watch_manager_permanent_failures.py (the ones that
call start_calendar_watch_service_role()) to also assert that the outgoing watch
request body includes the token by checking body["token"] == "test-token" (or
equivalent access pattern used in the mocked request payload) so the tests fail
if start_calendar_watch_service_role() stops populating the token.
In `@core-web/vercel.json`:
- Around line 7-8: Remove the unnecessary 'wasm-unsafe-eval' token from the
Content-Security-Policy header value so the parent page's script-src no longer
includes it; update the "Content-Security-Policy" entry in vercel.json (the
object with "key": "Content-Security-Policy") to omit 'wasm-unsafe-eval' while
preserving the rest of the policy string, since WASM runs only inside the
sandboxed iframe which has its own CSP.
🪄 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: 234b449b-f8f8-4473-bdf0-2ba6fe95b86d
📒 Files selected for processing (8)
core-api/api/config.pycore-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.pycore-web/src/components/ai-app-builder/BuilderPreview.tsxcore-web/vercel.json
- Use posixpath.realpath() instead of normpath() to prevent symlink sandbox escapes - Require GOOGLE_WEBHOOK_TOKEN for calendar webhook watches - URL-encode webhook token and redact from logs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core-api/api/services/syncs/watch_manager.py (1)
265-267: Normalize token once before validation/use.Consider centralizing token retrieval with
.strip()(shared helper) so whitespace-only values are rejected consistently and both call paths stay in sync.♻️ Suggested refactor
+def _require_google_webhook_token() -> str: + token = settings.google_webhook_token.strip() + if not token: + raise ValueError("GOOGLE_WEBHOOK_TOKEN is required for Calendar webhook watches") + return token + ... - if not settings.google_webhook_token: - raise ValueError("GOOGLE_WEBHOOK_TOKEN is required for Calendar webhook watches") - request_body['token'] = settings.google_webhook_token + request_body['token'] = _require_google_webhook_token() ... - if not settings.google_webhook_token: - raise ValueError("GOOGLE_WEBHOOK_TOKEN is required for Calendar webhook watches") - request_body['token'] = settings.google_webhook_token + request_body['token'] = _require_google_webhook_token()Also applies to: 803-805
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/api/services/syncs/watch_manager.py` around lines 265 - 267, Normalize and validate the Google webhook token once before use: create a small helper (e.g., get_normalized_google_webhook_token or normalize_google_webhook_token) that reads settings.google_webhook_token, calls .strip(), raises ValueError if the result is empty, and returns the stripped token; then replace direct uses (the check/raise that references settings.google_webhook_token and the assignment to request_body['token'] as well as the other occurrences around lines 803-805) to call this helper and use its returned token so whitespace-only values are rejected consistently and both call paths stay in sync.
🤖 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/services/agents/sandbox_files.py`:
- Around line 21-35: _sanitize_sandbox_path currently uses posixpath.realpath
which does not resolve symlinks, so update it to only perform path normalization
and boundary checks (remove the misleading "Resolve symlinks" comment) and avoid
claiming symlink resolution; then implement actual canonical/symlink validation
inside the sandbox filesystem before any file access by calling the sandbox's
resolver (e.g., use an in-sandbox command or API like sandbox.commands.run to
run a trusted canonicalization such as readlink -f or the sandbox's
resolve_canonical helper) and only accept the path if that in-sandbox canonical
path is under _SANDBOX_ROOT; keep returning a safely shell-quoted path from
_sanitize_sandbox_path but ensure the real symlink check happens in-sandbox
prior to using the quoted path in sandbox.commands.run.
In `@core-api/setup_pubsub_subscription.py`:
- Around line 92-97: The error path prints raw stderr which can leak the
--push-endpoint token; before any print/log of gcloud output sanitize the stderr
(and stdout if printed) the same way you build display_cmd: replace occurrences
of the original push endpoint value with redacted_push_endpoint (use the same
redaction logic used when building display_cmd from create_cmd) so that when you
print stderr you never output the raw token; update the code paths around the
variables create_cmd, display_cmd, redacted_push_endpoint and where stderr is
printed to perform this replacement.
---
Nitpick comments:
In `@core-api/api/services/syncs/watch_manager.py`:
- Around line 265-267: Normalize and validate the Google webhook token once
before use: create a small helper (e.g., get_normalized_google_webhook_token or
normalize_google_webhook_token) that reads settings.google_webhook_token, calls
.strip(), raises ValueError if the result is empty, and returns the stripped
token; then replace direct uses (the check/raise that references
settings.google_webhook_token and the assignment to request_body['token'] as
well as the other occurrences around lines 803-805) to call this helper and use
its returned token so whitespace-only values are rejected consistently and both
call paths stay in sync.
🪄 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: c0830d0f-adb4-4019-ba76-7e723be00f9e
📒 Files selected for processing (3)
core-api/api/services/agents/sandbox_files.pycore-api/api/services/syncs/watch_manager.pycore-api/setup_pubsub_subscription.py
|
Contributor
Author
|
@benwxng ready for review |
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