fix(coderd): scope provisioner module file downloads to the daemon's org - #26635
Conversation
The DownloadFile gRPC handler authorized module-file downloads using only file.CreatedBy == uuid.Nil and Mimetype == application/x-tar, which every organization's cached Terraform module archive satisfies. A provisioner key scoped to one organization could supply another organization's file UUID and receive its full Terraform module source. Add HasTemplateVersionsUsingCachedModuleFileInOrg to verify the requested module file is referenced by a template version in the calling daemon's organization, and gate the download on it. Cross-org requests return the same error as the metadata check so the handler does not confirm file existence in other organizations. Ref: ANT-2026-22440
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 3 findings (2 P3, 1 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledgedCRF-1 (P3, templateversionterraformvalues.sql:37) - No index on cached_module_files
Round logRound 1Panel (19 reviewers). 0 P0, 0 P1, 0 P2, 2 P3, 1 Nit new. 4 dropped (Nit). Reviewed against 51591e3..13bb844. Round 2BLOCKED. CRF-1 and CRF-3 silent. CRF-2 addressed. No review. Round 3Panel (9 reviewers). CRF-2 and CRF-3 fixes verified. CRF-1 accepted (human decision). 0 new findings. Reviewed against 51591e3..b4e6d8f. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-scoped security fix. The org check sits between the metadata pre-filter and data streaming, uses the same error to avoid confirming file existence across orgs, and the tests verify both rejection and the absence of data leakage. File deduplication (shared file IDs via hash) is handled correctly through relationship-based authorization. 19 reviewers, strong consensus: the fix is correct and proportional.
2 P3, 1 Nit.
"Oh, this is a clean stone." (Bisky)
Pre-existing observation: GetFileByID at line 1576 loads the full file (including binary data) before any authorization check. A cross-org probe allocates memory for the entire archive, then discards it on rejection. Not introduced by this PR; the metadata check was already after the load. Worth knowing if module archives grow large and cross-org probing becomes a concern.
🤖 This review was automatically generated with Coder Agents.
…d fix Add a debug-level log before the org-isolation rejection so operators have a breadcrumb without leaking file existence to the calling provisioner. Move the DownloadFile regression test into provisionerdserver_test.go and rename it to TestDownloadFile to match the existing per-RPC test naming.
|
/coder-agents-review |
There was a problem hiding this comment.
Further review is blocked until the following findings receive a response. The full review panel will not re-run until these are addressed, acknowledged, or contested.
CRF-1 (P3, templateversionterraformvalues.sql:37): No index on cached_module_files. The thread was resolved without a code change or explanation. If the index is intentionally omitted (table is small enough at expected scale), please say so. If a follow-up is planned, file a ticket. Otherwise, add the partial index in a migration.
CRF-3 (Nit, provisionerdserver_test.go:5540): crypto/rand.Read error check is dead code since Go 1.24. The test was moved from download_file_test.go to provisionerdserver_test.go, but the pattern persists at lines 5540 and 5574. GitHub marked the thread as outdated due to the file move; the finding's substance was not addressed.
CRF-2 (diagnostic log) was addressed: Warn-level log added in 5943884.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All prior findings resolved. CRF-2 (diagnostic log) and CRF-3 (dead error check) fixes verified by all 9 reviewers. CRF-1 (missing index) accepted by human author citing migration lock risk.
The security boundary is correctly implemented: org check runs before data streaming, error is opaque to prevent cross-org file existence probing, and tests verify both rejection and absence of data leakage. Komugi stress-tested with -race -count=20 -parallel=16 at GOMAXPROCS=2: no races.
"The two-query flow runs outside a transaction. This is safe: cached_module_files is write-once (INSERT only, no UPDATE queries exist), and the FK uses default RESTRICT, preventing file deletion while referenced. A concurrent template_version CASCADE delete would make the org check return false, which is a safe rejection, not a false approval." (Knuckle)
🤖 This review was automatically generated with Coder Agents.
…rg and use real DRPC stream in test Switch the HasTemplateVersionsUsingCachedModuleFileInOrg authorization from ResourceSystem to ResourceFile.InOrg, a least-privilege check that the caller can read files in the target organization. Replace the hand-rolled DownloadFile mock stream with a real in-memory DRPC client/server pipe and the production HandleReceivingDataUpload reassembly path.
2 participants
DownloadFile now verifies the requested module file belongs to a template version in the calling daemon's own organization. Cross-org requests get the same "is not a modules file" error.