From 1f9922a5f5b8a3f067b577d8a0fc7332bb1ad30b Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Thu, 18 Jun 2026 10:58:51 +0200 Subject: [PATCH 01/14] feat(action): post branded PR comments via backend webhook (PPSC-556) Send raw SARIF to the Armis backend (POST /api/v1/webhook/pr-comment) for server-side branded PR comments posted as armis-appsec[bot], instead of formatting comments client-side in the workflow. - action.yml: add pr-comment and api-url inputs plus a Post Branded PR Comment step. Resolves the API base URL (api-url/ARMIS_API_URL -> region -> production, HTTPS-enforced), exchanges client-id/secret for a JWT (Bearer) with Basic api-token fallback, builds the request body with jq, and POSTs the SARIF. Soft-fails everywhere (warn + exit 0) so commenting never fails the build. - reusable-security-scan.yml: delegate commenting to the action; remove the ~130-line github-script formatter block. - docs: document the new inputs, the armis-appsec App requirement, and changelog entries. --- .github/workflows/reusable-security-scan.yml | 198 +------------------ action.yml | 131 ++++++++++++ docs/CHANGELOG.md | 5 + docs/CI-INTEGRATION.md | 9 +- 4 files changed, 148 insertions(+), 195 deletions(-) diff --git a/.github/workflows/reusable-security-scan.yml b/.github/workflows/reusable-security-scan.yml index edb5963e..4aa0b565 100644 --- a/.github/workflows/reusable-security-scan.yml +++ b/.github/workflows/reusable-security-scan.yml @@ -108,6 +108,7 @@ jobs: scan-timeout: ${{ inputs.scan-timeout }} include-files: ${{ inputs.include-files }} build-from-source: ${{ inputs.build-from-source }} + pr-comment: ${{ inputs.pr-comment && github.event_name == 'pull_request' }} continue-on-error: true - name: Ensure SARIF exists @@ -117,199 +118,10 @@ jobs: echo '{"version":"2.1.0","$schema":"https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json","runs":[{"tool":{"driver":{"name":"armis-cli","version":"1.0.0"}},"results":[]}]}' > armis-results.sarif fi - - name: Post PR Comment with Results - if: always() && inputs.pr-comment && github.event_name == 'pull_request' - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 - with: - script: | - const fs = require('fs'); - - // Build a map of {filename: Set} for lines visible in the PR diff. - // These are the only lines GitHub Code Scanning annotates inline (added/context lines). - // Only '+' (added) and ' ' (context) lines are included; '-' (removed) lines are skipped - // because they don't exist in the new file. Lines starting with '\' (e.g. "\ No newline - // at end of file") are also skipped to avoid misaligning the computed line numbers. - function parseDiffLineNumbers(patch) { - const lines = new Set(); - if (!patch) return lines; - let currentLine = 0; - for (const raw of patch.split('\n')) { - const m = raw.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/); - if (m) { currentLine = parseInt(m[1], 10); continue; } - if (currentLine === 0) continue; - if (raw.startsWith('-') || raw.startsWith('\\')) continue; - if (raw.startsWith('+') || raw.startsWith(' ')) { - lines.add(currentLine++); - } - } - return lines; - } - - // Use a null-prototype object to avoid prototype pollution from filenames - // like '__proto__' or 'constructor' that would corrupt a regular object. - // null sentinel means the file is changed but has no patch (e.g. large diffs - // or binary files) — findings for those files are always included so we never - // silently under-report issues on files GitHub couldn't supply a patch for. - const diffLines = Object.create(null); - let page = 1; - while (true) { - const { data: files } = await github.rest.pulls.listFiles({ - ...context.repo, - pull_number: context.issue.number, - per_page: 100, - page, - }); - if (!files.length) break; - for (const f of files) { - diffLines[f.filename] = f.patch ? parseDiffLineNumbers(f.patch) : null; - } - if (files.length < 100) break; - page++; - } - - // Read SARIF results - const sarif = JSON.parse(fs.readFileSync('armis-results.sarif', 'utf8')); - const allResults = sarif.runs?.[0]?.results || []; - - // Keep only findings whose (file, line) falls on a diff-visible line — - // these are the only ones GitHub Code Scanning will annotate inline. - // Findings for files with no patch data (null sentinel) are always kept - // to avoid under-reporting on large or binary files. - const results = allResults.filter(r => { - const file = r.locations?.[0]?.physicalLocation?.artifactLocation?.uri || ''; - const line = r.locations?.[0]?.physicalLocation?.region?.startLine; - if (!file || !line) return false; - if (!(file in diffLines)) return false; - const lineSet = diffLines[file]; - return lineSet === null || lineSet.has(line); - }); - - // Count by severity - read from properties.severity if available - const counts = { CRITICAL: 0, HIGH: 0, MEDIUM: 0, LOW: 0, INFO: 0 }; - for (const r of results) { - // Prefer properties.severity (set by armis-cli), fallback to level mapping - const severity = r.properties?.severity || { - 'error': 'HIGH', - 'warning': 'MEDIUM', - 'note': 'LOW', - 'none': 'INFO' - }[r.level || 'warning'] || 'INFO'; - counts[severity]++; - } - const total = results.length; - - // Build comment body - const marker = ''; - const status = counts.CRITICAL > 0 ? '🔴 CRITICAL issues found' : - counts.HIGH > 0 ? '🟠 HIGH issues found' : - total > 0 ? '🟡 Issues found' : '✅ No issues'; - - const workflowRef = process.env.GITHUB_WORKFLOW_REF || ''; - const ref = workflowRef.split('@')[1] || 'main'; - const logoLight = `https://raw.githubusercontent.com/ArmisSecurity/armis-cli/${ref}/docs/assets/appsec-logo-light.png`; - const logoDark = `https://raw.githubusercontent.com/ArmisSecurity/armis-cli/${ref}/docs/assets/appsec-logo-dark.png`; - // GitHub theme-aware images with height constraint (24px to match heading text) - const logo = `Armis AppSecArmis AppSec`; - - let body = `${marker}\n## ${logo} Security Scan Results\n\n**${status}**\n`; - if (total > 0) { - body += `\n| Severity | Count |\n|----------|-------|\n`; - if (counts.CRITICAL > 0) body += `| 🔴 CRITICAL | ${counts.CRITICAL} |\n`; - if (counts.HIGH > 0) body += `| 🟠 HIGH | ${counts.HIGH} |\n`; - if (counts.MEDIUM > 0) body += `| 🟡 MEDIUM | ${counts.MEDIUM} |\n`; - if (counts.LOW > 0) body += `| 🔵 LOW | ${counts.LOW} |\n`; - if (counts.INFO > 0) body += `| ⚪ INFO | ${counts.INFO} |\n`; - body += `\n**Total: ${total}**\n`; - } - - // Build detailed findings section - if (total > 0) { - body += `\n
View all ${total} findings\n\n`; - - // Group results by severity - const severityOrder = ['CRITICAL', 'HIGH', 'MEDIUM', 'LOW', 'INFO']; - const severityEmoji = { CRITICAL: '🔴', HIGH: '🟠', MEDIUM: '🟡', LOW: '🔵', INFO: '⚪' }; - - for (const severity of severityOrder) { - const severityResults = results.filter(r => - (r.properties?.severity || 'INFO') === severity - ); - - if (severityResults.length > 0) { - body += `### ${severityEmoji[severity]} ${severity} (${severityResults.length})\n\n`; - - for (const r of severityResults) { - const file = r.locations?.[0]?.physicalLocation?.artifactLocation?.uri || ''; - const line = r.locations?.[0]?.physicalLocation?.region?.startLine || ''; - const location = file ? (line ? `${file}:${line}` : file) : 'Unknown location'; - - // Parse title and description from message - const msgParts = (r.message?.text || '').split(': '); - const title = msgParts[0] || r.ruleId; - const description = msgParts.slice(1).join(': ') || ''; - - body += `
${r.ruleId} - ${title}\n\n`; - body += `**Location:** \`${location}\`\n\n`; - - if (description) { - body += `${description}\n\n`; - } - - // Code snippet - const snippet = r.properties?.codeSnippet; - if (snippet) { - body += '```\n' + snippet + '\n```\n\n'; - } - - // CVEs and CWEs - const cves = r.properties?.cves || []; - const cwes = r.properties?.cwes || []; - if (cves.length > 0) { - body += `**CVEs:** ${cves.join(', ')}\n\n`; - } - if (cwes.length > 0) { - body += `**CWEs:** ${cwes.join(', ')}\n\n`; - } - - // Package info - const pkg = r.properties?.package; - const version = r.properties?.version; - const fixVersion = r.properties?.fixVersion; - if (pkg) { - let pkgInfo = `**Package:** ${pkg}`; - if (version) pkgInfo += ` (${version})`; - if (fixVersion) pkgInfo += ` → Fix: ${fixVersion}`; - body += pkgInfo + '\n\n'; - } - - body += `
\n\n`; - } - } - } - - body += `
`; - } - - // Find and update existing comment, or create new - const { data: comments } = await github.rest.issues.listComments({ - ...context.repo, - issue_number: context.issue.number - }); - const existing = comments.find(c => c.body.includes(marker)); - - if (existing) { - await github.rest.issues.updateComment({ - ...context.repo, - comment_id: existing.id, - body - }); - } else { - await github.rest.issues.createComment({ - ...context.repo, - issue_number: context.issue.number, - body - }); - } + # PR commenting now happens inside the Armis action (Post Branded PR Comment + # step), which sends raw SARIF to the backend for server-side branded + # formatting (posts as armis-appsec[bot]). See action.yml. The previous + # ~130-line github-script formatter was removed in PPSC-556. - name: Upload SARIF to GitHub Code Scanning if: always() diff --git a/action.yml b/action.yml index 7de8dafa..fa764d2e 100644 --- a/action.yml +++ b/action.yml @@ -64,6 +64,14 @@ inputs: description: 'Build CLI from source instead of downloading release (for testing)' required: false default: 'false' + pr-comment: + description: 'Post a branded PR comment via the Armis backend (requires the armis-appsec GitHub App installed on the repo). Only runs on pull_request events.' + required: false + default: 'false' + api-url: + description: 'Override the Armis API base URL (advanced; defaults to region-derived or production). Equivalent to ARMIS_API_URL.' + required: false + default: '' outputs: results: @@ -261,3 +269,126 @@ runs: echo "EOF" >> $GITHUB_OUTPUT exit $SCAN_EXIT_CODE + + - name: Post Branded PR Comment + # Only on pull requests when explicitly enabled. Requires the armis-appsec + # GitHub App installed on the target repo (server-side branded comment). + # always() so the comment is still posted when the scan step exits non-zero + # (e.g. findings exceeded --fail-on), matching the prior workflow behavior. + if: always() && inputs.pr-comment == 'true' && github.event_name == 'pull_request' + shell: bash + env: + INPUT_CLIENT_ID: ${{ inputs.client-id }} + INPUT_CLIENT_SECRET: ${{ inputs.client-secret }} + INPUT_API_TOKEN: ${{ inputs.api-token }} + INPUT_REGION: ${{ inputs.region }} + INPUT_API_URL: ${{ inputs.api-url }} + INPUT_OUTPUT_FILE: ${{ inputs.output-file }} + GITHUB_REPOSITORY: ${{ github.repository }} + GITHUB_EVENT_PATH: ${{ github.event_path }} + run: | + # Commenting must NEVER fail the build: soft-fail everywhere (warn + exit 0). + set +e + + warn() { echo "::warning::Armis branded PR comment skipped: $1"; exit 0; } + + # 1. Resolve API base URL: ARMIS_API_URL/api-url override -> region -> production. + # The base URL comes from operator-controlled action config (not PR content); + # we still require HTTPS so the credential sent below can never traverse + # plaintext, mirroring the CLI's NewAuthClient HTTPS enforcement. + # armis:ignore cwe:918 reason:BASE_URL is operator config (api-url/ARMIS_API_URL/region), HTTPS-enforced; not derived from untrusted request data + BASE_URL="${INPUT_API_URL:-${ARMIS_API_URL:-}}" + if [ -z "$BASE_URL" ]; then + if [ -n "$INPUT_REGION" ]; then + if echo "$INPUT_REGION" | grep -Eq '^[a-z0-9]([a-z0-9-]*[a-z0-9])?$'; then + BASE_URL="https://moose.${INPUT_REGION}.armis.com" + else + warn "invalid region '$INPUT_REGION'" + fi + else + BASE_URL="https://moose.armis.com" + fi + fi + BASE_URL="${BASE_URL%/}" + # armis:ignore cwe:918 reason:BASE_URL is operator config (api-url/ARMIS_API_URL/region), HTTPS-enforced here; not derived from untrusted request data + case "$BASE_URL" in + https://*) ;; + *) warn "API base URL must use HTTPS (got '${BASE_URL%%:*}://...')" ;; + esac + + # 2. Locate the SARIF file produced by the scan step. + SARIF_FILE="${INPUT_OUTPUT_FILE:-armis-results.sarif}" + if [ ! -s "$SARIF_FILE" ]; then + warn "SARIF file '$SARIF_FILE' is missing or empty" + fi + if ! jq -e '.runs' "$SARIF_FILE" > /dev/null 2>&1; then + warn "SARIF file '$SARIF_FILE' has no 'runs' key" + fi + + # 3. Resolve repo and PR number (composite actions lack github.event.* context). + REPO="$GITHUB_REPOSITORY" + if ! echo "$REPO" | grep -Eq '^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$'; then + warn "unexpected repository name '$REPO'" + fi + PR_NUMBER=$(jq -r '.pull_request.number // empty' "$GITHUB_EVENT_PATH" 2>/dev/null) + if ! echo "$PR_NUMBER" | grep -Eq '^[1-9][0-9]*$'; then + warn "could not determine pull request number" + fi + + # 4. Build the Authorization header. + # Prefer JWT (exchange client-id/secret); fall back to Basic api-token. + AUTH="" + if [ -n "$INPUT_CLIENT_ID" ] && [ -n "$INPUT_CLIENT_SECRET" ]; then + # Write the credential payload to a private file and send it via @file so the + # client secret never appears on the curl command line (process list / /proc). + TOKEN_REQ_FILE=$(mktemp) + chmod 600 "$TOKEN_REQ_FILE" + if [ -n "$INPUT_REGION" ]; then + jq -n --arg id "$INPUT_CLIENT_ID" --arg secret "$INPUT_CLIENT_SECRET" --arg region "$INPUT_REGION" \ + '{client_id:$id, client_secret:$secret, region:$region}' > "$TOKEN_REQ_FILE" + else + jq -n --arg id "$INPUT_CLIENT_ID" --arg secret "$INPUT_CLIENT_SECRET" \ + '{client_id:$id, client_secret:$secret}' > "$TOKEN_REQ_FILE" + fi + TOKEN_RESP=$(curl -sS --max-time 30 -X POST "${BASE_URL}/api/v1/auth/token" \ + -H 'Content-Type: application/json' --data-binary @"$TOKEN_REQ_FILE") + rm -f "$TOKEN_REQ_FILE" + JWT=$(echo "$TOKEN_RESP" | jq -r '.token // empty' 2>/dev/null) + if [ -z "$JWT" ]; then + warn "JWT token exchange failed (check client-id/client-secret)" + fi + AUTH="Bearer ${JWT}" + elif [ -n "$INPUT_API_TOKEN" ]; then + # Backend compares the value after 'Basic ' verbatim (not base64-encoded), + # matching the CLI's own scheme; base64-encoding would break authentication. + # Sent over HTTPS only (enforced above); never logged. + # armis:ignore cwe:522 reason:Basic scheme requires the raw token in the header per the backend contract (verbatim hmac compare); sent over HTTPS, never echoed + AUTH="Basic ${INPUT_API_TOKEN}" + else + warn "no credentials provided (set client-id/client-secret or api-token)" + fi + + # 5. Build the request payload safely (no string interpolation of SARIF). + if ! jq -n --arg repo "$REPO" --argjson pr "$PR_NUMBER" --slurpfile sarif "$SARIF_FILE" \ + '{repo:$repo, pr_number:$pr, sarif:$sarif[0]}' > pr-comment-payload.json 2>/dev/null; then + warn "failed to build request payload" + fi + + # 6. POST to the branded-comment webhook. + HTTP_CODE=$(curl -sS --max-time 60 -o pr-comment-response.json -w '%{http_code}' \ + -X POST "${BASE_URL}/api/v1/webhook/pr-comment" \ + -H "Authorization: ${AUTH}" \ + -H 'Content-Type: application/json' \ + --data-binary @pr-comment-payload.json) + CURL_EXIT=$? + + if [ "$CURL_EXIT" -ne 0 ]; then + warn "request to backend failed (curl exit $CURL_EXIT)" + fi + if [ "$HTTP_CODE" -lt 200 ] || [ "$HTTP_CODE" -ge 300 ]; then + DETAIL=$(jq -r '.detail // empty' pr-comment-response.json 2>/dev/null) + warn "backend returned HTTP $HTTP_CODE${DETAIL:+ ($DETAIL)}" + fi + + COMMENT_URL=$(jq -r '.comment_url // empty' pr-comment-response.json 2>/dev/null) + echo "Branded PR comment posted: ${COMMENT_URL:-ok}" diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index cf6a1909..63a5fb5c 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,15 +10,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Documentation: an SSO Setup Guide for IT admins (`docs/SSO-SETUP.md`) covering the end-to-end workflow — registering `armis-cli` as an OIDC app in IdP, and deploying to developer machines via MDM. +- GitHub Action: new `pr-comment` input posts a branded PR comment (as `armis-appsec[bot]`) by sending the scan's SARIF to the Armis backend for server-side formatting. Requires the `armis-appsec` GitHub App installed on the repository. Authentication reuses existing `client-id`/`client-secret` (JWT) or `api-token` (Basic) credentials, and commenting failures never fail the build. (PPSC-556) +- GitHub Action: new `api-url` input to override the Armis API base URL (equivalent to `ARMIS_API_URL`). (PPSC-556) ### Changed - `auth setup`: now auto-detects your tenant ID and region from your Armis credentials — the detected tenant seeds the interactive prompt, and the region routes the registration request to the correct regional host. On success the command prints the exact environment variables to deploy to developer machines. +- Reusable security-scan workflow now delegates PR commenting to the Armis action's branded backend flow instead of formatting comments in-workflow, giving consistent branding across the bot-assignment and Action integrations. (PPSC-556) ### Deprecated ### Removed +- Reusable security-scan workflow: removed the ~130-line `actions/github-script` block that formatted PR comments client-side (superseded by server-side branded comments). (PPSC-556) + ### Fixed ### Security diff --git a/docs/CI-INTEGRATION.md b/docs/CI-INTEGRATION.md index 5ca24815..29319776 100644 --- a/docs/CI-INTEGRATION.md +++ b/docs/CI-INTEGRATION.md @@ -144,7 +144,7 @@ jobs: | `scan-type` | string | `repo` | Type of scan: `repo` or `image` | | `scan-target` | string | `.` | Path for repo scan, image name for image scan | | `fail-on` | string | `CRITICAL` | Comma-separated severity levels to fail on (e.g., `HIGH,CRITICAL`). Set to empty string to never fail. | -| `pr-comment` | boolean | `true` | Post scan results as PR comment | +| `pr-comment` | boolean | `true` | Post a branded PR comment (as `armis-appsec[bot]`) via the Armis backend. Requires the [armis-appsec GitHub App](#branded-pr-comments) installed on the repo; otherwise the comment is silently skipped. | | `upload-artifact` | boolean | `true` | Upload SARIF results as artifact | | `artifact-retention-days` | number | `30` | Days to retain artifacts | | `image-tarball` | string | | Path to image tarball (for image scans) | @@ -174,7 +174,7 @@ permissions: #### What You Get -**PR Comments**: Detailed breakdown of findings by severity with expandable details for each issue: +**Branded PR Comments**: A detailed breakdown of findings by severity with expandable details for each issue, posted as `armis-appsec[bot]`: | Severity | Count | |----------|-------| @@ -182,6 +182,9 @@ permissions: | HIGH | 5 | | MEDIUM | 12 | + +> **How it works:** The action sends the raw SARIF results to the Armis backend, which formats and posts the comment server-side (the same formatter used by the `armis-appsec` bot when assigned to a PR). This keeps formatting consistent across integrations and lets it improve without an action release. It requires the **armis-appsec GitHub App** to be installed on the repository — without it the backend cannot post as the bot and the comment is skipped (the scan itself still runs and uploads results). Authentication reuses your existing `client-id`/`client-secret` (JWT) or `api-token` (legacy) credentials; commenting failures never fail the build. + **GitHub Code Scanning**: Findings appear in the Security tab, inline in PR diffs, and as check annotations. **Artifacts**: SARIF results are stored for the configured retention period, enabling historical analysis. @@ -252,6 +255,8 @@ jobs: | `scan-timeout` | No | `60` | Timeout in minutes | | `include-files` | No | | Comma-separated file paths to scan | | `build-from-source` | No | `false` | Build from source (testing) | +| `pr-comment` | No | `false` | Post a branded PR comment (as `armis-appsec[bot]`) via the Armis backend. Only runs on `pull_request` events and requires the [armis-appsec GitHub App](#branded-pr-comments) installed on the repo. | +| `api-url` | No | | Override the Armis API base URL (advanced; defaults to region-derived or production). Equivalent to `ARMIS_API_URL`. | \* One authentication method is required: either `client-id` + `client-secret` (recommended) or `api-token` + `tenant-id` (legacy). From 127a7fa34310f0386318f45edba65389f8cf80b9 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Thu, 18 Jun 2026 11:44:42 +0200 Subject: [PATCH 02/14] fix(action): wire api-url into scan step; correct HTTPS comment (PPSC-556) Address Copilot review on PR #230: - api-url is now exported as ARMIS_API_URL for the scan step, matching its documented 'Equivalent to ARMIS_API_URL' contract (previously the input only reached the PR-comment webhook step). - Correct the PR-comment HTTPS comment: the action enforces HTTPS for all base URLs, which is intentionally stricter than the CLI's NewAuthClient (which permits http://localhost for local dev). --- action.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/action.yml b/action.yml index fa764d2e..36426ea2 100644 --- a/action.yml +++ b/action.yml @@ -174,6 +174,7 @@ runs: INPUT_REGION: ${{ inputs.region }} INPUT_API_TOKEN: ${{ inputs.api-token }} INPUT_TENANT_ID: ${{ inputs.tenant-id }} + INPUT_API_URL: ${{ inputs.api-url }} SCAN_TYPE: ${{ inputs.scan-type }} SCAN_TARGET: ${{ inputs.scan-target }} OUTPUT_FORMAT: ${{ inputs.format }} @@ -193,6 +194,7 @@ runs: if [ -n "$INPUT_REGION" ]; then export ARMIS_REGION="$INPUT_REGION"; fi if [ -n "$INPUT_API_TOKEN" ]; then export ARMIS_API_TOKEN="$INPUT_API_TOKEN"; fi if [ -n "$INPUT_TENANT_ID" ]; then export ARMIS_TENANT_ID="$INPUT_TENANT_ID"; fi + if [ -n "$INPUT_API_URL" ]; then export ARMIS_API_URL="$INPUT_API_URL"; fi # Validate authentication: require JWT (client-id + client-secret) OR Basic (api-token + tenant-id) if [ -n "$ARMIS_CLIENT_ID" ] && [ -z "$ARMIS_CLIENT_SECRET" ]; then @@ -294,8 +296,10 @@ runs: # 1. Resolve API base URL: ARMIS_API_URL/api-url override -> region -> production. # The base URL comes from operator-controlled action config (not PR content); - # we still require HTTPS so the credential sent below can never traverse - # plaintext, mirroring the CLI's NewAuthClient HTTPS enforcement. + # we still require HTTPS for ALL base URLs so the credential sent below can + # never traverse plaintext. This is stricter than the CLI's NewAuthClient, + # which additionally permits http://localhost / http://127.0.0.1 for local + # development — an exception that has no use case in CI. # armis:ignore cwe:918 reason:BASE_URL is operator config (api-url/ARMIS_API_URL/region), HTTPS-enforced; not derived from untrusted request data BASE_URL="${INPUT_API_URL:-${ARMIS_API_URL:-}}" if [ -z "$BASE_URL" ]; then From 01592cbc224793645c359229d8fdecdcbf46e083 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Thu, 18 Jun 2026 12:00:13 +0200 Subject: [PATCH 03/14] fix(action): document SARIF requirement for pr-comment; tidy temp files (PPSC-556) Address Copilot review round 2 on PR #230: - Document that pr-comment requires a persisted SARIF file (keep format: sarif and set output-file); clarify in the input description, the missing-SARIF warning, and the CI-INTEGRATION docs so enabling pr-comment without output-file doesn't appear to silently do nothing. - Write the request payload and backend response to mktemp files with an EXIT trap instead of leaving pr-comment-payload.json / -response.json in the workspace, so the SARIF payload stays private and is cleaned up even on early soft-fail exits (matches the token-request file pattern). --- action.yml | 22 +++++++++++++++------- docs/CI-INTEGRATION.md | 2 +- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/action.yml b/action.yml index 36426ea2..954a0fce 100644 --- a/action.yml +++ b/action.yml @@ -65,7 +65,7 @@ inputs: required: false default: 'false' pr-comment: - description: 'Post a branded PR comment via the Armis backend (requires the armis-appsec GitHub App installed on the repo). Only runs on pull_request events.' + description: 'Post a branded PR comment via the Armis backend (requires the armis-appsec GitHub App installed on the repo). Only runs on pull_request events. Requires a persisted SARIF file: keep the default format (sarif) and set output-file (e.g. armis-results.sarif), otherwise the comment is skipped.' required: false default: 'false' api-url: @@ -294,6 +294,14 @@ runs: warn() { echo "::warning::Armis branded PR comment skipped: $1"; exit 0; } + # Keep the request payload (full SARIF) and response private and transient: + # mktemp + an EXIT trap guarantees cleanup even on an early warn (exit 0), so + # these never linger in the workspace for later steps to pick up. + PAYLOAD_FILE=$(mktemp) + RESPONSE_FILE=$(mktemp) + chmod 600 "$PAYLOAD_FILE" "$RESPONSE_FILE" + trap 'rm -f "$PAYLOAD_FILE" "$RESPONSE_FILE"' EXIT + # 1. Resolve API base URL: ARMIS_API_URL/api-url override -> region -> production. # The base URL comes from operator-controlled action config (not PR content); # we still require HTTPS for ALL base URLs so the credential sent below can @@ -323,7 +331,7 @@ runs: # 2. Locate the SARIF file produced by the scan step. SARIF_FILE="${INPUT_OUTPUT_FILE:-armis-results.sarif}" if [ ! -s "$SARIF_FILE" ]; then - warn "SARIF file '$SARIF_FILE' is missing or empty" + warn "SARIF file '$SARIF_FILE' is missing or empty — pr-comment needs a persisted SARIF file (keep format: sarif and set output-file)" fi if ! jq -e '.runs' "$SARIF_FILE" > /dev/null 2>&1; then warn "SARIF file '$SARIF_FILE' has no 'runs' key" @@ -374,25 +382,25 @@ runs: # 5. Build the request payload safely (no string interpolation of SARIF). if ! jq -n --arg repo "$REPO" --argjson pr "$PR_NUMBER" --slurpfile sarif "$SARIF_FILE" \ - '{repo:$repo, pr_number:$pr, sarif:$sarif[0]}' > pr-comment-payload.json 2>/dev/null; then + '{repo:$repo, pr_number:$pr, sarif:$sarif[0]}' > "$PAYLOAD_FILE" 2>/dev/null; then warn "failed to build request payload" fi # 6. POST to the branded-comment webhook. - HTTP_CODE=$(curl -sS --max-time 60 -o pr-comment-response.json -w '%{http_code}' \ + HTTP_CODE=$(curl -sS --max-time 60 -o "$RESPONSE_FILE" -w '%{http_code}' \ -X POST "${BASE_URL}/api/v1/webhook/pr-comment" \ -H "Authorization: ${AUTH}" \ -H 'Content-Type: application/json' \ - --data-binary @pr-comment-payload.json) + --data-binary @"$PAYLOAD_FILE") CURL_EXIT=$? if [ "$CURL_EXIT" -ne 0 ]; then warn "request to backend failed (curl exit $CURL_EXIT)" fi if [ "$HTTP_CODE" -lt 200 ] || [ "$HTTP_CODE" -ge 300 ]; then - DETAIL=$(jq -r '.detail // empty' pr-comment-response.json 2>/dev/null) + DETAIL=$(jq -r '.detail // empty' "$RESPONSE_FILE" 2>/dev/null) warn "backend returned HTTP $HTTP_CODE${DETAIL:+ ($DETAIL)}" fi - COMMENT_URL=$(jq -r '.comment_url // empty' pr-comment-response.json 2>/dev/null) + COMMENT_URL=$(jq -r '.comment_url // empty' "$RESPONSE_FILE" 2>/dev/null) echo "Branded PR comment posted: ${COMMENT_URL:-ok}" diff --git a/docs/CI-INTEGRATION.md b/docs/CI-INTEGRATION.md index 29319776..8551893a 100644 --- a/docs/CI-INTEGRATION.md +++ b/docs/CI-INTEGRATION.md @@ -255,7 +255,7 @@ jobs: | `scan-timeout` | No | `60` | Timeout in minutes | | `include-files` | No | | Comma-separated file paths to scan | | `build-from-source` | No | `false` | Build from source (testing) | -| `pr-comment` | No | `false` | Post a branded PR comment (as `armis-appsec[bot]`) via the Armis backend. Only runs on `pull_request` events and requires the [armis-appsec GitHub App](#branded-pr-comments) installed on the repo. | +| `pr-comment` | No | `false` | Post a branded PR comment (as `armis-appsec[bot]`) via the Armis backend. Only runs on `pull_request` events and requires the [armis-appsec GitHub App](#branded-pr-comments) installed on the repo. Needs a persisted SARIF file — keep the default `format: sarif` and set `output-file` (e.g. `armis-results.sarif`), or the comment is skipped. | | `api-url` | No | | Override the Armis API base URL (advanced; defaults to region-derived or production). Equivalent to `ARMIS_API_URL`. | \* One authentication method is required: either `client-id` + `client-secret` (recommended) or `api-token` + `tenant-id` (legacy). From 98d7e060167d6c6d365ac59309f3550cb8d2ee97 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 15:42:06 +0300 Subject: [PATCH 04/14] fix(action): escape workflow-cmd chars in warn(); fix region URL mapping; correct fail-on docs (PPSC-556) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - warn(): escape %, CR, LF before ::warning:: to prevent workflow-command injection - Region URL: replace interpolation with explicit allowlist (eu1→eu.moose.armis.com, us1→moose.armis.com) matching internal/auth.RegionalBaseURL — fixes wrong endpoint for eu1 and prevents CWE-918 redirect - docs/CI-INTEGRATION.md: correct two fail-on default entries from CRITICAL to '' (informational mode) --- action.yml | 25 +++++++++++++++++++------ docs/CI-INTEGRATION.md | 4 ++-- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/action.yml b/action.yml index 954a0fce..29f3b59d 100644 --- a/action.yml +++ b/action.yml @@ -292,7 +292,15 @@ runs: # Commenting must NEVER fail the build: soft-fail everywhere (warn + exit 0). set +e - warn() { echo "::warning::Armis branded PR comment skipped: $1"; exit 0; } + # Escape %, CR, and LF before emitting a workflow command so that + # user-supplied input (region, backend detail text) cannot inject + # additional workflow commands into the log stream. + warn() { + local msg + msg=$(printf '%s' "$1" | sed 's/%/%25/g; s/\r/%0D/g; s/\n/%0A/g') + echo "::warning::Armis branded PR comment skipped: ${msg}" + exit 0 + } # Keep the request payload (full SARIF) and response private and transient: # mktemp + an EXIT trap guarantees cleanup even on an early warn (exit 0), so @@ -312,11 +320,16 @@ runs: BASE_URL="${INPUT_API_URL:-${ARMIS_API_URL:-}}" if [ -z "$BASE_URL" ]; then if [ -n "$INPUT_REGION" ]; then - if echo "$INPUT_REGION" | grep -Eq '^[a-z0-9]([a-z0-9-]*[a-z0-9])?$'; then - BASE_URL="https://moose.${INPUT_REGION}.armis.com" - else - warn "invalid region '$INPUT_REGION'" - fi + # Explicit allowlist mirroring internal/auth.RegionalBaseURL. + # Interpolating the region into the hostname would produce wrong + # endpoints (e.g. moose.eu1.armis.com instead of eu.moose.armis.com) + # and would also allow an attacker-controlled region to redirect + # credentials to an arbitrary host (CWE-918). + case "$INPUT_REGION" in + eu1) BASE_URL="https://eu.moose.armis.com" ;; + us1|"") BASE_URL="https://moose.armis.com" ;; + *) warn "unrecognized region '$INPUT_REGION'; supported values: us1, eu1" ;; + esac else BASE_URL="https://moose.armis.com" fi diff --git a/docs/CI-INTEGRATION.md b/docs/CI-INTEGRATION.md index 8551893a..7a28ede4 100644 --- a/docs/CI-INTEGRATION.md +++ b/docs/CI-INTEGRATION.md @@ -143,7 +143,7 @@ jobs: |-------|------|---------|-------------| | `scan-type` | string | `repo` | Type of scan: `repo` or `image` | | `scan-target` | string | `.` | Path for repo scan, image name for image scan | -| `fail-on` | string | `CRITICAL` | Comma-separated severity levels to fail on (e.g., `HIGH,CRITICAL`). Set to empty string to never fail. | +| `fail-on` | string | `''` | Comma-separated severity levels to fail on (e.g., `HIGH,CRITICAL`). Empty string (default) means informational mode — never fail on findings. | | `pr-comment` | boolean | `true` | Post a branded PR comment (as `armis-appsec[bot]`) via the Armis backend. Requires the [armis-appsec GitHub App](#branded-pr-comments) installed on the repo; otherwise the comment is silently skipped. | | `upload-artifact` | boolean | `true` | Upload SARIF results as artifact | | `artifact-retention-days` | number | `30` | Days to retain artifacts | @@ -247,7 +247,7 @@ jobs: | `api-token` | No* | | Armis API token (legacy) | | `tenant-id` | No* | | Tenant identifier (legacy, not needed with JWT) | | `format` | No | `sarif` | Output format: `human`, `json`, `sarif`, `junit` | -| `fail-on` | No | `CRITICAL` | Severity levels to fail on | +| `fail-on` | No | `''` | Severity levels to fail on (empty = informational mode, never fail) | | `exit-code` | No | `1` | Exit code when failing | | `no-progress` | No | `true` | Disable progress indicators | | `image-tarball` | No | | Path to image tarball (image scans) | From 11ea532f8bbc099529701cc8a5124727a22fb7a3 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 15:46:54 +0300 Subject: [PATCH 05/14] fix(action): fall back to primary host for unknown regions (PPSC-556) Mirrors internal/auth.RegionalBaseURL: au1 and any future region codes resolve to moose.armis.com instead of failing with a warning, so branded PR comments are not silently skipped for au1 tenants. --- action.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/action.yml b/action.yml index 29f3b59d..054907a5 100644 --- a/action.yml +++ b/action.yml @@ -325,10 +325,14 @@ runs: # endpoints (e.g. moose.eu1.armis.com instead of eu.moose.armis.com) # and would also allow an attacker-controlled region to redirect # credentials to an arbitrary host (CWE-918). + # Mirroring internal/auth.RegionalBaseURL: only eu1 has a distinct + # data plane; all other valid region codes (us1, au1, future ones) + # fall through to the primary host rather than failing — matching + # the CLI's graceful fallback behavior and avoiding silently + # skipping comments for au1 tenants. case "$INPUT_REGION" in eu1) BASE_URL="https://eu.moose.armis.com" ;; - us1|"") BASE_URL="https://moose.armis.com" ;; - *) warn "unrecognized region '$INPUT_REGION'; supported values: us1, eu1" ;; + *) BASE_URL="https://moose.armis.com" ;; esac else BASE_URL="https://moose.armis.com" From a6f9231fc4fd59c37d5e914cc87ad5a6528e14a2 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 15:52:46 +0300 Subject: [PATCH 06/14] fix(action): use bash substitution for workflow-cmd escaping; also escape '::' (PPSC-556) sed is line-by-line and cannot match embedded \r/\n in a shell string. Switch to bash parameter substitution which operates on the full variable in memory. Also escape '::' to prevent ::error:: and similar patterns in user-supplied messages from injecting additional workflow commands. --- action.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/action.yml b/action.yml index 054907a5..17933b59 100644 --- a/action.yml +++ b/action.yml @@ -295,9 +295,14 @@ runs: # Escape %, CR, and LF before emitting a workflow command so that # user-supplied input (region, backend detail text) cannot inject # additional workflow commands into the log stream. + # bash parameter substitution operates on the full string (unlike sed, + # which is line-by-line and can't match embedded newlines portably). warn() { - local msg - msg=$(printf '%s' "$1" | sed 's/%/%25/g; s/\r/%0D/g; s/\n/%0A/g') + local msg="$1" + msg="${msg//%/%25}" + msg="${msg//$'\r'/%0D}" + msg="${msg//$'\n'/%0A}" + msg="${msg//::/%3A%3A}" echo "::warning::Armis branded PR comment skipped: ${msg}" exit 0 } From cbd7201489f30831ea3cfb259a90e9c0faa45ea8 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 16:00:30 +0300 Subject: [PATCH 07/14] fix(action): portable mktemp templates for BSD/macOS compatibility (PPSC-556) Use explicit XXXXXX templates with ${TMPDIR:-/tmp} prefix for all mktemp calls so they work portably across GNU/Linux and BSD/macOS runners. --- action.yml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/action.yml b/action.yml index 17933b59..47d17c7b 100644 --- a/action.yml +++ b/action.yml @@ -310,8 +310,9 @@ runs: # Keep the request payload (full SARIF) and response private and transient: # mktemp + an EXIT trap guarantees cleanup even on an early warn (exit 0), so # these never linger in the workspace for later steps to pick up. - PAYLOAD_FILE=$(mktemp) - RESPONSE_FILE=$(mktemp) + # armis:ignore cwe:377 reason:explicit XXXXXX template prevents name prediction; chmod 600 + EXIT trap ensure no exposure + PAYLOAD_FILE=$(mktemp "${TMPDIR:-/tmp}/armis-pr-comment.XXXXXX") + RESPONSE_FILE=$(mktemp "${TMPDIR:-/tmp}/armis-pr-response.XXXXXX") chmod 600 "$PAYLOAD_FILE" "$RESPONSE_FILE" trap 'rm -f "$PAYLOAD_FILE" "$RESPONSE_FILE"' EXIT @@ -375,8 +376,10 @@ runs: if [ -n "$INPUT_CLIENT_ID" ] && [ -n "$INPUT_CLIENT_SECRET" ]; then # Write the credential payload to a private file and send it via @file so the # client secret never appears on the curl command line (process list / /proc). - TOKEN_REQ_FILE=$(mktemp) + # armis:ignore cwe:377 reason:explicit XXXXXX template prevents name prediction; chmod 600 + immediate rm ensure no exposure + TOKEN_REQ_FILE=$(mktemp "${TMPDIR:-/tmp}/armis-token-req.XXXXXX") chmod 600 "$TOKEN_REQ_FILE" + # armis:ignore cwe:522 reason:credentials written to a chmod-600 temp file (never on argv) and rm'd immediately after use; this IS the secure pattern to avoid process-list exposure if [ -n "$INPUT_REGION" ]; then jq -n --arg id "$INPUT_CLIENT_ID" --arg secret "$INPUT_CLIENT_SECRET" --arg region "$INPUT_REGION" \ '{client_id:$id, client_secret:$secret, region:$region}' > "$TOKEN_REQ_FILE" From 1b6f4960b694f72aa9a24a64699405238b34bf6e Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 16:07:15 +0300 Subject: [PATCH 08/14] fix(action): preflight jq/curl check; capture token-exchange HTTP status (PPSC-556) - Add command-availability preflight for jq and curl so missing tools give a clear prerequisite error rather than a misleading SARIF-parsing warning - Token exchange now captures curl exit code + HTTP status code (matching the webhook POST pattern), so network errors and non-2xx responses surface the specific problem instead of a generic credential hint - Collapse two-branch jq credential write into one call to reduce attack surface --- action.yml | 41 +++++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/action.yml b/action.yml index 47d17c7b..b16ec5c9 100644 --- a/action.yml +++ b/action.yml @@ -307,6 +307,12 @@ runs: exit 0 } + # Preflight: jq and curl must be available; missing tools should surface as a + # clear prerequisite error rather than a misleading SARIF-parsing warning. + for cmd in jq curl; do + command -v "$cmd" > /dev/null 2>&1 || warn "$cmd is required but not found on this runner" + done + # Keep the request payload (full SARIF) and response private and transient: # mktemp + an EXIT trap guarantees cleanup even on an early warn (exit 0), so # these never linger in the workspace for later steps to pick up. @@ -380,19 +386,34 @@ runs: TOKEN_REQ_FILE=$(mktemp "${TMPDIR:-/tmp}/armis-token-req.XXXXXX") chmod 600 "$TOKEN_REQ_FILE" # armis:ignore cwe:522 reason:credentials written to a chmod-600 temp file (never on argv) and rm'd immediately after use; this IS the secure pattern to avoid process-list exposure - if [ -n "$INPUT_REGION" ]; then - jq -n --arg id "$INPUT_CLIENT_ID" --arg secret "$INPUT_CLIENT_SECRET" --arg region "$INPUT_REGION" \ - '{client_id:$id, client_secret:$secret, region:$region}' > "$TOKEN_REQ_FILE" - else - jq -n --arg id "$INPUT_CLIENT_ID" --arg secret "$INPUT_CLIENT_SECRET" \ - '{client_id:$id, client_secret:$secret}' > "$TOKEN_REQ_FILE" - fi - TOKEN_RESP=$(curl -sS --max-time 30 -X POST "${BASE_URL}/api/v1/auth/token" \ + # armis:ignore cwe:522 reason:jq --arg args are root-only visible in ps and transient; value is written to a chmod-600 temp file, consumed via @file (never on curl argv), and rm'd immediately after use + jq -n \ + --arg id "$INPUT_CLIENT_ID" \ + --arg secret "$INPUT_CLIENT_SECRET" \ + --arg region "${INPUT_REGION:-}" \ + 'if $region != "" then {client_id:$id, client_secret:$secret, region:$region} else {client_id:$id, client_secret:$secret} end' \ + > "$TOKEN_REQ_FILE" + TOKEN_RESP_FILE=$(mktemp "${TMPDIR:-/tmp}/armis-token-resp.XXXXXX") + chmod 600 "$TOKEN_RESP_FILE" + # armis:ignore cwe:377 reason:explicit XXXXXX template; chmod 600 + rm after use + TOKEN_HTTP=$(curl -sS --max-time 30 -o "$TOKEN_RESP_FILE" -w '%{http_code}' \ + -X POST "${BASE_URL}/api/v1/auth/token" \ -H 'Content-Type: application/json' --data-binary @"$TOKEN_REQ_FILE") + TOKEN_CURL_EXIT=$? rm -f "$TOKEN_REQ_FILE" - JWT=$(echo "$TOKEN_RESP" | jq -r '.token // empty' 2>/dev/null) + if [ "$TOKEN_CURL_EXIT" -ne 0 ]; then + rm -f "$TOKEN_RESP_FILE" + warn "JWT token exchange network error (curl exit $TOKEN_CURL_EXIT)" + fi + if [ "$TOKEN_HTTP" -lt 200 ] || [ "$TOKEN_HTTP" -ge 300 ]; then + TOKEN_DETAIL=$(jq -r '.detail // empty' "$TOKEN_RESP_FILE" 2>/dev/null) + rm -f "$TOKEN_RESP_FILE" + warn "JWT token exchange failed HTTP $TOKEN_HTTP${TOKEN_DETAIL:+ ($TOKEN_DETAIL)}" + fi + JWT=$(jq -r '.token // empty' "$TOKEN_RESP_FILE" 2>/dev/null) + rm -f "$TOKEN_RESP_FILE" if [ -z "$JWT" ]; then - warn "JWT token exchange failed (check client-id/client-secret)" + warn "JWT token exchange failed: no token in response (check client-id/client-secret)" fi AUTH="Bearer ${JWT}" elif [ -n "$INPUT_API_TOKEN" ]; then From bd01c45798c96b73ec734c34efda1cd9a1072617 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 16:12:19 +0300 Subject: [PATCH 09/14] fix(action): clarify pr-comment event restriction in docs; sanitize comment_url log output (PPSC-556) - docs/CI-INTEGRATION.md: note that pr-comment only fires on pull_request events - action.yml: strip CR, LF, '::' from comment_url before echoing to prevent workflow-command injection from a backend-controlled response value --- action.yml | 5 +++++ docs/CI-INTEGRATION.md | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/action.yml b/action.yml index b16ec5c9..d0a030d6 100644 --- a/action.yml +++ b/action.yml @@ -448,5 +448,10 @@ runs: warn "backend returned HTTP $HTTP_CODE${DETAIL:+ ($DETAIL)}" fi + # armis:ignore cwe:20 reason:COMMENT_URL is only echoed to log (never used in commands/API calls); CR, LF, and '::' stripped to prevent workflow-command injection COMMENT_URL=$(jq -r '.comment_url // empty' "$RESPONSE_FILE" 2>/dev/null) + # Strip CR, LF, and '::' so a backend-injected newline can't inject workflow commands. + COMMENT_URL="${COMMENT_URL//$'\r'/}" + COMMENT_URL="${COMMENT_URL//$'\n'/}" + COMMENT_URL="${COMMENT_URL//::/%3A%3A}" echo "Branded PR comment posted: ${COMMENT_URL:-ok}" diff --git a/docs/CI-INTEGRATION.md b/docs/CI-INTEGRATION.md index 7a28ede4..c8de9162 100644 --- a/docs/CI-INTEGRATION.md +++ b/docs/CI-INTEGRATION.md @@ -144,7 +144,7 @@ jobs: | `scan-type` | string | `repo` | Type of scan: `repo` or `image` | | `scan-target` | string | `.` | Path for repo scan, image name for image scan | | `fail-on` | string | `''` | Comma-separated severity levels to fail on (e.g., `HIGH,CRITICAL`). Empty string (default) means informational mode — never fail on findings. | -| `pr-comment` | boolean | `true` | Post a branded PR comment (as `armis-appsec[bot]`) via the Armis backend. Requires the [armis-appsec GitHub App](#branded-pr-comments) installed on the repo; otherwise the comment is silently skipped. | +| `pr-comment` | boolean | `true` | Post a branded PR comment (as `armis-appsec[bot]`) via the Armis backend on `pull_request` events. Ignored on push/schedule/other triggers. Requires the [armis-appsec GitHub App](#branded-pr-comments) installed on the repo; otherwise the comment is silently skipped. | | `upload-artifact` | boolean | `true` | Upload SARIF results as artifact | | `artifact-retention-days` | number | `30` | Days to retain artifacts | | `image-tarball` | string | | Path to image tarball (for image scans) | From d3ea01d8999836704020603c41662b5ea0da426d Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 16:24:20 +0300 Subject: [PATCH 10/14] =?UTF-8?q?fix(action):=20correct=20fail-on=20defaul?= =?UTF-8?q?t=20description=20=E2=80=94=20empty=20uses=20CLI=20default=20CR?= =?UTF-8?q?ITICAL=20(PPSC-556)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The action description and docs incorrectly stated that an empty fail-on input enables 'informational mode (never fail)'. In reality, when fail-on is empty the action omits --fail-on entirely, so the CLI falls back to its built-in default (CRITICAL). Correct both places to reflect actual behavior. --- action.yml | 2 +- docs/CI-INTEGRATION.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/action.yml b/action.yml index d0a030d6..e785504e 100644 --- a/action.yml +++ b/action.yml @@ -35,7 +35,7 @@ inputs: required: false default: 'sarif' fail-on: - description: 'Comma-separated severity levels to fail on (e.g., HIGH,CRITICAL). Empty string for informational mode (never fail).' + description: 'Comma-separated severity levels to fail on (e.g., HIGH,CRITICAL). Empty string (default) uses the CLI default (CRITICAL). Pass a value like HIGH,CRITICAL to override, or use the reusable workflow with fail-on: "" for informational mode.' required: false default: '' exit-code: diff --git a/docs/CI-INTEGRATION.md b/docs/CI-INTEGRATION.md index c8de9162..673197b3 100644 --- a/docs/CI-INTEGRATION.md +++ b/docs/CI-INTEGRATION.md @@ -247,7 +247,7 @@ jobs: | `api-token` | No* | | Armis API token (legacy) | | `tenant-id` | No* | | Tenant identifier (legacy, not needed with JWT) | | `format` | No | `sarif` | Output format: `human`, `json`, `sarif`, `junit` | -| `fail-on` | No | `''` | Severity levels to fail on (empty = informational mode, never fail) | +| `fail-on` | No | `''` | Severity levels to fail on. Empty string (default) uses the CLI default (`CRITICAL`). Set explicitly (e.g. `HIGH,CRITICAL`) to override. | | `exit-code` | No | `1` | Exit code when failing | | `no-progress` | No | `true` | Disable progress indicators | | `image-tarball` | No | | Path to image tarball (image scans) | From 8d37e1f69a8a78f1e097667717289fd9ba8716c8 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 16:32:59 +0300 Subject: [PATCH 11/14] fix(action): drop pull-requests:write permission; log actual BASE_URL on HTTPS fail (PPSC-556) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - reusable-security-scan.yml: remove pull-requests:write from top-level and job-level permissions — PR commenting now runs via armis-appsec[bot] backend and never uses the workflow's GITHUB_TOKEN for write access (least privilege) - docs/CI-INTEGRATION.md: remove pull-requests:write from all example permission blocks and add a note explaining the backend App handles commenting - action.yml: log actual BASE_URL in HTTPS enforcement warning so operators can diagnose misconfiguration directly --- .github/workflows/reusable-security-scan.yml | 5 +++-- action.yml | 4 ++-- docs/CI-INTEGRATION.md | 5 ++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/reusable-security-scan.yml b/.github/workflows/reusable-security-scan.yml index 4aa0b565..d7da0102 100644 --- a/.github/workflows/reusable-security-scan.yml +++ b/.github/workflows/reusable-security-scan.yml @@ -67,11 +67,13 @@ on: # Top-level permissions define the maximum permissions available to this workflow. # Job-level permissions further restrict as needed. +# pull-requests: write was removed in PPSC-556: PR commenting is now handled by +# the Armis backend GitHub App (armis-appsec[bot]) and no longer requires the +# workflow's GITHUB_TOKEN to have PR write access. permissions: contents: read security-events: write actions: read - pull-requests: write jobs: security-scan: @@ -82,7 +84,6 @@ jobs: contents: read security-events: write actions: read - pull-requests: write steps: - name: Checkout repository diff --git a/action.yml b/action.yml index e785504e..8c464984 100644 --- a/action.yml +++ b/action.yml @@ -351,10 +351,10 @@ runs: fi fi BASE_URL="${BASE_URL%/}" - # armis:ignore cwe:918 reason:BASE_URL is operator config (api-url/ARMIS_API_URL/region), HTTPS-enforced here; not derived from untrusted request data case "$BASE_URL" in https://*) ;; - *) warn "API base URL must use HTTPS (got '${BASE_URL%%:*}://...')" ;; + # armis:ignore cwe:918 reason:BASE_URL is operator config (api-url/ARMIS_API_URL/region), not attacker-reachable; this case IS the HTTPS guard that prevents SSRF — warn and exit 0 + *) warn "API base URL must use HTTPS (got '${BASE_URL}')" ;; esac # 2. Locate the SARIF file produced by the scan step. diff --git a/docs/CI-INTEGRATION.md b/docs/CI-INTEGRATION.md index 673197b3..5b4ab2c3 100644 --- a/docs/CI-INTEGRATION.md +++ b/docs/CI-INTEGRATION.md @@ -124,7 +124,6 @@ on: permissions: contents: read security-events: write - pull-requests: write jobs: security-scan: @@ -168,8 +167,9 @@ jobs: permissions: contents: read # Read repository content security-events: write # Upload SARIF to Code Scanning - pull-requests: write # Post PR comments actions: read # Access workflow artifacts +# Note: pull-requests: write is NOT required. PR commenting is handled by +# the armis-appsec GitHub App backend — no GITHUB_TOKEN PR write needed. ``` #### What You Get @@ -324,7 +324,6 @@ on: permissions: contents: read security-events: write - pull-requests: write jobs: get-changed-files: From 42c301036bb5c1aaad39c7a1be28d51ddf1ddadd Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 16:38:34 +0300 Subject: [PATCH 12/14] fix(action): use jq $ENV for client secret to avoid argv exposure (PPSC-556) Replace --arg secret with $ENV.INPUT_CLIENT_SECRET so the secret is read from the process environment rather than jq's argument vector. On Linux, /proc/PID/cmdline is readable by the same runner user, making --arg an unnecessary exposure vector; environment variables are not visible there. --- action.yml | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/action.yml b/action.yml index 8c464984..12f10e8d 100644 --- a/action.yml +++ b/action.yml @@ -381,17 +381,19 @@ runs: AUTH="" if [ -n "$INPUT_CLIENT_ID" ] && [ -n "$INPUT_CLIENT_SECRET" ]; then # Write the credential payload to a private file and send it via @file so the - # client secret never appears on the curl command line (process list / /proc). + # client secret never appears on the curl command line. Secret is also kept out + # of jq's argv by using $ENV.INPUT_CLIENT_SECRET (read from process env, not arg). # armis:ignore cwe:377 reason:explicit XXXXXX template prevents name prediction; chmod 600 + immediate rm ensure no exposure TOKEN_REQ_FILE=$(mktemp "${TMPDIR:-/tmp}/armis-token-req.XXXXXX") chmod 600 "$TOKEN_REQ_FILE" - # armis:ignore cwe:522 reason:credentials written to a chmod-600 temp file (never on argv) and rm'd immediately after use; this IS the secure pattern to avoid process-list exposure - # armis:ignore cwe:522 reason:jq --arg args are root-only visible in ps and transient; value is written to a chmod-600 temp file, consumed via @file (never on curl argv), and rm'd immediately after use + # Use $ENV.INPUT_CLIENT_SECRET so the secret is read from the process + # environment (not placed in jq's argv, which is readable via /proc/*/cmdline). + # armis:ignore cwe:522 reason:secret accessed via $ENV (not argv); result written to chmod-600 temp file, consumed via @file, rm'd immediately after use + # armis:ignore cwe:522 reason:chmod-600 temp file; secret via $ENV (not argv); consumed via curl @file (never on cmdline); rm'd immediately after curl jq -n \ --arg id "$INPUT_CLIENT_ID" \ - --arg secret "$INPUT_CLIENT_SECRET" \ --arg region "${INPUT_REGION:-}" \ - 'if $region != "" then {client_id:$id, client_secret:$secret, region:$region} else {client_id:$id, client_secret:$secret} end' \ + 'if $region != "" then {client_id:$id, client_secret:$ENV.INPUT_CLIENT_SECRET, region:$region} else {client_id:$id, client_secret:$ENV.INPUT_CLIENT_SECRET} end' \ > "$TOKEN_REQ_FILE" TOKEN_RESP_FILE=$(mktemp "${TMPDIR:-/tmp}/armis-token-resp.XXXXXX") chmod 600 "$TOKEN_RESP_FILE" From 24e87d1b07ab7434f7e89c371e76caefae0ba3d6 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 16:44:20 +0300 Subject: [PATCH 13/14] fix(action): fall back to ARMIS_REGION env var in pr-comment step (PPSC-556) When inputs.region is empty, use ARMIS_REGION env var as fallback so the pr-comment step uses the same region as the scan step (which already exports ARMIS_REGION). Fixes incorrect base URL and missing region in the JWT token-exchange payload when region is configured via env var. --- action.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/action.yml b/action.yml index 12f10e8d..6e3685d1 100644 --- a/action.yml +++ b/action.yml @@ -330,8 +330,10 @@ runs: # development — an exception that has no use case in CI. # armis:ignore cwe:918 reason:BASE_URL is operator config (api-url/ARMIS_API_URL/region), HTTPS-enforced; not derived from untrusted request data BASE_URL="${INPUT_API_URL:-${ARMIS_API_URL:-}}" + # Fall back to ARMIS_REGION env var when inputs.region is empty, matching CLI env var behavior. + EFFECTIVE_REGION="${INPUT_REGION:-${ARMIS_REGION:-}}" if [ -z "$BASE_URL" ]; then - if [ -n "$INPUT_REGION" ]; then + if [ -n "$EFFECTIVE_REGION" ]; then # Explicit allowlist mirroring internal/auth.RegionalBaseURL. # Interpolating the region into the hostname would produce wrong # endpoints (e.g. moose.eu1.armis.com instead of eu.moose.armis.com) @@ -342,7 +344,7 @@ runs: # fall through to the primary host rather than failing — matching # the CLI's graceful fallback behavior and avoiding silently # skipping comments for au1 tenants. - case "$INPUT_REGION" in + case "$EFFECTIVE_REGION" in eu1) BASE_URL="https://eu.moose.armis.com" ;; *) BASE_URL="https://moose.armis.com" ;; esac @@ -392,7 +394,7 @@ runs: # armis:ignore cwe:522 reason:chmod-600 temp file; secret via $ENV (not argv); consumed via curl @file (never on cmdline); rm'd immediately after curl jq -n \ --arg id "$INPUT_CLIENT_ID" \ - --arg region "${INPUT_REGION:-}" \ + --arg region "${EFFECTIVE_REGION:-}" \ 'if $region != "" then {client_id:$id, client_secret:$ENV.INPUT_CLIENT_SECRET, region:$region} else {client_id:$id, client_secret:$ENV.INPUT_CLIENT_SECRET} end' \ > "$TOKEN_REQ_FILE" TOKEN_RESP_FILE=$(mktemp "${TMPDIR:-/tmp}/armis-token-resp.XXXXXX") From 4a73ffd45d2a2e7dbee6f409b107a7c1afbd590f Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Sun, 12 Jul 2026 16:49:34 +0300 Subject: [PATCH 14/14] fix(action): strip CR/LF from api-token before Authorization header (PPSC-556) A newline in INPUT_API_TOKEN (e.g. from copy/paste) could inject additional HTTP headers into the curl request. Strip carriage return and newline before constructing the Basic authorization header. --- action.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/action.yml b/action.yml index 6e3685d1..dc3a6703 100644 --- a/action.yml +++ b/action.yml @@ -421,11 +421,16 @@ runs: fi AUTH="Bearer ${JWT}" elif [ -n "$INPUT_API_TOKEN" ]; then + # Strip CR/LF to prevent HTTP header injection before constructing the + # Authorization header — a token accidentally containing a newline could + # inject additional headers into the curl request. + SANITIZED_TOKEN="${INPUT_API_TOKEN//$'\r'/}" + SANITIZED_TOKEN="${SANITIZED_TOKEN//$'\n'/}" # Backend compares the value after 'Basic ' verbatim (not base64-encoded), # matching the CLI's own scheme; base64-encoding would break authentication. # Sent over HTTPS only (enforced above); never logged. # armis:ignore cwe:522 reason:Basic scheme requires the raw token in the header per the backend contract (verbatim hmac compare); sent over HTTPS, never echoed - AUTH="Basic ${INPUT_API_TOKEN}" + AUTH="Basic ${SANITIZED_TOKEN}" else warn "no credentials provided (set client-id/client-secret or api-token)" fi