-
Notifications
You must be signed in to change notification settings - Fork 363
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.
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
Check index or plugin name safety in cmd #583
Check index or plugin name safety in cmd #583
Conversation
cb6ec33
to
41fd6e3
Compare
cmd/krew/cmd/install.go
Outdated
for _, name := range pluginNames { | ||
if !validation.IsSafePluginName(name) { | ||
return unsafePluginNameErr(name) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd feel better if we validate plugin names in batch upfront. One way we could achieve this is by changing the API to
func IsSafePluginName(names ...string) bool { .. }
(same in several other places)
Also, it seems that unsafePluginNameErr()
could be returned from this function (making it obsolete), so that we have instead:
func CheckSafePluginName(names ...string) error { .. }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah although there's another method in validation.go
that's geared towards plugin authors (and therefore it prints a regexp) as it runs during manifest validation tests.
return errors.Errorf("the plugin name %q is not allowed, must match %q", name, safePluginRegexp.String())
that's why I kept it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that you want to keep the API in validation
consistent.
Maybe this helper can be put in cmd/internal
instead? Just an idea. It's fine as it stands, but validating upfront would be even better IMO.
`../receipts/` + validPlugin, | ||
`..\receipts\` + validPlugin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is /receipt/
different from /default/
? Why is this needed?
Otherwise, let's remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
receipt/ is needed because we need to point this to a receipt file. and at the moment all these paths parseable as a Receipt
cases := []string{ | ||
`../index/` + validPlugin, | ||
`..\index\` + validPlugin, | ||
`../default/` + validPlugin, | ||
`..\default\` + validPlugin, | ||
`index-name/sub-directory/plugin-name`, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list of test cases is repeated 3 times (if my question about receipt
applies). Should we extract this to a central place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the test unsafe functions are all relatively similar (some of the error messaging is slightly different). The cases, expected error string, and arguments to test.Krew
are the only differences. I think this could be useful as a testutil
function like test.AssertFailureWithUnsafeName
(not really sure what a good name would be)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think duplication in tests aren't as bad, and each test actually has a slightly different things it's looking at, so I'm not entirely sure if the de-duplication helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have the unsafe names in a variable, so that it is easily re-used without the need to copy. But let's merge as-is and iterate from there.
Test failure seems legit:
|
Another round of reviews please. 🙏 |
@ahmetb Can you take a final look and rebase? Then I'll lgtm. |
Moving plugin name or index name safety checks to cmd/ where they are given by the user to the program. This way we shift the need for validation from being unit tests of low-level machinery to the integration_test which tests user-facing concerns. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
e715087
to
f5d6254
Compare
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Rebased + test run https://github.com/ahmetb/krew/runs/559418218 actually has captured a legitimate bug. Posted a fix. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tl;dr: Some krew cmds are currently susceptible to path manipulation through positional arguments. Low-level machinery isn't responsible for this, so move them to cmd + add tests.
Moving plugin name or index name safety checks to cmd/ where they are given by
the user to the program. This way we shift the need for validation from being
unit tests of low-level machinery to the integration_test which tests
user-facing concerns.
Fixes #581
/assign @corneliusweig
/assign @chriskim06