fix(attest): don't preserve file ownership when packaging attachments - #1076
Merged
Merged
Conversation
Contributor
|
Claude finished @mbevc1's task in 1m 10s —— View job ReviewI re-reviewed after the latest push. The fix is correct, minimal, and well-tested. Both points from the earlier review are now resolved. Static analysis
Correctness of the fix
Tests — thorough and thoughtfully written:
Minor observations (non-blocking)
Nice work — the commit messages and the in-code comment ( LGTM. 👍 |
Passing two or more evidence paths to `kosli attest` staged each path into a
temp dir before tarring, and asked github.com/otiai10/copy to preserve uid and
gid. That is implemented with Lchown, which a non-root process cannot use to
chown a file to a foreign uid, so the command aborted with
chown /tmp/1800768837/reports/rubocop/junit.xml: operation not permitted
whenever any attachment had been written by a docker container rather than by
the CLI user. Lint, test and coverage output produced in a container is the
common case, so adding a second --attachments broke commands that were green
the day before.
The copy is only a staging area for the tarball that gets uploaded, and nothing
downstream reads uid or gid. The single-path branch never copies and so never
chowned, which is why it kept working. Dropping `PreserveOwner` makes the
multi-path branch agree with it.
Also wrap the copy error with the evidence path the user passed, so a genuine
failure names their own input rather than only a temp directory they never
created.
Fixes #1075
The Lint job flagged four errcheck violations in the new tests: two deferred
Close calls in readTarHeaders and two deferred os.RemoveAll cleanups. All four
are deliberately ignorable, so ignore them explicitly with the
`defer func() { _ = ... }()` idiom already used elsewhere in these tests.
Also wrap the copy error with %w rather than %v, so a caller can still reach the
underlying error through errors.Is/errors.As, and assert that in the test.
mbevc1
enabled auto-merge (squash)
August 3, 2026 15:33
JonJagger
approved these changes
Aug 3, 2026
social4hyq
pushed a commit
to social4hyq/homebrew-core
that referenced
this pull request
Sep 20, 2026
kosli-cli 2.36.4 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>- Added `pull_request`, `merge_request` as additional command aliases for `assert pullrequest` and `attest pullrequest` (alongside existing `pr`, `mr`, `mergerequest`). - Fixed a bug where packaging multiple evidence paths (e.g. `--attachments`) would fail with "operation not permitted" when any attachment was owned by a different user (e.g. files written by a Docker container). Ownership is no longer preserved during staging. - Improved error messages when an evidence attachment path cannot be packaged: the error now names the path the user provided rather than an internal temp directory. ## What's Changed * chore: add cmd aliases by @mbevc1 in kosli-dev/cli#1066 * test(aws): add S3 contract tests by @mbevc1 in kosli-dev/cli#1065 * fix(docs): separate flag types into a column by @mbevc1 in kosli-dev/cli#1068 * chore(deps): bump the github-actions-dependencies group with 2 updates by @dependabot[bot] in kosli-dev/cli#1070 * chore(deps): bump the go-dependencies group with 10 updates by @dependabot[bot] in kosli-dev/cli#1071 * fix(attest): don't preserve file ownership when packaging attachments by @mbevc1 in kosli-dev/cli#1076 **Full Changelog**: https://github.com/kosli-dev/cli/compare/v2.36.3...v2.36.4</pre> <p>View the full release notes at <a href="https://github.com/kosli-dev/cli/releases/tag/v2.36.4">https://github.com/kosli-dev/cli/releases/tag/v2.36.4</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!15762
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
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.
Passing two or more evidence paths to
kosli atteststaged each path into a temp dir before tarring, and asked github.com/otiai10/copy to preserve uid and gid. That is implemented with Lchown, which a non-root process cannot use to chown a file to a foreign uid, so the command aborted withwhenever any attachment had been written by a docker container rather than by the CLI user. Lint, test and coverage output produced in a container is the common case, so adding a second --attachments broke commands that were green the day before.
The copy is only a staging area for the tarball that gets uploaded, and nothing downstream reads uid or gid. The single-path branch never copies and so never chowned, which is why it kept working. Dropping
PreserveOwnermakes the multi-path branch agree with it.Also wrap the copy error with the evidence path the user passed, so a genuine failure names their own input rather than only a temp directory they never created.
Fixes #1075
Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR