fix: support OWNERS files with both top-level config and filters#5128
fix: support OWNERS files with both top-level config and filters#5128christophermancini wants to merge 3 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughresolveOwnerAliases and getOwnersHTTP were changed to decide output format based on presence of filters in FullConfig; top-level simple config is expanded/merged into a synthesized ".*" filter when filters exist. Tests were added for mixed and filters-only OWNERS scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: christophermancini The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/autoowners/main_test.go`:
- Around line 481-498: The test expectation is currently dropping top-level
approvers/reviewers when Filters are present; update both the test and the
production logic that constructs FullConfig (the code path that creates a
FullConfig from an OWNERS file where Filters is set—see the code that assigns
FullConfig.Filters) so it preserves and populates the top-level Approvers and
Reviewers fields alongside Filters; modify the test case in main_test.go to
include the expected FullConfig.Approvers and FullConfig.Reviewers (e.g.,
include "approver1","approver2" and "reviewer1","reviewer2") and change the
production function that builds FullConfig to merge or copy top-level
approvers/reviewers into the FullConfig struct instead of dropping them.
In `@cmd/autoowners/main.go`:
- Around line 211-216: The current logic returns immediately when
repoowners.LoadFullConfig(data) fails, which breaks previously valid
SimpleConfig files; change the flow in main.go so that if LoadFullConfig(data)
returns an error you then attempt repoowners.LoadSimpleConfig(data) as a
fallback (and only return an error if both fail); update the control variables
(e.g., the existing full result and any simple result variables) and subsequent
code paths that rely on the loaded config to handle either a FullConfig or a
SimpleConfig result appropriately, and add/adjust tests to cover a pure
SimpleConfig file parsing via LoadSimpleConfig.
- Around line 211-227: The current flow drops top-level approvers/reviewers when
a file has filters because you only set httpResult.fullConfig when full.Filters
is non-empty and never populate httpResult.simpleConfig; fix this by keeping
both configs: after LoadFullConfig and LoadSimpleConfig successfully parse,
always assign httpResult.fullConfig = full and httpResult.simpleConfig = simple
(if LoadSimpleConfig succeeds) so mixed OWNERS retain top-level and filter
blocks; then update resolveOwnerAliases to merge/return a combined
representation that emits top-level fields from simpleConfig and filters from
fullConfig (or construct a new combined struct) instead of choosing one or the
other; update SaveFullConfig or the save/write logic to serialize both top-level
approvers/reviewers/labels (from simpleConfig) and filters (from fullConfig)
into the single OWNERS output; finally adjust the test expectations
(fakeMixedOwners) to assert both top-level approvers/reviewers and the filter
entry are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d4313870-460d-4515-a30b-fd178e1c2ff6
📒 Files selected for processing (2)
cmd/autoowners/main.gocmd/autoowners/main_test.go
There was a problem hiding this comment.
Pull request overview
This PR updates autoowners so it no longer ignores filters when an upstream OWNERS file also includes top-level approvers/reviewers, enabling “mixed” OWNERS configurations to be synced correctly.
Changes:
- Change OWNERS parsing to always attempt
FullConfigfirst and use it when filters are present. - Fall back to
SimpleConfigonly when no filters exist. - Add a unit test case covering an OWNERS file that includes both top-level config and filters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/autoowners/main.go | Adjusts OWNERS parsing logic to prefer FullConfig when filters are present. |
| cmd/autoowners/main_test.go | Adds fixture and test case for mixed top-level + filters OWNERS configs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try to load as FullConfig first to check for filters | ||
| full, err := repoowners.LoadFullConfig(data) | ||
| if err != nil { | ||
| logrus.WithError(err).Error("Unable to load simple config.") | ||
| logrus.WithError(err).Error("Unable to load full config.") | ||
| return httpResult, err | ||
| } | ||
| httpResult.simpleConfig = simple | ||
| if httpResult.simpleConfig.Empty() { | ||
| full, err := repoowners.LoadFullConfig(data) | ||
| // If the file has filters, use FullConfig; otherwise use SimpleConfig | ||
| if len(full.Filters) > 0 { | ||
| httpResult.fullConfig = full | ||
| } else { | ||
| simple, err := repoowners.LoadSimpleConfig(data) | ||
| if err != nil { | ||
| logrus.WithError(err).Error("Unable to load full config.") | ||
| logrus.WithError(err).Error("Unable to load simple config.") | ||
| return httpResult, err | ||
| } | ||
| httpResult.fullConfig = full | ||
| httpResult.simpleConfig = simple | ||
| } |
There was a problem hiding this comment.
When an OWNERS file contains both top-level approvers/reviewers and filters, selecting FullConfig here will cause writeOwners() to emit a FullConfig via resolveOwnerAliases(). However, resolveOwnerAliases() currently rebuilds a FullConfig using only Filters and Options, so any top-level config present in full will be dropped in the generated OWNERS output. Update resolveOwnerAliases() (and related tests) to preserve/clean the top-level config fields from r.fullConfig in addition to filter entries, so mixed configs don’t lose defaults.
| { | ||
| description: "OWNERS with both top-level config and filters should use FullConfig", | ||
| given: orgRepo{ | ||
| Organization: "org8", | ||
| Repository: "repo8", | ||
| }, | ||
| expectedHTTPResult: httpResult{ | ||
| fullConfig: FullConfig{ | ||
| Filters: map[string]repoowners.Config{ | ||
| "^nightly-.*\\.yaml$": { | ||
| Approvers: []string{"nightly-approver1", "nightly-approver2", "approver1"}, | ||
| }, | ||
| }, | ||
| }, | ||
| ownersFileExists: true, | ||
| }, |
There was a problem hiding this comment.
This test case for a mixed OWNERS config only asserts fullConfig.Filters, but it doesn’t assert that the top-level approvers/reviewers from fakeMixedOwners are preserved when using FullConfig. Given the PR’s goal (“support OWNERS files with both top-level config and filters”), the expected FullConfig should include those top-level defaults as well; otherwise the tool could still drop them when writing the synced OWNERS file.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // If we also have top-level config, add it as a ".*" catch-all filter | ||
| // (unless a ".*" filter already exists in fullConfig) | ||
| if !r.simpleConfig.Empty() && r.fullConfig.Filters[".*"].Approvers == nil { |
There was a problem hiding this comment.
The check for an existing catch-all filter is incorrect: r.fullConfig.Filters[".*"].Approvers == nil will also be true when a "." filter exists but only sets reviewers/required_reviewers/labels (or explicitly has nil approvers), causing the top-level config to overwrite the user-provided "." filter. Use a map presence check (_, ok := ...) (and/or check all relevant fields) to avoid clobbering an existing ".*" rule.
| if !r.simpleConfig.Empty() && r.fullConfig.Filters[".*"].Approvers == nil { | |
| _, hasCatchAllFilter := r.fullConfig.Filters[".*"] | |
| if !r.simpleConfig.Empty() && !hasCatchAllFilter { |
| full, err := repoowners.LoadFullConfig(data) | ||
| if err != nil { | ||
| // If FullConfig fails but we have SimpleConfig, that's OK | ||
| if !simple.Empty() { | ||
| break | ||
| } | ||
| logrus.WithError(err).Error("Unable to load full config.") | ||
| return httpResult, err |
There was a problem hiding this comment.
On LoadFullConfig failure, the code silently falls back to SimpleConfig when simple is non-empty (break without even a warning). This can mask malformed filters: sections in mixed OWNERS files and reintroduce the “filters ignored” outcome in an error scenario. Consider returning the error (or at least logging a warning and only falling back when you’ve confirmed the file has no filters: key).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/autoowners/main.go (1)
149-176:⚠️ Potential issue | 🟠 MajorMerge top-level config with an existing
".*"filter.Line 151 checks
r.fullConfig.Filters[".*"].Approvers == nil, then Lines 165-175 can overwrite the synthesized catch-all when the source already contains a".*"filter. A mixed OWNERS file with both top-level owners and an explicit catch-all still loses the top-level owners. Build the cleaned top-level config, add/clean all full filters, then merge the top-level config intofc.Filters[".*"]instead of skipping or overwriting it.🐛 Proposed direction
- if !r.simpleConfig.Empty() && r.fullConfig.Filters[".*"].Approvers == nil { - topLevelCfg := repoowners.Config{ + var topLevelCfg *repoowners.Config + if !r.simpleConfig.Empty() { + cfg := repoowners.Config{ Approvers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Approvers)))), Reviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Reviewers)))), RequiredReviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.RequiredReviewers)))), Labels: sets.List(sets.New[string](r.simpleConfig.Labels...)), } - if len(topLevelCfg.Reviewers) == 0 { - topLevelCfg.Reviewers = topLevelCfg.Approvers + if len(cfg.Reviewers) == 0 { + cfg.Reviewers = cfg.Approvers } - fc.Filters[".*"] = topLevelCfg + if len(cfg.Approvers) > 0 || len(cfg.Reviewers) > 0 || len(cfg.RequiredReviewers) > 0 || len(cfg.Labels) > 0 { + topLevelCfg = &cfg + } } // Add all the specific filters from fullConfig for k, v := range r.fullConfig.Filters { @@ } fc.Filters[k] = cfg } + if topLevelCfg != nil { + cfg := *topLevelCfg + if existing, ok := fc.Filters[".*"]; ok { + cfg.Approvers = sets.List(sets.New[string](append(cfg.Approvers, existing.Approvers...)...)) + cfg.Reviewers = sets.List(sets.New[string](append(cfg.Reviewers, existing.Reviewers...)...)) + cfg.RequiredReviewers = sets.List(sets.New[string](append(cfg.RequiredReviewers, existing.RequiredReviewers...)...)) + cfg.Labels = sets.List(sets.New[string](append(cfg.Labels, existing.Labels...)...)) + } + fc.Filters[".*"] = cfg + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/autoowners/main.go` around lines 149 - 176, The current logic builds a topLevelCfg then later unconditionally writes fullConfig filters which can overwrite or ignore top-level owners; instead, construct topLevelCfg from r.simpleConfig as you already do, then populate fc.Filters from r.fullConfig.Filters, and finally merge topLevelCfg into fc.Filters[".*"] (creating it if missing) by taking the union of Approvers, Reviewers, RequiredReviewers and Labels (using the same cleaner/sets/ExpandAliases/NormLogins flow), and ensure the Reviewer fallback (if len(Reviewers)==0 then Reviewers=Approvers) happens after the merge; update references to r.simpleConfig, r.fullConfig, topLevelCfg and fc.Filters accordingly.
🧹 Nitpick comments (2)
go.mod (1)
135-135:github.com/tektoncd/pipeline v1.6.1is an indirect transitive dependency with available security updates.While v1.6.1 is pinned in go.mod (line 135), the package does not appear to be directly imported or actively used in the codebase—it exists only as an indirect dependency. However, v1.6.2 and later contain additional security fixes (CVE-2026-40161, CVE-2026-40938) not included in v1.6.1. Consider updating to v1.6.2 or later if feasible, though the risk is mitigated by the apparent lack of direct usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 135, go.mod pins the indirect module github.com/tektoncd/pipeline at v1.6.1 which has known security fixes in v1.6.2+; update the requirement to v1.6.2 (or later) by running "go get github.com/tektoncd/pipeline@v1.6.2" (or editing the require line for github.com/tektoncd/pipeline) and then run "go mod tidy" to regenerate the lockfile, followed by your test suite and a build to ensure no regressions from the indirect upgrade.cmd/autoowners/main_test.go (1)
588-613: Make the mixed alias-cleaning test input-sensitive.The cleaner at Lines 558-560 returns
"hans"for every input, so this new mixed case would still pass ifresolveOwnerAliasesaccidentally cleaned the wrong field or an empty slice. UsenoOpCleaneror an input-aware cleaner here so the test proves top-level owners and filter-specific owners are preserved in the expected".*"and".*\\.go$"entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/autoowners/main_test.go` around lines 588 - 613, The test "mixed config - top-level becomes .* filter" currently relies on a cleaner that always returns "hans", which hides bugs; change the test to use noOpCleaner or an input-aware cleaner instead of the constant-returning cleaner so the cleaner preserves actual inputs and the test verifies that top-level owners (SimpleConfig Approvers/Reviewers) end up under ".*" while the filter-specific owners remain under ".*\\.go$"; update the call that invokes resolveOwnerAliases (or where the cleaner is passed) to use noOpCleaner (or a lambda that returns its input) and ensure expectedResult still asserts the original input owners appear in the ".*" and ".*\\.go$" entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 210: Update the OpenTelemetry modules in go.mod from v1.40.0 to v1.43.0
for all affected packages to address CVE-2026-39883: change versions for
go.opentelemetry.io/otel/sdk, go.opentelemetry.io/otel/sdk/metric,
go.opentelemetry.io/otel, go.opentelemetry.io/otel/metric, and
go.opentelemetry.io/otel/trace to v1.43.0, then run dependency resolution (e.g.,
go get go.opentelemetry.io/otel@v1.43.0 and go mod tidy) to update go.sum and
ensure the module graph is consistent.
- Line 275: The dependency github.com/go-jose/go-jose/v4 listed as v4.1.3 in the
module manifest is vulnerable (GHSA-78h2-9frx-2jm8); update that module version
to v4.1.4 in the go.mod to incorporate the JWE decryption panic fix, then run
`go get github.com/go-jose/go-jose/v4@v4.1.4` (or edit go.mod to v4.1.4 and run
`go mod tidy`) to update go.sum and vendor state so the project uses the patched
release.
---
Outside diff comments:
In `@cmd/autoowners/main.go`:
- Around line 149-176: The current logic builds a topLevelCfg then later
unconditionally writes fullConfig filters which can overwrite or ignore
top-level owners; instead, construct topLevelCfg from r.simpleConfig as you
already do, then populate fc.Filters from r.fullConfig.Filters, and finally
merge topLevelCfg into fc.Filters[".*"] (creating it if missing) by taking the
union of Approvers, Reviewers, RequiredReviewers and Labels (using the same
cleaner/sets/ExpandAliases/NormLogins flow), and ensure the Reviewer fallback
(if len(Reviewers)==0 then Reviewers=Approvers) happens after the merge; update
references to r.simpleConfig, r.fullConfig, topLevelCfg and fc.Filters
accordingly.
---
Nitpick comments:
In `@cmd/autoowners/main_test.go`:
- Around line 588-613: The test "mixed config - top-level becomes .* filter"
currently relies on a cleaner that always returns "hans", which hides bugs;
change the test to use noOpCleaner or an input-aware cleaner instead of the
constant-returning cleaner so the cleaner preserves actual inputs and the test
verifies that top-level owners (SimpleConfig Approvers/Reviewers) end up under
".*" while the filter-specific owners remain under ".*\\.go$"; update the call
that invokes resolveOwnerAliases (or where the cleaner is passed) to use
noOpCleaner (or a lambda that returns its input) and ensure expectedResult still
asserts the original input owners appear in the ".*" and ".*\\.go$" entries.
In `@go.mod`:
- Line 135: go.mod pins the indirect module github.com/tektoncd/pipeline at
v1.6.1 which has known security fixes in v1.6.2+; update the requirement to
v1.6.2 (or later) by running "go get github.com/tektoncd/pipeline@v1.6.2" (or
editing the require line for github.com/tektoncd/pipeline) and then run "go mod
tidy" to regenerate the lockfile, followed by your test suite and a build to
ensure no regressions from the indirect upgrade.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 34b038a0-d05d-480e-be61-417d8fe1ea47
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/autoowners/main.gocmd/autoowners/main_test.gogo.modpkg/prowconfigsharding/prowconfigsharding_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/prowconfigsharding/prowconfigsharding_test.go
| go.opentelemetry.io/otel/sdk/metric v1.37.0 // indirect | ||
| go.opentelemetry.io/auto/sdk v1.2.1 // indirect | ||
| go.opentelemetry.io/contrib/detectors/gcp v1.39.0 // indirect | ||
| go.opentelemetry.io/otel/sdk v1.40.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm OTel pins in go.mod:"
rg -n 'go\.opentelemetry\.io/otel(/sdk|/metric|/trace|/sdk/metric)?\s+v1\.40\.0' go.mod
echo
echo "2) Check if otel/sdk is imported in code:"
rg -n --type=go -C2 'go\.opentelemetry\.io/otel/sdk' .
echo
echo "3) Query OSV for v1.40.0:"
curl -s https://api.osv.dev/v1/query -d '{
"package":{"name":"go.opentelemetry.io/otel/sdk","ecosystem":"Go"},
"version":"1.40.0"
}' | jq .Repository: openshift/ci-tools
Length of output: 414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Querying OSV for otel/sdk v1.40.0 ==="
curl -s -X POST https://api.osv.dev/v1/query \
-H "Content-Type: application/json" \
-d '{
"package": {"name": "go.opentelemetry.io/otel/sdk", "ecosystem": "Go"},
"version": "1.40.0"
}' 2>&1 | head -100
echo
echo "=== Querying OSV for otel v1.40.0 ==="
curl -s -X POST https://api.osv.dev/v1/query \
-H "Content-Type: application/json" \
-d '{
"package": {"name": "go.opentelemetry.io/otel", "ecosystem": "Go"},
"version": "1.40.0"
}' 2>&1 | head -100Repository: openshift/ci-tools
Length of output: 3158
🌐 Web query:
go.opentelemetry.io/otel v1.40.0 security vulnerability advisory
💡 Result:
There is no security vulnerability in go.opentelemetry.io/otel v1.40.0 itself. Instead, v1.40.0 fixes a vulnerability (CVE-2026-24051 / GHSA-9h8m-3fm2-qjrq / likely GO-2026-4394) affecting prior versions v1.20.0 to v1.39.0. The vulnerability is Path Hijacking (Untrusted Search Paths, CWE-426) on macOS/Darwin systems in the resource detection code (sdk/resource/host_id.go). It executes the 'ioreg' system command using a PATH-dependent search, allowing a local attacker who can modify the PATH environment variable to achieve Arbitrary Code Execution (ACE) in the application's context. Fix: Commit d45961b uses the full path for the ioreg command, released in v1.40.0 on 2026-02-02. Many projects (etcd, helm, livekit, kubernetes/autoscaler, etc.) upgraded to v1.40.0 to remediate this. Note: A later vulnerability (CVE-2026-39882, GHSA-w8rr-5gcm-pp58) affects OTLP HTTP exporters prior to v1.43.0 (memory exhaustion from unbounded response bodies), but v1.40.0 is not vulnerable to that.
Citations:
- 1: Bump go.opentelemetry.io/otel/sdk to v1.40.0 to resolve https://pkg.go.dev/vuln/GO-2026-4394 etcd-io/etcd#21337
- 2: High severity vulnerability in LiveKit dependency: go.opentelemetry.io/otel/sdk/resource (SNYK-GOLANG-…15182758) livekit/livekit#4300
- 3: fix(deps): update module go.opentelemetry.io/otel/sdk to v1.40.0 [security] open-telemetry/opentelemetry-ebpf-instrumentation#1384
- 4: helm/helm@5b26d4f
- 5: https://nvd.nist.gov/vuln/detail/CVE-2026-24051
- 6: https://vulert.com/vuln-db/CVE-2026-24051
- 7: https://www.tenable.com/cve/CVE-2026-24051
- 8: https://nvd.nist.gov/vuln/detail/cve-2026-24051
- 9: https://github.com/advisories/GHSA-9h8m-3fm2-qjrq/dependabot
- 10: https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.40.0
🌐 Web query:
go.opentelemetry.io/otel SDK v1.40.0 patched version fix
💡 Result:
The go.opentelemetry.io/otel SDK v1.40.0 is the patched version that fixes a security vulnerability (CVE-2026-24051, GO-2026-4394). Versions v1.20.0 through v1.39.0 are vulnerable to path hijacking on macOS/Darwin systems, where the resource detection code in sdk/resource/host_id.go executes the 'ioreg' command using an untrusted PATH, allowing arbitrary code execution if PATH is modified locally. The fix, in commit d45961b, uses the full path for the 'ioreg' command on Darwin (macOS). This was released on 2026-02-02. To upgrade, update your go.mod: go get go.opentelemetry.io/otel/sdk@v1.40.0 Official release notes: https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.40.0
Citations:
- 1: Bump go.opentelemetry.io/otel/sdk to v1.40.0 to resolve https://pkg.go.dev/vuln/GO-2026-4394 etcd-io/etcd#21337
- 2: helm/helm@5b26d4f
- 3: fix(deps): update module go.opentelemetry.io/otel/sdk to v1.40.0 [security] open-telemetry/opentelemetry-ebpf-instrumentation#1384
- 4: https://nvd.nist.gov/vuln/detail/CVE-2026-24051
- 5: https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.40.0
Bump go.opentelemetry.io/otel* packages from v1.40.0 to v1.43.0 to resolve HIGH-severity advisory GHSA-hfvc-g4fc-pqhx (CVE-2026-39883).
Version 1.40.0 is vulnerable to BSD kenv command PATH hijacking (CWE-426), affecting FreeBSD, OpenBSD, NetBSD, DragonFly BSD, and Solaris. Update all five packages consistently (lines 210, 211, 328–330):
go.opentelemetry.io/otel/sdkgo.opentelemetry.io/otel/sdk/metricgo.opentelemetry.io/otelgo.opentelemetry.io/otel/metricgo.opentelemetry.io/otel/trace
🧰 Tools
🪛 OSV Scanner (2.3.5)
[HIGH] 210-210: go.opentelemetry.io/otel/sdk 1.40.0: opentelemetry-go: BSD kenv command not using absolute path enables PATH hijacking
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 210, Update the OpenTelemetry modules in go.mod from v1.40.0
to v1.43.0 for all affected packages to address CVE-2026-39883: change versions
for go.opentelemetry.io/otel/sdk, go.opentelemetry.io/otel/sdk/metric,
go.opentelemetry.io/otel, go.opentelemetry.io/otel/metric, and
go.opentelemetry.io/otel/trace to v1.43.0, then run dependency resolution (e.g.,
go get go.opentelemetry.io/otel@v1.43.0 and go mod tidy) to update go.sum and
ensure the module graph is consistent.
| github.com/go-git/go-billy/v5 v5.6.2 // indirect | ||
| github.com/go-git/go-git/v5 v5.14.0 // indirect | ||
| github.com/go-jose/go-jose/v4 v4.1.2 // indirect | ||
| github.com/go-jose/go-jose/v4 v4.1.3 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm pin in go.mod:"
rg -n 'github.com/go-jose/go-jose/v4\s+v4\.1\.3' go.mod
echo
echo "2) Check reachability/imports:"
rg -n --type=go -C2 'go-jose|jose\.New|jwt' .
echo
echo "3) Query OSV for exact version:"
curl -s https://api.osv.dev/v1/query -d '{
"package":{"name":"github.com/go-jose/go-jose/v4","ecosystem":"Go"},
"version":"4.1.3"
}' | jq .Repository: openshift/ci-tools
Length of output: 7289
🏁 Script executed:
#!/bin/bash
echo "1) Check go.sum for go-jose transitive dependency source:"
rg 'go-jose/go-jose' go.sum | head -20
echo
echo "2) Search for any go-jose imports (case-insensitive, broader pattern):"
rg -i 'go\.jose|gojose|go-jose' --type=go . 2>/dev/null | grep -i import | head -20
echo
echo "3) List all direct imports in main modules to find what brings in go-jose:"
rg '^require \(' -A 50 go.mod | grep -v indirect | head -30
echo
echo "4) Check if go-jose is used in any non-test code via ast-grep:"
ast-grep --pattern 'jose' 2>/dev/null | head -20Repository: openshift/ci-tools
Length of output: 1904
🏁 Script executed:
#!/bin/bash
echo "1) Check Vault dependency to confirm it brings in go-jose:"
grep -A 2 'github.com/hashicorp/vault' go.mod
echo
echo "2) Search OSV for Vault advisories related to go-jose or similar issues:"
curl -s https://api.osv.dev/v1/query -d '{
"package":{"name":"github.com/hashicorp/vault","ecosystem":"Go"},
"version":"1.16.0"
}' 2>/dev/null | jq '.vulns[] | select(.details | contains("jose") or contains("JWE")) | {id, summary}' 2>/dev/null
echo
echo "3) Check if Vault uses go-jose for JWT vs JWE (vault typically uses JWT, not JWE):"
curl -s "https://api.osv.dev/v1/query" -d '{
"package":{"name":"github.com/hashicorp/vault/api","ecosystem":"Go"}
}' 2>/dev/null | jq '.vulns[0:3] | .[] | {id, summary, details: (.details | split("\n") | .[0:2])}' 2>/dev/nullRepository: openshift/ci-tools
Length of output: 436
Update github.com/go-jose/go-jose/v4 from v4.1.3 to v4.1.4 to patch HIGH severity vulnerability GHSA-78h2-9frx-2jm8.
Version 4.1.3 is affected by a JWE decryption panic that can lead to denial of service when key wrapping algorithms are used with an empty encrypted_key field. The fix is available in v4.1.4.
🧰 Tools
🪛 OSV Scanner (2.3.5)
[HIGH] 275-275: github.com/go-jose/go-jose/v4 4.1.3: Go JOSE Panics in JWE decryption
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 275, The dependency github.com/go-jose/go-jose/v4 listed as
v4.1.3 in the module manifest is vulnerable (GHSA-78h2-9frx-2jm8); update that
module version to v4.1.4 in the go.mod to incorporate the JWE decryption panic
fix, then run `go get github.com/go-jose/go-jose/v4@v4.1.4` (or edit go.mod to
v4.1.4 and run `go mod tidy`) to update go.sum and vendor state so the project
uses the patched release.
The autoowners tool previously only checked for filters if the top-level SimpleConfig was empty. This caused OWNERS files with both top-level approvers/reviewers AND filters to have their filters silently ignored. This change modifies the logic to: 1. Always attempt to load the file as FullConfig first 2. Use FullConfig if filters are present 3. Fall back to SimpleConfig only if no filters exist This allows repository OWNERS files to define both default approvers/reviewers at the top level and filter-specific overrides for different file patterns, which is a valid and useful configuration pattern. Added test coverage for the mixed config scenario to prevent regression. Fixes: DPTP-XXXX
Address CodeRabbit feedback to fix edge cases and regressions: 1. **Fallback to SimpleConfig when FullConfig fails**: Previously, if LoadFullConfig failed on a pure SimpleConfig file, the entire parsing would fail. Now we try SimpleConfig first (which works for all valid OWNERS files), then also try FullConfig to extract filters. 2. **Preserve both top-level config AND filters**: When an OWNERS file has both top-level approvers/reviewers AND filters, we now preserve both in httpResult. Previously only one would be kept. 3. **Merge top-level into FullConfig output**: When writing OWNERS files with both top-level and filters, we convert the top-level config into a ".*" catch-all filter, matching Prow's FullConfig format. This ensures files not matched by specific filters fall back to top-level approvers/reviewers. 4. **Comprehensive test coverage**: Added tests for: - Pure SimpleConfig files (no filters) - Pure FullConfig files (only filters, no top-level) - Mixed files (both top-level and filters) - Verify resolveOwnerAliases correctly merges both into FullConfig These changes ensure autoowners handles all valid OWNERS file formats without data loss or parsing failures.
christophermancini
force-pushed
the
fix/autoowners-support-mixed-config
branch
from
e95d1e9 to
62b1fa4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If FullConfig fails but we have SimpleConfig, that's OK | ||
| if !simple.Empty() { |
There was a problem hiding this comment.
When repoowners.LoadFullConfig fails and simple is non-empty, the error is currently swallowed without any log/visibility. This can hide broken/misindented filters: blocks and lead to silently ignoring intended filter rules. Consider logging a warning/debug message in this branch indicating that FullConfig parsing failed and filters (if any) will be ignored, while continuing with SimpleConfig.
| // If FullConfig fails but we have SimpleConfig, that's OK | |
| if !simple.Empty() { | |
| // If FullConfig fails but we have SimpleConfig, continue with it but | |
| // make the degraded behavior visible because any filters will be ignored. | |
| if !simple.Empty() { | |
| logrus.WithError(err).Warn("Unable to load full config; continuing with simple config and ignoring filters, if any.") |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/autoowners/main.go`:
- Around line 245-252: Currently when repoowners.LoadFullConfig(data) returns an
error the code silently falls back to the previously-parsed simple config if
simple.Empty() is false; change this so parsing errors from LoadFullConfig are
surfaced instead of ignored: remove the conditional fallback (the if
!simple.Empty() break logic) and always treat a non-nil err from
repoowners.LoadFullConfig as a fatal parse error—log the error with context
(using logrus.WithError(err).Error with a clear message referencing OWNERS
parsing) and return the error (return httpResult, err) so malformed mixed
configs are rejected; adjust any related control flow around the full, err,
simple variables accordingly.
- Around line 178-195: The code treats a SimpleConfig with only Options set as
empty because repoowners.SimpleConfig.Empty() ignores Options; update the logic
so Options are considered non-empty: either modify
repoowners.SimpleConfig.Empty() to return false when Options (e.g.,
no_parent_owners, auto_approve_unowned_subfolders) are set, or change the branch
in main.go (the block constructing sc and the fallback that returns
SimpleConfig{}) to check r.simpleConfig.Options (or a helper like
SimpleConfig.HasOptions()) in addition to approver/reviewer lists before
treating the config as empty; ensure references to SimpleConfig.Empty(),
r.simpleConfig, sc, and the fallback return are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6092ad24-b23b-4b2a-bcf9-f0aeff8887b7
📒 Files selected for processing (2)
cmd/autoowners/main.gocmd/autoowners/main_test.go
| } else if !r.simpleConfig.Empty() { | ||
| // No filters, just use SimpleConfig | ||
| sc := SimpleConfig{ | ||
| Config: repoowners.Config{ | ||
| Approvers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Approvers)))), | ||
| Reviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Reviewers)))), | ||
| RequiredReviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.RequiredReviewers)))), | ||
| Labels: sets.List(sets.New[string](r.simpleConfig.Labels...)), | ||
| }, | ||
| Options: r.simpleConfig.Options, | ||
| } | ||
| if len(sc.Reviewers) == 0 { | ||
| sc.Reviewers = sc.Approvers | ||
| } | ||
| return sc | ||
| } | ||
| // Empty config - should not happen but return empty SimpleConfig | ||
| return SimpleConfig{} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In sigs.k8s.io/prow/pkg/repoowners, does repoowners.SimpleConfig define its own Empty method, or is Empty promoted from the embedded Config? Does that emptiness check include Options such as no_parent_owners and auto_approve_unowned_subfolders?
💡 Result:
In sigs.k8s.io/prow/pkg/repoowners, repoowners.SimpleConfig defines its own Empty method, not promoted from the embedded Config. The method checks only the lists from the embedded Config (Approvers, Reviewers, RequiredReviewers, Labels) and ignores the Options field: func (s *SimpleConfig) Empty bool { return len(s.Approvers) == 0 && len(s.Reviewers) == 0 && len(s.RequiredReviewers) == 0 && len(s.Labels) == 0 } Thus, the emptiness check does NOT include Options such as no_parent_owners and auto_approve_unowned_subfolders. A SimpleConfig with only options set (e.g., no_parent_owners: true) and empty lists would return true from Empty.
Citations:
🏁 Script executed:
#!/bin/bash
# Check the actual code in cmd/autoowners/main.go around the flagged lines
cat -n cmd/autoowners/main.go | sed -n '135,260p'Repository: openshift/ci-tools
Length of output: 1870
OWNERS files with only options: set are dropped during round-trip.
repoowners.SimpleConfig.Empty() checks only the approver/reviewer lists and ignores the Options field entirely. A file with just no_parent_owners: true or auto_approve_unowned_subfolders: true and no approvers/reviewers returns Empty() == true, causing valid configuration to be lost on read-write cycles.
This affects both the serialization gate at lines 178–195 and the fallback logic at lines 232–241.
Also applies to: 239-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/autoowners/main.go` around lines 178 - 195, The code treats a
SimpleConfig with only Options set as empty because
repoowners.SimpleConfig.Empty() ignores Options; update the logic so Options are
considered non-empty: either modify repoowners.SimpleConfig.Empty() to return
false when Options (e.g., no_parent_owners, auto_approve_unowned_subfolders) are
set, or change the branch in main.go (the block constructing sc and the fallback
that returns SimpleConfig{}) to check r.simpleConfig.Options (or a helper like
SimpleConfig.HasOptions()) in addition to approver/reviewer lists before
treating the config as empty; ensure references to SimpleConfig.Empty(),
r.simpleConfig, sc, and the fallback return are updated accordingly.
| full, err := repoowners.LoadFullConfig(data) | ||
| if err != nil { | ||
| // If FullConfig fails but we have SimpleConfig, that's OK | ||
| if !simple.Empty() { | ||
| break | ||
| } | ||
| logrus.WithError(err).Error("Unable to load full config.") | ||
| return httpResult, err |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In sigs.k8s.io/prow/pkg/repoowners, how do LoadSimpleConfig and LoadFullConfig behave for an OWNERS file with valid top-level approvers/reviewers but an invalid filters section? Does LoadSimpleConfig ignore the filters key while LoadFullConfig returns an error?
💡 Result:
Yes, LoadSimpleConfig ignores the filters key while LoadFullConfig returns an error for an OWNERS file with valid top-level approvers/reviewers but an invalid filters section. LoadSimpleConfig unmarshals YAML bytes into a SimpleConfig struct, which has fields Options and inline Config (containing approvers, reviewers, etc.) but no filters field. Go's yaml.Unmarshal ignores unknown fields by default, so an invalid filters section (e.g., malformed YAML) would still parse the valid top-level fields successfully if the YAML is otherwise valid up to that point, though strictly invalid YAML causes unmarshal error for both. LoadFullConfig unmarshals into FullConfig, which has Options and Filters map[string]Config. An invalid filters section (malformed map or Config values) causes unmarshal to fail with a YAML syntax or type error. Both functions use plain yaml.Unmarshal without custom error handling or strict mode, returning the unmarshal error directly.
Citations:
- 1: https://sigs.k8s.io/prow/pkg/repoowners/repoowners.go
- 2: https://github.com/kubernetes-sigs/prow/blob/master/pkg/repoowners/repoowners.go
Reject silently falling back to SimpleConfig when FullConfig parsing fails.
If an OWNERS file has valid top-level approvers/reviewers but an invalid filters: block, LoadSimpleConfig succeeds (ignoring the unknown key), while LoadFullConfig fails. The current code falls back to SimpleConfig, converting a malformed mixed config into a valid default-only file and dropping filter overrides instead of surfacing the parsing error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/autoowners/main.go` around lines 245 - 252, Currently when
repoowners.LoadFullConfig(data) returns an error the code silently falls back to
the previously-parsed simple config if simple.Empty() is false; change this so
parsing errors from LoadFullConfig are surfaced instead of ignored: remove the
conditional fallback (the if !simple.Empty() break logic) and always treat a
non-nil err from repoowners.LoadFullConfig as a fatal parse error—log the error
with context (using logrus.WithError(err).Error with a clear message referencing
OWNERS parsing) and return the error (return httpResult, err) so malformed mixed
configs are rejected; adjust any related control flow around the full, err,
simple variables accordingly.
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
2 similar comments
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
Previously, if an OWNERS file had both top-level approvers/reviewers AND an explicit ".*" filter, the top-level config was silently dropped because the check `r.fullConfig.Filters[".*"].Approvers == nil` would skip the merge. Changes: - Process all filters from fullConfig first - Then merge top-level config into ".*" filter (creating or merging) - Use set union to combine approvers/reviewers/labels when merging Added comprehensive test coverage: - Test case for top-level + non-".*" filters (becomes ".*") - Test case for top-level + explicit ".*" filter (merge case) - Tests use noOpCleaner to verify actual values (not hidden by cleaner) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmd/autoowners/main.go (2)
252-259:⚠️ Potential issue | 🟠 MajorDo not swallow
LoadFullConfigparse errors.At Line 255, falling back via
breakhides malformedfiltersand silently drops filter-based ownership rules. IfLoadFullConfigfails, return the parse error instead of proceeding with only simple config.Suggested fix
full, err := repoowners.LoadFullConfig(data) if err != nil { - // If FullConfig fails but we have SimpleConfig, that's OK - if !simple.Empty() { - break - } logrus.WithError(err).Error("Unable to load full config.") return httpResult, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/autoowners/main.go` around lines 252 - 259, The code currently swallows parse errors from repoowners.LoadFullConfig by executing a break when err != nil and simple.Empty() == false; change this to return the parse error instead of breaking so malformed `filters` aren't silently dropped: in the block handling `full, err := repoowners.LoadFullConfig(data)` (and referencing `simple.Empty()`), replace the `break` path with returning the error (e.g., return httpResult, err) so LoadFullConfig parse errors are propagated rather than ignored.
164-165:⚠️ Potential issue | 🟠 MajorOptions-only
OWNERSconfig can still be dropped.Line 247 and Line 185 rely on
simple.Empty(), so an options-only file can be treated as empty and written back as{}. That loses validoptionssettings during round-trip. Please gate on “has config entries OR has options” instead ofEmpty()alone.In sigs.k8s.io/prow/pkg/repoowners (version v0.0.0-20260227184331-937f24a5dcd2), does SimpleConfig.Empty() include Options fields (no_parent_owners, auto_approve_unowned_subfolders), or only approvers/reviewers/required_reviewers/labels?Also applies to: 185-202, 247-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/autoowners/main.go` around lines 164 - 165, The current guard uses r.simpleConfig.Empty() which treats an options-only OWNERS file as empty and causes options to be dropped; change the condition where topLevelCfg is created (the r.simpleConfig.Empty() check) to also detect options presence by checking the options fields on simpleConfig (e.g., r.simpleConfig.Options.NoParentOwners, r.simpleConfig.Options.AutoApproveUnownedSubfolders, or any Options struct non-nil/filled) so you only treat the config as empty when there are neither approvers/reviewers/required_reviewers/labels nor any options set; apply the same change to the other similar checks that rely on simpleConfig.Empty() (the blocks around where repoowners.Config is constructed and where the file is written back).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/autoowners/main.go`:
- Around line 252-259: The code currently swallows parse errors from
repoowners.LoadFullConfig by executing a break when err != nil and
simple.Empty() == false; change this to return the parse error instead of
breaking so malformed `filters` aren't silently dropped: in the block handling
`full, err := repoowners.LoadFullConfig(data)` (and referencing
`simple.Empty()`), replace the `break` path with returning the error (e.g.,
return httpResult, err) so LoadFullConfig parse errors are propagated rather
than ignored.
- Around line 164-165: The current guard uses r.simpleConfig.Empty() which
treats an options-only OWNERS file as empty and causes options to be dropped;
change the condition where topLevelCfg is created (the r.simpleConfig.Empty()
check) to also detect options presence by checking the options fields on
simpleConfig (e.g., r.simpleConfig.Options.NoParentOwners,
r.simpleConfig.Options.AutoApproveUnownedSubfolders, or any Options struct
non-nil/filled) so you only treat the config as empty when there are neither
approvers/reviewers/required_reviewers/labels nor any options set; apply the
same change to the other similar checks that rely on simpleConfig.Empty() (the
blocks around where repoowners.Config is constructed and where the file is
written back).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1423d4b2-a7ac-4680-bcff-630b0fa71d13
📒 Files selected for processing (2)
cmd/autoowners/main.gocmd/autoowners/main_test.go
|
@christophermancini: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
The autoowners tool previously only checked for filters if the top-level SimpleConfig was empty. This caused OWNERS files with both top-level approvers/reviewers AND filters to have their filters silently ignored.
This change modifies the logic to:
This allows repository OWNERS files to define both default approvers/reviewers at the top level and filter-specific overrides for different file patterns, which is a valid and useful configuration pattern.
Added test coverage for the mixed config scenario to prevent regression.
Fixes: DPTP-XXXX
Summary by CodeRabbit
Bug Fixes
Tests