dep-review: invert change_type filter to fail safe against schema drift#229
Merged
Conversation
vulnerable_changes_to_sarif.sh selected `change_type == "added"`. The
dependency-review change_type enum is exactly {added, removed}, so a
version bump arrives as a removed+added pair and `added` already catches
every introduced version — there is no `updated` type to miss. But the
`== "added"` allowlist fails *silent*: if the upstream enum ever gains a
value, a vulnerable change of that type would be dropped, not flagged.
Test `!= "removed"` instead. The question the converter answers is "is
this vulnerable package present at the PR head?" — true for everything
except a removal. Behavior is identical today; it now fails safe against
schema drift. Adds a test locking that in.
Also document (no behavior change) that vulnerable changes are converted
regardless of `scope` — development/build-time deps are flagged like
runtime ones, the conservative default.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TomHennen
had a problem deploying
to
integration-test
GitHub Actions
Failure
With the filter now testing `!= "removed"`, the `// "added"` default no longer affects any decision: a missing or null change_type is `null`, and `null != "removed"` already keeps it. Simplify the filter to `.change_type != "removed"` and reword the header comment, which still claimed a missing change_type "defaults to added" — true under the old `== "added"` test, but vacuous now. Behavior is unchanged.
TomHennen
temporarily deployed
to
integration-test
GitHub Actions
Inactive
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
1 participant
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.
What
tools/dependency-review/vulnerable_changes_to_sarif.shfiltered changes withselect((.change_type // "added") == "added"). This switches it to!= "removed".Why
Reviewing the change_type handling raised the question: the filter keeps
addedand dropsremoved— is there anupdated(or other) case being missed?There isn't.
actions/dependency-review-action'sChangeSchemadefineschange_type: z.enum(['added', 'removed'])— exactly two values. A dependency version bump is not a third type; it's emitted as aremoved(old version) +added(new version) pair, soaddedalready catches every introduced version.But
== "added"is an allowlist that fails silent: if the upstream enum ever gains a value, a vulnerable change of that new type would be dropped rather than flagged — the wrong direction for a security gate.The question the converter actually answers is "is this vulnerable package present at the PR's head?" — true for every change except a
removed. So!= "removed"is both the semantically precise test and fails safe: an unknown futurechange_typeis still surfaced.Behavior is identical today (only
added/removedexist). A new test (converter: unrecognized change_type is surfaced) locks in the fail-safe behavior; the existingremovedand missing-change_typetests still pass unchanged.Also: documented the
scopechoice (no behavior change)While here, documented something the converter does silently: it converts vulnerable changes regardless of
scope(runtime/development/unknown) — a vulnerable dev/build-time dependency is flagged exactly like a runtime one. That's the conservative default (block on any introduced vuln); a comment in the script and a note inSPEC.mdnow make it an explicit, intentional choice. Scope-aware policy (à lafail-on-scopes) would belong with the per-tool config design in #221. No code change — just documentation, per review preference.Testing
bats tools/dependency-review/test.bats— 31/31 pass (was 30; +1 new).shellcheckclean.Origin
Review discussion on #220.