Skip to content

fix: fixes the handling of secret survey variables for remote runners and extends their support beyond Ansible jobs#3871

Open
JulianKap wants to merge 1 commit into
semaphoreui:developfrom
JulianKap:develop
Open

fix: fixes the handling of secret survey variables for remote runners and extends their support beyond Ansible jobs#3871
JulianKap wants to merge 1 commit into
semaphoreui:developfrom
JulianKap:develop

Conversation

@JulianKap
Copy link
Copy Markdown
Contributor

Summary

This PR fixes the handling of secret survey variables for remote runners and extends their support beyond Ansible jobs.

Changes

1. Fix secret survey variables propagation to remote runners

Previously, secret survey variables were only available when the server was running in standalone mode (where the server also acted as a runner). In setups using external/remote runners, secret survey variables were not forwarded to the runner, resulting in missing variables during job execution.

2. Pass secret survey variables to non-Ansible jobs

Before this change, secret survey variables were injected only into Ansible jobs. Other job types, such as Terraform jobs, did not receive these variables.

This update extends secret survey variable injection to all supported job types, including Terraform and other non-Ansible tasks.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security review (PR #3871)

Scope: Secret survey variables for remote runners and merging survey secrets into LocalJob extra vars (plus JobData.Secret / runner job enqueue).

Outcome: No medium, high, or critical issues identified in the added or modified code paths.

Why this looks safe (attack-path check):

  • Runner API exposure: Secret is added to JobData only in GetRunner, which is behind RunnerMiddleware (X-Runner-Token) and only includes tasks where task.runner_id matches the authenticated runner. That is the same trust boundary already used for decrypted access keys in the payload.
  • Persistence / accidental API leak: TaskPool.AddTask still copies survey secrets into extraSecretVars, sets taskObj.Secret to "{}" before CreateTask, and keeps the real payload on RemoteJob for remote execution. db.Task.Secret remains db:"-" and is not a stored column; in-memory Task used in JSON should remain the cleared placeholder rather than live survey answers.
  • Merge semantics: maps.Copy(extraVars, extraSecretVars) runs before semaphore_vars / task_details are assigned, so survey JSON cannot strip or replace the structured semaphore_vars block via key overlap.
  • Dependencies / supply chain: No dependency manifest changes in this diff.

Prior automation threads: Cleaned up so this assessment is the active one. Re-evaluated the current diff against typical runner/secrets concerns (IDOR on job pickup, duplicate serialization on db.Task, injection via json.Unmarshal into map[string]any); none rose to a credible medium+ finding with a realistic attacker who is not already a compromised runner or task operator.


Slack summary (copy/paste): PR #3871 security review: clean — no medium+ findings. Changes only move survey secrets to the assigned remote runner over the existing runner-token + runner_id gate, merge JSON into extra vars without clobbering semaphore_vars, and keep DB task rows from storing survey secrets. No new authz bypass or injection sink identified in the diff.

Open in Web View Automation 

Sent by Cursor Automation: Find vulnerabilities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant