Skip to content

fix(sandbox): stop sending preview token as URL query param in from_session#186

Merged
drappier-charles merged 1 commit into
mainfrom
cdrappier/devin/remove-preview-token-query-param
Jul 13, 2026
Merged

fix(sandbox): stop sending preview token as URL query param in from_session#186
drappier-charles merged 1 commit into
mainfrom
cdrappier/devin/remove-preview-token-query-param

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Fixes ENG-3840

Summary

Fixes Tolmo finding 5a4bcd81 (LOW). SandboxInstance.from_session() attached the session preview token in two places — the X-Blaxel-Preview-Token header and a bl_preview_token entry in SandboxConfiguration.params. On this programmatic client the header alone authenticates, so the query-param copy only risked leaking a ~24h full-sandbox-control credential (filesystem CRUD, process.exec, network proxy) into access logs, Referer headers, and browser history. The query-param transport exists for browser navigation (which can't set custom headers), not for the SDK.

This removes the params kwarg from from_session() in both the async and sync variants:

 return cls(
     sandbox=sandbox,
     force_url=session.url,
     headers={"X-Blaxel-Preview-Token": session.token},
-    params={"bl_preview_token": session.token},
 )

Behavior-preserving: SandboxConfiguration.params was never actually attached to any outgoing request in the current code (both request paths — get_client() and the generated Client — send headers only; the params=... calls inside filesystem.py are unrelated method-local query dicts). So this deletes dead, credential-bearing config and hardens against a future change accidentally wiring it onto requests. Header-based auth is already exercised by the session integration tests (fs.ls, process.exec, stream_logs, watch).

Changes

  • src/blaxel/core/sandbox/default/sandbox.py — drop params={"bl_preview_token": ...} from async from_session().
  • src/blaxel/core/sandbox/sync/sandbox.py — same for sync from_session().
  • tests/core/test_sandbox.py — regression tests (async + sync) asserting from_session().config.params == {}, the header is still set, and the token never appears in the persistent HTTP client's default params.

Out of scope (separate repo)

The report also flags the devbox launcher printing the token-bearing preview URL to stdout (launch.py in blaxel-ai/blaxel-devbox), which is the realistic CI-log leak path. That lives in another repository and is not addressed here — it should be tracked/fixed separately (redact the token before printing). The report's longer-term recommendation (token-exchange → HttpOnly cookie for the browser path, shorter session TTL) is likewise a control-plane/proxy change, not an SDK change.

Link to Devin session: https://app.devin.ai/sessions/1599b0c6fe27498ba14d6e691b4b406e
Requested by: @drappier-charles


Note

Removes the redundant bl_preview_token query parameter from from_session() in both async and sync sandbox variants, keeping only the X-Blaxel-Preview-Token header for authentication. Adds regression tests to prevent reintroduction.

Written by Mendral for commit 870b6d7.

…ession

The session preview token was attached both as the X-Blaxel-Preview-Token
header and a redundant bl_preview_token entry in SandboxConfiguration.params.
The header alone authenticates this programmatic client, so the query-param
copy only risks leaking a ~24h full-sandbox-control credential into access
logs, Referer headers, and browser history. Remove it from from_session() in
both the async and sync variants and add regression tests asserting
from_session() produces no credential-bearing params.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@drappier-charles drappier-charles self-assigned this Jul 10, 2026
@drappier-charles
drappier-charles self-requested a review July 10, 2026 17:30
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@mendral-app

mendral-app Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

🧪 Testing Guide

What this PR addresses

SandboxInstance.from_session() (both async and sync variants) was sending the session preview token in two places: the X-Blaxel-Preview-Token header and as a bl_preview_token query parameter. The query-param copy is unnecessary for this programmatic client (it exists only for browser navigation) and risks leaking the credential into access logs, Referer headers, and browser history.

This PR removes the params={"bl_preview_token": session.token} argument so the token is only sent via the header.

Steps to reproduce the original issue

  1. Create a SandboxInstance using await SandboxInstance.from_session(session_dict) (or SyncSandboxInstance.from_session(...)) where session_dict contains a token field.
  2. Inspect instance.config.params — before this fix it would contain {"bl_preview_token": "<token>"}.
  3. Observe that any HTTP request made by the sandbox client would append the token to the URL query string.

What to verify (expected behavior)

  1. New tests pass: Run pytest tests/core/test_sandbox.py::test_from_session_does_not_leak_token_in_params and pytest tests/core/test_sandbox.py::test_sync_from_session_does_not_leak_token_in_params — both should pass.
  2. No token in params: After calling from_session(...), instance.config.params should be {}.
  3. Token still in headers: instance.config.headers should contain {"X-Blaxel-Preview-Token": "<token>"}.
  4. HTTP client clean: instance.process.get_client().params should not contain the token string.
  5. No regression: Existing sandbox tests still pass (pytest tests/core/test_sandbox.py). The sandbox should still authenticate correctly via the header alone.

Note

Posted by PR Testing Guide · Tag @mendral-app with feedback.

@mendral-app mendral-app Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Clean, minimal security hardening. The removal is safe because SandboxConfiguration.params defaults to {} when omitted, and the tests correctly verify this. No correctness, concurrency, or data risks introduced.

Tag @mendral-app with feedback or questions. View session

@mendral-app

mendral-app Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

🔀 Interaction Flow Diagram

Here's how the from_session() authentication flow changes with this PR:

sequenceDiagram
    participant Caller as Caller Code
    participant SF as SandboxInstance.from_session()
    participant Cfg as SandboxConfiguration
    participant HTTP as httpx Client
    participant API as Sandbox API

    Caller->>SF: from_session(session)
    SF->>Cfg: Create config (host, scheme)
    Note over SF,Cfg: ❌ No longer sets params={"bl_preview_token": token}
    SF->>HTTP: Build client with headers
    Note over HTTP: X-Blaxel-Preview-Token: session.token
    HTTP->>API: Request (token in header only)
    Note right of API: ✅ Token not leaked in URL/query string
    API-->>HTTP: Response
    HTTP-->>SF: Result
    SF-->>Caller: SandboxInstance
Loading

Summary

Before: The preview token was sent in both the X-Blaxel-Preview-Token header and as a bl_preview_token query parameter — risking credential leakage in access logs, Referer headers, and browser history.

After: The token is transmitted exclusively via the HTTP header, which is the correct channel for programmatic SDK clients. The query-param path exists only for browser navigation (which can't set custom headers) and is not needed here.

Components affected:

File Role
core/sandbox/default/sandbox.py Async from_session() — removed query param
core/sandbox/sync/sandbox.py Sync from_session() — removed query param
tests/core/test_sandbox.py New tests asserting token is header-only

Note

Posted by PR Sequence Diagram · Tag @mendral-app with feedback.

@mendral-app

mendral-app Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

📋 Created Linear issue ENG-3840 — status: In Progress

  • Assignee: Charles Drappier (reviewer — bot PR)
  • Labels: Bug, Security, SDK, Sandbox
  • Estimate: S
  • PR linked: ✅ Issue will auto-close when this PR merges

Auto-created because no Linear reference was found in the PR title, description, or branch name.

Note

Posted by Linear Issue Enforcer · Tag @mendral-app with feedback.

@drappier-charles
drappier-charles marked this pull request as ready for review July 10, 2026 18:31
@cursor

cursor Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Bugbot is not enabled for this team, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Impact summary & bot-feedback triage

Automated feedback: Mendral LGTM (safe minimal hardening); Testing Guide, Sequence Diagram, Linear Enforcer (ENG-3840), Observability = informational/pass. Supply Chain Review correctly skipped (no dep changes). Cursor Bugbot not run (team free-tier disabled); Cursor Security Agent + CodeQL pass. No actionable code finding attributable to this PR — nothing to change on the branch.

Devin Review: posts No Issues Found but the badge notes "1 additional finding" only visible in the web review. Flagging for a human to confirm it's not a blocker: https://app.devin.ai/review/blaxel-ai/sdk-python/pull/186

CI — 2 failures verified NOT from this diff (preexisting/environmental):

  • Integration (Core): server-side 400 unsupported extraArgs key "nvme" from controlplane (sandbox.go:978); test hardcodes nvme.
  • Integration (pydantic): model gateway 400 Invalid schema for function 'codegenParallelApply'.

Neither test references from_session. Evidence: unrelated PR #185 (a cp() shlex fix) shows the identical 19 passed / 2 failed with the same two error strings in its logs. Both should be fixed separately; they don't block this PR.

Impact: behavior-preserving — removes a dead, credential-bearing bl_preview_token from SandboxConfiguration.params (never reached the wire in current code; both request paths send headers only). Closes the credential-in-URL leak class and hardens against a future change wiring config.params onto requests. Only from_session() is affected; header auth unchanged. Pure library change → ships via normal SDK release, no migration; rollback = revert 4 lines.

Out of scope (separate): devbox launch.py stdout URL redaction (the realistic CI-log leak vector); sdk-typescript parity.

Not merging — this is analysis only.

@drappier-charles
drappier-charles merged commit ca26b71 into main Jul 13, 2026
21 of 24 checks passed
@drappier-charles
drappier-charles deleted the cdrappier/devin/remove-preview-token-query-param branch July 13, 2026 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant