You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
High Risk
Changes how survey secrets are stored, encrypted, dispatched to runners, and cleaned up; mistakes could leak secrets, drop them silently, or break HA/remote execution.
Overview
Fixes survey secret variables arriving empty on remote runners and not surviving server restarts or HA dispatch by persisting them in the shared DB instead of only in memory on the node that accepted the task.
On task creation, non-empty survey secret JSON is written as an access_key row with owner task, task_id, and expire_at (TTL from MaxTaskDurationSec + 1h, or 24h when unlimited). Creation failure fails the task; the task row still keeps secret as {}. Runner poll loads secrets via GetTaskSurveySecrets, sets jobData.Task.Secret, and fails the task if decrypt/expiry fails rather than dispatching empty vars. LocalExecutorProvider passes task.Secret into the executor so runners actually merge survey vars.
DeserializeSecret rejects expired keys (ErrAccessKeyExpired). Keys are deleted on finishRun, with an hourly expired-key sweep as backstop. Task-owned keys are hidden from project key APIs, omitted from backups, and cascade-delete with the task.
Note: Local execution in AddTask still wires LocalExecutor.Secret from in-memory extraSecretVars at enqueue time (plan called for DB read at local dispatch too—that may be follow-up).
It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
Changes
Task survey secrets are stored as encrypted, task-bound access keys with expiration. Task creation stores them, runner dispatch retrieves them for remote and local execution, and completion or periodic cleanup removes them. Generic key APIs and backups exclude task-secret keys.
Outcome: No medium, high, or critical vulnerabilities identified.
Reviewed the survey-secret persistence changes end-to-end: task-bound access_key rows, runner dispatch, API exposure, backup/export paths, and cleanup/expiry.
Controls verified
API isolation:KeyMiddleware and AddKey reject owner=task keys; the project keys list only returns shared keys (owner='').
Encryption & TTL: Secrets are keyring-encrypted; DeserializeSecret enforces expire_at; hourly sweep + finishRun deletion provide lifecycle backstops.
Runner delivery: Decrypted secrets are sent only on the runner poll path (X-Runner-Token), scoped to tasks assigned to that runner while status is waiting/starting.
Backup: Task-bound keys are filtered out of project backups; Secret remains backup:"-".
Paths traced (no exploitable gap found)
Direct key ID enumeration / CRUD via /keys/{id}
Cross-project or cross-task secret reads via GetTaskAccessKey
Environment or inventory key endpoints referencing task-owned rows
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
✅ Fixed: Local dispatch ignores persisted secrets
Local dispatch now reloads survey secrets via loadTaskSurveySecretsForDispatch in hydrateTaskRunner and TaskRunner.run before execution.
✅ Fixed: Missing key dispatches without secrets
GetTaskSurveySecrets now returns ErrTaskSurveySecretsNotFound when the key row is absent, and the expired-key sweep no longer deletes secrets for active tasks so dispatch fails on expiry instead of running silently empty.
diff --git a/api/runners/runners.go b/api/runners/runners.go--- a/api/runners/runners.go+++ b/api/runners/runners.go@@ -124,15 +124,19 @@
// instead of dispatching it with silently empty variables.
surveySecrets, err := c.encryptionService.GetTaskSurveySecrets(tsk.Task.ProjectID, tsk.Task.ID)
if err != nil {
- log.WithFields(log.Fields{- "runner_id": runner.ID,- "task_id": tsk.Task.ID,- "context": "runner",- }).WithError(err).Error("Failed to read task survey secrets")- tsk.Log("Error: failed to read survey secrets: " + err.Error())- tsk.SetStatus(task_logger.TaskFailStatus)- c.taskPool.FinalizeRemoteTask(tsk, &runner)- continue+ if errors.Is(err, server.ErrTaskSurveySecretsNotFound) {+ surveySecrets = ""+ } else {+ log.WithFields(log.Fields{+ "runner_id": runner.ID,+ "task_id": tsk.Task.ID,+ "context": "runner",+ }).WithError(err).Error("Failed to read task survey secrets")+ tsk.Log("Error: failed to read survey secrets: " + err.Error())+ tsk.SetStatus(task_logger.TaskFailStatus)+ c.taskPool.FinalizeRemoteTask(tsk, &runner)+ continue+ }
}
jobData := runners.JobData{
diff --git a/db/Store.go b/db/Store.go--- a/db/Store.go+++ b/db/Store.go@@ -338,9 +338,10 @@
// DeleteTaskAccessKeys removes all AccessKeyTaskSecret-owned keys of a task.
// Deleting for a task without such keys is not an error.
DeleteTaskAccessKeys(projectID int, taskID int) error
- // DeleteExpiredTaskAccessKeys removes all AccessKeyTaskSecret-owned keys- // whose expire_at is in the past, across all projects. Idempotent, safe to- // run concurrently on several HA nodes.+ // DeleteExpiredTaskAccessKeys removes expired AccessKeyTaskSecret-owned keys+ // for finished or orphaned tasks only. Active tasks keep their rows so+ // dispatch can fail clearly when a key is expired. Idempotent, safe to run+ // concurrently on several HA nodes.
DeleteExpiredTaskAccessKeys() error
}
diff --git a/db/sql/access_key.go b/db/sql/access_key.go--- a/db/sql/access_key.go+++ b/db/sql/access_key.go@@ -210,8 +210,27 @@
}
func (d *SqlDb) DeleteExpiredTaskAccessKeys() error {
+ // Do not delete keys for tasks that are still queued or running: an active+ // task with an expired key must fail dispatch with ErrAccessKeyExpired, not+ // silently run with empty survey variables after the row disappears.
_, err := d.exec(
- "delete from access_key where owner=? and expire_at is not null and expire_at < ?",+ `delete from access_key ak+ where ak.owner=?+ and ak.expire_at is not null+ and ak.expire_at < ?+ and (+ ak.task_id is null+ or not exists (+ select 1 from task t+ where t.id = ak.task_id and t.project_id = ak.project_id+ )+ or exists (+ select 1 from task t+ where t.id = ak.task_id+ and t.project_id = ak.project_id+ and t.status in ('stopped', 'success', 'error')+ )+ )`,
db.AccessKeyTaskSecret,
tz.Now())
return err
diff --git a/services/server/task_secret_svc.go b/services/server/task_secret_svc.go--- a/services/server/task_secret_svc.go+++ b/services/server/task_secret_svc.go@@ -14,6 +14,11 @@
// other secret, and unusable after ExpireAt. This keeps them readable by any
// HA node serving the runner dispatch while never persisting plaintext.
+// ErrTaskSurveySecretsNotFound is returned when a task has no stored survey-+// secret key. Callers that require secrets must treat this as a dispatch+// failure; callers dispatching tasks that never had survey secrets may ignore it.+var ErrTaskSurveySecretsNotFound = errors.New("task survey secrets not found")+
// CreateTaskSurveySecrets stores the survey-secrets JSON of a task as an
// encrypted task-bound access key. secrets must be a JSON object string.
func (s *accessKeyEncryptionServiceImpl) CreateTaskSurveySecrets(
@@ -42,14 +47,14 @@
}
// GetTaskSurveySecrets returns the decrypted survey-secrets JSON of a task.
-// A task without survey secrets yields "" with no error; an expired secret-// yields ErrAccessKeyExpired.+// A task without survey secrets yields ErrTaskSurveySecretsNotFound; an+// expired secret yields ErrAccessKeyExpired.
func (s *accessKeyEncryptionServiceImpl) GetTaskSurveySecrets(projectID int, taskID int) (string, error) {
key, err := s.accessKeyRepo.GetTaskAccessKey(projectID, taskID)
if err != nil {
if errors.Is(err, db.ErrNotFound) {
- return "", nil+ return "", ErrTaskSurveySecretsNotFound
}
return "", err
}
diff --git a/services/server/task_secret_svc_test.go b/services/server/task_secret_svc_test.go--- a/services/server/task_secret_svc_test.go+++ b/services/server/task_secret_svc_test.go@@ -89,7 +89,7 @@
secrets, err := svc.GetTaskSurveySecrets(1, 42)
- require.NoError(t, err)+ assert.ErrorIs(t, err, ErrTaskSurveySecretsNotFound)
assert.Empty(t, secrets)
}
diff --git a/services/tasks/TaskPool.go b/services/tasks/TaskPool.go--- a/services/tasks/TaskPool.go+++ b/services/tasks/TaskPool.go@@ -2,6 +2,7 @@
import (
"encoding/json"
+ "errors"
"fmt"
"strconv"
"time"
@@ -551,13 +552,17 @@
job = &RemoteJob{RunnerTag: tag, Task: tr.Task, taskPool: p}
} else {
app := db_lib.CreateApp(tr.Template, tr.Repository, tr.Inventory, tr)
+ secret, err := p.loadTaskSurveySecretsForDispatch(projectID, taskID)+ if err != nil {+ return nil, err+ }
job = &LocalExecutor{
Task: tr.Task,
Template: tr.Template,
Inventory: tr.Inventory,
Repository: tr.Repository,
Environment: tr.Environment,
- Secret: "{}",+ Secret: secret,
Logger: app.SetLogger(tr),
App: app,
KeyInstaller: p.keyInstallationService,
@@ -954,6 +959,22 @@
return len(vars) > 0
}
+// loadTaskSurveySecretsForDispatch reads persisted survey secrets for a task+// at local dispatch time. Tasks without stored secrets yield "{}".+func (p *TaskPool) loadTaskSurveySecretsForDispatch(projectID int, taskID int) (string, error) {+ secrets, err := p.encryptionService.GetTaskSurveySecrets(projectID, taskID)+ if err != nil {+ if errors.Is(err, server.ErrTaskSurveySecretsNotFound) {+ return "{}", nil+ }+ return "", err+ }+ if secrets == "" {+ return "{}", nil+ }+ return secrets, nil+}+
// taskSecretExpireAt returns the expiry moment for a task's survey-secret key:
// MaxTaskDurationSec plus an hour of queue allowance when the limit is set,
// otherwise 24 hours. Expiry is a backstop — the key is deleted when the task
diff --git a/services/tasks/TaskRunner.go b/services/tasks/TaskRunner.go--- a/services/tasks/TaskRunner.go+++ b/services/tasks/TaskRunner.go@@ -249,6 +249,14 @@
// can be exposed to the playbook as SEMAPHORE_JWT. Remote runners receive
// the JWT inside the JobData payload returned by the API.
if localJob, ok := t.job.(*LocalExecutor); ok {
+ secret, err := t.pool.loadTaskSurveySecretsForDispatch(t.Task.ProjectID, t.Task.ID)+ if err != nil {+ t.Log("Error: failed to read survey secrets: " + err.Error())+ t.SetStatus(task_logger.TaskFailStatus)+ return+ }+ localJob.Secret = secret+
if t.pool.signer != nil && t.Template.JWTParams != nil && t.Template.JWTParams.Enabled {
ttl, terr := t.Template.JWTParams.ParsedTTL()
if terr != nil {
The reason will be displayed to describe this comment to others. Learn more.
Local dispatch ignores persisted secrets
High Severity
Locally executed tasks lose their survey secrets after rehydration (e.g., HA recovery or restart). While secrets are correctly persisted to the database, the LocalExecutor doesn't reload them, causing tasks to run without these variables.
The reason will be displayed to describe this comment to others. Learn more.
Missing key dispatches without secrets
High Severity
The GetTaskSurveySecrets function returns an empty string and no error when a task's survey secret access key is missing. This allows tasks to be dispatched without their expected survey secrets if the key is removed (e.g., by an expiration sweep) before the task is finalized, bypassing the intended failure for truly missing secrets.
The reason will be displayed to describe this comment to others. Learn more.
Exclude task survey keys from store migrations
When this creates task-bound access_key rows, the database store migration path still loads every key with GetAccessKeys(... IgnoreOwner: true) in services/export/AccessKey.go:21 and appends them without the filter that was added to project backups. If a migration/export runs while a task with survey secrets is queued or running, it copies these ephemeral encrypted answers and restores them with the original, unmapped TaskID, which can create orphan/wrong task-secret rows or FK errors; filter AccessKeyTaskSecret out there as well.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR changes how survey secret variables are handled by persisting them (encrypted) in the shared DB as task-bound access_key rows, so secrets reliably survive restarts/HA dispatch and are delivered to remote runners without silently arriving empty.
Changes:
Persist non-empty survey secret JSON as access_key rows owned by AccessKeyTaskSecret with task_id and expire_at, and delete them on task completion with an hourly expired-key sweep backstop.
Enforce expire_at centrally in AccessKeyEncryptionService.DeserializeSecret and add task-secret CRUD methods to the encryption service/repo layer.
Inject decrypted survey secrets into runner job payloads and wire task.Secret through to the local executor.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
File
Description
services/tasks/TaskRunner.go
Deletes task-bound survey secret keys on terminal task completion.
services/tasks/TaskRunner_test.go
Updates encryption service mock to satisfy new interface methods and injects it into pool creation.
services/tasks/TaskPool.go
Adds secret detection, TTL calculation, periodic sweep, and persistence on task creation.
services/tasks/task_secret_test.go
Adds unit tests for hasSurveySecrets and taskSecretExpireAt.
services/tasks/local_executor_provider.go
Wires task.Secret into LocalExecutor.Secret so executors can merge survey secrets.
services/server/task_secret_svc.go
Implements create/get/delete for task survey secrets via access keys.
services/server/task_secret_svc_test.go
Adds unit tests for task secret service behaviors and expiry enforcement.
services/server/project_svc_test.go
Extends access key manager mock with new task-secret methods.
services/server/AccessKey_test.go
Extends access key repo mock with new task-secret methods.
services/server/access_key_encryption_svc.go
Adds ExpireAt enforcement and new task-secret service methods to the encryption service interface.
services/schedules/SchedulePool_test.go
Extends encryption service mock with new task-secret methods.
services/runners/executor_factory_test.go
Ensures JobData.Task.Secret is passed through to the created executor.
services/project/backup.go
Excludes task-bound survey secret keys from project backups.
db/Store.go
Extends AccessKeyManager with task-secret lookup and cleanup methods; adds TaskID option.
db/sql/migrations/v2.20.1.sql
Adds task_id + expire_at columns and index to access_key.
db/sql/access_key.go
Adds persistence for task_id/expire_at and implements task-secret CRUD + expired sweep queries.
db/Migration.go
Registers migration version 2.20.1.
db/AccessKey.go
Adds AccessKeyTaskSecret owner plus TaskID and ExpireAt fields on AccessKey.
api/runners/runners.go
Loads/decrypts task survey secrets on runner poll and injects into jobData.Task.Secret (fail task on read/decrypt/expiry error).
api/projects/keys.go
Blocks read/mutation of task-bound secret keys via generic project key endpoints.
api/projects/environment_test.go
Extends access key repo mock with new task-secret methods.
Documents the design/approach and rationale for task-bound access key storage.
Comments suppressed due to low confidence (1)
services/server/task_secret_svc_test.go:105
Same compilation issue here: new(base64.StdEncoding.EncodeToString(...)) is invalid because new requires a type, not a value. Use a local variable and take its address.
The mock discards DeleteTaskSurveySecrets arguments, so this test passes if cleanup is removed or called for the wrong task. Record the project/task IDs and assert them after taskRunner.run().
Also applies to: 97-97
🤖 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 `@services/tasks/TaskRunner_test.go` around lines 61 - 71, The
EncryptionServiceMock.DeleteTaskSurveySecrets method currently discards its
arguments, so cleanup calls cannot be verified. Update the mock to record the
received projectID and taskID, then after taskRunner.run() assert those recorded
values match the expected task and project identifiers.
DeleteExpiredTaskAccessKeys filters by owner and ranges on expire_at; add (owner, expire_at) to avoid periodic full-table scans.
Proposed migration
create index `access_key__task_id` on `access_key`(`task_id`);
+create index `access_key__owner__expire_at` on `access_key`(`owner`, `expire_at`);
🤖 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 `@db/sql/migrations/v2.20.1.sql` around lines 2 - 3, Update the migration that
adds the access_key expiry field to create a composite index on
access_key(owner, expire_at), supporting the filters and range used by
DeleteExpiredTaskAccessKeys while retaining the existing task_id index.
🤖 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.
Inline comments:
In `@AGENTS/plans/2_20/survey-secrets-remote-runners.md`:
- Around line 7-8: Update the plan’s pro executor wiring scope to address docker
and k8s providers, ensuring remote executions pass task.Secret through the
Secret field; alternatively, explicitly block rollout for those providers until
this wiring is delivered.
In `@services/tasks/task_secret_test.go`:
- Line 45: In the test setup around the MaxTaskDurationSec assignment, save the
existing util.Config pointer before replacing it and register a t.Cleanup
callback to restore that pointer after the test completes, ensuring later tests
see the original configuration.
In `@services/tasks/TaskPool.go`:
- Around line 961-966: Update taskSecretExpireAt and its dispatch flow so secret
expiry cannot occur while a task is still queued; derive or refresh the expiry
when the task is dispatched, or apply a queue deadline matching the expiry
policy. Ensure MaxTaskDurationSec limits execution time without prematurely
invalidating keys needed by queued tasks.
- Around line 1075-1086: Update local task dispatch around the survey-secret
persistence block in services/tasks/TaskPool.go lines 1075-1086 so it retrieves
secrets through the task-secret service using GetTaskSurveySecrets and
DeserializeSecret immediately before execution, instead of retaining or passing
raw extraSecretVars; preserve failure handling for retrieval or deserialization.
After this behavior is implemented, retain the “both execution branches read
back” claim in AGENTS/plans/2_20/survey-secrets-remote-runners.md lines 145-146;
no direct plan change is otherwise required.
---
Nitpick comments:
In `@db/sql/migrations/v2.20.1.sql`:
- Around line 2-3: Update the migration that adds the access_key expiry field to
create a composite index on access_key(owner, expire_at), supporting the filters
and range used by DeleteExpiredTaskAccessKeys while retaining the existing
task_id index.
In `@services/tasks/TaskRunner_test.go`:
- Around line 61-71: The EncryptionServiceMock.DeleteTaskSurveySecrets method
currently discards its arguments, so cleanup calls cannot be verified. Update
the mock to record the received projectID and taskID, then after
taskRunner.run() assert those recorded values match the expected task and
project identifiers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 41d685f1-a883-4e2b-8d7f-d5ae0c831de8
📥 Commits
Reviewing files that changed from the base of the PR and between 3c953e4 and ac55526.
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Complete the pro executor wiring before treating this as implemented.
The plan confirms docker/k8s providers still drop Secret, so those remote executions retain the original empty-secret failure. Include that wiring or block rollout for those providers.
🤖 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 `@AGENTS/plans/2_20/survey-secrets-remote-runners.md` around lines 7 - 8,
Update the plan’s pro executor wiring scope to address docker and k8s providers,
ensuring remote executions pass task.Secret through the Secret field;
alternatively, explicitly block rollout for those providers until this wiring is
delivered.
‼️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.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/tasks/task_secret_test.go` at line 45, In the test setup around the
MaxTaskDurationSec assignment, save the existing util.Config pointer before
replacing it and register a t.Cleanup callback to restore that pointer after the
test completes, ensuring later tests see the original configuration.
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Local execution bypasses task-secret expiry enforcement.
The new key has an expiry, but the local job still receives raw extraSecretVars instead of calling GetTaskSurveySecrets/DeserializeSecret. A queued local task can therefore execute after expiry; the plan’s “both execution branches read back” claim is not true.
services/tasks/TaskPool.go#L1075-L1086: make local execution load the secret through the task-secret service immediately before use, rather than retaining the submitted plaintext.
AGENTS/plans/2_20/survey-secrets-remote-runners.md#L145-L146: retain this claim only once local dispatch follows that retrieval path.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/tasks/TaskPool.go` around lines 1075 - 1086, Update local task
dispatch around the survey-secret persistence block in
services/tasks/TaskPool.go lines 1075-1086 so it retrieves secrets through the
task-secret service using GetTaskSurveySecrets and DeserializeSecret immediately
before execution, instead of retaining or passing raw extraSecretVars; preserve
failure handling for retrieval or deserialization. After this behavior is
implemented, retain the “both execution branches read back” claim in
AGENTS/plans/2_20/survey-secrets-remote-runners.md lines 145-146; no direct plan
change is otherwise required.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review — PR #4086 (re-validated on 2408aae7)
Outcome: No medium, high, or critical vulnerabilities identified.
Re-reviewed after the fix(survey secrets): fill secrets from db commit. Prior automation findings from other bots (local rehydration gap, expired-key sweep deleting active rows) are addressed in this revision.
Controls verified
API isolation:KeyMiddleware and AddKey block owner=task keys; project key listing still filters owner=''.
Encryption & TTL: Survey secrets are keyring-encrypted; DeserializeSecret enforces expire_at; expired keys for active tasks now fail dispatch instead of being swept.
Runner delivery: Decrypted secrets are delivered only on the runner poll path (X-Runner-Token), scoped to tasks assigned to that runner in waiting/starting state; jobData.Task.Secret is always DB-derived.
HA recovery:hydrateTaskRunner reloads secrets via GetTaskSurveySecrets for local executors.
Backup/export: Project backup filters out AccessKeyTaskSecret rows; Task.Secret is not persisted (db:"-") and is cleared before task creation.
Paths traced (no exploitable gap found)
Direct key ID enumeration / CRUD via /keys/{id}
Cross-project or cross-task secret reads via GetTaskAccessKey
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
services/tasks/task_secret_test.go:45
This test mutates the global util.Config pointer but never restores it, which can leak configuration into other tests and cause order-dependent failures. Save and restore util.Config via t.Cleanup before overriding it.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review — PR #4086 (re-validated on 50110d99)
Outcome: No medium, high, or critical vulnerabilities identified.
Re-reviewed after the runner dispatch refactor (refactor(runner): move secrets decryption to separate method).
Prior threads re-validated
Local dispatch / HA rehydration:hydrateTaskRunner and TaskRunner.run() both load survey secrets via GetTaskSurveySecrets (with central ExpireAt enforcement) before execution — prior Bugbot concern is addressed.
Missing-key silent dispatch:GetTaskSurveySecrets still maps ErrNotFound to "", but there is no attacker-controlled API path to delete task-bound keys while a task is queued/running (KeyMiddleware blocks CRUD; the expiry sweep only deletes keys for terminal tasks). Residual risk is operational, not an exploitable authz bypass.
Local expiry bypass: addressed by the TaskRunner.run() reload path.
Controls verified
API isolation:KeyMiddleware and AddKey reject AccessKeyTaskSecret keys; list endpoint still filters owner=''.
No plaintext persistence:task.secret stays db:"-"; secrets are keyring-encrypted in access_key.
Runner dispatch: secrets decrypted only for tasks assigned to the authenticated runner; expired/missing decrypt fails the task instead of leaking partial key material across jobs (collectTaskAccessKeys staging).
Backup hygiene: project backup excludes task-bound keys (export/migrate gap remains admin-only and encrypted — below reporting threshold).
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
services/server/task_secret_svc_test.go:77
This test doesn't compile: new(base64.StdEncoding.EncodeToString(...)) is invalid because new expects a type, not a value. Use a local encoded string variable and take its address instead.
This test doesn't compile: new(base64.StdEncoding.EncodeToString(...)) is invalid because new expects a type, not a value. Use a local encoded string variable and take its address instead.
DELETE FROM access_key ak ... uses a table alias in a DELETE statement; SQLite doesn't support aliases in DELETE, so this will fail on the SQLite dialect. Rework the query to avoid the alias while keeping the correlated subquery semantics.
func (d *SqlDb) DeleteExpiredTaskAccessKeys() error {
// Do not delete keys for tasks that are still queued or running: an active
// task with an expired key must fail dispatch with ErrAccessKeyExpired, not
// silently run with empty survey variables after the row disappears.
_, err := d.exec(
`delete from access_key ak
where ak.owner=?
and ak.expire_at is not null
and ak.expire_at < ?
and (
ak.task_id is null
or exists (
select 1 from task t
where t.id = ak.task_id
and t.project_id = ak.project_id
and t.status in ('stopped', 'success', 'error')
)
)`,
db.AccessKeyTaskSecret,
tz.Now())
return err
Duplicated with TaskRunner.run() — see consolidated comment.
This JWT issuance block mirrors services/tasks/TaskRunner.go (lines 274-306) almost exactly. See the consolidated comment for a shared-helper suggestion.
🤖 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 `@api/runners/runners.go` around lines 161 - 210, Extract the duplicated JWT
issuance logic from the runner flow around c.signer.Sign and the corresponding
TaskRunner.run implementation into a shared helper. Reuse that helper from both
call sites while preserving TTL validation, error logging, task failure
finalization, and assignment to jobData.JWT.
Survey-secret retrieval + JWT issuance logic duplicated with RunnerController.GetRunner.
This block (secret retrieval with ErrAccessKeyExpired handling, then TTL parsing + JWT signing with fail-on-error) is nearly line-for-line duplicated in api/runners/runners.go (lines 113-196). Consider extracting a shared helper (e.g., in a common package) that both the local and remote dispatch paths call, to avoid the two copies drifting apart over time.
🤖 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 `@services/tasks/TaskRunner.go` around lines 253 - 306, Extract the duplicated
survey-secret retrieval and optional JWT issuance logic from the task runner
flow around localJob.Secret/localJob.JWT into a shared helper, including
ErrAccessKeyExpired handling, TTL parsing, signing, logging context, and failure
propagation. Update both this flow and RunnerController.GetRunner to call the
helper while preserving their existing task-failure behavior and generated
secret/JWT values.
🤖 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.
Inline comments:
In `@api/runners/runners.go`:
- Around line 271-289: Before adding tsk.Repository.SSHKey to keys, pass it
through encryptionService.DeserializeSecret using the same error logging and
return behavior as the other repository key. Preserve the existing access-key
map assignment after successful decryption, and run golangci-lint on the updated
function.
---
Nitpick comments:
In `@api/runners/runners.go`:
- Around line 161-210: Extract the duplicated JWT issuance logic from the runner
flow around c.signer.Sign and the corresponding TaskRunner.run implementation
into a shared helper. Reuse that helper from both call sites while preserving
TTL validation, error logging, task failure finalization, and assignment to
jobData.JWT.
In `@services/tasks/TaskRunner.go`:
- Around line 253-306: Extract the duplicated survey-secret retrieval and
optional JWT issuance logic from the task runner flow around
localJob.Secret/localJob.JWT into a shared helper, including ErrAccessKeyExpired
handling, TTL parsing, signing, logging context, and failure propagation. Update
both this flow and RunnerController.GetRunner to call the helper while
preserving their existing task-failure behavior and generated secret/JWT values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ddd66879-2fc9-496d-8b87-f1f2b0bc515b
📥 Commits
Reviewing files that changed from the base of the PR and between 2408aae and 50110d9.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review — PR #4086 (re-validated on de1bb939)
Outcome: No medium, high, or critical vulnerabilities identified.
Reviewed the survey-secret persistence changes end-to-end: task-bound access_key rows, runner dispatch, API exposure, backup/export paths, expiry enforcement, and cleanup.
Prior findings validated
Local dispatch / rehydration — Addressed. hydrateTaskRunner and TaskRunner.run() both load secrets via GetTaskSurveySecrets (with DeserializeSecret expiry enforcement) before execution.
API exposure — KeyMiddleware and AddKey block AccessKeyTaskSecret keys; project key listing still filters owner=''.
Backup/export — Project backup filters task-owned keys; AccessKey.Secret is excluded from backup serialization (backup:"-").
Runner channel — Decrypted survey secrets are delivered only to authenticated runners (X-Runner-Token), consistent with existing access-key dispatch.
Missing-key returns empty — GetTaskSurveySecrets still returns "" when no row exists; this matches tasks without survey secrets and has no attacker-controlled deletion path (sweep skips active tasks; deletion is internal/on terminal).
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
services/tasks/TaskRunner.go:293
jwt.TaskInfo.Audience expects type jwt.Audience ([]string). Passing TemplateJWTParams.Audience directly is a []string and won’t compile; convert it to jwt.Audience like the previous code did.
This test won’t compile: new() can only be used with a type, not an expression. Create an encoded string variable and take its address for the AccessKey.Secret pointer.
This test won’t compile: new() can only be used with a type, not an expression. Create an encoded string variable and take its address for the AccessKey.Secret pointer.
The comment says "{}" comes from the UI and AddTask sanitization, but AddTask now sanitizes taskObj.Secret to an empty string (""), not "{}". Update the comment to avoid documenting a behavior that no longer exists.
// TestGetEnvironmentExtraVars_EmptySecret pins that "" and "{}" are equivalent
// "no survey secrets" values: "" comes from DB-loaded tasks and API clients
// omitting the field, "{}" from the UI and the AddTask sanitization.
Re-reviewed after the latest synchronize (9fb509b). No medium, high, or critical vulnerabilities found in the added/modified code.
Prior findings validated
Local dispatch ignores persisted secrets — Addressed in 2408aae7 / ea039f27: hydrateTaskRunner and TaskRunner.run() now load survey secrets via GetTaskSurveySecrets from the DB-backed access key, so HA recovery/restart no longer drops them.
Missing key dispatches without secrets — Addressed in 2408aae7: DeleteExpiredTaskAccessKeys no longer deletes keys for queued/running tasks; expired keys return ErrAccessKeyExpired and fail dispatch instead of silently running empty. ErrNotFound → "" remains intentional for tasks without survey secrets.
Controls verified
Task-bound keys blocked on generic key API (KeyMiddleware, AddKey)
Hidden from project secrets list (owner='' filter)
Excluded from project backup
Encrypted at rest via existing keyring; decrypted only at dispatch to authorized runners/local executor
Re-reviewed after synchronize to 2211ef40. No medium, high, or critical vulnerabilities found in the added/modified code.
Prior findings re-validated
Local dispatch / HA rehydration — Addressed. hydrateTaskRunner and TaskRunner.run() load survey secrets via GetTaskSurveySecrets, with central ExpireAt enforcement in DeserializeSecret.
Missing-key silent dispatch — Not an exploitable authz gap. GetTaskSurveySecrets still returns "" for ErrNotFound (tasks without survey secrets), but there is no attacker-controlled API path to delete task-bound keys while a task is queued/running (KeyMiddleware blocks CRUD; DeleteExpiredTaskAccessKeys only removes keys for terminal tasks).
Local expiry bypass — Addressed. Local execution reloads from the DB-backed key in TaskRunner.run() before use.
Export/migration inclusion — services/export/AccessKey.go still loads task-bound keys during DB store migration (IgnoreOwner: true), but this is an admin-only CLI path and ciphertext is keyring-encrypted; project backup/export already filters them. Below reporting threshold for external attack paths.
Controls verified
Task-bound keys blocked on generic key API (KeyMiddleware, AddKey); hidden from project secrets list (owner='' filter)
Secrets keyring-encrypted at rest; decrypted only at dispatch to authorized runners/local executor
Runner poll scoped by X-Runner-Token and RunnerID assignment; jobData.Task.Secret always DB-derived
Project backup excludes AccessKeyTaskSecret rows; Task.Secret is not persisted (db:"-")
New SQL uses parameterization; GetAccessKeyOptions.Validate() prevents unscoped owner queries
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
db/sql/access_key.go:234
DeleteExpiredTaskAccessKeys uses a table alias in a single-table DELETE ("delete from access_key ak"). This syntax is not supported by SQLite (and is non-portable across SQL dialects), which can break secret sweeping depending on the configured DB. Removing the alias keeps the correlated subquery and should work across SQLite/MySQL/Postgres.
_, err := d.exec(
`delete from access_key
where owner=?
and expire_at is not null
and expire_at < ?
and (
task_id is null
or exists (
select 1 from task t
where t.id = task_id
and t.project_id = access_key.project_id
and t.status in ('stopped', 'success', 'error')
)
)`,
db.AccessKeyTaskSecret,
tz.Now())
services/server/task_secret_svc_test.go:76
This test builds a *string for AccessKey.Secret using new(base64.StdEncoding.EncodeToString(...)), but the built-in new expects a type, not a value expression. Use a local variable and take its address, consistent with the pattern already used later in this file (TestDeserializeSecret_ExpireAt).
Same issue as above: new(base64.StdEncoding.EncodeToString(...)) is not valid for obtaining a *string. Use a local string variable and take its address.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
services/tasks/TaskPool.go:1086
If extraSecretVars is non-empty but invalid JSON (or valid JSON but not an object, e.g. []), hasSurveySecrets returns false and the secrets are silently dropped for remote runners (no access key row is created). Local execution still passes the invalid payload via LocalExecutor.Secret, so behavior becomes inconsistent and can lead to tasks running with unexpectedly empty survey secrets. Consider failing the task at enqueue time when the secret payload is non-empty but not a JSON object.
if hasSurveySecrets(extraSecretVars) {
err = p.encryptionService.CreateTaskSurveySecrets(
projectID, newTask.ID, extraSecretVars, taskSecretExpireAt())
services/server/task_secret_svc_test.go:75
new(base64.StdEncoding.EncodeToString(...)) does not compile because new expects a type, not a value. Use a local variable and take its address (or a helper) to get a *string.
Re-reviewed after synchronize to 05906b65. No medium, high, or critical vulnerabilities found in the added/modified code.
Prior findings re-validated
Local dispatch / HA rehydration — Addressed. hydrateTaskRunner and TaskRunner.run() load survey secrets via GetTaskSurveySecrets, with central ExpireAt enforcement in DeserializeSecret. AddTask still seeds LocalExecutor.Secret with in-memory plaintext, but execution always reloads from the DB-backed key before use.
Missing-key silent dispatch — Not an exploitable authz gap. GetTaskSurveySecrets returns "" for ErrNotFound (tasks without survey secrets), but there is no attacker-controlled API path to delete task-bound keys while a task is queued/running (KeyMiddleware blocks CRUD; DeleteExpiredTaskAccessKeys only removes keys for terminal tasks).
Local expiry bypass — Addressed at execution time via TaskRunner.run() / runner prepareRemoteJob DB reload.
Export/migration inclusion — services/export/AccessKey.go still loads task-bound keys during DB store migration (IgnoreOwner: true), but this is an admin-only CLI path and ciphertext is keyring-encrypted; project backup already filters them. Below reporting threshold for external attack paths.
Controls verified
Task-bound keys blocked on generic key API (KeyMiddleware, AddKey); hidden from project secrets list (owner='' filter)
Secrets keyring-encrypted at rest; decrypted only at dispatch to authorized runners/local executor
Runner poll scoped by X-Runner-Token and RunnerID assignment; jobData.Task.Secret always DB-derived
Project backup excludes AccessKeyTaskSecret rows; Task.Secret is not persisted (db:"-")
New SQL uses parameterization; GetAccessKeyOptions.Validate() prevents unscoped owner queries
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
services/tasks/TaskPool.go:958
hasSurveySecrets returns false on JSON unmarshal error, which means a non-empty but malformed taskObj.Secret will be silently dropped (no DB key created) and the task will run without its survey secrets. That contradicts the PR goal of avoiding silent secret loss; malformed JSON should not be ignored.
vars := make(map[string]any)
if err := json.Unmarshal([]byte(secretVars), &vars); err != nil {
return false
}
services/tasks/local_executor_test.go:64
This test comment says "{}" comes from the UI and the AddTask sanitization, but AddTask now sanitizes taskObj.Secret to "" (see services/tasks/TaskPool.go). Update the comment so it matches the current behavior.
// TestGetEnvironmentExtraVars_EmptySecret pins that "" and "{}" are equivalent
// "no survey secrets" values: "" comes from DB-loaded tasks and API clients
// omitting the field, "{}" from the UI and the AddTask sanitization.
services/server/task_secret_svc_test.go:76
new(base64.StdEncoding.EncodeToString(...)) is not valid Go (new expects a type, not an expression), so this test file will not compile. Use a local variable (or an inline func) and take its address for the Secret field.
Make collectTaskAccessKeys self-sufficient by decrypting the task
repository SSH key itself instead of relying on it having been
decrypted earlier in TaskRunner.populateDetails. Decryption reads the
untouched ciphertext from key.Secret, so the call is idempotent even
when the key is already decrypted, and the function no longer breaks
silently if that upstream side effect changes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review — PR #4086 (re-validated on b16b8ff)
Outcome: No medium, high, or critical vulnerabilities identified.
Reviewed the survey-secret persistence changes end-to-end: task-bound access_key rows, runner dispatch, API exposure, backup/export paths, expiry enforcement, and cleanup.
Prior findings validated
Local dispatch / HA rehydration — Addressed. hydrateTaskRunner (line 555) and TaskRunner.run() (line 254) both load survey secrets via GetTaskSurveySecrets with central ExpireAt enforcement in DeserializeSecret. The AddTask local branch still seeds extraSecretVars at enqueue time, but execution always reloads from the DB-backed key before job.Run().
Missing-key silent dispatch — Not an exploitable vulnerability. GetTaskSurveySecrets intentionally returns "" for tasks without survey secrets (the common case). DeleteExpiredTaskAccessKeys now preserves keys for non-terminal tasks, and task-owned keys are blocked from user-facing key APIs (KeyMiddleware, AddKey). Premature deletion would require direct DB access outside normal auth boundaries.
Repository SSH key decryption — Addressed in collectTaskAccessKeys (explicit DeserializeSecret for tsk.Repository.SSHKey).
Controls verified
API isolation: Task-bound keys are hidden from the project keys list (owner='' filter) and rejected by KeyMiddleware / AddKey.
Encryption: Secrets are keyring-encrypted at rest; plaintext is not persisted on the task row (taskObj.Secret = "").
Runner dispatch: Secrets are delivered only to the assigned runner over the existing TLS + runner-token channel; expired/missing decrypt paths fail the task instead of dispatching silently (remote and local).
Backup: Task-owned keys are excluded from project backups.
Cleanup: Keys are deleted on terminal status; expired-key sweep is scoped to finished tasks only.
The reason will be displayed to describe this comment to others. Learn more.
Security review — PR #4086 (re-validated on 4da92575)
Outcome: No medium, high, or critical vulnerabilities identified.
Reviewed the survey-secret persistence changes end-to-end on the latest synchronize: task-bound access_key rows, runner dispatch, API exposure, backup/export paths, expiry enforcement, and cleanup.
Prior findings validated
Local dispatch / HA rehydration — Addressed. TaskRunner.run() loads survey secrets via GetTaskSurveySecrets immediately before local execution (with ExpireAt enforced in DeserializeSecret). Commit 4da92575 intentionally leaves hydrateTaskRunner snapshots without secrets so stop/confirm/reject and HA rehydration do not fail on expired keys; execution still re-reads from the DB at run time.
Missing-key silent dispatch — Not an exploitable authz bypass. GetTaskSurveySecrets returns "" only for ErrNotFound (tasks with no survey secrets). DeleteExpiredTaskAccessKeys preserves keys for non-terminal tasks, so active queued/running jobs cannot lose secrets via the hourly sweep.
API / key endpoint isolation — KeyMiddleware and AddKey block owner=task keys; the project keys list continues to filter owner=''.
Runner dispatch — prepareRemoteJob reads secrets from the DB per poll, fails the task on ErrAccessKeyExpired/decrypt errors, and only serves jobs where task.runner_id matches the authenticated runner token.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
services/server/task_secret_svc_test.go:75
new(base64.StdEncoding.EncodeToString(...)) is not valid Go (the built-in new takes a type, not a value). This should be a string variable with its address taken so AccessKey.Secret gets a *string.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
services/server/task_secret_svc_test.go:75
new(...) here is invalid Go (the built-in new only accepts a type, not an expression), so this test won’t compile. Use a local string variable (or an inline closure) and take its address.
Same issue as above: new(...) with an expression argument is invalid Go and will fail to compile. Create a string and take its address (or use an inline closure returning *string).
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
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.
Note
High Risk
Changes how survey secrets are stored, encrypted, dispatched to runners, and cleaned up; mistakes could leak secrets, drop them silently, or break HA/remote execution.
Overview
Fixes survey secret variables arriving empty on remote runners and not surviving server restarts or HA dispatch by persisting them in the shared DB instead of only in memory on the node that accepted the task.
On task creation, non-empty survey secret JSON is written as an
access_keyrow with ownertask,task_id, andexpire_at(TTL fromMaxTaskDurationSec+ 1h, or 24h when unlimited). Creation failure fails the task; the task row still keepssecretas{}. Runner poll loads secrets viaGetTaskSurveySecrets, setsjobData.Task.Secret, and fails the task if decrypt/expiry fails rather than dispatching empty vars.LocalExecutorProviderpassestask.Secretinto the executor so runners actually merge survey vars.DeserializeSecretrejects expired keys (ErrAccessKeyExpired). Keys are deleted onfinishRun, with an hourly expired-key sweep as backstop. Task-owned keys are hidden from project key APIs, omitted from backups, and cascade-delete with the task.Note: Local execution in
AddTaskstill wiresLocalExecutor.Secretfrom in-memoryextraSecretVarsat enqueue time (plan called for DB read at local dispatch too—that may be follow-up).Reviewed by Cursor Bugbot for commit ac55526. Configure here.
Summary by CodeRabbit