Keep sandbox inventory from exposing resolved secrets - #1154
Conversation
|
Warning Review limit reachedNext included review available in 19 minutes. View limit detailsLimit details: You’ve used all 3 included reviews currently available. Your 72 included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (16)
Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit:
This draft fixes a real security problem: the old sandbox list --format json embedded compute_v1alpha.Sandbox directly into the response struct, which meant the full SandboxSpec — including resolved container environment variables and therefore secrets — could leak to any caller of that command. The fix is architecturally sound: a new servers/sandbox package acts as the explicit security boundary, a dedicated SandboxInfo RPC type carries only the safe inventory fields, the CLI renders from that allow-list, and tests assert (with canary values) that the secret-bearing data never makes it to the wire. That's the right shape for this kind of fix.
A few things I want to flag before this graduates to merge:
The reexportSandboxes.List panic is intentional but undocumented. rpc.gen.go line 330–332 panic("not implemented"). The generated Export() path is a code-gen pattern I'd expect to exist for completeness, and the Export() method itself isn't called anywhere in the current codebase. But a panic in a generated exported function with no comment explaining why it panics or what it's for is a landmine for the next person reading this. A // re-export is a no-op for generated clients; not called server-side comment would cost nothing.
sandboxTerminal is inconsistent with the legacy path. In listSandboxesFromEntities, the status is produced by ui.CleanStatus(string(sb.Status)). The sandboxTerminal function checks for "stopped" || "dead". In the new RPC server path (server.go line 120), the server strips the "status." prefix itself with strings.TrimPrefix. Both paths should produce normalized strings, so this should be fine in practice — but the assumption depends on ui.CleanStatus and strings.TrimPrefix("status.", …) being equivalent across all status values. There's no test asserting this equivalence, and if a new status value is added with a different prefix format, the dead-hiding logic could silently break for one path but not the other.
The fallback discriminator is tight and correct, but its narrowness is the whole game. serverPredatesSandboxInventory returns true only for rpc.ErrResolveLookup. The unit test (TestServerPredatesSandboxInventoryOnlyForLookupFailures) does a good job asserting what must not trigger the fallback (auth errors, unreachable errors, generic errors). The comment above the function is explicit. This is the most security-critical decision in the PR and it's the right call. Worth making sure the relevant RPC layer truly only returns ErrResolveLookup for "no such capability" and not for any auth-adjacent failure, but the test coverage for the negative cases here is exactly what it should look like.
cleanEntityID in servers/sandbox/server.go is a local duplicate. It handles a fixed prefix list (sandbox/, app_version/, app/, pool/) whereas ui.CleanEntityID (used everywhere else) presumably covers the same or broader set. Since the server is in a package that can't import ui without introducing a cycle, this might be necessary — but if the two ever diverge, the inventory will show inconsistently cleaned IDs. A comment linking them would help.
The test coverage for the core security guarantee (canary value must not appear in output) is present at three layers: server_test.go, sandbox_list_test.go, and blackbox/sandbox_test.go. That's the right defence-in-depth for a leak fix.
This is a draft and not yet merge-ready, but the substance of the change is sound. The caveats above are things worth addressing before this leaves draft — they're not blocking the design direction.
🍪 full review note · comment /biscuit review to run biscuit again.
Sandbox entities carry resolved container environment, so returning them from sandbox list made JSON a secret disclosure path. Serve an allowlisted inventory projection through a dedicated capability instead, while letting new clients fall back to entities only for servers that predate it. This projection is intentionally narrow. It gives sandbox list an explicit disclosure boundary now without prematurely designing the broader replacement for CRUD entity access.
aca6f6b to
fbed19b
Compare
|
/biscuit review |
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
All three concerns from my previous review have been addressed — two substantively in code, one convincingly in argument.
rpc.gen.go panic comment — The author's point stands: the same generated panic("not implemented") exists in 153 other methods in the repo, and the right place for that documentation is the code generator, not a one-off annotation in this file. I accept that reasoning.
cleanEntityID divergence — The generic local copy is gone. The server now strips "app/" with a single strings.TrimPrefix inline where it's used (line 60 of server.go), which is narrower and honest about what it's doing rather than maintaining a partial reimplementation of ui.CleanEntityID. The divergence risk I raised is resolved at the source.
Status normalization equivalence — This is the cleanest fix of the three. Both the RPC path and the legacy entity path now funnel through newSandboxListEntry, which calls ui.CleanStatus unconditionally. sandboxTerminal only ever sees already-normalized strings. The structural invariant I wanted enforced by convention is now enforced by the code path itself. TestLegacySandboxListDropsExecutionSpec seeds a raw compute_v1alpha.DEAD status and asserts the dead sandbox is hidden by the default filter — that's exactly the regression test I asked for.
The core security guarantee — that sandboxListEntry is an allow-list that can never serialize SandboxSpec, and that the serverPredatesSandboxInventory fallback cannot be triggered by auth or unreachable errors — remains intact and is tested at three independent layers (server_test.go, sandbox_list_test.go, blackbox/sandbox_test.go). This is ready to graduate from draft to human review.
🍪 full review note · comment /biscuit review to run biscuit again.
2 participants
miren sandbox list --jsonhad a nasty surprise: it printed raw sandbox entities, including the resolved container environment. Secrets and all.This gives sandbox listing its own RPC with a deliberately boring, allowlisted response. Table and JSON output now use the same safe inventory data. A new CLI only falls back to raw entities when it can tell that the server is too old to have the new RPC.
The projection is pretty bespoke, and I don’t think we should pretend it’s the final shape of this API. It fixes the real exposure without making us design generic filtered entity views at the same time. As more CLI commands move away from broad CRUD entity access, we can look for the common shape and add pagination where it actually becomes useful.
Compatibility also means carrying some of the old world for now. Older CLIs still use raw entities against new servers, and new CLIs use the raw fallback against old servers. Tightening broad entity access later may break that fallback, which feels like an acceptable way to finish the transition.
Closes MIR-1765