Conversation
…fixes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolves conflicts in chat/index.tsx, harnesses/$harnessId.tsx, onboarding.tsx, and config.py — combining sandbox/Daytona features with staging's chat search, fork/edit, selectable MCPs, and URL validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Old messages created before the userId field was added to the messages table are missing userId, which excludes them from the search index. This migration copies userId from the parent conversation onto each message, processing 500 per invocation to stay within Convex limits. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace editFork + sendMessage with a single editForkAndSend mutation that creates the fork and inserts the edited message in one transaction, eliminating the brief flicker where the fork appeared without the message. - Add isEditSaving ref guard to prevent double-submit when pressing Enter/clicking Save twice quickly, which was creating duplicate forks. - Cap .collect() with .take(8192) in both fork and editForkAndSend to avoid hitting Convex's per-query document read limit. - Add bounds validation on upToMessageCount. - Add max-depth guard (100) on the ancestor walk to prevent infinite loops from data corruption. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix editForkAndSend rejecting first message edit (upToMessageCount=0) - Restrict message editing to only the last user message - Add Enter-to-send and Shift+Enter-for-newline in edit textarea - Right-align edit version arrows so they don't shift with message width - Suppress fade-in animations on conversation switches - Implement sticky-bottom auto-scroll with wheel/touch unpin detection - Add scroll-to-bottom button when unpinned Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…andboxes # Conflicts: # apps/web/src/routes/chat/index.tsx
|
Claude finished @DIodide's task in 2m 13s —— View job PR Review: Feat/filesystems sandboxes
This is a large, well-structured PR adding Daytona sandbox integration. Overall the code is clean and thoughtfully organized. Here are my findings, grouped by severity. 🔴 Critical Issues1. Missing ownership authorization on all sandbox REST endpoints
# sandbox.py:65 - only checks auth, not ownership
@router.get("/{sandbox_id}")
async def get_sandbox(
sandbox_id: str,
user: dict = Depends(get_current_user), # ← never checked against sandbox owner
):Fix: Query Convex to verify 2. Shell injection risk in
cred_line = f"https://{username}:{token}@github.com"
sandbox.process.exec(
f'printf "%s\\n" "{cred_line}" > /home/daytona/.git-credentials' # token unescaped
...
)Fix: Write the credentials file via 🟡 Medium Issues3.
params = CreateSandboxFromSnapshotParams(
language=language,
auto_stop_interval=15,
labels=sandbox_labels,
ephemeral=ephemeral,
# ← resource_tier values (cpu, memory, disk) never applied
)4. Traceback leaked to client in tool error responses
return json.dumps({
"type": "error",
"message": str(e),
"traceback": traceback.format_exc(), # ← remove from prod responses
})5. Public GitHub repos blocked without credentials
6. Unsafe
harnessId: harnessIdStr ? (harnessIdStr as any) : undefined,
// ...
const harness = await ctx.db.get(harnessIdStr as any);7. In-memory
8. JWT audience validation disabled in terminal WebSocket
🟢 Minor / Style9.
10. Every filesystem, git, and command operation calls 11. No file size guard on
12. Stale comment in
13. Double blank line in
Positive Notes
|
- Introduced a new internal query `getOwnerByDaytonaId` to check sandbox ownership in Convex. - Added `_assert_sandbox_owner` function in FastAPI routes to enforce ownership checks for sandbox operations. - Updated multiple routes to include ownership verification before executing actions on sandboxes. - Improved DaytonaService to manage sandbox lifecycle with a caching mechanism for running sandboxes. - Cleaned up imports and adjusted comments for clarity. This commit enhances security by ensuring only authorized users can access their respective sandboxes.
|
@claude review |
|
Claude finished @DIodide's task in 2m 34s —— View job PR Review: Feat/filesystems sandboxes
This PR has been significantly improved since the initial review — most of the previously flagged issues have been addressed. Here's the updated assessment. ✅ Previously Flagged Issues: Now Fixed
🔴 Still Open: Critical1. Shell injection in The OAuth token is still interpolated directly into a shell command string: cred_line = f"https://{username}:{token}@github.com"
sandbox.process.exec(
f'printf "%s\\n" "{cred_line}" > /home/daytona/.git-credentials'
# ↑ token is unquoted inside double-quotes — a token with `"` or `$()` breaks this
)GitHub tokens are currently alphanumeric so this doesn't exploit in practice, but it's one format change away from being a real injection. Fix by using the filesystem API instead of shell: Fix this → 🟡 Still Open: Medium2. JWT audience validation skipped in terminal WebSocket ( payload = jwt.decode(
token, key, algorithms=["RS256"], options={"verify_aud": False}
)Tokens issued for other services are accepted. The terminal connects a live PTY — this is worth fixing. The expected audience is typically the Clerk client ID or a configured 3. Traceback leaked in agentic tool errors ( return json.dumps({
"type": "error",
"message": str(e),
"traceback": traceback.format_exc(), # ← returned to the LLM context and potentially the frontend
})This exposes internal file paths and implementation details. The traceback is useful for debugging but should be logged server-side only, not returned in the payload. Fix this → 4. if not settings.convex_url or not settings.convex_deploy_key:
logger.warning("Convex not configured — skipping ownership check")
return True # ← any user can access any sandbox in this configThis is explicitly a dev convenience, but it means a misconfigured production deployment would have no ownership enforcement on any endpoint. Consider raising an exception or returning 5. downloadUrl(sandboxId: string, path: string) {
return `${API_URL}/api/sandbox/${sandboxId}/files/download?path=...`;
// ↑ no auth token — used directly as <img src> or <a href>
}The backend 6.
🟢 Minor7. loop = asyncio.get_event_loop()In Python 3.10+ this emits a deprecation warning inside an async context. Use 8. Private method access in route handler ( sandbox = service._ensure_running(sandbox_id) # ← directly accessing private method
data = sandbox.fs.download_file(path)The 9. The log format 10. No file size guard on No max-size check before loading a file into memory. A large binary in the sandbox could OOM the server. A simple check like rejecting files over 10MB before calling SummaryThe backend security posture is substantially improved from the initial version. The two remaining issues I'd prioritize before merge are the shell injection in credential setup (#1) and the traceback in error responses (#3). The JWT audience issue (#2) is worth fixing before this is publicly deployed. |
|
@claude review this |
|
Claude finished @DIodide's task in 2m 49s —— View job PR Review: Feat/filesystems sandboxes
This is the third review of this PR. The codebase is in good shape overall — the major ownership/auth rework from the first two reviews is solid. Here's the current state of all open issues. ✅ Resolved Since Last Review
🔴 Critical (Still Open)1. Shell injection in The OAuth token is still interpolated directly into a shell double-quoted string: cred_line = f"https://{username}:{token}@github.com"
sandbox.process.exec(
f'printf "%s\\n" "{cred_line}" > /home/daytona/.git-credentials'
)A token containing cred_line = f"https://{username}:{token}@github.com\n"
sandbox.fs.upload_file(cred_line.encode(), "/home/daytona/.git-credentials")
sandbox.process.exec("chmod 600 /home/daytona/.git-credentials && git config --global credential.helper store")🟡 Medium (Still Open)2. Traceback leaked in agentic tool errors ( return json.dumps({
"type": "error",
"message": str(e),
"traceback": traceback.format_exc(), # ← returned to LLM context and frontend
})The traceback is already logged server-side at line 661 via 3. JWT audience validation disabled in terminal WebSocket ( payload = jwt.decode(
token, key, algorithms=["RS256"], options={"verify_aud": False}
)This accepts tokens issued for any service as valid terminal auth. The terminal connects a live PTY — worth enforcing that tokens are actually issued for this application. At minimum, validate the 4. if not settings.convex_url or not settings.convex_deploy_key:
logger.warning("Convex not configured — skipping ownership check")
return True # ← any user can access any sandboxA misconfigured production deployment silently disables all ownership enforcement. Consider raising a 5. return Response(
content=data if isinstance(data, bytes) else data.encode(),
media_type=content_type,
headers={"Cache-Control": "public, max-age=300"}, # ← wrong for auth'd content
)
6.
🟢 Minor (Still Open)7.
8. Private method access in route handler (
9. Format 10. No file size guard on No max-size check before loading a file into memory. A large binary in the sandbox could OOM the server. A simple rejection of files over ~10MB before 11. Dead code: The function is never called — all components use SummaryBefore merge I'd prioritize: #1 (shell injection), #2 (traceback leak), and #5 (public cache header on auth'd endpoint) as these are straightforward one-line fixes with clear security implications. The JWT audience issue (#3) and fail-open Convex check (#4) are worth addressing before public rollout. |
1 participant
Daytona sandbox integration: Full sandbox lifecycle management (create, start, stop,
snapshot) with Convex persistence and FastAPI service layer
terminal, git panel, and command input — accessible from the chat interface
results with file diffs, terminal output, and created files
positioning, Enter-to-send in edit textarea, suppressed fade-in animations on
conversation switches, and sticky-bottom auto-scroll with unpin-on-scroll-up
backfill migration, auto-start sleeping sandboxes, GitHub OAuth fixes
Test plan
files)
appears