Skip to content

fix: prevent script injection in analysis workflow#327

Merged
driessamyn merged 2 commits into
mainfrom
fix/ci-fork-pr-analysis
Mar 28, 2026
Merged

fix: prevent script injection in analysis workflow#327
driessamyn merged 2 commits into
mainfrom
fix/ci-fork-pr-analysis

Conversation

@driessamyn
Copy link
Copy Markdown
Owner

@driessamyn driessamyn commented Mar 28, 2026

Summary

  • Move user-controllable workflow_run expressions (head_branch, head_sha, PR number) to environment variables instead of interpolating them directly in run blocks
  • Fixes SonarCloud blocker S7630 — script injection via crafted branch names

Context

The analysis.yml workflow introduced in #324 interpolated github.event.workflow_run.head_branch directly in a shell script. An attacker could craft a branch name with shell metacharacters to inject arbitrary commands. The fix passes all user-controllable values through env: instead of ${{ }} in run: blocks.

Test plan

  • Verify analysis workflow YAML is valid
  • Verify SonarCloud blocker is resolved after merge

Summary by CodeRabbit

  • Chores
    • Refined GitHub Actions workflow configuration to enhance security practices and maintainability of automated code analysis and coverage reporting processes.

Move user-controllable workflow_run expressions (head_branch, head_sha,
pull request number) to environment variables instead of interpolating
them directly in run blocks. Fixes SonarCloud blocker S7630.
Move checks, pull-requests, and actions permissions from workflow level
to the specific jobs that need them. Follows principle of least
privilege. Fixes SonarCloud S8233 and S8264.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 28, 2026

Code Coverage

Metric Coverage
Line N/A
Branch N/A
Instruction N/A

Updated for commit 4bc0210

@github-actions
Copy link
Copy Markdown

Unit Tests

 62 files  ±0   62 suites  ±0   3m 11s ⏱️ +12s
562 tests ±0  562 ✅ ±0  0 💤 ±0  0 ❌ ±0 
578 runs  ±0  578 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4bc0210. ± Comparison against base commit c62a51f.

This pull request removes 39 and adds 39 tests. Note that renamed tests count towards both.
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [10] LONGVARCHAR, "LONGVARCHAR", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1130/0x00007f42e4580880@2874fe6
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [11] NCHAR, "NCHAR", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1131/0x00007f42e4580aa0@1a4385d5
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [12] INSTANT, net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$972/0x00007f42e4543b28@d38d3a9
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [12] NCLOB, "NCLOB", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1132/0x00007f42e4580cc0@62c57692
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [13] DATE, net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$973/0x00007f42e4543d48@46b64e11
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [13] NVARCHAR, "NVARCHAR", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1133/0x00007f42e4580ee0@b584172
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [14] LOCALDATE, net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$974/0x00007f42e4542800@46cc8b4f
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [14] ROWID, "ROWID", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1134/0x00007f42e4581100@28f35984
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [15] LOCALDATETIME, net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$975/0x00007f42e4542a20@5fc9630a
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [15] SQLXML, "SQLXML", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1135/0x00007f42e4581320@23a2eb4b
…
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [10] LONGVARCHAR, "LONGVARCHAR", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1104/0x00007f51d458b220@11f60824
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [11] NCHAR, "NCHAR", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1105/0x00007f51d458b440@64f3aa0c
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [12] INSTANT, net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$947/0x00007f51d4557a08@61430552
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [12] NCLOB, "NCLOB", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1106/0x00007f51d458b660@7ab98f5
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [13] DATE, net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$948/0x00007f51d4557c28@2c39df8
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [13] NVARCHAR, "NVARCHAR", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1107/0x00007f51d458b880@682e33b5
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [14] LOCALDATE, net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$949/0x00007f51d4555000@444656b5
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [14] ROWID, "ROWID", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1108/0x00007f51d458baa0@28d911c8
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [15] LOCALDATETIME, net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$950/0x00007f51d4555220@38900301
net.samyn.kapper.internal.automapper.SQLTypesConverterTest ‑ [15] SQLXML, "SQLXML", net.samyn.kapper.internal.automapper.SQLTypesConverterTest$Companion$$Lambda$1109/0x00007f51d458bcc0@59682c75
…

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 28, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 35a40a22-5483-4894-bf4c-79c9fa6bf1b8

📥 Commits

Reviewing files that changed from the base of the PR and between c62a51f and 4bc0210.

📒 Files selected for processing (1)
  • .github/workflows/analysis.yml

📝 Walkthrough

Walkthrough

GitHub Actions workflow refactored to move permissions from top-level to job-level scope. Environment variables introduced to reference GitHub event workflow data. Updated Sonar and coverage steps to use these variables for PR metadata and API calls instead of inline expressions.

Changes

Cohort / File(s) Summary
Workflow Permissions & Environment Variables
.github/workflows/analysis.yml
Relocated top-level permissions to job-scoped permissions (sonar: checks: write, actions: read; coverage-comment: pull-requests: write, actions: read). Introduced environment variables (WR_PR_NUMBER, WR_HEAD_SHA, WR_HEAD_BRANCH, WR_BASE_REF) to reference github.event.workflow_run data. Updated Sonar PR invocation to use WR_HEAD_BRANCH and WR_BASE_REF, and coverage-comment step to use WR_HEAD_SHA and new REPO env var in gh api endpoints.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐰 Permissions now tightly scoped, not spread so wide,
Environment variables guide us with measured pride,
Workflow steps reference cleanly, no more inline speech,
GitHub actions dance with grace, each variable in reach! 🎭

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ci-fork-pr-analysis

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

DUCKDB Integration Tests

12 files  ±0  12 suites  ±0   24s ⏱️ ±0s
55 tests ±0  55 ✅ ±0  0 💤 ±0  0 ❌ ±0 
85 runs  ±0  85 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4bc0210. ± Comparison against base commit c62a51f.

@github-actions
Copy link
Copy Markdown

POSTGRESQL Integration Tests

12 files  ±0  12 suites  ±0   25s ⏱️ +3s
55 tests ±0  55 ✅ ±0  0 💤 ±0  0 ❌ ±0 
85 runs  ±0  85 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4bc0210. ± Comparison against base commit c62a51f.

@github-actions
Copy link
Copy Markdown

SQLITE Integration Tests

12 files  ±0  12 suites  ±0   32s ⏱️ +7s
55 tests ±0  50 ✅ ±0  5 💤 ±0  0 ❌ ±0 
85 runs  ±0  80 ✅ ±0  5 💤 ±0  0 ❌ ±0 

Results for commit 4bc0210. ± Comparison against base commit c62a51f.

@github-actions
Copy link
Copy Markdown

MSSQLSERVER Integration Tests

12 files  ±0  12 suites  ±0   42s ⏱️ ±0s
55 tests ±0  46 ✅ ±0  9 💤 ±0  0 ❌ ±0 
85 runs  ±0  76 ✅ ±0  9 💤 ±0  0 ❌ ±0 

Results for commit 4bc0210. ± Comparison against base commit c62a51f.

@github-actions
Copy link
Copy Markdown

MYSQL Integration Tests

12 files  ±0  12 suites  ±0   46s ⏱️ -2s
55 tests ±0  46 ✅ ±0  9 💤 ±0  0 ❌ ±0 
85 runs  ±0  76 ✅ ±0  9 💤 ±0  0 ❌ ±0 

Results for commit 4bc0210. ± Comparison against base commit c62a51f.

@github-actions
Copy link
Copy Markdown

ORACLE Integration Tests

12 files  ±0  12 suites  ±0   1m 11s ⏱️ -3s
55 tests ±0  46 ✅ ±0  9 💤 ±0  0 ❌ ±0 
85 runs  ±0  76 ✅ ±0  9 💤 ±0  0 ❌ ±0 

Results for commit 4bc0210. ± Comparison against base commit c62a51f.

@driessamyn driessamyn merged commit 977e48c into main Mar 28, 2026
18 of 19 checks passed
@driessamyn driessamyn deleted the fix/ci-fork-pr-analysis branch March 28, 2026 12:43
@sonarqubecloud
Copy link
Copy Markdown

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