Dependabot Alert #16 (GHSA-m7jm-9gc2-mpf2 / CVE-2026-25896)#164
Conversation
Signed-off-by: Derek Misler <derek.misler@docker.com>
46c3330 to
24bb538
Compare
|
why the env var changes? |
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR correctly remediates CVE-2026-25896 (GHSA-m7jm-9gc2-mpf2) by upgrading fast-xml-parser to 5.7.2 and adding a pnpm override. The review-pr/action.yml refactoring to use env vars instead of inline template interpolation is a security improvement. Three low-severity observations are noted inline.
| ] | ||
| ], | ||
| "overrides": { | ||
| "fast-xml-parser": "^5.3.5" |
There was a problem hiding this comment.
[LOW] Semver range in CVE override may drift on lockfile regeneration
The pnpm override for fast-xml-parser uses "^5.3.5" (caret range) rather than an exact version pin like "5.7.2". For a security-fix override targeting a specific CVE (GHSA-m7jm-9gc2-mpf2), this means a future pnpm install could resolve to any 5.x >= 5.3.5 — potentially an unvetted newer version. Best practice for CVE remediations is to pin the exact known-safe version:
"overrides": {
"fast-xml-parser": "5.7.2"
}| '@jridgewell/trace-mapping@0.3.31': | ||
| resolution: {integrity: sha512-zzNR+SdQSDJzc8joaeP8QQoCQr8NuYx2dIIytl1QeBEZHJ9uW6hebsrYgbz8hJwUQao3TWCMtmfV8Nu1twOLAw==} | ||
|
|
||
| '@nodable/entities@2.1.0': |
There was a problem hiding this comment.
[LOW] Three new transitive dependencies introduced — supply chain audit recommended
Upgrading fast-xml-parser from 5.2.5 to 5.7.2 adds three previously-absent transitive dependencies:
@nodable/entities@2.1.0— scoped package for HTML entity encoding (security-sensitive domain)fast-xml-builder@1.1.7path-expression-matcher@1.5.0
These packages are from less-familiar authors/scopes not previously present in this codebase. The lockfile includes SHA-512 integrity hashes (which prevents tampering of already-resolved packages), but does not guard against malicious code in the upstream published packages themselves.
It is worth running pnpm audit and briefly reviewing the NPM pages for these packages to confirm legitimacy before merging, particularly @nodable/entities given its role in entity encoding.
| shell: bash | ||
| env: | ||
| PR_NUMBER_INPUT: ${{ inputs.pr-number }} | ||
| COMMENT_ID_INPUT: ${{ inputs.comment-id }} |
There was a problem hiding this comment.
[LOW] comment-id input used in gh api URL path without numeric validation
COMMENT_ID_INPUT is populated from the inputs.comment-id action input and later injected into gh api URL paths like:
gh api "repos/$REPO/issues/comments/$COMMENT_ID/reactions"
There is no validation that COMMENT_ID is a numeric value before use. While the env-var approach in this PR (vs. inline ${{ }} interpolation) correctly eliminates shell injection, a crafted non-numeric comment-id (e.g., containing /) could theoretically manipulate the API path. Consider adding a numeric guard:
if [[ -n "$COMMENT_ID" && ! "$COMMENT_ID" =~ ^[0-9]+$ ]]; then
echo "❌ Invalid comment-id: must be numeric"
exit 1
fiNote: The env-var refactoring in this PR is an improvement over the previous inline interpolation pattern.
we see things like this once-in-a-while: https://github.com/docker/cagent-action/security/code-scanning/204 i don't know if they're actually issues, but they're so easy to fix, so 🤷🏻 |
Summary
Resolves Dependabot alert GHSA-m7jm-9gc2-mpf2 (CVE-2026-25896) by upgrading
fast-xml-parserfrom 5.2.5 to 5.7.2 and adding a pnpm override to enforce the patched version across the dependency tree.Changes
fast-xml-parserto 5.7.2 (security patch)@nodable/entities,fast-xml-builder,path-expression-matcher)Closes: https://github.com/docker/gordon/issues/486