fix(worker): restrict Azure lease selectors#790
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 2, 2026, 4:46 PM ET / 20:46 UTC. Summary Reproducibility: yes. Source inspection on current main shows non-admin broker requests can supply Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land the Azure provider-hook fix after maintainer approval of the fail-closed selector policy, remove the release-owned changelog edit, and add redacted real behavior proof for the broker response. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection on current main shows non-admin broker requests can supply Is this the best way to solve the issue? Yes for Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ab1c29f3fd40. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58368a77fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| restrictedLeaseRequestFields(input: LeaseRequest): string[] { | ||
| return [input.azureImage ? "azureImage" : "", input.azureOSDisk ? "azureOSDisk" : ""].filter( |
There was a problem hiding this comment.
Detect empty Azure OS-disk overrides
When the coordinator default is CRABBOX_AZURE_OS_DISK=ephemeral, a non-admin broker caller can send "azureOSDisk":""; this truthiness check reports no restricted fields, but leaseConfig still treats the present empty string as the selected value and normalizes it to managed, so the caller silently overrides the operator-managed OS-disk policy without admin auth. Check field presence (or trim/nullish semantics) rather than truthiness for azureOSDisk.
Useful? React with 👍 / 👎.
Summary
Closes #785
Verification
npm test --prefix worker -- test/fleet.test.ts(343 passed)npm run format:check --prefix workernpm run lint --prefix workernpm run check --prefix workerThe regression tests prove denial occurs before provider fetch, storage writes, or alarm scheduling; live Azure mutation cannot strengthen that preflight authorization proof.