From ae4d7b663c23dc58bb985eda68d7254da8e745d0 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Sat, 23 May 2026 06:51:44 +0200 Subject: [PATCH 1/2] fix: address unresolved review comments from merged PRs #15 + #16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both PRs were merged with bot-reviewer (copilot + gemini) threads still in `unresolved` state. Audit triggered by user feedback. The two HIGH- severity threads were genuine bugs in shipped code: PR #16 — Dockerfile token leak via `set -x` (HIGH) ================================================== The composer install RUN had `set -eux` (xtrace ON), which causes the shell to echo every executed command — including `export COMPOSER_AUTH="..."${GH_TOKEN}"..."` with $GH_TOKEN expanded to the literal token value. The token landed in the build log, defeating the BuildKit-secret pattern's entire purpose. Flagged by both copilot-pull-request-reviewer and gemini-code-assist (the latter labelled it "security-high"). Fix: - Drop `x` from the RUN's initial `set -eu` (xtrace off by default). - Explicit `set +x` around secret read + export (belt-and-suspenders). - `unset GH_TOKEN` after exporting COMPOSER_AUTH (drop the bare variable from the environment). - Restore `set -x` after secret handling so the rest of the build still echoes for diagnostics. Locally verified — `docker buildx build --progress plain` no longer shows the token value in stderr. PR #16 — pull_request supply-chain exposure (HIGH) ================================================== `secrets: GH_TOKEN=${{ secrets.GITHUB_TOKEN }}` was unconditional, so a hostile PR (even from a same-repo branch, since `pull_request` doesn't need fork-status to consume secrets in this configuration) could modify the Dockerfile to exfiltrate the token via the secret mount. Fix: gate the secret on `github.event_name != 'pull_request'`. PR-event builds fall back to anonymous github.com — fine because PR runs are amd64-only and don't fan a track×composer matrix, so the 60-req/h limit holds. PR #15 — matrix.exclude verbatim-repeats the track object (MEDIUM) ================================================================== The `exclude:` entry duplicated `{name: tag, ref_source: file, tags_extra: ''}` verbatim from the `track:` definition. Adding/changing a field on the tag track would silently desync the exclude and the `tag/rolling` cell would start running again. Fix: use a YAML anchor `&tag-track` on the track definition and reference `*tag-track` in exclude. Verified via `python -c 'yaml.safe_load(...)'` that both resolve to the same object. PR #15 — upstream issue link too generic (LOW) ============================================== Linked to https://github.com/grokability/snipe-it/issues (top-level listing) rather than a specific issue or filter. Fix: link to a filtered search URL scoped to symfony/dom-crawler. Also note the closest related (closed) upstream ticket #19054 even though it's not about this exact advisory. All four threads have been replied-to + resolved on the original PRs pointing at this follow-up commit. Signed-off-by: Sebastian Mendel --- .github/workflows/build.yml | 41 ++++++++++++++++++++++++++----------- Dockerfile | 15 +++++++++++--- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index ad3833a..69ecad8 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -66,7 +66,14 @@ jobs: matrix: track: # ─ stable release line — reads .snipe-it-version (Renovate-managed) ─ - - name: tag + # YAML anchor lets the matrix.exclude entry reference this exact + # object, so adding/changing a field here automatically keeps the + # exclude in sync. Without the anchor, the exclude block had to + # repeat all 3 properties verbatim and would silently stop + # excluding the intended cell if any of them drifted. Bot-reviewer- + # flagged on PR netresearch/snipe-it-docker-compose-stack#15. + - &tag-track + name: tag ref_source: file # build.yml will read .snipe-it-version tags_extra: '' # plus the auto-generated 8.5.0 / 8.5 / 8 / latest tags # ─ branch tracker: master (= last released code, may include @@ -92,16 +99,16 @@ jobs: # PKSA-5r1g-c7b7-y1zg. No version satisfies the constraint, so # `composer install` exits 100. The only fix is upstream: # snipe-it must relax to `^5.0` (or bump the lock for the - # remediated 4.4.46+ once Symfony publishes it). Tracked at - # https://github.com/grokability/snipe-it/issues ; until then, - # the cell is excluded so CI is genuinely green. master/rolling - # and develop/rolling still exercise the rolling-deps path - # against their HEAD constraints, which is where any new - # transitive CVE would surface first. - - track: - name: tag - ref_source: file - tags_extra: '' + # remediated 4.4.46+ once Symfony publishes it). + # No specific upstream issue exists yet — filtered search: + # https://github.com/grokability/snipe-it/issues?q=is%3Aissue+symfony%2Fdom-crawler + # (related but distinct: closed #19054 was about composer + # dependencies after the 8.5 upgrade, not this advisory). + # Until then the cell is excluded so CI is genuinely green; + # master/rolling and develop/rolling still exercise the + # rolling-deps path against their HEAD constraints, which is + # where any new transitive CVE would surface first. + - track: *tag-track composer: rolling steps: - name: Filter — should this track run? @@ -288,8 +295,18 @@ jobs: # parallel matrix cells share the anonymous 60-req/h rate limit and # randomly fail on "Could not authenticate against github.com". # GITHUB_TOKEN is scoped to this job and expires when the job ends. + # + # Empty on pull_request events: a hostile PR could otherwise modify + # the Dockerfile to exfiltrate the token via the secret mount. + # `pull_request` (not `pull_request_target`) does limit secrets to + # same-repo branches by default, but explicit gating closes the + # inside-attacker path too. Bot-reviewer-flagged on PR + # netresearch/snipe-it-docker-compose-stack#16. + # The Dockerfile's empty-token fallback (anonymous github.com, + # 60 req/h) is enough for PR-event smoke builds since they only + # build amd64 and don't fan a track×composer matrix. secrets: | - GH_TOKEN=${{ secrets.GITHUB_TOKEN }} + GH_TOKEN=${{ github.event_name != 'pull_request' && secrets.GITHUB_TOKEN || '' }} provenance: mode=max sbom: true cache-from: type=gha,scope=${{ matrix.track.name }}-${{ matrix.composer }} diff --git a/Dockerfile b/Dockerfile index d211d58..7df0c46 100644 --- a/Dockerfile +++ b/Dockerfile @@ -69,14 +69,23 @@ RUN set -eux; \ # any image layer. RUN --mount=type=cache,target=/root/.composer/cache \ --mount=type=secret,id=GH_TOKEN \ - set -eux; \ + set -eu; \ if [ -s /run/secrets/GH_TOKEN ]; then \ - # Split declare + export so shellcheck SC2155 doesn't fire — \ - # otherwise cat's exit code would be masked by export. \ + # Token handling kept OUT of xtrace (set -x). `set +x` here is \ + # belt-and-suspenders even though `set -eu` (no x) is the outer \ + # default; once secret handling is done we re-enable xtrace for \ + # the rest of the build. Without this, the export line would \ + # be echoed with the token value expanded into the build log — \ + # bot-reviewer-flagged on PR netresearch/snipe-it-docker-compose-stack#16 \ + # (copilot + gemini, HIGH severity). \ + set +x; \ GH_TOKEN=$(cat /run/secrets/GH_TOKEN); \ export COMPOSER_AUTH="{\"github-oauth\":{\"github.com\":\"${GH_TOKEN}\"}}"; \ + unset GH_TOKEN; \ + set -x; \ echo "[composer] github.com authenticated via BuildKit secret"; \ else \ + set -x; \ echo "[composer] no GH_TOKEN secret available — falling back to anonymous github.com (60 req/h)"; \ fi; \ if [ "${ROLLING_DEPS}" = "true" ]; then \ From 4a8895edfb49cfab37723231e00c5a912957fc20 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Sat, 23 May 2026 07:33:46 +0200 Subject: [PATCH 2/2] review(PR-17): address gemini-code-assist medium-priority feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gemini-code-assist left three medium-priority comments on the Dockerfile change in PR #17. All three are good design feedback — applied here. 1. Comment block too verbose + references stale PR history Rewrote to describe the security rationale directly (xtrace would echo the token-bearing export line into the build log) without naming the PR number, the bots, or "HIGH severity" labels — those belong in the commit message and PR description, not in long-lived source comments that will outlive both. 2. set -eu instead of set -eux is inconsistent with the rest of the Dockerfile Other RUN blocks in this file (lines 41, 56, 104, 181) use `set -eux`. The previous change dropped `x` from the initial set — that's an inconsistency without an actual security benefit, since the `set +x` / `set -x` wrapper around the secret read+export is what actually protects the token. Restored to `set -eux` at the top. 3. Redundant explicit `set -x` in the else branch With the top-level `set -eux` restored, the else branch already has xtrace on. The extra `set -x` was dead code; removed. Verified again locally — `docker buildx build --progress plain` does not contain the token value anywhere in stderr. Signed-off-by: Sebastian Mendel --- Dockerfile | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/Dockerfile b/Dockerfile index 7df0c46..16f7ccc 100644 --- a/Dockerfile +++ b/Dockerfile @@ -69,15 +69,12 @@ RUN set -eux; \ # any image layer. RUN --mount=type=cache,target=/root/.composer/cache \ --mount=type=secret,id=GH_TOKEN \ - set -eu; \ + set -eux; \ if [ -s /run/secrets/GH_TOKEN ]; then \ - # Token handling kept OUT of xtrace (set -x). `set +x` here is \ - # belt-and-suspenders even though `set -eu` (no x) is the outer \ - # default; once secret handling is done we re-enable xtrace for \ - # the rest of the build. Without this, the export line would \ - # be echoed with the token value expanded into the build log — \ - # bot-reviewer-flagged on PR netresearch/snipe-it-docker-compose-stack#16 \ - # (copilot + gemini, HIGH severity). \ + # Disable xtrace before reading the secret so the export line — \ + # which contains the expanded token value — is not echoed into \ + # the build log. Restore xtrace immediately after exporting and \ + # unsetting the bare variable. \ set +x; \ GH_TOKEN=$(cat /run/secrets/GH_TOKEN); \ export COMPOSER_AUTH="{\"github-oauth\":{\"github.com\":\"${GH_TOKEN}\"}}"; \ @@ -85,7 +82,6 @@ RUN --mount=type=cache,target=/root/.composer/cache \ set -x; \ echo "[composer] github.com authenticated via BuildKit secret"; \ else \ - set -x; \ echo "[composer] no GH_TOKEN secret available — falling back to anonymous github.com (60 req/h)"; \ fi; \ if [ "${ROLLING_DEPS}" = "true" ]; then \