fix: address unresolved review comments from merged PRs #15 + #16#17
Merged
Conversation
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 <info@sebastianmendel.de>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves security in the Dockerfile by preventing the GH_TOKEN from being leaked in build logs through shell tracing (xtrace). The changes include wrapping sensitive token handling with set +x and set -x, and unsetting the token after use. Review feedback suggests simplifying the documentation by removing external PR references and maintaining consistency with the rest of the file by keeping the initial set -eux command, which would also eliminate redundant tracing calls.
3 tasks
There was a problem hiding this comment.
Pull request overview
This PR follows up on unresolved review threads from previously merged PRs by hardening token handling during Docker image builds and reducing workflow drift in the build matrix configuration.
Changes:
- Prevent GitHub token leakage in Docker builds by removing global
set -xaround secret-derived exports and explicitly disabling xtrace while handling the secret. - Avoid passing
GITHUB_TOKENinto the Docker build secret onpull_requestevents (fallback to anonymous GitHub access for PR builds). - Reduce matrix drift risk by using a YAML anchor/alias for the
tagtrack object referenced bymatrix.exclude, and improve upstream issue link specificity.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Dockerfile |
Prevents secret value exposure in build logs by controlling xtrace during secret handling and unsetting GH_TOKEN after use. |
.github/workflows/build.yml |
Uses YAML anchor for matrix exclusion stability and gates secret injection on non-PR events to reduce exfiltration risk. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 <info@sebastianmendel.de>
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
CybotTM
added a commit
to netresearch/phpbu-docker
that referenced
this pull request
May 23, 2026
## Why PR #124 was merged with 2 unresolved gemini-code-assist threads on the netresearch/* packageRule. Both addressed here. ## What changed | Reviewer point | Fix | |---|---| | Description referenced past commit 309fca0 — config should focus on policy, not history | Removed the incident-history sentence; kept the policy statement + trust-model contrast | | `enabled: false` is over-restrictive and blocks future security alerts on these refs | Dropped to `pinDigests: false` alone — Renovate still surfaces vulnerability alerts and (eventually) `@vN` tag migration, but never produces digest-pin PRs that violated org policy | ## Test plan - [x] `renovate.json` valid JSON - [ ] CI green - [x] Original PR #124 threads replied-to + resolved via GraphQL with a link here ## Related Part of a session-wide audit triggered by the user: 3 of 11 merged PRs (this one, snipe-it-docker-compose-stack#15, snipe-it-docker-compose-stack#16) had been merged with unresolved bot-reviewer threads. New memory rule \`feedback_never_merge_with_unresolved_threads\` now requires the GraphQL unresolved-threads query before every `gh pr merge`. Equivalent follow-up PR for the snipe-it threads (which included a HIGH-severity token-leak) is netresearch/snipe-it-docker-compose-stack#17.
9 tasks
CybotTM
added a commit
to netresearch/github-project-skill
that referenced
this pull request
May 23, 2026
Add a runnable script + JSON template that operators invoke immediately
after `gh repo create`, before pushing the first commit or opening
the first PR. The script applies the Netresearch baseline branch
protection:
- required_conversation_resolution: true (load-bearing)
- required_approving_review_count: 1
- allow_force_pushes: false
- allow_deletions: false
- required_linear_history: false
The template deliberately omits required_signatures (so the script
never resets a repo that has already enabled signing) and ships
enforce_admins as false to accommodate solo-maintainer repos.
Operators tighten both per-repo via the documented one-liners in the
script header.
Two-step flow:
1. bash init-branch-protection.sh OWNER/REPO
(applies baseline; no required status checks yet)
2. bash init-branch-protection.sh OWNER/REPO --from-current-checks
(after first CI run completes, captures check-run names as
required contexts with strict=true)
Idempotent: a second run on an already-compliant repo reports drift
on opinionated fields only (or 'already compliant') and exits 0.
Live-tested against netresearch/snipe-it-docker-compose-stack —
correctly reports already-compliant. shellcheck + bash -n clean.
Closes the enforcement gap demonstrated in
netresearch/snipe-it-docker-compose-stack#17 where 3 of 8 merged PRs
shipped with unresolved bot-reviewer threads (including a HIGH-severity
token-leak flagged by both Copilot and gemini-code-assist) because
branch protection was never applied at repo creation time.
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
added a commit
to netresearch/github-project-skill
that referenced
this pull request
May 23, 2026
…ode in GH-31 Severity was already 'error' (verified). This expands the desc to name the concrete user-visible failure mode that motivated promoting this to a hard checkpoint in the first place: - PR merges may silently include unresolved bot-reviewer feedback - Including security findings like token leakage in build logs - Concrete incident: netresearch/snipe-it-docker-compose-stack#17 The desc now also points operators at the new init script as the apply path, so /assess findings have an actionable remediation embedded in the failure message rather than a generic 'enable it'. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
added a commit
to netresearch/github-project-skill
that referenced
this pull request
May 23, 2026
…77) ## Summary Close the structural enforcement gap that just allowed three unresolved bot-reviewer threads — including a HIGH-severity token leak — to ship to `main` in [netresearch/snipe-it-docker-compose-stack#17](netresearch/snipe-it-docker-compose-stack#17). The skill already documented branch protection and already had a checkpoint (`GH-31`) for `required_conversation_resolution: true`, but there was nothing prompting an operator to actually apply protection at `gh repo create` time, before the first PR opens. By the time `/assess github-project` would have caught the miss, PRs were already merging. This PR adds a first-class init surface so the structural rule is applied at repo-creation time, not audited after the fact. ## What's in the PR - **`skills/github-project/scripts/init-branch-protection.sh`** — runnable, idempotent. Takes `<owner>/<repo>`, reads the new template, PUTs to `repos/{owner}/{repo}/branches/{default_branch}/protection`. Uses `gh api -X PUT --input -` with JSON body (NOT `--raw-field`, which silently breaks on nested JSON — that's exactly what wasted three retries in [snipe-it#15](netresearch/snipe-it-docker-compose-stack#15)). Handles empty-repo (exit 4), no access (exit 3), already-compliant (exit 0), and drift on opinionated fields (exit 1 with a per-field diff, no silent clobber). - **`skills/github-project/assets/branch-protection.json.template`** — synthesized from snipe-it's live post-fix state with two deliberate adjustments: `enforce_admins: false` explicit (so solo-maintainer repos like ldap-selfservice-… or usercentrics-widgets aren't painted into a corner), and `required_signatures` *omitted* (not set to `false`) so the script never resets a repo that has already opted into signing. Both are documented in the script header as per-repo tightening one-liners. - **`--from-current-checks` follow-up flag** — after the first CI run completes on the default branch, re-invoke with this flag and the script discovers the successful check-run names from the most recent completed run and PATCHes them in as required status contexts with `strict: true`. - **SKILL.md** — new \"Required First Step After \`gh repo create\`\" section right after \"When to Use\". The \"When to Use\" list now leads with the init step. \"Quick Diagnostics\" gets a one-liner (`gh api repos/OWNER/REPO/branches/.../protection --jq '.required_conversation_resolution.enabled'`). \"Running Scripts\" lists the new script alongside the existing verifier. - **`checkpoints.yaml` GH-31** — severity was already `error` (verified). The `desc:` now names the user-visible failure mode (silent unresolved-thread merges including token leaks like snipe-it#17) and points operators at the new script as the apply path. - **README.md** — 7-line callout at the top so someone landing on the repo page sees the init step without scrolling. ## The new flow ```bash gh repo create netresearch/new-thing --public --clone cd new-thing git push -u origin main # need at least one commit so the default branch ref exists bash ../github-project-skill/main/skills/github-project/scripts/init-branch-protection.sh \\ netresearch/new-thing # ... open PRs, run CI ... bash .../init-branch-protection.sh netresearch/new-thing --from-current-checks ``` ## What this does NOT fix (deliberately, follow-up) GitHub branch protection still cannot block on a *requested-but-pending* review. The snipe-it#17 leak slipped through the **unresolved-threads** class, which this PR's baseline (`required_conversation_resolution: true`) DOES close structurally. The **pending-reviewer** class (Copilot is mid-review, merge anyway) remains a workflow-discipline gap — see `references/security-config.md` lines 144-152. Closing it requires a status-check workflow (out of scope here). ## Verification before merge Run the unresolved-threads GraphQL check before any merge attempt: ```bash gh api graphql -f query='query{repository(owner:\"netresearch\",name:\"github-project-skill\"){ pullRequest(number:NUMBER){reviewThreads(first:100){nodes{isResolved}}} }}' --jq '.data.repository.pullRequest.reviewThreads.nodes | map(select(.isResolved == false)) | length' ``` Abort merge if non-zero. (This PR's own correctness is also a useful test of the rule it documents.) ## Test plan - [x] `shellcheck` clean on `init-branch-protection.sh` - [x] `bash -n` clean - [x] Live-tested against `netresearch/snipe-it-docker-compose-stack` — reports `already compliant` (idempotent path works against the repo whose incident motivated this PR) - [x] Error paths exercised: no arg → exit 2, bad slug → exit 2, 404 repo → exit 3, bad flag → exit 2 - [x] `markdownlint-cli2` clean on `SKILL.md` and `README.md` - [x] `yamllint` clean on `checkpoints.yaml` - [x] All four commits signed (verified via `git log --show-signature`) - [ ] CI green on this branch - [ ] Unresolved-threads GraphQL check returns 0 before merge ## Atomic commits 1. `feat(scripts): add init-branch-protection.sh for new-repo bootstrap` 2. `docs(skill): require branch-protection init step before first PR/commit` 3. `feat(checkpoints): clarify required_conversation_resolution failure mode in GH-31` 4. `docs(readme): announce required init step at top of README`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
PRs #15 and #16 were merged with 5 unresolved bot-reviewer threads. Two of them (copilot + gemini, same finding) flagged a real HIGH-severity security bug — token leakage via
set -xin the Dockerfile. The user caught the gap and asked to address them properly.What this fixes
set -euxechoes the token-bearingexport COMPOSER_AUTH=...line into the build logxfromset -eu, wrap secret handling inset +x/set -x, unset$GH_TOKENafter exportpull_requestevent also gets the secret — hostile PR could exfiltrategithub.event_name != 'pull_request'; PR-event falls back to anonymous github.com (PR runs are amd64-only)matrix.excluderepeats the track object verbatim; will drift silently&tag-track+ alias*tag-trackVerification
python -c 'yaml.safe_load(...)'confirms&tag-trackand*tag-trackresolve to the same objectdocker buildx build --progress plainconfirms the token VALUE does not appear anywhere in the build logProcess note
The session that produced PRs #15 + #16 also produced 9 other merged PRs across this repo, netresearch/.github, and netresearch/phpbu-docker. An audit found 7 unresolved threads total across 3 of them — PR #15 (2), PR #16 (3), phpbu-docker#124 (2). Equivalent follow-ups for the phpbu-docker thread go up under a separate PR there.
A new memory entry (`feedback_never_merge_with_unresolved_threads`) now requires running the GraphQL unresolved-threads query before every
gh pr merge, even on repos withoutrequired_conversation_resolutionbranch protection.