Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .changeset/patch-refresh-gh-aw-node.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion actions/setup/js/Dockerfile.safe-outputs-mcp
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,16 @@ ARG NODE_IMAGE
ARG NODE_IMAGE_DIGEST=""
ARG NODE_IMAGE_UPDATED_AT=""
ARG DOCKERFILE_HASH=""
ARG NPM_VERSION=11.18.0
ARG NPM_VERSION=11.19.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The CVE identifiers in this comment (CVE-2025-tar, CVE-2025-brace-expansion) appear to be placeholder names rather than real CVE IDs — they will not be useful for auditors tracing this fix back to a published advisory.

💡 Suggestion

If the npm-bundled tar / brace-expansion vulnerabilities do not yet have assigned CVE numbers, say so explicitly, or link to the grype finding / npm advisory instead. For example:

# (no CVE assigned yet for tar < 7.5.22 or brace-expansion < 5.0.8; tracked via grype finding in issue #49295)

Using invented CVE IDs can cause confusion for security scanners and compliance audits that expect the format CVE-YYYY-NNNNN.

@copilot please address this.

# Intentional: upgrade all packages to pick up security fixes; downstream digest pins the result.
# After upgrading npm, patch its bundled dependencies to meet minimum safe versions
# (tar >= 7.5.21 for CVE-2025-tar, brace-expansion >= 5.0.8 for CVE-2025-brace-expansion).
Comment on lines +13 to +14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment cites fabricated CVE identifiers that don't exist.

💡 Fabricated CVE IDs in code comment

CVE-2025-tar and CVE-2025-brace-expansion are not real CVE identifiers — they look like placeholder names invented for this comment. The actual CVEs being addressed, per the changeset, are CVE-2026-58055 and CVE-2025-60876, and those apply to Alpine base packages, not to tar/brace-expansion (which appear to have no assigned CVE at all, likely just grype flagging outdated versions against known-vulnerable ranges).

Fix: Either drop the fake CVE references entirely or replace them with the actual grype/GHSA advisory IDs for the tar and brace-expansion findings (if any exist), e.g.:

# Patch npm's bundled tar/brace-expansion to versions above the vulnerable range
# flagged by grype (no CVE assigned; see PR #49314 description for details).

Leaving fabricated CVE IDs in source is actively misleading for anyone auditing security fixes later.

RUN apk upgrade --no-cache \
&& apk add --no-cache git \
&& apk info -v | sort \
&& npm install --global "npm@${NPM_VERSION}" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment says tar >= 7.5.21 but the install command pins tar@^7.5.22 (minimum 7.5.22). Update the comment to match: tar >= 7.5.22. Minor doc inconsistency only — the actual version pinned is safe.

@copilot please address this.

&& npm install --prefix "$(npm root -g)/npm" --no-save "tar@^7.5.22" "brace-expansion@^5.0.8" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No verification step confirms the override actually replaced the vulnerable nested versions, so a future silent regression wouldn't be caught by CI.

💡 Missing post-install verification

The RUN step installs tar@^7.5.22 and brace-expansion@^5.0.8 into npm's global package directory, but nothing confirms the versions actually landed or that npm resolves to them at runtime. If a future base-image or npm-version bump changes npm's internal node_modules layout (e.g., nesting tar/brace-expansion under a transitive dependency instead of npm's own top-level node_modules), this override could become a silent no-op and CI would still pass.

Suggested fix: add a verification line after the install, e.g.:

&& npm install --prefix "$(npm root -g)/npm" --no-save "tar@^7.5.22" "brace-expansion@^5.0.8" \
&& node -e "console.log(require('$(npm root -g)/npm/node_modules/tar/package.json').version)" \

or use npm ls --prefix "$(npm root -g)/npm" tar brace-expansion and fail the build if the resolved version doesn't satisfy the minimum. Without this, the fix's effectiveness is unverified and could silently regress.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caret ranges let future rebuilds silently pull unvetted versions instead of the specific patched release that was actually verified.

💡 Unpinned caret ranges undermine the point of a security patch

tar@^7.5.22 and brace-expansion@^5.0.8 allow npm to resolve to any later 7.x/5.x release on each image rebuild. Since this Dockerfile is rebuilt periodically (per the apk upgrade --no-cache comment above it, implying repeat builds over time), a later minor/patch release of either package could introduce a regression or even a new vulnerability, and the build would pick it up automatically without any review — exactly what an intentional security patch should avoid.

Fix: Pin to the exact verified-safe versions instead of caret ranges, e.g. "tar@7.5.22" and "brace-expansion@5.0.8", and bump them deliberately (with a changeset) when a newer patched version is needed.

&& npm cache clean --force

LABEL org.opencontainers.image.source="https://github.com/github/gh-aw" \
Expand Down
Loading