Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per‑DAG HMAC-SHA256 webhook authentication modes (token_only, token_and_hmac, hmac_only) with strict/observe enforcement, encrypted-at-rest HMAC secrets, HMAC lifecycle APIs (enable/configure/regenerate/disable), request authorization combining token and/or HMAC validation, and corresponding storage, service, API, UI, schema, and tests updates. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server
participant AuthService
participant Storage
Client->>Server: POST /webhooks/{dag} (Authorization?, X-Dagu-Signature?, body)
activate Server
Server->>AuthService: AuthorizeWebhookRequest(dag, token, signature, body)
activate AuthService
AuthService->>Storage: GetWebhook(dag)
activate Storage
Storage-->>AuthService: Webhook (authMode, hmacSecret?, enforcement)
deactivate Storage
alt authMode == token_and_hmac or hmac_only
AuthService->>AuthService: Compute HMAC-SHA256(secret, body) -> hex
AuthService->>AuthService: Compare const-time(signature, "sha256="+hex)
alt enforcement == strict and mismatch
AuthService-->>Server: 401 Unauthorized
else enforcement == observe and mismatch
AuthService-->>Server: OK (log warning)
else match
AuthService-->>Server: OK
end
else authMode == token_only
AuthService->>AuthService: Validate bearer token
alt token valid
AuthService-->>Server: OK
else
AuthService-->>Server: 401 Unauthorized
end
end
deactivate AuthService
alt Authorized
Server->>Server: Enqueue DAG run
Server-->>Client: 200 OK
else Not authorized
Server-->>Client: 401 Unauthorized
end
deactivate Server
sequenceDiagram
actor Admin
participant API
participant AuthService
participant Storage
participant Crypto
Admin->>API: POST /dags/{dag}/webhook/hmac/enable (authMode, enforcement)
activate API
API->>AuthService: EnableWebhookHMAC(dag, authMode, enforcement)
activate AuthService
AuthService->>Crypto: Generate random bytes -> secret
activate Crypto
Crypto-->>AuthService: secret bytes
deactivate Crypto
AuthService->>Storage: GetWebhook(dag)
activate Storage
Storage-->>AuthService: webhook
deactivate Storage
AuthService->>Crypto: Encrypt(secret) -> encryptedSecret
activate Crypto
Crypto-->>AuthService: encryptedSecret
deactivate Crypto
AuthService->>Storage: Save webhook (hmacSecretEnc)
activate Storage
Storage-->>AuthService: OK
deactivate Storage
AuthService-->>API: WebhookHMACSecretResult {FullSecret returned once}
API-->>Admin: 200 OK {hmacSecret: "...", webhook: {...}}
deactivate API
deactivate AuthService
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/persis/filewebhook/store_test.go (1)
226-228: Assert the sentinel error instead of a message substring.
assert.Contains(err.Error(), "encrypt")will also pass for unrelated encryption failures. Since this path now has a dedicated contract, make the test checkauth.ErrWebhookHMACEncryptorRequireddirectly.Suggested fix
_, err = New(tmpDir) require.Error(t, err) - assert.Contains(t, err.Error(), "encrypt") + assert.ErrorIs(t, err, auth.ErrWebhookHMACEncryptorRequired)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/filewebhook/store_test.go` around lines 226 - 228, The test currently checks the error message substring after calling New(tmpDir); replace that with a sentinel check using errors.Is (or the testify helper) to assert the specific contract: use require.ErrorIs(t, err, auth.ErrWebhookHMACEncryptorRequired) (or assert.ErrorIs) instead of assert.Contains, and ensure the auth package is imported so auth.ErrWebhookHMACEncryptorRequired can be referenced; keep the call to New unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/frontend/api/v1/webhooks.go`:
- Around line 267-290: Add an explicit branch that checks errors.Is(err,
authservice.ErrWebhookHMACNotSupported) (in the same places where you currently
handle ErrWebhookNotFound and ErrInvalidWebhookAuthMode — i.e. the
EnableDAGWebhookHMAC handlers that return
api.EnableDAGWebhookHMAC404JSONResponse /
api.EnableDAGWebhookHMACdefaultJSONResponse) and return a clear “not supported”
response instead of letting it fall through to the 500 case; specifically,
detect authservice.ErrWebhookHMACNotSupported and return an
api.EnableDAGWebhookHMACdefaultJSONResponse with an appropriate non-500
StatusCode (e.g. http.StatusNotImplemented) and an api.Error body containing
err.Error() so the caller/UI can distinguish capability-not-supported from
server faults; apply the same explicit check in the other similar handler blocks
mentioned (the other EnableDAGWebhookHMAC handler occurrences).
In `@internal/service/frontend/server.go`:
- Around line 592-606: The code silently falls back to plaintext when
crypto.ResolveKey or crypto.NewEncryptor fails for webhookEncryptor; instead,
treat these failures as fatal during auth initialization: when
ResolveKey(cfg.Paths.DataDir) or NewEncryptor(encKey) returns an error, log the
error with context and return the error (or wrap it) to abort startup rather
than continuing to call filewebhook.New(cfg.Paths.WebhooksDir,...); update the
block handling webhookEncryptor (references: webhookEncryptor,
crypto.ResolveKey, crypto.NewEncryptor, filewebhook.New, logger.Warn,
cfg.Paths.DataDir, cfg.Paths.WebhooksDir) so any encryption setup error causes
immediate failure of auth initialization.
In `@ui/src/api/v1/schema.ts`:
- Around line 3175-3179: Update the OpenAPI schema so
WebhookHMACConfigureRequest.authMode does not allow token_only: replace the
generic reference to WebhookAuthMode with an explicit enum limited to
["token_and_hmac","hmac_only"] in the request schema that defines
WebhookHMACConfigureRequest; then regenerate the frontend types (run the
project’s API generation, e.g. pnpm gen:api) so the types used by
enableDAGWebhookHMAC and configureDAGWebhookHMAC match server validation and
cannot accept token_only (leaving disableDAGWebhookHMAC unchanged).
---
Nitpick comments:
In `@internal/persis/filewebhook/store_test.go`:
- Around line 226-228: The test currently checks the error message substring
after calling New(tmpDir); replace that with a sentinel check using errors.Is
(or the testify helper) to assert the specific contract: use require.ErrorIs(t,
err, auth.ErrWebhookHMACEncryptorRequired) (or assert.ErrorIs) instead of
assert.Contains, and ensure the auth package is imported so
auth.ErrWebhookHMACEncryptorRequired can be referenced; keep the call to New
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee605288-bb05-4c89-a26c-5418681a2dc6
📒 Files selected for processing (15)
api/v1/api.gen.goapi/v1/api.yamlinternal/auth/store.gointernal/auth/webhook.gointernal/persis/filewebhook/store.gointernal/persis/filewebhook/store_test.gointernal/service/auth/service.gointernal/service/auth/webhook_test.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/webhooks.gointernal/service/frontend/api/v1/webhooks_test.gointernal/service/frontend/server.goui/src/api/v1/schema.tsui/src/features/dags/components/dag-details/WebhookTab.tsxui/src/pages/webhooks/index.tsx
👮 Files not reviewed due to content moderation or server errors (1)
- api/v1/api.yaml
Comment on lines
+592
to
+606
| var webhookEncryptor *crypto.Encryptor | ||
| encKey, encErr := crypto.ResolveKey(cfg.Paths.DataDir) | ||
| if encErr != nil { | ||
| logger.Warn(ctx, "Failed to resolve encryption key for webhook store", tag.Error(encErr)) | ||
| } else { | ||
| webhookEncryptor, encErr = crypto.NewEncryptor(encKey) | ||
| if encErr != nil { | ||
| logger.Warn(ctx, "Failed to create encryptor for webhook store", tag.Error(encErr)) | ||
| } | ||
| } | ||
| webhookOpts := []filewebhook.Option{filewebhook.WithFileCache(webhookCache)} | ||
| if webhookEncryptor != nil { | ||
| webhookOpts = append(webhookOpts, filewebhook.WithEncryptor(webhookEncryptor)) | ||
| } | ||
| webhookStore, err := filewebhook.New(cfg.Paths.WebhooksDir, webhookOpts...) |
There was a problem hiding this comment.
Do not silently downgrade webhook secret storage to plaintext.
If ResolveKey or NewEncryptor fails here, the code still constructs filewebhook.New(...) without an encryptor. That breaks the encrypted-at-rest guarantee for newly written HMAC secrets and can leave the data directory in a mixed encrypted/plaintext state. Please fail auth initialization instead of falling back for the webhook store.
Suggested fix
var webhookEncryptor *crypto.Encryptor
encKey, encErr := crypto.ResolveKey(cfg.Paths.DataDir)
if encErr != nil {
- logger.Warn(ctx, "Failed to resolve encryption key for webhook store", tag.Error(encErr))
- } else {
- webhookEncryptor, encErr = crypto.NewEncryptor(encKey)
- if encErr != nil {
- logger.Warn(ctx, "Failed to create encryptor for webhook store", tag.Error(encErr))
- }
+ return nil, false, fmt.Errorf("failed to resolve encryption key for webhook store: %w", encErr)
}
- webhookOpts := []filewebhook.Option{filewebhook.WithFileCache(webhookCache)}
- if webhookEncryptor != nil {
- webhookOpts = append(webhookOpts, filewebhook.WithEncryptor(webhookEncryptor))
+ webhookEncryptor, encErr = crypto.NewEncryptor(encKey)
+ if encErr != nil {
+ return nil, false, fmt.Errorf("failed to create encryptor for webhook store: %w", encErr)
}
- webhookStore, err := filewebhook.New(cfg.Paths.WebhooksDir, webhookOpts...)
+ webhookStore, err := filewebhook.New(
+ cfg.Paths.WebhooksDir,
+ filewebhook.WithFileCache(webhookCache),
+ filewebhook.WithEncryptor(webhookEncryptor),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var webhookEncryptor *crypto.Encryptor | |
| encKey, encErr := crypto.ResolveKey(cfg.Paths.DataDir) | |
| if encErr != nil { | |
| logger.Warn(ctx, "Failed to resolve encryption key for webhook store", tag.Error(encErr)) | |
| } else { | |
| webhookEncryptor, encErr = crypto.NewEncryptor(encKey) | |
| if encErr != nil { | |
| logger.Warn(ctx, "Failed to create encryptor for webhook store", tag.Error(encErr)) | |
| } | |
| } | |
| webhookOpts := []filewebhook.Option{filewebhook.WithFileCache(webhookCache)} | |
| if webhookEncryptor != nil { | |
| webhookOpts = append(webhookOpts, filewebhook.WithEncryptor(webhookEncryptor)) | |
| } | |
| webhookStore, err := filewebhook.New(cfg.Paths.WebhooksDir, webhookOpts...) | |
| var webhookEncryptor *crypto.Encryptor | |
| encKey, encErr := crypto.ResolveKey(cfg.Paths.DataDir) | |
| if encErr != nil { | |
| return nil, false, fmt.Errorf("failed to resolve encryption key for webhook store: %w", encErr) | |
| } | |
| webhookEncryptor, encErr = crypto.NewEncryptor(encKey) | |
| if encErr != nil { | |
| return nil, false, fmt.Errorf("failed to create encryptor for webhook store: %w", encErr) | |
| } | |
| webhookStore, err := filewebhook.New( | |
| cfg.Paths.WebhooksDir, | |
| filewebhook.WithFileCache(webhookCache), | |
| filewebhook.WithEncryptor(webhookEncryptor), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/frontend/server.go` around lines 592 - 606, The code
silently falls back to plaintext when crypto.ResolveKey or crypto.NewEncryptor
fails for webhookEncryptor; instead, treat these failures as fatal during auth
initialization: when ResolveKey(cfg.Paths.DataDir) or NewEncryptor(encKey)
returns an error, log the error with context and return the error (or wrap it)
to abort startup rather than continuing to call
filewebhook.New(cfg.Paths.WebhooksDir,...); update the block handling
webhookEncryptor (references: webhookEncryptor, crypto.ResolveKey,
crypto.NewEncryptor, filewebhook.New, logger.Warn, cfg.Paths.DataDir,
cfg.Paths.WebhooksDir) so any encryption setup error causes immediate failure of
auth initialization.
Collaborator
Author
There was a problem hiding this comment.
Not applying this change. In the current design, webhook HMAC is optional and must not break existing token-only webhook setups when encryption cannot be initialized. The fallback here does not write HMAC secrets in plaintext: filewebhook rejects HMAC writes and reads without an encryptor via ErrWebhookHMACEncryptorRequired, so HMAC remains unavailable rather than silently degrading storage guarantees. Making encryptor initialization fatal would widen the blast radius to all webhook auth startup, which is not the intended behavior for #2036.
There was a problem hiding this comment.
@yottahmd, understood — thank you for the clarification. The store-layer guard (ErrWebhookHMACEncryptorRequired) is the right place to enforce the invariant: HMAC operations are rejected outright when no encryptor is present, so there is no silent plaintext downgrade path. Keeping encryptor failure non-fatal at server init is intentional to preserve token-only webhook availability when encryption setup fails. I'll note this for future reviews.
✏️ Learnings added
Learnt from: yottahmd
Repo: dagucloud/dagu PR: 2045
File: internal/service/frontend/server.go:592-606
Timestamp: 2026-04-28T00:59:21.025Z
Learning: In `internal/service/frontend/server.go` (`initBuiltinAuthService`), the webhook encryptor failure is intentionally non-fatal. Webhook HMAC is an optional feature in the dagu project: the `filewebhook` store (package `internal/persis/filewebhook`) enforces `auth.ErrWebhookHMACEncryptorRequired` at the store layer, rejecting all HMAC secret reads/writes when no encryptor is configured. This prevents silent plaintext fallback without widening the startup blast radius to token-only webhook setups. Making encryptor init fatal here would break all webhook auth startup, which is not the intended behavior for issue `#2036`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Collaborator
Author
There was a problem hiding this comment.
Not taking this change. Failing auth initialization here would break existing token-only webhook behavior on nodes where encryption bootstrap is unavailable. The current implementation does not write plaintext HMAC secrets: HMAC enable/update paths return ErrWebhookHMACNotSupported when no encryptor exists, and encrypted records surface the same capability error on read. That preserves the encrypted-at-rest invariant for HMAC without regressing legacy token-only webhook availability.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
internal/service/frontend/api/v1/webhooks.go (1)
267-299: Extract the shared HMAC error mapping.These handlers all duplicate the same sentinel-error translation. A small helper returning
(status, api.Error, handled)would keep the 400/404/501/500 mapping in one place and make future auth-layer changes less error-prone.♻️ Refactor sketch
+func mapWebhookHMACError(err error, notFoundMessage string) (int, api.Error, bool) { + switch { + case errors.Is(err, auth.ErrWebhookNotFound): + return http.StatusNotFound, api.Error{ + Code: api.ErrorCodeNotFound, + Message: notFoundMessage, + }, true + case errors.Is(err, authservice.ErrInvalidWebhookAuthMode), + errors.Is(err, authservice.ErrInvalidWebhookHMACEnforcementMode), + errors.Is(err, authservice.ErrWebhookHMACNotConfigured): + return http.StatusBadRequest, api.Error{ + Code: api.ErrorCodeBadRequest, + Message: err.Error(), + }, true + case errors.Is(err, authservice.ErrWebhookHMACNotSupported): + return http.StatusNotImplemented, api.Error{ + Code: api.ErrorCodeInternalError, + Message: err.Error(), + }, true + default: + return 0, api.Error{}, false + } +}Then each handler only wraps the mapped
status/bodyinto its own generated response type.Also applies to: 337-369, 390-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/webhooks.go` around lines 267 - 299, Extract a shared helper (e.g., mapWebhookHMACError(err error) (int, api.Error, bool)) that centralizes the sentinel-error to HTTP status + api.Error mapping used in EnableDAGWebhookHMAC handler (the error-handling block that returns api.EnableDAGWebhookHMAC404JSONResponse / api.EnableDAGWebhookHMACdefaultJSONResponse) and replace the duplicated logic; have the helper return (status, body, handled) and in the handler call it, logging and returning the appropriate generated response type only when handled, otherwise fall through to the existing 500-path. Apply the same replacement to the analogous error blocks around lines 337-369 and 390-423 so all webhook HMAC error translations use this single helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1/api.yaml`:
- Around line 8047-8059: The WebhookHMACConfigureRequest schema leaves
enforcementMode behavior unspecified which confuses clients calling
/dags/{fileName}/webhook/hmac/enable vs /dags/{fileName}/webhook/hmac/configure;
update the API spec by either (A) documenting enforcementMode omission semantics
on WebhookHMACConfigureRequest (e.g., "if omitted on enable defaults to
'strict'; if omitted on configure preserves existing value") or (B) create two
distinct request schemas (e.g., WebhookHMACEnableRequest and
WebhookHMACConfigureRequest) and reference the appropriate one from the /enable
and /configure operations so each endpoint has explicit omission semantics for
enforcementMode.
- Around line 3874-3880: Update the X-Dagu-Signature header description to state
it is required only when HMAC authentication is configured in strict enforcement
(not in observe mode where signature failures are logged as warnings and do not
block requests), e.g., "HMAC webhook signature in the format 'sha256=<hex>';
required only when HMAC authentication is enabled with strict enforcement (in
observe mode signature failures are logged but requests are allowed)." Also
revise the 401 response description to clarify that 401 is returned for
missing/invalid bearer tokens or when HMAC validation is enabled in strict
enforcement; when HMAC enforcement is set to observe, missing/invalid signatures
are logged as warnings and do not produce a 401.
- Around line 3875-3880: The OpenAPI schema for header "X-Dagu-Signature"
advertises the format "sha256=<hex>" but currently only declares type: string;
update the schema for X-Dagu-Signature to include a pattern that enforces the
advertised format (e.g. regex like ^sha256=[0-9a-fA-F]{64}$) and optionally add
an example value to match the description so the contract aligns with runtime
HMAC validation.
In `@internal/core/spec/dag_params_runtime.go`:
- Around line 259-295: splitInternalRuntimeOverridePairs currently
unconditionally treats "WEBHOOK_PAYLOAD" as internal which breaks user params
with the same name; change the logic to detect collisions with DAG-declared
params and treat collided names as user params (or explicitly reject them at DAG
load). Concretely: update splitInternalRuntimeOverridePairs (or its caller) to
receive the set of declared param names and use that set when classifying each
pair (i.e., only call isInternalRuntimeParam when the name is not present in the
declared set), or alternatively add a validation step during DAG load that
checks for reserved names like "WEBHOOK_PAYLOAD" in declared params and returns
a clear error; also ensure appendInternalRuntimeEntries/isInternalRuntimeParam
behavior is consistent with this collision rule so internal entries are only
appended when no declared param with the same name exists.
- Around line 85-102: The code currently appends internalPairs verbatim which
allows duplicate internal keys and breaks last-write-wins semantics; after
splitting internalPairs from overridePairs (via
splitInternalRuntimeOverridePairs) merge internalPairs into finalPairs using the
same override logic used by overrideParams (or apply the same deduplication that
overrideParams uses) instead of simply appending, so the last source (rawParams
then paramsList) wins; apply the same fix to the other occurrence of this
pattern referenced around the 270-285 block.
In `@internal/service/frontend/api/v1/webhooks.go`:
- Around line 484-487: Ensure the 1 MiB request-size cap is enforced on the
exact raw request bytes before performing HMAC verification or calling
a.authService.AuthorizeWebhookRequest: call rawBodyFromContext and check
len(rawBody) (or equivalent size check) against 1<<20 and immediately
reject/return an error if it exceeds the limit, then only after that size check
invoke
a.authService.AuthorizeWebhookRequest(signature/valueOf(request.Params.XDaguSignature),
request.FileName, token, rawBody). Also update the same pattern in the other
occurrence (around the block handling lines ~544-556) so the HMAC is never
computed/stored for oversized payloads and ensure the check uses the rawBody
bytes (not a normalized payload) for the size decision.
In `@ui/src/api/v1/schema.ts`:
- Around line 9841-9845: Update the OpenAPI description for the
"X-Dagu-Signature" header in ui/src/api/v1/schema.ts so it no longer states the
header is required whenever HMAC auth is enabled; instead clarify that the HMAC
signature header is required only when HMAC auth is enabled and the enforcement
mode is strict (i.e., not for WebhookHMACEnforcementMode.observe). Edit the
"X-Dagu-Signature" property's JSDoc to reflect “required only when HMAC is
enabled and enforcement is strict” and then regenerate the generated schema file
so the updated description is persisted.
---
Nitpick comments:
In `@internal/service/frontend/api/v1/webhooks.go`:
- Around line 267-299: Extract a shared helper (e.g., mapWebhookHMACError(err
error) (int, api.Error, bool)) that centralizes the sentinel-error to HTTP
status + api.Error mapping used in EnableDAGWebhookHMAC handler (the
error-handling block that returns api.EnableDAGWebhookHMAC404JSONResponse /
api.EnableDAGWebhookHMACdefaultJSONResponse) and replace the duplicated logic;
have the helper return (status, body, handled) and in the handler call it,
logging and returning the appropriate generated response type only when handled,
otherwise fall through to the existing 500-path. Apply the same replacement to
the analogous error blocks around lines 337-369 and 390-423 so all webhook HMAC
error translations use this single helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccf01273-0d81-4d7c-8989-bc6dec95dccc
📒 Files selected for processing (13)
api/v1/api.gen.goapi/v1/api.yamlinternal/core/spec/dag_params_inline_schema.gointernal/core/spec/dag_params_runtime.gointernal/core/spec/dag_params_schema.gointernal/core/spec/runtime_params_test.gointernal/intg/webhook_test.gointernal/persis/filewebhook/store_test.gointernal/service/auth/service.gointernal/service/frontend/api/v1/webhooks.gointernal/service/frontend/api/v1/webhooks_test.goui/src/api/v1/schema.tsui/src/features/dags/components/dag-details/WebhookTab.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/persis/filewebhook/store_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/intg/webhook_test.go`:
- Around line 258-265: The helper functions dagRunIDPtr and
webhookHMACEnforcementModePtr are using new() incorrectly with parameter values;
replace the new(...) calls with the address-of operator to return pointers to
the parameters (i.e., return &id in dagRunIDPtr and return &mode in
webhookHMACEnforcementModePtr) so the functions compile and return *string and
*api.WebhookHMACEnforcementMode respectively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bb241c5-7e07-4545-9862-827dda841d10
📒 Files selected for processing (1)
internal/intg/webhook_test.go
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
internal/intg/webhook_test.go (1)
125-125:⚠️ Potential issue | 🔴 CriticalFix invalid
new(...)usage (compile blocker).Line 125 and Line 137 pass values/constants to
new, which is invalid in Go. Use address-of on variables instead.Suggested fix
+strict := api.WebhookHMACEnforcementModeStrict +runIDValue := runID enableResp := server.Client().Post("/api/v1/dags/"+dagName+"/webhook/hmac/enable", api.WebhookHMACConfigureRequest{ AuthMode: api.WebhookHMACConfigureRequestAuthModeHmacOnly, - EnforcementMode: new(api.WebhookHMACEnforcementModeStrict), + EnforcementMode: &strict, }).WithBearerToken(adminToken).ExpectStatus(http.StatusOK).Send(t) body := api.WebhookRequest{ - DagRunId: new(runID), + DagRunId: &runIDValue, Payload: &payload, }#!/bin/bash # Verify invalid new(...) value usage in this file. rg -n 'new\(api\.WebhookHMACEnforcementModeStrict\)|new\(runID\)' internal/intg/webhook_test.goAlso applies to: 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/intg/webhook_test.go` at line 125, The code incorrectly uses new(...) with values/constants (e.g., new(api.WebhookHMACEnforcementModeStrict) and new(runID)), which is invalid in Go; fix by creating a local variable holding the value and passing its address instead (for example create a local m := api.WebhookHMACEnforcementModeStrict and set EnforcementMode: &m, and for runID create a local copy like id := runID and pass &id wherever new(runID) was used). Ensure you update the places referencing EnforcementMode and the runID pointer to use the address-of local variables.internal/core/spec/dag_params_inline_schema.go (1)
142-143:⚠️ Potential issue | 🟠 MajorUse full schema-declared names when splitting internal runtime params.
declaredRuntimeParamNames(plan.entries)is defaults-derived here, so declared schema properties without defaults are missing from collision checks. That can misclassify declared params (notablyWEBHOOK_PAYLOAD) as internal.Suggested fix
-overridePairs, internalPairs := splitInternalRuntimeOverridePairs(overridePairs, declaredRuntimeParamNames(plan.entries)) +declared := make(map[string]struct{}, len(plan.schemaProperties)) +for name := range plan.schemaProperties { + declared[name] = struct{}{} +} +overridePairs, internalPairs := splitInternalRuntimeOverridePairs(overridePairs, declared)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/spec/dag_params_inline_schema.go` around lines 142 - 143, splitInternalRuntimeOverridePairs is currently passed declaredRuntimeParamNames(plan.entries), which is derived from defaults and omits declared schema properties without defaults (causing declared params like WEBHOOK_PAYLOAD to be treated as internal); change the call to compute declared runtime param names from the full schema (e.g., declaredRuntimeParamNames(plan.schema) or the plan object property that holds the schema) and pass that result into splitInternalRuntimeOverridePairs so collision checks use schema-declared names rather than defaults-derived names.api/v1/api.yaml (1)
3875-3901:⚠️ Potential issue | 🟡 Minor
401description still under-documents strict HMAC failures.Line 3901 says token-only unauthorized, but strict HMAC failures can also produce
401. This behavior mismatch is still present.Suggested doc fix
"401": - description: "Unauthorized - missing or invalid token" + description: "Unauthorized - missing/invalid token, or invalid/missing HMAC signature when strict HMAC enforcement is active"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 3875 - 3901, Update the "401" response description to reflect that unauthorized responses can result from missing/invalid tokens OR strict HMAC signature failures; specifically mention the X-Dagu-Signature header and strict HMAC webhook auth mode so the responses section (response "401") matches actual behavior when HMAC verification fails as well as token problems.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1/api.yaml`:
- Around line 8052-8068: The schema WebhookHMACConfigureRequest allows authMode
"hmac_only" while permitting non-strict enforcement; update the OpenAPI schema
to enforce that when authMode is "hmac_only" the enforcementMode is fixed to
"strict" by replacing the simple properties block with a oneOf (or conditional)
schema: one branch where authMode: {const: "hmac_only"} and enforcementMode:
{const: "strict"}, and another branch where authMode: {const: "token_and_hmac"}
and enforcementMode keeps the existing $ref to WebhookHMACEnforcementMode; keep
authMode required. This change ensures requests with authMode "hmac_only" can
only accept enforcementMode strict.
- Around line 4169-4220: Add explicit "400" and "501" response entries to the
webhook HMAC configure and regenerate operations so the OpenAPI spec matches the
handlers; for operationId "regenerateDAGWebhookHMACSecret" (and the
corresponding configure operation) add responses "400" and "501" with
descriptions and the application/json content pointing to the existing Error
schema ($ref: "#/components/schemas/Error") so generated clients and API docs
include these status codes.
In `@internal/service/auth/service.go`:
- Around line 849-852: The webhook read using s.webhookStore.GetByDAGName is
returning raw store capability errors (specifically
auth.ErrWebhookHMACEncryptorRequired) which should be mapped to the
service-level ErrWebhookHMACNotSupported; update each place where
s.webhookStore.GetByDAGName is called (e.g., in the
configure/disable/rotate/authorize flows and the other occurrences you noted) to
check the returned error with errors.Is(err,
auth.ErrWebhookHMACEncryptorRequired) and, when true, return
auth.ErrWebhookHMACNotSupported instead of the raw error, otherwise return the
original error as before.
---
Duplicate comments:
In `@api/v1/api.yaml`:
- Around line 3875-3901: Update the "401" response description to reflect that
unauthorized responses can result from missing/invalid tokens OR strict HMAC
signature failures; specifically mention the X-Dagu-Signature header and strict
HMAC webhook auth mode so the responses section (response "401") matches actual
behavior when HMAC verification fails as well as token problems.
In `@internal/core/spec/dag_params_inline_schema.go`:
- Around line 142-143: splitInternalRuntimeOverridePairs is currently passed
declaredRuntimeParamNames(plan.entries), which is derived from defaults and
omits declared schema properties without defaults (causing declared params like
WEBHOOK_PAYLOAD to be treated as internal); change the call to compute declared
runtime param names from the full schema (e.g.,
declaredRuntimeParamNames(plan.schema) or the plan object property that holds
the schema) and pass that result into splitInternalRuntimeOverridePairs so
collision checks use schema-declared names rather than defaults-derived names.
In `@internal/intg/webhook_test.go`:
- Line 125: The code incorrectly uses new(...) with values/constants (e.g.,
new(api.WebhookHMACEnforcementModeStrict) and new(runID)), which is invalid in
Go; fix by creating a local variable holding the value and passing its address
instead (for example create a local m := api.WebhookHMACEnforcementModeStrict
and set EnforcementMode: &m, and for runID create a local copy like id := runID
and pass &id wherever new(runID) was used). Ensure you update the places
referencing EnforcementMode and the runID pointer to use the address-of local
variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80bc7fb9-3761-461d-80ec-f0294321fb74
📒 Files selected for processing (13)
api/v1/api.gen.goapi/v1/api.yamlinternal/core/spec/dag_params_inline_schema.gointernal/core/spec/dag_params_runtime.gointernal/core/spec/dag_params_schema.gointernal/core/spec/runtime_params_test.gointernal/intg/webhook_test.gointernal/service/auth/service.gointernal/service/auth/webhook_test.gointernal/service/frontend/api/v1/webhook_middleware.gointernal/service/frontend/api/v1/webhooks.gointernal/service/frontend/api/v1/webhooks_test.goui/src/api/v1/schema.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/core/spec/dag_params_schema.go
- internal/core/spec/runtime_params_test.go
- internal/service/auth/webhook_test.go
Comment on lines
+8052
to
+8068
| WebhookHMACConfigureRequest: | ||
| type: object | ||
| description: | | ||
| Request to configure webhook HMAC auth mode and enforcement. | ||
| If enforcementMode is omitted when enabling HMAC, it defaults to strict. | ||
| If omitted when configuring an existing webhook, the current enforcement | ||
| mode is preserved for token_and_hmac, while hmac_only always uses strict. | ||
| required: | ||
| - authMode | ||
| properties: | ||
| authMode: | ||
| type: string | ||
| enum: | ||
| - token_and_hmac | ||
| - hmac_only | ||
| enforcementMode: | ||
| $ref: "#/components/schemas/WebhookHMACEnforcementMode" |
There was a problem hiding this comment.
Constrain hmac_only to strict directly in schema.
The description says hmac_only always uses strict enforcement, but the schema currently accepts hmac_only with observe. Encoding this rule in schema prevents invalid payloads.
Suggested schema constraint
WebhookHMACConfigureRequest:
type: object
description: |
Request to configure webhook HMAC auth mode and enforcement.
If enforcementMode is omitted when enabling HMAC, it defaults to strict.
If omitted when configuring an existing webhook, the current enforcement
mode is preserved for token_and_hmac, while hmac_only always uses strict.
- required:
- - authMode
- properties:
- authMode:
- type: string
- enum:
- - token_and_hmac
- - hmac_only
- enforcementMode:
- $ref: "#/components/schemas/WebhookHMACEnforcementMode"
+ oneOf:
+ - type: object
+ required: [authMode]
+ properties:
+ authMode:
+ type: string
+ enum: [token_and_hmac]
+ enforcementMode:
+ $ref: "#/components/schemas/WebhookHMACEnforcementMode"
+ - type: object
+ required: [authMode]
+ properties:
+ authMode:
+ type: string
+ enum: [hmac_only]
+ enforcementMode:
+ type: string
+ enum: [strict]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v1/api.yaml` around lines 8052 - 8068, The schema
WebhookHMACConfigureRequest allows authMode "hmac_only" while permitting
non-strict enforcement; update the OpenAPI schema to enforce that when authMode
is "hmac_only" the enforcementMode is fixed to "strict" by replacing the simple
properties block with a oneOf (or conditional) schema: one branch where
authMode: {const: "hmac_only"} and enforcementMode: {const: "strict"}, and
another branch where authMode: {const: "token_and_hmac"} and enforcementMode
keeps the existing $ref to WebhookHMACEnforcementMode; keep authMode required.
This change ensures requests with authMode "hmac_only" can only accept
enforcementMode strict.
Collaborator
Author
There was a problem hiding this comment.
Not taking this schema change. The backend already enforces hmac_only -> strict, and the generated UI request type is already narrowed to token_and_hmac | hmac_only. Encoding the cross-field rule as oneOf here would materially complicate the generated Go and TypeScript types for a small validation gain, so I kept this as server-side validation plus explicit schema documentation instead.
There was a problem hiding this comment.
@yottahmd Got it—SGTM to keep this rule enforced server-side and avoid complicating the generated Go/TS types.
As a tiny, type-safe improvement that preserves your current codegen shape, here are optional doc-only tweaks:
- Add a property-level note so clients see the constraint right where they pick enforcementMode.
- Provide request examples for enable/configure to make the “omit for hmac_only” path obvious.
YAML snippets (no shape change, just docs/examples):
components:
schemas:
WebhookHMACConfigureRequest:
properties:
enforcementMode:
$ref: "#/components/schemas/WebhookHMACEnforcementMode"
description: "Only applies when authMode=token_and_hmac. When authMode=hmac_only, the server enforces 'strict' and rejects 'observe'; clients SHOULD omit this field."paths:
/dags/{fileName}/webhook/hmac/enable:
post:
requestBody:
content:
application/json:
examples:
token_and_hmac_strict:
value: { authMode: token_and_hmac, enforcementMode: strict }
token_and_hmac_observe:
value: { authMode: token_and_hmac, enforcementMode: observe }
hmac_only:
value: { authMode: hmac_only }
/dags/{fileName}/webhook/hmac/configure:
post:
requestBody:
content:
application/json:
examples:
token_and_hmac_strict:
value: { authMode: token_and_hmac, enforcementMode: strict }
token_and_hmac_observe:
value: { authMode: token_and_hmac, enforcementMode: observe }
hmac_only:
value: { authMode: hmac_only }If you’d like, I can prep a tiny follow-up commit with just these doc additions.
✏️ Learnings added
Learnt from: yottahmd
Repo: dagucloud/dagu PR: 2045
File: api/v1/api.yaml:8096-8113
Timestamp: 2026-04-28T01:43:23.747Z
Learning: Repo: dagucloud/dagu PR: 2045 — Preference: Avoid using cross-field OpenAPI constraints (e.g., oneOf) that couple fields like authMode/enforcementMode because they complicate generated Go/TypeScript types. For WebhookHMACConfigureRequest in api/v1/api.yaml, keep the contract simple and rely on server-side validation to enforce "hmac_only => strict", documenting the rule in the schema/endpoint descriptions and examples.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yottahmd
Repo: dagucloud/dagu PR: 2045
File: internal/service/frontend/server.go:592-606
Timestamp: 2026-04-28T00:59:21.025Z
Learning: In `internal/service/frontend/server.go` (`initBuiltinAuthService`), the webhook encryptor failure is intentionally non-fatal. Webhook HMAC is an optional feature in the dagu project: the `filewebhook` store (package `internal/persis/filewebhook`) enforces `auth.ErrWebhookHMACEncryptorRequired` at the store layer, rejecting all HMAC secret reads/writes when no encryptor is configured. This prevents silent plaintext fallback without widening the startup blast radius to token-only webhook setups. Making encryptor init fatal here would break all webhook auth startup, which is not the intended behavior for issue `#2036`.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
internal/service/frontend/api/v1/webhooks.go (1)
481-520:⚠️ Potential issue | 🟠 MajorHandle
ErrWebhookHMACNotSupportedexplicitly in the trigger path.
AuthorizeWebhookRequestcan now returnauthservice.ErrWebhookHMACNotSupportedwhen this node cannot read encrypted HMAC secrets, but this branch still falls through to the generic 500. That makes HMAC-enabled webhooks on such nodes fail as opaque internal errors instead of an actionable capability response.Suggested change
webhook, err := a.authService.AuthorizeWebhookRequest(ctx, request.FileName, token, signature, rawBody) if err != nil { + if errors.Is(err, authservice.ErrWebhookHMACNotSupported) { + logger.Warn(ctx, "Webhook: HMAC not supported on this server", + tag.Name(request.FileName), + tag.Error(err), + ) + return nil, &Error{ + HTTPStatus: http.StatusNotImplemented, + Code: api.ErrorCodeInternalError, + Message: err.Error(), + } + } if errors.Is(err, authservice.ErrInvalidWebhookToken) { logger.Warn(ctx, "Webhook: invalid token", tag.Name(request.FileName)) return nil, &Error{Based on learnings: webhook encryptor initialization is intentionally non-fatal, and capability gaps should be surfaced without breaking token-only webhook setups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/webhooks.go` around lines 481 - 520, Add explicit handling for authservice.ErrWebhookHMACNotSupported inside the error branch after calling a.authService.AuthorizeWebhookRequest in webhooks.go: check errors.Is(err, authservice.ErrWebhookHMACNotSupported), log a warning via logger.Warn(ctx, "Webhook: HMAC not supported on this node", tag.Name(request.FileName)), and return a clear capability response (for example HTTPStatus http.StatusNotImplemented with an appropriate API error code/message such as "webhook HMAC not supported on this node" — or introduce api.ErrorCodeNotSupported if one does not exist) instead of falling through to the generic 500 path so HMAC-capability gaps are surfaced without breaking token-only webhooks.
🧹 Nitpick comments (1)
internal/intg/webhook_test.go (1)
102-102: Usefilepath.Joinfor the temp payload path.Line 102 hardcodes
/even though the rest of this helper already branches on OS-specific shell behavior. Usingfilepath.Joinkeeps the test portable on Windows runners too.Suggested change
import ( "crypto/hmac" "crypto/sha256" "encoding/hex" "encoding/json" "fmt" "net/http" "os" + "path/filepath" "testing" "time" @@ - payloadFile := t.TempDir() + "/webhook-payload.json" + payloadFile := filepath.Join(t.TempDir(), "webhook-payload.json")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/intg/webhook_test.go` at line 102, Replace the hardcoded path construction for payloadFile with an OS-safe join: use filepath.Join(t.TempDir(), "webhook-payload.json") instead of concatenating with "/" and ensure the "path/filepath" package is imported; update the payloadFile variable assignment in the test helper where payloadFile and t.TempDir() are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/service/frontend/api/v1/webhooks.go`:
- Around line 481-520: Add explicit handling for
authservice.ErrWebhookHMACNotSupported inside the error branch after calling
a.authService.AuthorizeWebhookRequest in webhooks.go: check errors.Is(err,
authservice.ErrWebhookHMACNotSupported), log a warning via logger.Warn(ctx,
"Webhook: HMAC not supported on this node", tag.Name(request.FileName)), and
return a clear capability response (for example HTTPStatus
http.StatusNotImplemented with an appropriate API error code/message such as
"webhook HMAC not supported on this node" — or introduce
api.ErrorCodeNotSupported if one does not exist) instead of falling through to
the generic 500 path so HMAC-capability gaps are surfaced without breaking
token-only webhooks.
---
Nitpick comments:
In `@internal/intg/webhook_test.go`:
- Line 102: Replace the hardcoded path construction for payloadFile with an
OS-safe join: use filepath.Join(t.TempDir(), "webhook-payload.json") instead of
concatenating with "/" and ensure the "path/filepath" package is imported;
update the payloadFile variable assignment in the test helper where payloadFile
and t.TempDir() are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 844f19f2-e5fc-45b2-95ab-37d07df9faa7
📒 Files selected for processing (8)
api/v1/api.gen.goapi/v1/api.yamlinternal/core/spec/dag_params_inline_schema.gointernal/core/spec/dag_params_runtime.gointernal/intg/webhook_test.gointernal/service/auth/service.gointernal/service/frontend/api/v1/webhooks.goui/src/api/v1/schema.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/core/spec/dag_params_inline_schema.go
- api/v1/api.yaml
- ui/src/api/v1/schema.ts
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
token_only,token_and_hmac, andhmac_onlymodesX-Dagu-Signature: sha256=<hex>against the exact raw request body before enqueueing DAG runsScope
Testing
go test ./internal/auth ./internal/service/auth ./internal/persis/filewebhook ./internal/service/frontend/... -count=1pnpm typecheckpnpm buildCloses #2036
Summary by CodeRabbit
New Features
UI
Documentation
Tests
Bug Fixes