charm: disallow juju-* hooks on read

charm: disallow juju-* hooks on read



https://code.launchpad.net/~fwereade/juju-core/charm-disallow-juju-hooks/+merge/130310

(do not edit description out of merge proposal)

[fwereade, 2012-10-18 09:44:28]

Please take a look.

[rog, 2012-10-18 09:51:52]

LGTM modulo the below consideration.

https://codereview.appspot.com/6734046/diff/1/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/6734046/diff/1/charm/dir.go#newcode61
charm/dir.go:61: fis, err := ioutil.ReadDir(dir.join("hooks"))
i wonder if you should use filepath.Walk here to validate all the paths in the
tree. that way we can easily change validatePath to check things other than
hooks in the future.

https://codereview.appspot.com/6734046/diff/1/charm/dir.go#newcode66
charm/dir.go:66: if err := validatePath("hooks/" + fi.Name()); err != nil {
if you don't do the above, then this should probably use filepath.Join, to match
the call in validatePath.

[fwereade, 2012-10-18 12:42:36]

Please take a look.

https://codereview.appspot.com/6734046/diff/1/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/6734046/diff/1/charm/dir.go#newcode61
charm/dir.go:61: fis, err := ioutil.ReadDir(dir.join("hooks"))
On 2012/10/18 09:51:52, rog wrote:
> i wonder if you should use filepath.Walk here to validate all the paths in the
> tree. that way we can easily change validatePath to check things other than
> hooks in the future.

Hmm, that feels to me like a less-likely possible future change, I'm not sure
it's worth addressing it at this stage...

https://codereview.appspot.com/6734046/diff/1/charm/dir.go#newcode66
charm/dir.go:66: if err := validatePath("hooks/" + fi.Name()); err != nil {
On 2012/10/18 09:51:52, rog wrote:
> if you don't do the above, then this should probably use filepath.Join, to
match
> the call in validatePath.

...but you are very right here; thank you, well spotted.

[rog, 2012-10-18 13:35:20]

https://codereview.appspot.com/6734046/diff/1/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/6734046/diff/1/charm/dir.go#newcode61
charm/dir.go:61: fis, err := ioutil.ReadDir(dir.join("hooks"))
On 2012/10/18 12:42:37, fwereade wrote:
> On 2012/10/18 09:51:52, rog wrote:
> > i wonder if you should use filepath.Walk here to validate all the paths in
the
> > tree. that way we can easily change validatePath to check things other than
> > hooks in the future.
> 
> Hmm, that feels to me like a less-likely possible future change, I'm not sure
> it's worth addressing it at this stage...

FWIW, it's probably less code; the zip file checker passes all paths through
validatePath, so it seems reasonable that this should do.

[niemeyer, 2012-10-19 01:07:03]

LGTM assuming the following:

https://codereview.appspot.com/6734046/diff/4001/charm/charm.go
File charm/charm.go (right):

https://codereview.appspot.com/6734046/diff/4001/charm/charm.go#newcode52
charm/charm.go:52: if strings.HasPrefix(path, filepath.Join("hooks", "juju-")) {
The path is coming from a zip file, which may be put together in different ways,
and in theory will use slashes no matter what the local convention is.

I suggest using *path*.Clean, and then checking for the "hooks/juju-" prefix.

https://codereview.appspot.com/6734046/diff/4001/charm/charm.go#newcode53
charm/charm.go:53: return fmt.Errorf("reserved hook name %q", path)
s/name/name:/

https://codereview.appspot.com/6734046/diff/4001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/6734046/diff/4001/charm/dir.go#newcode66
charm/dir.go:66: if err := validatePath(filepath.Join("hooks", fi.Name())); err
!= nil {
The original logic was actually fine here, for the reason stated.

[fwereade, 2012-10-19 07:22:09]

*** Submitted:

charm: disallow juju-* hooks on read

R=
CC=
https://codereview.appspot.com/6734046

https://codereview.appspot.com/6734046/diff/4001/charm/charm.go
File charm/charm.go (right):

https://codereview.appspot.com/6734046/diff/4001/charm/charm.go#newcode52
charm/charm.go:52: if strings.HasPrefix(path, filepath.Join("hooks", "juju-")) {
On 2012/10/19 01:07:03, niemeyer wrote:
> The path is coming from a zip file, which may be put together in different
ways,
> and in theory will use slashes no matter what the local convention is.
> 
> I suggest using *path*.Clean, and then checking for the "hooks/juju-" prefix.

Done.

https://codereview.appspot.com/6734046/diff/4001/charm/charm.go#newcode53
charm/charm.go:53: return fmt.Errorf("reserved hook name %q", path)
On 2012/10/19 01:07:03, niemeyer wrote:
> s/name/name:/

This doesn't feel consistent with what I've been asked to do elsewhere... I had
developed an understanding that a colon indicated that the bits following are
intended to be the *reason* for the error, rather than just, er, random
supplementary data. So "blah: inconsistent state" is fine, but `invalid whatever
"bar"` is better for that situation. I guess there's some nuance I'm missing...