Skip to content

Commit d922717

Browse files
woodruffwwoodruffw
authored
feat: generalize RepositoryUsesPattern (#670)
1 parent db30721 commit d922717

14 files changed

+706
-404
lines changed

‎docs/audits.md‎

Lines changed: 378 additions & 332 deletions
Large diffs are not rendered by default.

‎src/audit/unpinned_uses.rs‎

Lines changed: 28 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,18 @@ impl UnpinnedUses {
5252
Some(RepositoryUsesPattern::Any) | None => "blanket".into(),
5353
Some(RepositoryUsesPattern::InOwner(owner)) => format!("{owner}/*"),
5454
Some(RepositoryUsesPattern::InRepo { owner, repo }) => {
55+
format!("{owner}/{repo}/*")
56+
}
57+
Some(RepositoryUsesPattern::ExactRepo { owner, repo }) => {
5558
format!("{owner}/{repo}")
5659
}
60+
Some(RepositoryUsesPattern::ExactPath {
61+
owner,
62+
repo,
63+
subpath,
64+
}) => {
65+
format!("{owner}/{repo}/{subpath}")
66+
}
5767
};
5868

5969
match policy {
@@ -230,18 +240,8 @@ impl UnpinnedUsesPolicies {
230240
// Policies are ordered by specificity, so we can
231241
// iterate and return eagerly.
232242
for (uses_pattern, policy) in policies {
233-
match uses_pattern {
234-
RepositoryUsesPattern::InRepo { owner: _, repo } => {
235-
if repo == &uses.repo {
236-
return (Some(uses_pattern), *policy);
237-
} else {
238-
continue;
239-
}
240-
}
241-
RepositoryUsesPattern::InOwner(_) => return (Some(uses_pattern), *policy),
242-
// NOTE: Unreachable because we only
243-
// allow `*` to configure the default policy.
244-
RepositoryUsesPattern::Any => unreachable!(),
243+
if uses_pattern.matches(uses) {
244+
return (Some(uses_pattern), *policy);
245245
}
246246
}
247247
// The policies under `owner/` might be fully divergent
@@ -262,17 +262,29 @@ impl From<UnpinnedUsesConfig> for UnpinnedUsesPolicies {
262262

263263
for (pattern, policy) in config.policies {
264264
match pattern {
265-
RepositoryUsesPattern::InRepo { owner, repo } => {
265+
RepositoryUsesPattern::ExactPath { ref owner, .. } => {
266266
policy_tree
267267
.entry(owner.clone())
268268
.or_default()
269-
.push((RepositoryUsesPattern::InRepo { owner, repo }, policy));
269+
.push((pattern, policy));
270270
}
271-
RepositoryUsesPattern::InOwner(owner) => {
271+
RepositoryUsesPattern::ExactRepo { ref owner, .. } => {
272272
policy_tree
273273
.entry(owner.clone())
274274
.or_default()
275-
.push((RepositoryUsesPattern::InOwner(owner), policy));
275+
.push((pattern, policy));
276+
}
277+
RepositoryUsesPattern::InRepo { ref owner, .. } => {
278+
policy_tree
279+
.entry(owner.clone())
280+
.or_default()
281+
.push((pattern, policy));
282+
}
283+
RepositoryUsesPattern::InOwner(ref owner) => {
284+
policy_tree
285+
.entry(owner.clone())
286+
.or_default()
287+
.push((pattern, policy));
276288
}
277289
RepositoryUsesPattern::Any => {
278290
default_policy = policy;
@@ -291,34 +303,3 @@ impl From<UnpinnedUsesConfig> for UnpinnedUsesPolicies {
291303
}
292304
}
293305
}
294-
295-
#[cfg(test)]
296-
mod tests {
297-
use crate::models::uses::RepositoryUsesPattern;
298-
299-
#[test]
300-
fn test_uses_pattern_ord() {
301-
let mut patterns = vec![
302-
RepositoryUsesPattern::Any,
303-
RepositoryUsesPattern::InRepo {
304-
owner: "owner".into(),
305-
repo: "repo".into(),
306-
},
307-
RepositoryUsesPattern::InOwner("owner/*".into()),
308-
];
309-
310-
patterns.sort();
311-
312-
assert_eq!(
313-
patterns,
314-
vec![
315-
RepositoryUsesPattern::InRepo {
316-
owner: "owner".into(),
317-
repo: "repo".into()
318-
},
319-
RepositoryUsesPattern::InOwner("owner/*".into()),
320-
RepositoryUsesPattern::Any,
321-
]
322-
);
323-
}
324-
}

‎src/models/uses.rs‎

Lines changed: 189 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,24 +6,55 @@ use github_actions_models::common::{RepositoryUses, Uses};
66
use regex::Regex;
77
use serde::Deserialize;
88

9-
// Matches patterns like `owner/repo` and `owner/*`.
9+
/// Matches all variants of [`RepositoryUsesPattern`] except `*`.
1010
static REPOSITORY_USES_PATTERN: LazyLock<Regex> =
11-
LazyLock::new(|| Regex::new(r#"(?mi)^([\w-]+)/([\w\.-]+|\*)$"#).unwrap());
11+
LazyLock::new(|| Regex::new(r#"(?mi)^([\w-]+)/([\w\.-]+|\*)(?:/(.+))?$"#).unwrap());
1212

1313
/// Represents a pattern for matching repository `uses` references.
1414
/// These patterns are ordered by specificity; more specific patterns
1515
/// should be listed first.
1616
#[derive(Debug, Eq, PartialEq, Hash, PartialOrd, Ord)]
1717
pub(crate) enum RepositoryUsesPattern {
18-
// TODO: InRepoPath for `owner/repo/path`?
18+
/// Matches exactly `owner/repo/subpath`.
19+
ExactPath {
20+
owner: String,
21+
repo: String,
22+
subpath: String,
23+
},
24+
/// Matches exactly `owner/repo`.
25+
ExactRepo { owner: String, repo: String },
26+
/// Matches `owner/repo/*` (i.e. any subpath under the given repo, including
27+
/// the empty subpath).
1928
InRepo { owner: String, repo: String },
29+
/// Matches `owner/*` (i.e. any repo under the given owner).
2030
InOwner(String),
31+
/// Matches any `owner/repo`.
2132
Any,
2233
}
2334

2435
impl RepositoryUsesPattern {
2536
pub(crate) fn matches(&self, uses: &RepositoryUses) -> bool {
2637
match self {
38+
RepositoryUsesPattern::ExactPath {
39+
owner,
40+
repo,
41+
subpath,
42+
} => {
43+
// TODO: Normalize the subpath here.
44+
// This is nontrivial, since we need to normalize
45+
// both leading slashes *and* arbitrary ./.. components.
46+
// Utf8Path gets us part of the way there, but is
47+
// platform dependent (i.e. will do the wrong thing
48+
// if the platform separator is not /).
49+
uses.owner.eq_ignore_ascii_case(owner)
50+
&& uses.repo.eq_ignore_ascii_case(repo)
51+
&& uses.subpath.as_deref().is_some_and(|s| s == subpath)
52+
}
53+
RepositoryUsesPattern::ExactRepo { owner, repo } => {
54+
uses.owner.eq_ignore_ascii_case(owner)
55+
&& uses.repo.eq_ignore_ascii_case(repo)
56+
&& uses.subpath.is_none()
57+
}
2758
RepositoryUsesPattern::InRepo { owner, repo } => {
2859
uses.owner.eq_ignore_ascii_case(owner) && uses.repo.eq_ignore_ascii_case(repo)
2960
}
@@ -41,21 +72,31 @@ impl FromStr for RepositoryUsesPattern {
4172
return Ok(RepositoryUsesPattern::Any);
4273
}
4374

44-
let caps = REPOSITORY_USES_PATTERN.captures(s).ok_or_else(|| {
45-
anyhow::anyhow!("invalid repository pattern: {s} (expected owner/repo or owner/*)")
46-
})?;
75+
let caps = REPOSITORY_USES_PATTERN
76+
.captures(s)
77+
.ok_or_else(|| anyhow::anyhow!("invalid pattern: {s}"))?;
4778

4879
let owner = &caps[1];
4980
let repo = &caps[2];
81+
let subpath = caps.get(3).map(|m| m.as_str());
5082

51-
Ok(if repo == "*" {
52-
RepositoryUsesPattern::InOwner(owner.into())
53-
} else {
54-
RepositoryUsesPattern::InRepo {
83+
match (owner, repo, subpath) {
84+
(owner, "*", None) => Ok(RepositoryUsesPattern::InOwner(owner.into())),
85+
(owner, repo, None) => Ok(RepositoryUsesPattern::ExactRepo {
5586
owner: owner.into(),
5687
repo: repo.into(),
57-
}
58-
})
88+
}),
89+
(_, "*", Some(_)) => Err(anyhow::anyhow!("invalid pattern: {s}")),
90+
(owner, repo, Some("*")) => Ok(RepositoryUsesPattern::InRepo {
91+
owner: owner.into(),
92+
repo: repo.into(),
93+
}),
94+
(owner, repo, Some(subpath)) => Ok(RepositoryUsesPattern::ExactPath {
95+
owner: owner.into(),
96+
repo: repo.into(),
97+
subpath: subpath.into(),
98+
}),
99+
}
59100
}
60101
}
61102

@@ -217,18 +258,145 @@ mod tests {
217258
}
218259
}
219260

261+
#[test]
262+
fn test_repositoryusespattern_parse() {
263+
for (pattern, expected) in [
264+
("", None), // Invalid, empty
265+
("/", None), // Invalid, not well formed
266+
("//", None), // Invalid, not well formed
267+
("///", None), // Invalid, not well formed
268+
("owner", None), // Invalid, should be owner/*
269+
("**", None), // Invalid, should be *
270+
("*", Some(RepositoryUsesPattern::Any)),
271+
(
272+
"owner/*",
273+
Some(RepositoryUsesPattern::InOwner("owner".into())),
274+
),
275+
("owner/*/", None), // Invalid, should be owner/*
276+
("owner/*/foo", None), // Invalid, not well formed
277+
("owner/*/*", None), // Invalid, not well formed
278+
(
279+
"owner/repo/*",
280+
Some(RepositoryUsesPattern::InRepo {
281+
owner: "owner".into(),
282+
repo: "repo".into(),
283+
}),
284+
),
285+
(
286+
"owner/repo",
287+
Some(RepositoryUsesPattern::ExactRepo {
288+
owner: "owner".into(),
289+
repo: "repo".into(),
290+
}),
291+
),
292+
(
293+
"owner/repo/subpath",
294+
Some(RepositoryUsesPattern::ExactPath {
295+
owner: "owner".into(),
296+
repo: "repo".into(),
297+
subpath: "subpath".into(),
298+
}),
299+
),
300+
// We don't do any subpath normalization at construction time.
301+
(
302+
"owner/repo//",
303+
Some(RepositoryUsesPattern::ExactPath {
304+
owner: "owner".into(),
305+
repo: "repo".into(),
306+
subpath: "/".into(),
307+
}),
308+
),
309+
// Weird, but we allow it (for now).
310+
(
311+
"owner/repo/**",
312+
Some(RepositoryUsesPattern::ExactPath {
313+
owner: "owner".into(),
314+
repo: "repo".into(),
315+
subpath: "**".into(),
316+
}),
317+
),
318+
(
319+
"owner/repo/subpath/",
320+
Some(RepositoryUsesPattern::ExactPath {
321+
owner: "owner".into(),
322+
repo: "repo".into(),
323+
subpath: "subpath/".into(),
324+
}),
325+
),
326+
(
327+
"owner/repo/subpath/very/nested////and/literal",
328+
Some(RepositoryUsesPattern::ExactPath {
329+
owner: "owner".into(),
330+
repo: "repo".into(),
331+
subpath: "subpath/very/nested////and/literal".into(),
332+
}),
333+
),
334+
] {
335+
let pattern = RepositoryUsesPattern::from_str(pattern).ok();
336+
assert_eq!(pattern, expected);
337+
}
338+
}
339+
340+
#[test]
341+
fn test_repositoryusespattern_ord() {
342+
let mut patterns = vec![
343+
RepositoryUsesPattern::Any,
344+
RepositoryUsesPattern::ExactRepo {
345+
owner: "owner".into(),
346+
repo: "repo".into(),
347+
},
348+
RepositoryUsesPattern::InOwner("owner".into()),
349+
];
350+
351+
patterns.sort();
352+
353+
assert_eq!(
354+
patterns,
355+
vec![
356+
RepositoryUsesPattern::ExactRepo {
357+
owner: "owner".into(),
358+
repo: "repo".into()
359+
},
360+
RepositoryUsesPattern::InOwner("owner".into()),
361+
RepositoryUsesPattern::Any,
362+
]
363+
);
364+
}
365+
220366
#[test]
221367
fn test_repositoryusespattern_matches() -> anyhow::Result<()> {
222368
for (uses, pattern, matches) in [
223-
// owner/repo matches regardless of ref, casing, and subpath
224-
// but rejects when owner/repo diverges
369+
// owner/repo/subpath matches regardless of ref and casing
370+
// but only when the subpath matches.
371+
// the subpath must share the same case but might not be
372+
// normalized
373+
("actions/checkout/foo", "actions/checkout/foo", true),
374+
("ACTIONS/CHECKOUT/foo", "actions/checkout/foo", true),
375+
("ACTIONS/CHECKOUT/foo@v3", "actions/checkout/foo", true),
376+
// TODO: See comment in `RepositoryUsesPattern::matches`
377+
// ("ACTIONS/CHECKOUT/foo@v3", "actions/checkout/foo/", true),
378+
// ("ACTIONS/CHECKOUT/foo@v3", "actions/checkout/foo//", true),
379+
// ("ACTIONS/CHECKOUT//foo////@v3", "actions/checkout/foo", true),
380+
("actions/checkout/FOO", "actions/checkout/foo", false),
381+
("actions/checkout/foo/bar", "actions/checkout/foo", false),
382+
// owner/repo matches regardless of ref and casing
383+
// but does not match subpaths
225384
("actions/checkout", "actions/checkout", true),
226385
("ACTIONS/CHECKOUT", "actions/checkout", true),
227386
("actions/checkout@v3", "actions/checkout", true),
228-
("actions/checkout/foo@v3", "actions/checkout", true),
387+
("actions/checkout/foo@v3", "actions/checkout", false),
229388
("actions/somethingelse", "actions/checkout", false),
230389
("whatever/checkout", "actions/checkout", false),
231-
// owner/* matches of ref, casing, and subpath
390+
// owner/repo/* matches regardless of ref and casing
391+
// including subpaths
392+
// but does not match when owner diverges
393+
("actions/checkout", "actions/checkout/*", true),
394+
("ACTIONS/CHECKOUT", "actions/checkout/*", true),
395+
("actions/checkout@v3", "actions/checkout/*", true),
396+
("actions/checkout/foo@v3", "actions/checkout/*", true),
397+
("actions/checkout/foo/bar@v3", "actions/checkout/*", true),
398+
("someoneelse/checkout", "actions/checkout/*", false),
399+
// owner/* matches regardless of ref, casing, and subpath
232400
// but rejects when owner diverges
233401
("actions/checkout", "actions/*", true),
234402
("ACTIONS/CHECKOUT", "actions/*", true),
@@ -247,7 +415,11 @@ mod tests {
247415

248416
let pattern = RepositoryUsesPattern::from_str(pattern)?;
249417

250-
assert_eq!(pattern.matches(&uses), matches);
418+
assert_eq!(
419+
pattern.matches(&uses),
420+
matches,
421+
"pattern: {pattern:?}, uses: {uses:?}, matches: {matches}"
422+
);
251423
}
252424

253425
Ok(())

‎tests/integration/snapshot.rs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ fn unpinned_uses() -> Result<()> {
205205
.run()?
206206
);
207207

208-
// Composite config.
208+
// Composite config cases.
209209
insta::assert_snapshot!(
210210
"unpinned-uses-composite-config",
211211
zizmor()
@@ -214,6 +214,14 @@ fn unpinned_uses() -> Result<()> {
214214
.run()?
215215
);
216216

217+
insta::assert_snapshot!(
218+
"unpinned-uses-composite-config-2",
219+
zizmor()
220+
.config(input_under_test("unpinned-uses/configs/composite-2.yml"))
221+
.input(input_under_test("unpinned-uses/menagerie-of-uses.yml"))
222+
.run()?
223+
);
224+
217225
// Empty config.
218226
insta::assert_snapshot!(
219227
"unpinned-uses-empty-config",

0 commit comments

Comments
 (0)