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
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02531823-0a15-4c4d-ba9c-fd68d5a86940
📥 Commits
Reviewing files that changed from the base of the PR and between 75ea78c and 73ed7b2.
📒 Files selected for processing (6)
api-docs.yml
db/Migration.go
db/Template.go
db/sql/template.go
web/src/components/TemplateForm.vue
web/src/lang/en.js
🚧 Files skipped from review as they are similar to previous changes (6)
web/src/lang/en.js
web/src/components/TemplateForm.vue
db/Migration.go
api-docs.yml
db/Template.go
db/sql/template.go
📝 Walkthrough
Walkthrough
The change adds template-controlled task survey-variable validation, persists the allow_any_vars_in_task option, validates task creation requests, documents the API behavior, and updates template and task forms.
Adds survey-variable lookup and value validation, parses task payloads, enforces declared and secret-variable rules, and tests strict and arbitrary-variable modes.
Adds the non-null allow_any_vars_in_task column, migration registration, template create/update persistence, rollback SQL, and persistence tests.
Task creation validation and API schema api/projects/tasks.go, api-docs.yml
Validates survey variables before task insertion and documents environment, secret, and template option behavior in the API schemas.
Template and task form behavior web/src/components/TemplateForm.vue, web/src/components/TaskForm.vue, web/src/lang/en.js
Adds the template checkbox and translations, and filters rerun environment variables to declared survey variables unless arbitrary variables are enabled.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review
Outcome: No medium/high/critical vulnerabilities found in the added or modified code.
This PR is a security hardening change. It adds backend validation (ValidateSurveyVars) on the primary task-creation API path (POST /project/{id}/tasks) to block undeclared survey variables — including Ansible connection overrides such as ansible_ssh_common_args that can execute code on the Semaphore server.
What was reviewed
Input → sink tracing:Task.Environment / Task.Secret are validated against template.survey_vars before TaskPool.AddTask runs on the user-facing endpoint.
Controls verified: Undeclared keys are rejected by default; secret survey vars must use secret; int/enum types are checked; AllowAnyVarsInTask is opt-in and defaults to false.
Bypass paths checked: Integrations, schedules, and autorun child tasks call TaskPool.AddTask directly without this validation, but those paths only accept environment keys configured by project managers (integration extract mappings / schedule task params), not arbitrary keys from task runners. That is a different trust boundary than the vulnerability this PR fixes.
Other classes: No new injection surfaces, authz bypasses, secret leakage, or dependency changes were introduced.
Residual note (not reported as a finding)
Placing ValidateSurveyVars in the HTTP handler instead of TaskPool.AddTask means future task-creation entry points must remember to call it. Consider centralizing enforcement in TaskPool.AddTask for defense in depth.
The reason will be displayed to describe this comment to others. Learn more.
Enforce survey validation in the shared task path
When a task is created by an integration or schedule, api/integration.go:366 and services/schedules/SchedulePool.go:164 call TaskPool.AddTask directly, bypassing this controller-only check. Consequently, a webhook extractor can still place request-controlled, undeclared or type-invalid values into the task environment despite the new default restriction, including the connection options this change is intended to block. Apply the validation in the shared task creation path before the secret is cleared.
The reason will be displayed to describe this comment to others. Learn more.
Summary
Moved survey-variable validation into the shared TaskPool.AddTask path, ensuring API, integration, and scheduled tasks all receive identical validation. Secret variables are validated before the secret payload is cleared. services/tasks/TaskPool.goL1023-L1047
Preserved the REST endpoint’s existing client-facing behavior by returning validation failures through helpers.WriteError rather than converting them into internal-server errors. api/projects/tasks.goL51-L78
Committed the changes as a848e9e (fix(tasks): validate survey vars for every task source) and created the follow-up pull request metadata titled “fix(tasks): enforce survey validation across task sources.”
Testing
✅ git diff --check
✅ go test ./db ./services/tasks ./api/projects
✅ git status --short --branch (working tree clean)
The reason will be displayed to describe this comment to others. Learn more.
Return the opt-in flag from template list queries
With the SQL store, GetTemplates and GetTemplatesWithPermissions use the explicit field list in db/sql/template.go:264-288, which omits pt.allow_any_vars_in_task. A template that has this flag enabled is therefore serialized as if it were disabled by the project/view template-list endpoints; clients that round-trip a listed template can also unintentionally clear the setting on update. Add the new column to that select list.
The reason will be displayed to describe this comment to others. Learn more.
Reject structured values for text survey variables
For SurveyVarStr and SurveyVarText, every non-null, non-empty value falls through to this unconditional success, so payloads such as {"app_version":{"unexpected":true}} pass validation even though the survey declares a text input. Secret values have the same problem because the secret loop only checks the variable name and never validates its value. These structured values then reach the application as objects or are stringified for env-target variables; require JSON strings for the string, text, and secret types.
The reason will be displayed to describe this comment to others. Learn more.
Reject non-string enum inputs instead of coercing them
When an enum contains the string value "1" or "true", a caller can submit the JSON number 1 or boolean true and this fmt.Sprintf comparison accepts it. The original non-string value is then passed to the application, violating the declared enum type and potentially changing application behavior. Check that the input is a string before comparing it with the allowed enum values.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds backend enforcement for template “survey variable” constraints so that, by default, tasks can only submit variables declared in a template’s survey_vars, with a new per-template opt-out (allow_any_vars_in_task) for advanced use cases.
Changes:
Adds allow_any_vars_in_task template flag (UI + DB + API docs) to optionally permit undeclared task variables.
Implements backend validation (Task.ValidateSurveyVars) to reject undeclared variables and enforce declared types (with support for “secret” survey vars via Task.Secret).
Adds SQL migration + tests covering both the new validation behavior and migration persistence.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
File
Description
web/src/lang/en.js
Adds i18n strings for the new template checkbox and hint text.
web/src/components/TemplateForm.vue
Adds UI control to toggle allow_any_vars_in_task on templates.
web/src/components/TaskForm.vue
Filters undeclared vars on re-run unless the template allows arbitrary vars.
db/Template.go
Introduces SurveyVarSecret, fixes survey var const typing, and adds value validation helpers + AllowAnyVarsInTask field.
db/Task.go
Adds parsing + validation of task-provided environment/secret vars against template survey definitions.
db/Task_test.go
Adds unit tests for ValidateSurveyVars and GetSurveyVar.
db/sql/template.go
Persists allow_any_vars_in_task on create/update in SQL store.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 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/projects/tasks.go`:
- Around line 51-61: Move the ValidateSurveyVars check from the direct REST
handler into TaskPool.AddTask, using the task’s associated template before
accepting any task. Ensure all AddTask callers, including schedules and
integrations, enforce the same validation while preserving the existing error
propagation behavior.
In `@db/Task.go`:
- Around line 201-219: Update parseTaskVars to verify that no non-whitespace
data remains after the initial JSON object is decoded. Use the existing decoder
dec and reject trailing content, returning the same validation error for invalid
Task.Environment or Task.Secret payloads while preserving valid object parsing.
In `@db/Template.go`:
- Around line 130-165: Update Task.ValidateSurveyVars to validate every declared
SurveyVar, not only supplied values, and return a validation error when a
variable with Required set is absent or null. Preserve ValidateValue’s
optional-value behavior for non-required variables, while ensuring required
omissions fail during task creation/validation before execution.
In `@web/src/components/TaskForm.vue`:
- Around line 332-334: Update the rerun initialization near editedEnvironment
and editedSecretEnvironment so the parsed v.secret object also passes through
filterDeclaredVars before assignment. Preserve the existing JSON fallback and
ensure editedSecretEnvironment contains only declared variables, preventing
beforeSave from serializing undeclared secrets.
🪄 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: a4695007-0f76-44c8-ab2f-b1e2b7b2e710
📥 Commits
Reviewing files that changed from the base of the PR and between 0bd3589 and 75ea78c.
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Find other places that build/queue db.Task and check whether they call ValidateSurveyVars
rg -n --type=go 'AddTask\(' --glob '!*_test.go'
rg -n --type=go 'ValidateSurveyVars' --glob '!*_test.go'
Repository: semaphoreui/semaphore
Length of output: 159
🏁 Script executed:
#!/bin/bashset +e
printf'tracked files:\n'
git ls-files | sed -n '1,120p'printf'\nFind tasks.go:\n'
fd -a 'tasks\.go$'.2>/dev/null ||trueprintf'\nSearch AddTask ValidateSurveyVars (no glob):\n'
rg -n 'AddTask\(|ValidateSurveyVars' -g '*.go'.||trueprintf'\nSearch Schedule/Integration/Workflow task creation terms:\n'
rg -n 'schedule|Schedule|integration|Integration|workflow|Workflow|Environment:|Env:|TaskId|Task' -g '*.go'.| sed -n '1,240p'
Repository: semaphoreui/semaphore
Length of output: 25986
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'--- api/projects/tasks.go ---\n'
cat -n api/projects/tasks.go | sed -n '1,130p'printf'\n--- services/tasks/TaskPool.go AddTask section ---\n'
cat -n services/tasks/TaskPool.go | sed -n '960,1055p'printf'\n--- services/schedules/SchedulePool.go task creation section ---\n'
cat -n services/schedules/SchedulePool.go | sed -n '130,190p'printf'\n--- api/integration.go task creation section ---\n'
cat -n api/integration.go | sed -n '320,380p'printf'\n--- Pro workflow interfaces and call sites ---\n'
cat -n pro_interfaces/workflow_svc.go | sed -n '1,80p'
rg -n --type=go 'AddTask\(' services api pro* -g '*.go'||true
rg -n --type=go 'ValidateSurveyVars' services api db -g '*.go'||true
Repository: semaphoreui/semaphore
Length of output: 250
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- api/projects/tasks.go ---'
cat -n api/projects/tasks.go | sed -n '1,130p'printf'%s\n'''printf'%s\n''--- services/tasks/TaskPool.go AddTask section ---'
cat -n services/tasks/TaskPool.go | sed -n '960,1055p'printf'%s\n'''printf'%s\n''--- services/schedules/SchedulePool.go task creation section ---'
cat -n services/schedules/SchedulePool.go | sed -n '130,190p'printf'%s\n'''printf'%s\n''--- api/integration.go task creation section ---'
cat -n api/integration.go | sed -n '320,380p'printf'%s\n'''printf'%s\n''--- Pro workflow interfaces and call sites ---'
cat -n pro_interfaces/workflow_svc.go | sed -n '1,80p'
rg -n --type=go 'AddTask\(' services api pro* -g '*.go'||true
rg -n --type=go 'ValidateSurveyVars' services api db -g '*.go'||true
Repository: semaphoreui/semaphore
Length of output: 18511
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- db/Task.go relevant validations ---'
cat -n db/Task.go | sed -n '200,295p'printf'%s\n'''printf'%s\n''--- db/TaskParams and schedule task creation ---'
cat -n db/TaskParams.go | sed -n '1,220p'
cat -n db/Schedule.go | sed -n '1,140p'printf'%s\n'''printf'%s\n''--- api/integration.go GetTaskDefinition ---'
cat -n api/integration.go | sed -n '240,320p'printf'%s\n'''printf'%s\n''--- TaskPool validation remainder ---'
cat -n services/tasks/TaskPool.go | sed -n '1028,1080p'printf'%s\n'''printf'%s\n''--- validateSurveyVars implementation/test cases ---'
cat -n db/Task.go | sed -n '220,360p'
cat -n db/Task_test.go | sed -n '1,170p'
Repository: semaphoreui/semaphore
Length of output: 21739
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- db/Task.go ValidateNewTask ---'
rg -n -A80 -B10 'func \(task \*Task\) ValidateNewTask' db/Task.go
cat -n db/Task.go | sed -n '140,205p'printf'%s\n'''printf'%s\n''--- api integration task creation after GetTaskDefinition ---'
cat -n api/integration.go | sed -n '286,350p'printf'%s\n'''printf'%s\n''--- all AddTask call sites with enough context ---'forfin api/projects/tasks.go services/schedules/SchedulePool.go api/integration.go;doprintf'%s\n''FILE: '"$f"
rg -n -C3 'AddTask\('"$f" --glob '*.go'||truedone
Repository: semaphoreui/semaphore
Length of output: 8366
Enforce survey-var validation at the task-pool entry point.
ValidateSurveyVars is only checked by the direct REST task handler before calling TaskPool.AddTask; TaskPool.AddTask only calls ValidateNewTask, which does not validate environment/secret survey variables. Since schedules and integrations also pass tasks through TaskPool.AddTask, any Environment or Secret supplied by webhooks/schedules can bypass this validation. Move the validation into TaskPool.AddTask or validate the same way in every AddTask call site.
🤖 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/projects/tasks.go` around lines 51 - 61, Move the ValidateSurveyVars
check from the direct REST handler into TaskPool.AddTask, using the task’s
associated template before accepting any task. Ensure all AddTask callers,
including schedules and integrations, enforce the same validation while
preserving the existing error propagation behavior.
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject trailing data in parseTaskVars.
json.Decoder.Decode accepts {"app_version":"1.2.3"}garbage without error, only consuming the first object. Since this parser is meant to tighten parsing of Task.Environment / Task.Secret, reject payloads that contain non-whitespace data after the JSON object using dec.More().
🤖 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/Task.go` around lines 201 - 219, Update parseTaskVars to verify that no
non-whitespace data remains after the initial JSON object is decoded. Use the
existing decoder dec and reject trailing content, returning the same validation
error for invalid Task.Environment or Task.Secret payloads while preserving
valid object parsing.
#!/bin/bashset -euo pipefail
echo"== Go files containing SurveyVar/required =="
find . -type f -name '*.go' -not -path './bundle/*' -not -path './frontend/*' -not -path './node_modules/*' -print0 2>/dev/null \
| xargs -0 rg -n '\b(SurveyVar|survey variable|SurveyVars)\b|\brequired\b|ValidateValue|ValidateSurveyVars'||trueechoecho"== db/Template.go lines 100-180 =="
sed -n '100,180p' db/Template.go | cat -n
echoecho"== db/Task.go focused lines =="
wc -l db/Task.go
rg -n -C 12 'ValidateSurveyVars|SurveyVars|SurveyVar' db/Task.go
echoecho"== candidate Go files containing ValidateSurveyVars/ValidateValue/Required in tracked source only =="
git ls-files '*.go'| xargs rg -n 'ValidateSurveyVars|ValidateValue|SurveyVar\.Required|Required' db api ||true
Repository: semaphoreui/semaphore
Length of output: 19723
Enforce required on missing survey vars.
ValidateValue accepts absent/null values, and Task.ValidateSurveyVars only rejects undeclared or mistyped supplied values; SurveyVar.Required is only a persisted field. Add server-side validation at task creation/validation so omitted required vars fail before execution.
🤖 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/Template.go` around lines 130 - 165, Update Task.ValidateSurveyVars to
validate every declared SurveyVar, not only supplied values, and return a
validation error when a variable with Required set is absent or null. Preserve
ValidateValue’s optional-value behavior for non-required variables, while
ensuring required omissions fail during task creation/validation before
execution.
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter inherited secret variables too.
The rerun path filters only v.environment; undeclared keys in v.secret remain in editedSecretEnvironment and are serialized by beforeSave. With allow_any_vars_in_task disabled, the backend rejects those keys during ValidateSurveyVars, so rerunning affected tasks fails.
‼️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 `@web/src/components/TaskForm.vue` around lines 332 - 334, Update the rerun
initialization near editedEnvironment and editedSecretEnvironment so the parsed
v.secret object also passes through filterDeclaredVars before assignment.
Preserve the existing JSON fallback and ensure editedSecretEnvironment contains
only declared variables, preventing beforeSave from serializing undeclared
secrets.
The reason will be displayed to describe this comment to others. Learn more.
Security review
Outcome: No medium/high/critical vulnerabilities found in the added or modified code.
This PR is a security hardening change. It adds backend validation (ValidateSurveyVars) on the primary task-creation API path (POST /project/{id}/tasks) to block undeclared survey variables — including Ansible connection overrides such as ansible_ssh_common_args that can execute code on the Semaphore server.
What was reviewed
Input → sink tracing:Task.Environment / Task.Secret are validated against template.survey_vars before TaskPool.AddTask runs on the user-facing endpoint.
Controls verified: Undeclared keys are rejected by default; secret survey vars must use secret; int/enum types are checked; AllowAnyVarsInTask is opt-in and defaults to false.
Bypass paths checked: Integrations, schedules, and autorun child tasks call TaskPool.AddTask directly without this validation, but those paths only accept environment keys configured by project managers (integration extract mappings / schedule task params), not arbitrary keys from task runners. That is a different trust boundary than the vulnerability this PR fixes.
Other classes: No new injection surfaces, authz bypasses, secret leakage, or dependency changes were introduced.
Residual note (not reported as a finding)
Placing ValidateSurveyVars in the HTTP handler instead of TaskPool.AddTask means future task-creation entry points must remember to call it. Consider centralizing enforcement in TaskPool.AddTask for defense in depth.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
db/Task.go:253
When AllowAnyVarsInTask is enabled, env keys that aren't declared in the survey should be permitted. Currently the env loop returns an undeclared-variable error unconditionally when GetSurveyVar returns nil, so even after removing the early return the allow-any mode would still reject undeclared env keys.
if v == nil {
return undeclaredSurveyVarError(name)
}
db/Task.go:273
When AllowAnyVarsInTask is enabled, undeclared keys in the secret payload should be allowed (they stay encrypted/write-only), but declared non-secret vars should still be rejected if sent via Task.Secret. The current combined condition (v == nil || v.Type != SurveyVarSecret) can't express this distinction.
if v == nil || v.Type != SurveyVarSecret {
return undeclaredSurveyVarError(name)
}
db/Task_test.go:120
This AllowAnyVarsInTask test currently expects replicas:"many" (a declared int survey var) to be accepted. If AllowAnyVarsInTask is meant to relax only the undeclared-key restriction (while still validating declared vars and secret handling), change the test data to keep declared vars valid so the test only exercises the allow-any behavior for undeclared keys.
parseTaskVars currently accepts the JSON literal null as valid because decoding into a map yields a nil map without error. This contradicts the intent/error message ("must be a JSON object") and lets clients bypass variable validation by sending null.
if err := dec.Decode(&res); err != nil {
return nil, common_errors.NewValidationError("task " + field + " must be a JSON object")
}
db/Task_test.go:88
Add a regression test for environment/secret being the JSON literal null. Without this, parseTaskVars can accidentally accept null as a valid object and skip validation.
This issue also appears on line 117 of the same file.
{
name: "environment is not an object",
environment: `["app_version"]`,
errContains: "task environment must be a JSON object",
},
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Bug Fixes
Documentation