Repository navigation
fix(ci): resolve the open code-scanning alerts on development - #1196
Conversation
The 17 alerts GitHub Advanced Security reports on development (and on the development -> beta release PR #834): - actions/missing-workflow-permissions (12): the repository's default token is write-all. Every flagged workflow now declares its permissions: contents: read for the self-contained ones, contents: write only on the cli-release jobs that attach to a release, and for callers of the shared ConductionNL/.github workflows exactly the union the called jobs declare (release, sync-to-beta: contents + pull-requests write; documentation: contents + packages write; issue-triage, openspec-sync: issues write). - js/polynomial-redos (sdk/js): the trailing-slash strip of the caller's address backtracked quadratically on a long run of slashes; replaced by a loop, with a test for an address with trailing slashes. - js/incomplete-url-substring-sanitization (3, extension tests): the assertions compared URL prefixes, which also matched other hosts; they now compare the parsed origin. - js/insecure-randomness (e2e helper): the run-unique user name suffix uses crypto.randomInt instead of Math.random. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Quality Report — ConductionNL/keepiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 114/114 | |||
| npm | ✅ | ✅ 660/660 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ✅ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-10-06 13:44 UTC
Download the full PDF report from the workflow artifacts.
rjzondervan
left a comment
There was a problem hiding this comment.
Strict review (own PR, so COMMENT): no blockers, ready to merge. All 17 alerts are addressed. Every permissions grant equals the union of what the called jobs declare, checked against the shared ConductionNL/.github@main workflows; none of those makes nested calls, and image keeps packages: write. stripTrailingSlashes matches the old regex for every input and is linear, the origin comparisons are stricter than startsWith, and randomInt(1296) keeps the same range. The image and build-and-load failures are pre-existing (they fail on every recent development run) and are not required. Two 🟢 notes are below; they are optional.
| needs: test | ||
| runs-on: ubuntu-latest | ||
| # Attaches the binary to the GitHub release on a cli-v tag. | ||
| permissions: |
There was a problem hiding this comment.
🟢 Minor: build holds contents: write on every run, but only a tag run needs it
Only a cli-v tag run attaches to the release (startsWith(github.ref, 'refs/tags/cli-v')). The job also runs on branch pushes and same-repo pull_request, and it still gets a write token there. actions/checkout keeps that token in .git/config (persist-credentials defaults to true). The risk is low: go build runs no repo code, fork PRs get a read-only token, and it's already a big improvement on write-all. The same applies to checksums.
Suggest moving the two "Attach to release" steps into their own job with if: startsWith(github.ref, 'refs/tags/cli-v'), needs: [build, checksums] and contents: write, so that build and checksums drop back to read. At minimum, set persist-credentials: false on the checkout here.
| jobs: | ||
| sync: | ||
| uses: ConductionNL/.github/.github/workflows/openspec-sync.yml@main | ||
| # What the shared workflow's jobs declare: they label and assign issues. |
There was a problem hiding this comment.
🟢 Minor: the comment describes issue-triage, not openspec-sync
The shared openspec-sync sync job creates and updates issues and creates labels (issues.create, issues.update, issues.createLabel). It doesn't assign anything. The scopes are correct, only the wording is off. Suggest: # What the shared workflow's job declares: it creates and updates the OpenSpec issues and their labels.
Resolves the 17 code-scanning alerts that GitHub Advanced Security reports on
development, which also show up on the development → beta release PR #834.What changed
actions/missing-workflow-permissions(12, medium). The repository's defaultGITHUB_TOKENis write-all, and these workflows set no limit. Each one now declares its permissions:contents: readcontents: read; thebuildandchecksumsjobs, which attach to a release on acli-vtag,contents: write;imagekeeps its own blockcontents: write,pull-requests: writecontents: write,packages: writecontents: read,issues: writeA caller's block caps what a called reusable workflow gets. Every job in the shared
ConductionNL/.githubworkflows declares its own permissions, and each caller now grants exactly the union of those, so nothing gets less than it uses today. Static checks can't fully prove that: the release workflow is first exercised by the next release. If that fails on a permissions error, the block inrelease.ymlis the first place to look.js/polynomial-redos(sdk/js/src/client.ts, high). Stripping trailing slashes from the caller's address with/\/+$/backtracks quadratically on a long run of slashes. It's now a plain loop. A new test uses an address with trailing slashes and fails without the fix.js/incomplete-url-substring-sanitization(3, extension tests). These were test assertions, not sanitization, butstartsWith('https://cloud6.example')would also matchhttps://cloud6.example.evil. They now compare the parsedorigin.js/insecure-randomness(tests/e2e/workflows/_vault-users.ts). The suffix that keeps test usernames unique usescrypto.randomIntinstead ofMath.random.Tests
accounts.spec.jsandunlockAccounts.spec.jspass (43/43).The alerts should close when CodeQL next runs on
developmentafter this merges.AI disclosure: implemented with Claude Code; reviewed by the author.
🤖 Generated with Claude Code