Skip to content

fix(ci): least-privilege workflow permissions + drop a dead string replace — clears 10 CodeQL alerts - #538

Open
rubenvdlinde wants to merge 2 commits into
developmentfrom
fix/codeql-workflow-permissions
Open

fix(ci): least-privilege workflow permissions + drop a dead string replace — clears 10 CodeQL alerts#538
rubenvdlinde wants to merge 2 commits into
developmentfrom
fix/codeql-workflow-permissions

Conversation

@rubenvdlinde

@rubenvdlinde rubenvdlinde commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

What this is, accurately

The CodeQL check-run on development is red with the title
"7 new alerts including 3 high severity security vulnerabilities". That
title materially overstates what is there. Measured distribution of the 10
open alerts on refs/heads/development
:

where rule severity count
.github/workflows/*.yml actions/missing-workflow-permissions medium 9
tests/e2e/workflows/ js/insecure-randomness high 3
production src/ js/identity-replacement medium 1

All three "high severity security vulnerabilities" in the title are
Math.random() in Playwright fixture code.
The only production alert is
medium and is a dead string operation. This is Actions-hardening debt plus one
no-op — not a security incident.

1. Nine workflow alerts

An absent permissions: block does not mean a job was compromised — it means
the job ran with the repository default grant instead of a stated one.

Eight of the nine only call a reusable workflow in ConductionNL/.github, so
the block is not a new grant: the callee's own job already declares the same
set and the effective token is unchanged.

workflow job block why
release-beta / release-development / release-stable release contents: write cuts the tag, uploads the asset; App Store uses NEXTCLOUD_APPSTORE_TOKEN
sync-to-beta sync contents: write, pull-requests: write callee runs gh pr create
issue-triage triage issues: write, contents: read all three callee jobs
openspec-sync sync issues: write, contents: read callee sync
documentation deploy contents: write, packages: write union of build / deploy / image
branch-protection protect {} the callee is one bash string comparison — no checkout, no network, no API call
code-quality quality contents: write, actions: write, issues: write, pull-requests: write, packages: read see below

code-quality is the one that could have broken

Most jobs in the shared quality.yml declare no permissions of their own, so
they inherit the caller ceiling exactly. Every entry is load-bearing:
contents: write for the journeydoc / baseline git push, actions: write for
the re-dispatch, issues + pull-requests: write for the Quality Report
comment (issues.createComment, which 403s under read-only — quality.yml
documents this at its own step), packages: read for pulling org images.

The block is copied verbatim from openconnector, where it is live on
development with ~30 quality jobs green. A measured ceiling, not a guess.

A caller block is a CEILING, not a grant. GitHub validates the callee's
declared job permissions against it — including for jobs an if: will skip — so
tightening one to read makes the call fail to start with zero jobs and no
annotations
, which is quieter than the red it replaces.

packages: write on documentation is load-bearing at runtime as well: the
callee's build-image input is default: true and no caller overrides it, so
the image job really does run on a push to documentation and pushes to GHCR.
(An earlier revision of this PR's comments claimed otherwise — corrected in the
second commit.)

2. The single production alert — a dead operation, deleted

js/identity-replacement, medium, src/views/Dashboard.vue:496:
.replace(',', ',')"this replaces ',' with itself."

Before deleting it I checked what it was meant to do, because a dead
operation is often a bug in disguise:

  • It was born dead. git log -S puts it in 5c33f0b ("Working on the detail
    pages") in exactly this identical form. It has never done anything, so no
    working behaviour was lost and there is no recorded intent to recover.
  • The lookalike bug is not present. formatDate calls
    toLocaleDateString while passing hour/minute, which resembles the
    "the time is silently dropped" trap. It is not: ECMA-402 supplies date-part
    defaults only when no options are given, so explicit hour/minute are
    honoured. Measured — the function returns 17/08/2026, 08:33, comma and time
    both present.

So the comma the call targets does exist, and replacing it with itself is a true
no-op. Deleting it is output-preserving, verified against that string.
Guessing at .replace(',', '') would have invented a UI change nothing asked
for, so it was not done.

3. The three test alerts — not fixed here, dismissed with a reason

js/insecure-randomness (high) at crud-persistence.spec.ts:115,
organisatie-crud.spec.ts:69, org-export-workflow.spec.ts:75. All three trace
to one source, tests/e2e/workflows/_fixtures.ts:40:

export const RUN_ID = `e2e-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 6)}`

RUN_ID is a collision-avoidance suffix for Playwright fixture object names
in a disposable test database. It is not a token, secret, password, session id or
nonce, and nothing authenticates or authorises against it. Math.random() is the
right tool for the job it is doing. Three alerts, one source, three sinks.

Dismissed as used in tests with the reasoning recorded on each alert. The
rule is left enabled
— no blanket suppression.

Verification

  • All 11 workflows parse (yaml.safe_load).
  • Job-level sweep: 0 jobs without a permissions: block.
  • Positive control — the same sweep on the pre-change development tree
    reports 9, the same 9 job names CodeQL flagged. The check can say NO.

Not in scope, but found while measuring

quality / E2E Tests (Playwright) is red on development for an unrelated and
real product bug — the Standards index page is configured against register
voorzieningen + schema element, but element is attached to the vng-gemma
register. Filed separately; this PR does not touch it.

Conduction Release Bot added 2 commits August 17, 2026 06:46
…place

Clears all 10 open CodeQL alerts on `development`: 9 workflow-hardening findings
and 1 dead no-op in production code. Neither category is a vulnerability that
was exploitable, and the check-run title ("7 new alerts including 3 high
severity security vulnerabilities") overstates both. All three "high severity"
alerts are `js/insecure-randomness` in Playwright fixtures; they are handled by
dismissal, not by this commit.

1. Nine `actions/missing-workflow-permissions`, all MEDIUM, all in
   `.github/workflows/`. An absent `permissions:` block means the job runs with
   the repository default rather than a stated grant.

   Eight of the nine only CALL a reusable workflow in ConductionNL/.github, so
   the block restates what the callee's own job already declares and the
   effective token is unchanged:

     release-beta / release-development / release-stable   contents: write
     sync-to-beta          contents: write + pull-requests: write
     issue-triage          issues: write + contents: read
     openspec-sync         issues: write + contents: read
     documentation         contents: write + packages: write (UNION of the
                           callee's build / deploy / image jobs)
     branch-protection     {} — the callee is one bash string comparison with
                           no checkout, no network and no API call

   code-quality is the exception and the only risky one. Most jobs in the shared
   quality pipeline declare no permissions of their own, so they inherit the
   caller ceiling exactly. The block used is copied verbatim from openconnector,
   where it is live on `development` with ~30 quality jobs green — a measured
   ceiling, not a guess.

   A caller block is a CEILING, not a grant: GitHub validates the callee's
   declared job permissions against it, including for jobs an `if:` will skip,
   so tightening one to `read` makes the call fail to START with zero jobs.

2. One `js/identity-replacement` (MEDIUM) at src/views/Dashboard.vue:496 —
   `.replace(',', ',')`, replacing the comma with itself.

   It was born in that identical form in 5c33f0b ("Working on the detail
   pages"), so it never worked and no intent is recorded to recover. Deleting it
   is output-preserving: `formatDate` still returns `17/08/2026, 08:33`,
   verified against the actual string. Guessing at `.replace(',', '')` would
   have invented a UI change nothing asked for.

   Checked and ruled out while here: `toLocaleDateString` with explicit
   `hour`/`minute` options DOES emit the time (ECMA-402 supplies date-part
   defaults only when none are given), so this was not the "the time is silently
   missing" bug it resembles. Measured, not assumed.

Verified: all 11 workflows parse, and a job-level sweep reports 0 jobs without a
block, against 9 before the change — the same 9 CodeQL names.
…image defaults to true

The comment claimed `packages: write` was needed only because GitHub statically
validates a callee's declared job permissions, and that the `image` job "never
runs" here. That is wrong on the second half.

`build-image` in ConductionNL/.github/.github/workflows/documentation.yml is
`type: boolean, default: true`, and none of the callers pass it. So the `image`
job DOES run on a push to `documentation`, and it really does `docker buildx`
push to GHCR. `packages: write` is load-bearing at RUNTIME, not merely
statically — dropping it would 403 that push.

Comment only; the permissions block itself is unchanged and was already correct.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ c117fe2

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
composer ✅ 130/130
npm ✅ 704/704
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-17 04:56 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ d222836

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
composer ✅ 130/130
npm ✅ 704/704
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-17 05:13 UTC

Download the full PDF report from the workflow artifacts.

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