Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate all files have at least read permissions #186

Merged
merged 5 commits into from
Apr 25, 2024
Merged

Validate all files have at least read permissions #186

merged 5 commits into from
Apr 25, 2024

Conversation

academo
Copy link
Member

@academo academo commented Apr 25, 2024 •
edited

  • Add validation to check that files in the manifest have at least read permissions. Note that there is a separate validation step to check binary permissions
  • Formatted the files with gofmt, goimports and golines for more readability

Closes #185

@academo academo requested review from a team as code owners April 25, 2024 09:20
@academo academo requested review from Ukochka, wbrowne, andresmgot and xnyo and removed request for a team April 25, 2024 09:20
@academo academo self-assigned this Apr 25, 2024
academo
academo commented Apr 25, 2024
View reviewed changes
pkg/analysis/passes/manifest/manifest.go
@@ -9,13 +9,13 @@ import (
"path/filepath"
Copy link
Member Author

@academo academo Apr 25, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: format applied all the big changes are simply line splitting for better readability.

academo
academo commented Apr 25, 2024
View reviewed changes
pkg/analysis/passes/manifest/manifest.go
return nil
}

mode := file.Mode()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the relevant change.

academo
academo commented Apr 25, 2024
View reviewed changes
pkg/analysis/passes/manifest/manifest_test.go
)
}

func TestWithWrongPermissions(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the relevant new test.

academo
academo commented Apr 25, 2024
View reviewed changes
pkg/analysis/passes/manifest/testdata/with-wrong-permissions/CHANGELOG.md
@@ -0,0 +1,62 @@
# Change Log
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore all the files from here to the end. they are fixtures for the test

marefr
marefr approved these changes Apr 25, 2024
View reviewed changes
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@academo academo merged commit b4d0886 into main Apr 25, 2024
3 checks passed
@academo academo deleted the academo/add-full-read-permissions-validation branch April 25, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marefr marefr marefr approved these changes

@Ukochka Ukochka Awaiting requested review from Ukochka Ukochka is a code owner automatically assigned from grafana/plugins-platform-frontend

@wbrowne wbrowne Awaiting requested review from wbrowne wbrowne is a code owner automatically assigned from grafana/plugins-platform-backend

@andresmgot andresmgot Awaiting requested review from andresmgot andresmgot is a code owner automatically assigned from grafana/plugins-platform-backend

@xnyo xnyo Awaiting requested review from xnyo xnyo is a code owner automatically assigned from grafana/plugins-platform-backend

@tolzhabayev tolzhabayev Awaiting requested review from tolzhabayev

Assignees

@academo academo

Labels
None yet
Projects
Plugins Platform
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verify file permissions in zip files
2 participants
@academo @marefr