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

fix(cosmovisor): fixed cosmovisor add-upgrade permissions #20062

Merged
merged 6 commits into from
May 2, 2024
Merged

fix(cosmovisor): fixed cosmovisor add-upgrade permissions #20062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9751b9d
fix: fixed cosmovisor add-upgrade permissions
freak12techno Apr 16, 2024
d119102
chore: fixed permissions
freak12techno Apr 17, 2024
f29a766
chore: add changelog entry
freak12techno Apr 17, 2024
e81cdd5
chore: moved changelog entry
freak12techno Apr 17, 2024
905649b
chore: fixed logging upgrade-info path
freak12techno Apr 18, 2024
affe403
Merge branch 'main' into fix-cosmovisor-add-upgrade-permissions
freak12techno Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions tools/cosmovisor/CHANGELOG.md
View file
Open in desktop
Expand Up @@ -35,6 +35,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
# Changelog

## [Unreleased]
* [#20062](https://github.com/cosmos/cosmos-sdk/pull/20062) Fixed cosmovisor add-upgrade permissions
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure the changelog entry is surrounded by blank lines for better readability and adherence to the changelog guidelines.

37a38
> 
39a40
>
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#20062](https://github.com/cosmos/cosmos-sdk/pull/20062) Fixed cosmovisor add-upgrade permissions
* [#20062](https://github.com/cosmos/cosmos-sdk/pull/20062) Fixed cosmovisor add-upgrade permissions

The changelog entry for PR #20062 could be more descriptive. Consider specifying what was fixed and why it's important.

- * [#20062](https://github.com/cosmos/cosmos-sdk/pull/20062) Fixed cosmovisor add-upgrade permissions
+ * [#20062](https://github.com/cosmos/cosmos-sdk/pull/20062) Fixed cosmovisor add-upgrade permissions to allow executable access, resolving a critical issue where upgrades were failing due to permission errors.
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#20062](https://github.com/cosmos/cosmos-sdk/pull/20062) Fixed cosmovisor add-upgrade permissions
* [#20062](https://github.com/cosmos/cosmos-sdk/pull/20062) Fixed cosmovisor add-upgrade permissions to allow executable access, resolving a critical issue where upgrades were failing due to permission errors.



Copy link
Contributor

Choose a reason for hiding this comment

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

Multiple consecutive blank lines detected. It's best to maintain a single blank line to keep the file clean and organized.

40d39
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change

## v1.5.0 - 2023-07-17

Expand Down
8 changes: 4 additions & 4 deletions tools/cosmovisor/cmd/cosmovisor/add_upgrade.go
View file
Open in desktop
Expand Up @@ -5,7 +5,6 @@
"fmt"
"os"
"path"
"path/filepath"
"strings"

"github.com/spf13/cobra"
Expand Down Expand Up @@ -54,7 +53,7 @@

// create upgrade dir
upgradeLocation := cfg.UpgradeDir(upgradeName)
if err := os.MkdirAll(path.Join(upgradeLocation, "bin"), 0o750); err != nil {
if err := os.MkdirAll(path.Join(upgradeLocation, "bin"), 0o755); err != nil {
Dismissed Show dismissed Hide dismissed
return fmt.Errorf("failed to create upgrade directory: %w", err)
}

Expand Down Expand Up @@ -94,7 +93,7 @@
return err
}

logger.Info(fmt.Sprintf("%s created, %s upgrade binary will switch at height %d", filepath.Join(cfg.UpgradeInfoFilePath(), upgradetypes.UpgradeInfoFilename), upgradeName, upgradeHeight))
logger.Info(fmt.Sprintf("%s created, %s upgrade binary will switch at height %d", cfg.UpgradeInfoFilePath(), upgradeName, upgradeHeight))
}

return nil
Expand All @@ -110,7 +109,8 @@
return fmt.Errorf("failed to check if file exists: %w", err)
}

if err := os.WriteFile(path, data, 0o600); err != nil {
//nolint:gosec // We need broader permissions to make it executable
if err := os.WriteFile(path, data, 0o755); err != nil {
Dismissed Show dismissed Hide dismissed
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a changelog in cosmovisor and a nolint comment so we don't revert it back because of the gosec? And so that linting passes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cmd/cosmovisor/add_upgrade.go:58:2: directive `//nolint:gosec` is unused for linter "gosec" (nolintlint)
	//nolint:gosec
	^
cmd/cosmovisor/add_upgrade.go:115:2: directive `//nolint:gosec` should provide explanation such as `//nolint:gosec // this is why` (nolintlint)
	//nolint:gosec
	^

lol now it gives me this. do you think it's worth disabling? or what's better to do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nevermind, I fixed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and also added the changelog entry. @julienrbrt can you check?

return fmt.Errorf("failed to write binary to location: %w", err)
}

Expand Down