Repository navigation
fix(providers): refuse lease allocation when the daemon holds other provider credentials - #3207
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 27 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Reviewed 27dd43a. The refusal logic itself looks right, but the Coverage check fails on this diff, so it needs a change before merge. The eager-closure test in Not blocking: no test pins that the daemon digests its own startup env at Could the BrowserStack reader and its trimming rule live in one function that both the consumer and the fingerprint call? That would close the trim thread by construction and keep On the open threads, two P1s from cubic-dev-ai still apply: trim vs raw key mismatch and auth-hook strip on a local daemon. One P2 still applies: I did not run the eager-closure test locally. I took the failure from the CI log and the diff's import edges. There was no live run with real Limrun or BrowserStack keys. The refusal fires before any provider call, and the socket and HTTP No conflicts. Before merge, the eager-closure gate must pass, and the two held P1 threads need an answer. For the second, either cover the local auth-hook HTTP case or document it. |
|
Addressed in a0ac012.
|
There was a problem hiding this comment.
All reported issues were addressed across 17 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
The earlier findings at 27dd43a are fixed at a0ac012. The Coverage job that failed then now passes, because the change removes the providers.ts AppError edge and the static fingerprint import in daemon-client.ts. All 21 checks pass at this head, and I found no code problems in the delta. There are no conflicts. Not blocking: On the open threads: the Cubic P2 about the test spreading I did not run the eager-closure budget test locally. That result rests on green CI and on the import edges I read in the diff. I also did not run Once the Cubic P2 is answered or applied, this is ready for a human review. |
a0ac012 to
9b3c7ca
Compare
|
Rebased on main and addressed at 9b3c7ca.
|
|
The earlier findings from a0ac012 are now fixed at 9b3c7ca, and I found no new problems. The ambient hook is cleared for all tests in the shared test setup, the credential fingerprint reads through the same reader every consumer uses, and the connect error path and docs now match the behavior. CI is green: 21/21 checks pass at 9b3c7ca. The changes since the last review touch only the test setup, one connect error path and docs, and none of that overlaps a check route. I know of no conflicts. Nothing else needs to change before human review. On the open Cubic threads, no P1 or P2 thread still applies. These are fixed at this head and can be resolved: #3207 (comment) (ambient hook in tests), #3207 (comment) (fingerprint credential reader), #3207 (comment) (reader map lookup), #3207 (comment) (limrun docs wording), #3207 (comment) (help wording), #3207 (comment) (missing-credentials message) and #3207 (comment) (ledger rationale). These are intended behavior and can be resolved too. #3207 (comment): a hook-configured daemon serves remote callers on its operator's credentials, so stripping the fingerprint is intended and documented. #3207 (comment): lease allocation from a shell goes through local daemons, and the docs say remote daemons are not checked. #3207 (comment): the digest is a staleness check for local callers and grants no authentication. I read the code but did not run two things. I did not run the startup composition test with |
thymikee
added
the
ready-for-human
…rovider credentials A local daemon keeps the environment it started with, while connect checks the shell's. A stale daemon could act on old credentials, for example creating a billed Limrun instance for an attach-mode shell. On a local lease_allocate the CLI now sends a versioned hash of the values each provider's own reader uses (Limrun, BrowserStack), over the socket and local HTTP; the daemon compares it with the hash of its startup environment and refuses with INVALID_ARGS, reason provider-credentials-changed, and a hint naming its state dir. No hash is sent from a shell without credentials or to remote, proxy, or cloud daemons; a daemon with an HTTP auth hook treats every caller as remote. AWS Device Farm is excluded.
…HTTP auth hook in tests verifyBrowserStack reads credentials through the shared reader again but keeps its COMMAND_FAILED 'profile missed' error and reconnect hint. The hermetic test setup now also clears AGENT_DEVICE_HTTP_AUTH_HOOK and _EXPORT, so a host with a hook configured cannot turn a local daemon test into a remote one.
DaemonRequestMeta and toLeaseDaemonRequest now carry both the retainOnClose fields from #3208 and providerCredentialFingerprint; their acks keep both rationales and the digest of the merged declarations.
9b3c7ca to
3fccd86
Compare
|
The earlier review at 9b3c7ca found no code problems, and the rebased head 3fccd86 still has none. The new changes only touch the lease handler and test/wire-compat/ledger.json, and they keep the rule that lease allocation is refused when the daemon holds other provider credentials. All 21 checks pass at 3fccd86. That includes the wire-compat gate, which runs the route this change touched. I did not recompute the sha256 digests or rerun the lease handler tests locally after the rebase, so I relied on CI for both. Nothing else is needed from you. The PR is ready for human review. |
Brings in callstack#3207 (refuse lease allocation when the daemon holds other provider credentials) and callstack#3222. Conflicts: - provider-definitions.ts: keep main's requireBrowserStackCredentials and drop its local request helpers, which this branch moved to webdriver-utils.ts; requireEnv is no longer used here but stays exported from provider-webdriver/plugin for TestMu. - cli-help.ts: keep the TestMu AI provider and credential text and add main's daemon credential comparison line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dentials A provider plugin can declare agentDevicePlugin.credentialVariables in its manifest. providerCredentialFingerprint falls back to that declaration for providers without a built-in reader, so the client (shell env) and the daemon (startup env) both fingerprint plugin credentials without loading plugin code. TestMu declares LT_USERNAME and LT_ACCESS_KEY, which brings it under the provider-credentials-changed refusal from callstack#3207. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Summary
A local daemon keeps its startup env while
connectchecks the shell's, so a stale daemon could act on old credentials, such as creating a billed Limrun instance for an attach-mode shell. Lease allocation now refuses that before anything is created:lease_allocate, the CLI sends a versioned hash of the values each provider's own reader uses (Limrun, BrowserStack), never the values themselves. It goes over the socket and local HTTP. The daemon compares it with the hash of its startup env.lease_allocate.30 files, 814 gross lines; help and docs updated. An empty BrowserStack variable now counts as missing.
Validation
At
3fccd8615c, rebased on main after #3208 and #3209:pnpm check:affected --runpasses (5,044 related tests, eager-closure budgets, daemon-wire-compat). Remaining minor: changing onlyLIMRUN_REGION, or the other platform's variables, also refuses (fails closed, as documented). No live run with real keys.