fix: require DAG write permission for inline specs#2250
Merged
Conversation
📝 WalkthroughWalkthroughPermission model changes refactor DAG operation endpoints from requiring workspace "execute" permission to "DAG write" permission. Label extraction and authorization checks are moved earlier in execution and enqueue flows, and edit-retry permission checks are centralized via a new helper method. Authorization tests verify operator role constraints. ChangesDAG Operation Permission Model Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/service/frontend/api/v1/dagruns_edit_retry.go (1)
73-80:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove edit-retry authorization ahead of plan construction.
buildEditRetryPlanstill loads and resolves the submitted spec beforerequireEditRetryPermissionsruns, so a caller without DAG-write can drive the full edited-spec parse path and temp-file writes and only gets rejected at the end. If edit-retry specs are supposed to be gated by DAG-write, the workspace/auth decision needs to happen before building the plan.Also applies to: 128-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/frontend/api/v1/dagruns_edit_retry.go` around lines 73 - 80, Move the authorization check so it runs before any spec loading/plan construction: call requireEditRetryPermissions earlier (before buildEditRetryPlan) using the request identifiers (e.g., request.Name and request.DagRunId or any lightweight workspace/DAG identifier) so callers without DAG-write cannot trigger spec parsing or temp-file writes; to do this, adjust requireEditRetryPermissions' usage (and if necessary its signature) to accept the lightweight identifiers instead of a full plan, invoke it at the top of the handler prior to calling buildEditRetryPlan, and apply the same change to the second occurrence (the block that mirrors lines 128-139).internal/service/frontend/api/v1/dagruns.go (1)
198-210:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAuthorize before materializing the inline DAG.
Both paths still call
loadInlineDAGbefore the new workspace write check, so an operator without DAG-write can force full spec parsing and temp-file creation and can still get pre-auth validation failures from that load path. That weakens the boundary this PR is trying to introduce; the workspace decision needs to come from a lightweight pre-parse/auth step beforeloadInlineDAG.Also applies to: 280-292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/frontend/api/v1/dagruns.go` around lines 198 - 210, Move the workspace-write authorization before materializing the inline DAG: call extractLabelsParam(request.Body.Labels, request.Body.Tags) first, then derive the workspace with a lightweight helper (e.g., implement runtimeWorkspaceNameFromNameOrSpec(labels, request.Body.Name) or similar) that does not call loadInlineDAG or fully parse the spec, and then call requireDAGWriteForWorkspace(ctx, workspace). Only after that, call loadInlineDAG(ctx, request.Body.Spec, request.Body.Name, dagRunId) and defer cleanup; apply this change to both code paths (the block around loadInlineDAG at the shown lines and the similar block at lines 280-292).
🧹 Nitpick comments (1)
internal/service/frontend/api/v1/dagruns_test.go (1)
425-479: ⚡ Quick winAssert the operator still has source-run execute permission in this test.
This only proves preview/edit-retry return
403. It would still pass ifrequireDAGRunStatusExecutealso started rejecting operators, which is exactly the contract this PR says must be preserved. Add one operator-allowed action on the stored run first (for example a normal retry) so the test pins the failure specifically to the new DAG-write gate on edited specs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/frontend/api/v1/dagruns_test.go` around lines 425 - 479, Add an explicit check that the operatorKey can still perform an allowed action on the original run before asserting preview/edit-retry are forbidden: inside TestOperatorCannotSubmitEditedRetrySpec, after startBody.DagRunId is available (use dagName and startBody.DagRunId), send a normal operator retry request (POST to /api/v1/dag-runs/{dagName}/{startBody.DagRunId}/retry) with WithBearerToken(operatorKey) and require a successful status (e.g., http.StatusOK) to prove execute permission is intact, then proceed to assert that the preview (/edit-retry/preview) and edit (/edit-retry) requests return http.StatusForbidden as already written.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/service/frontend/api/v1/dagruns_edit_retry.go`:
- Around line 73-80: Move the authorization check so it runs before any spec
loading/plan construction: call requireEditRetryPermissions earlier (before
buildEditRetryPlan) using the request identifiers (e.g., request.Name and
request.DagRunId or any lightweight workspace/DAG identifier) so callers without
DAG-write cannot trigger spec parsing or temp-file writes; to do this, adjust
requireEditRetryPermissions' usage (and if necessary its signature) to accept
the lightweight identifiers instead of a full plan, invoke it at the top of the
handler prior to calling buildEditRetryPlan, and apply the same change to the
second occurrence (the block that mirrors lines 128-139).
In `@internal/service/frontend/api/v1/dagruns.go`:
- Around line 198-210: Move the workspace-write authorization before
materializing the inline DAG: call extractLabelsParam(request.Body.Labels,
request.Body.Tags) first, then derive the workspace with a lightweight helper
(e.g., implement runtimeWorkspaceNameFromNameOrSpec(labels, request.Body.Name)
or similar) that does not call loadInlineDAG or fully parse the spec, and then
call requireDAGWriteForWorkspace(ctx, workspace). Only after that, call
loadInlineDAG(ctx, request.Body.Spec, request.Body.Name, dagRunId) and defer
cleanup; apply this change to both code paths (the block around loadInlineDAG at
the shown lines and the similar block at lines 280-292).
---
Nitpick comments:
In `@internal/service/frontend/api/v1/dagruns_test.go`:
- Around line 425-479: Add an explicit check that the operatorKey can still
perform an allowed action on the original run before asserting
preview/edit-retry are forbidden: inside
TestOperatorCannotSubmitEditedRetrySpec, after startBody.DagRunId is available
(use dagName and startBody.DagRunId), send a normal operator retry request (POST
to /api/v1/dag-runs/{dagName}/{startBody.DagRunId}/retry) with
WithBearerToken(operatorKey) and require a successful status (e.g.,
http.StatusOK) to prove execute permission is intact, then proceed to assert
that the preview (/edit-retry/preview) and edit (/edit-retry) requests return
http.StatusForbidden as already written.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86af08c6-96b4-43aa-a88d-7f3ce9060595
📒 Files selected for processing (5)
internal/service/frontend/api/v1/apikeys_test.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_edit_retry.gointernal/service/frontend/api/v1/dagruns_edit_retry_internal_test.gointernal/service/frontend/api/v1/dagruns_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Require DAG write permission for submitted DAG specs while preserving stored-DAG execution for operator credentials.
Changes
Related Issues
Addresses GHSA-3cmh-rvhh-32pq.
Acknowledgements
Thanks to Yaohui Wang (@YHalo-wyh) for responsibly reporting GHSA-3cmh-rvhh-32pq.
Testing
Checklist