Fail closed on unconfigured auth; gate snapshot cleanup with an operator credential#85
Conversation
…tor credential Fixes two auth/authz bypasses in sandboxwich-api: - auth_and_tenant no longer trusts the client-supplied x-sandboxwich-tenant header when neither SANDBOXWICH_API_TOKEN nor SANDBOXWICH_TENANT_TOKENS is configured. It now rejects every non-probe request (probe paths /healthz and /readyz stay open). Existing tenant_tokens and shared_token behavior is unchanged. - POST /snapshots/cleanup deletes/expires data across every tenant with no tenant scoping. Rather than threading tenant_id through the cleanup controller's global queries (expire_due_snapshots, cleanup_archived_sandboxes, cleanup_runtime_resources_for_expired_snapshots), it is now gated behind a dedicated SANDBOXWICH_OPERATOR_TOKEN credential, checked via the x-sandboxwich-operator-token header and distinct from any tenant token. No ordinary tenant credential, for any tenant, is sufficient to run it, and cleanup is disabled until an operator token is configured. README documents that shared-token mode is single-tenant and describes the new operator token requirement. Test changes in http_contract.rs: TestServer now always configures real auth (tenant_tokens for two tenants, plus an operator token) instead of leaving auth unconfigured by default, so the existing suite exercises the fail-closed path rather than the removed header-trust fallback. Tenant-isolation assertions that used to spoof identity via the x-sandboxwich-tenant header now authenticate as a second tenant with its own bearer token. Adds two new tests: unauthenticated_deployment_rejects_tenant_header_spoofing (issue #63) and cleanup_is_not_usable_cross_tenant (issue #65). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015gLTjVy8zxQrStUaPEN9tJ
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| .filter(|token| !token.is_empty()); | ||
| let tenant_tokens = | ||
| parse_tenant_tokens(std::env::var("SANDBOXWICH_TENANT_TOKENS").ok().as_deref())?; | ||
| let operator_token = std::env::var("SANDBOXWICH_OPERATOR_TOKEN") | ||
| .ok() | ||
| .map(|token| token.trim().to_string()) | ||
| .filter(|token| !token.is_empty()); |
There was a problem hiding this comment.
🔍 Documented mutual exclusivity of auth modes is not enforced in code
The README states "Configure exactly one of: SANDBOXWICH_API_TOKEN ... or SANDBOXWICH_TENANT_TOKENS" but load_api_config() at crates/sandboxwich-api/src/main.rs:155-191 happily loads both without error. The auth_and_tenant middleware at crates/sandboxwich-api/src/main.rs:926 silently gives tenant_tokens precedence over shared_token via the if/else if chain. If an operator sets both env vars (e.g. during a migration from single-tenant to multi-tenant), the SANDBOXWICH_API_TOKEN credential is silently ignored, which could lock out clients still using the old token with no error message explaining why.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async fn cleanup_snapshots( | ||
| State(state): State<AppState>, | ||
| headers: HeaderMap, | ||
| ) -> Result<Json<SnapshotCleanupResponse>, ApiError> { | ||
| ensure_operator_authorized(&state, &headers)?; |
There was a problem hiding this comment.
📝 Info: Cleanup endpoint requires dual authentication (tenant + operator)
The /snapshots/cleanup route at crates/sandboxwich-api/src/main.rs:876 is inside the router that has the auth_and_tenant middleware applied at line 913. This means a request must first pass tenant authentication (valid bearer token matching a tenant or shared token) AND then pass the operator token check in ensure_operator_authorized at line 1808. This dual-gate design is reasonable but means the operator token alone is not sufficient — the caller must also present a valid tenant credential. The test at crates/sandboxwich-api/tests/http_contract.rs:360-372 confirms this by using server.client() (which includes the default tenant bearer token) plus the operator token header. This is worth documenting explicitly since the README at line 67 only mentions the operator token without noting the tenant credential requirement.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return ApiError::internal( | ||
| "sandboxwich-api has no authentication configured; set SANDBOXWICH_API_TOKEN \ | ||
| (single-tenant) or SANDBOXWICH_TENANT_TOKENS (multi-tenant) to serve \ | ||
| authenticated routes", | ||
| ) | ||
| .into_response(); |
There was a problem hiding this comment.
📝 Info: Fail-closed path returns 500 Internal Server Error rather than 401 or 403
When no auth is configured (neither SANDBOXWICH_API_TOKEN nor SANDBOXWICH_TENANT_TOKENS), the middleware at crates/sandboxwich-api/src/main.rs:951 returns ApiError::internal(...) which maps to HTTP 500. The test at crates/sandboxwich-api/tests/http_contract.rs:270-274 explicitly accepts both 401 and 500 (plain.status() == StatusCode::UNAUTHORIZED || plain.status() == StatusCode::INTERNAL_SERVER_ERROR). Using 500 is defensible since it's a server misconfiguration rather than a client auth failure, but it means monitoring systems that alert on 5xx rates will fire for every request to an unconfigured server, which could be noisy. A 503 Service Unavailable might be more semantically appropriate for a known misconfiguration.
Was this helpful? React with 👍 or 👎 to provide feedback.
Fixes the CI cargo fmt --check failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015gLTjVy8zxQrStUaPEN9tJ
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
| auth_token: None, | ||
| _data_dir: data_dir, |
There was a problem hiding this comment.
📝 Info: spawn() always initializes auth_token to None, relying on caller to fix it up
The spawn method at http_contract.rs:3216 always sets auth_token: None in the returned TestServer. Every caller is expected to chain .with_auth_token(...) afterward (e.g. http_contract.rs:3156, http_contract.rs:3170). This is a mild fragility: if someone adds a new start_* helper and forgets the .with_auth_token() call, server.client() will return an unauthenticated client, causing tests to fail with auth errors rather than a clear compile-time or construction-time error. Not a bug today since all current callers do the right thing, but a spawn signature that requires the token would be more robust.
Was this helpful? React with 👍 or 👎 to provide feedback.
Fail-closed remains the default. SANDBOXWICH_ALLOW_INSECURE_NO_AUTH=true lets the API serve with no credential configured (trusting the tenant header), logging a startup warning. The CI benchmark step opts in so it can drive the spawned API without a token. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015gLTjVy8zxQrStUaPEN9tJ
| async fn start_with_no_auth_configured( | ||
| database_url: String, | ||
| data_dir: Option<TempDir>, | ||
| ) -> Self { | ||
| Self::spawn(database_url, data_dir, true, |_command| {}) | ||
| .await | ||
| .with_auth_token(None) | ||
| } |
There was a problem hiding this comment.
🔍 No-auth test could be fragile if SANDBOXWICH_ALLOW_INSECURE_NO_AUTH is in the test runner's environment
The start_with_no_auth_configured helper (crates/sandboxwich-api/tests/http_contract.rs:3164-3171) spawns a server with no auth env vars set via the configure closure, but std::process::Command inherits the parent's full environment by default. If a developer or CI step has SANDBOXWICH_ALLOW_INSECURE_NO_AUTH=true in their environment (e.g. from running benchmarks in the same shell), the unauthenticated_deployment_rejects_tenant_header_spoofing test would fail because the server would accept requests instead of rejecting them. The test currently works in CI because cargo test runs before the benchmark step. Consider explicitly unsetting the variable with command.env_remove("SANDBOXWICH_ALLOW_INSECURE_NO_AUTH") in the no-auth spawn path for robustness.
Was this helpful? React with 👍 or 👎 to provide feedback.
…and-tenant-scoping # Conflicts: # crates/sandboxwich-api/src/main.rs
| fn ensure_operator_authorized(state: &AppState, headers: &HeaderMap) -> Result<(), ApiError> { | ||
| let Some(expected_token) = &state.auth.operator_token else { | ||
| return Err(ApiError::internal( | ||
| "snapshot cleanup is disabled: set SANDBOXWICH_OPERATOR_TOKEN to a dedicated \ | ||
| operator credential (distinct from tenant tokens) to enable /snapshots/cleanup", | ||
| )); | ||
| }; | ||
| let authorized = headers | ||
| .get(OPERATOR_TOKEN_HEADER) | ||
| .and_then(|value| value.to_str().ok()) | ||
| .is_some_and(|token| constant_time_eq(token.as_bytes(), expected_token.as_bytes())); | ||
| if !authorized { | ||
| return Err(ApiError::unauthorized(format!( | ||
| "a valid {OPERATOR_TOKEN_HEADER} header is required to run snapshot cleanup" | ||
| ))); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
📝 Info: Distinct HTTP status codes for missing vs misconfigured operator token
ensure_operator_authorized at crates/sandboxwich-api/src/main.rs:2061-2078 returns ApiError::internal (HTTP 500) when SANDBOXWICH_OPERATOR_TOKEN is not configured at all, versus ApiError::unauthorized (HTTP 401) when the token is present but wrong or missing from the request. This distinction is intentional — 500 signals a server misconfiguration rather than a client auth failure — but callers (monitoring, CI scripts) should be aware that a 500 from /snapshots/cleanup may indicate the operator token was never configured rather than a database or runtime error.
Was this helpful? React with 👍 or 👎 to provide feedback.
… server Merging #85 made TestServer::start configure auth; these tests must use server.client() rather than a raw unauthenticated client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015gLTjVy8zxQrStUaPEN9tJ
2 participants
Summary
Closes #63
Closes #65
#63 — unauthenticated by default, tenant derived from a client header
auth_and_tenant(crates/sandboxwich-api/src/main.rs) no longer trusts theclient-supplied
x-sandboxwich-tenantheader when neitherSANDBOXWICH_API_TOKENnorSANDBOXWICH_TENANT_TOKENSis configured. It nowrejects every non-probe request with a clear "no authentication configured"
error.
/healthzand/readyzstay open for k8s probes. The existingtenant_tokens(multi-tenant) andshared_token(single-tenant) branches areunchanged. README now documents that shared-token mode is single-tenant and
that there is no way to run the server unauthenticated.
#65 —
/snapshots/cleanupdeletes across all tenantscleanup_snapshotshad no tenant context at all —run_cleanup_controllerand its helpers (
expire_due_snapshots,cleanup_archived_sandboxes,cleanup_runtime_resources_for_expired_snapshots) run unfiltered, globalqueries. Threading
tenant_idthrough all of those (and everything they call)would be a much larger, riskier change than this issue warrants.
Chosen fix: gate the route behind a dedicated
SANDBOXWICH_OPERATOR_TOKENcredential, checked via a distinct
x-sandboxwich-operator-tokenheader. Thisis the "least invasive correct option" the issue calls out — no tenant
credential, for any tenant, is ever sufficient to run cleanup, and the route
is disabled (rejected) until an operator token is explicitly configured. The
route still requires normal tenant/shared auth as before; the operator token
is an additional, separate gate.
Tests
Added to
crates/sandboxwich-api/tests/http_contract.rs:unauthenticated_deployment_rejects_tenant_header_spoofing— proves adeployment with no auth configured rejects a request that tries to spoof
x-sandboxwich-tenant, while probe paths keep working.cleanup_is_not_usable_cross_tenant— proves neither the default tenant'snor
tenant-b's valid credential can invoke cleanup without the operatortoken, and that the operator token is required and sufficient.
TestServernow always configures real auth (two tenants viaSANDBOXWICH_TENANT_TOKENS, plus an operator token) instead of leaving authunconfigured by default, so the whole existing suite exercises the
fail-closed path instead of the removed header-trust fallback. The handful of
tenant-isolation assertions that used to spoof identity via the
x-sandboxwich-tenantheader now authenticate as a second tenant with itsown bearer token, which is a more faithful test of real isolation anyway.
Verification
cargo build --workspace— greencargo test -p sandboxwich-api— 4 unit tests + 7 integration tests, all pass🤖 Generated with Claude Code