Surface invalid delegation path patterns and match OS-independently - #753
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens delegation path-pattern matching in metadata by making matching OS-independent and by surfacing malformed glob patterns instead of silently treating them as non-matches.
Changes:
- Switch delegation path segment matching from
filepath.Matchtopath.Matchto ensure/-based behavior regardless of host OS. - Propagate
path.Matcherrors fromisTargetInPathPatternup throughDelegatedRole.IsDelegatedPath. - Add a regression test that asserts malformed patterns return
path.ErrBadPattern.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| metadata/metadata.go | Uses path.Match and propagates bad-pattern errors from delegated path matching. |
| metadata/metadata_test.go | Adds a regression test for malformed delegation patterns returning path.ErrBadPattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thank you for the PR. Maintainer availability is lower now during summer time, but a first skim this looks solid, but need to test this out more first, so will review may drag a few weeks. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #753 +/- ##
==========================================
+ Coverage 70.51% 74.49% +3.97%
==========================================
Files 10 10
Lines 2123 1866 -257
==========================================
- Hits 1497 1390 -107
+ Misses 505 471 -34
+ Partials 121 5 -116 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
isTargetInPathPattern discarded the error from filepath.Match, so a malformed delegation path pattern (like an unterminated character class "targets/[") was silently treated as a non-match. Callers could not tell "does not match" apart from "invalid pattern". It also used filepath.Match, whose separator handling is OS-dependent, while TUF target paths are always "/"-separated. Switch to path.Match, propagate the error through IsDelegatedPath, and add a regression test. Refs theupdateframework#747. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
ac2ff9d to
82dec08
Compare
kommendorkapten
left a comment
kommendorkapten
left a comment
There was a problem hiding this comment.
This looks good, thanks!
As a supply-chain-security researcher I was looking at delegation path matching and hit the fragility flagged in #747.
isTargetInPathPatternwas throwing away the error fromfilepath.Match, so a malformed pattern like an unterminated character class (targets/[) got silently treated as a plain non-match, and a caller couldn't tell "this pattern doesn't match" apart from "this pattern is invalid". It also usedfilepath.Match, whose separator handling depends on the host OS, even though TUF target paths are always/-separated.This switches to
path.Match(always/-separated,\as a glob escape) and propagates the error up throughIsDelegatedPath, which already returns(bool, error). I kept the change scoped tometadata.go; the siblingmultirepo.gocode already captures and returns that error.I verified it by adding a regression test: on the current code the malformed-pattern case returns
err == nil, and after the fix it returnspath.ErrBadPattern, while the existing spec-derived path cases still pass (go test ./metadata -run TestIsDelegatedPath -count=1). Thanks for taking a look.