Merge pull request juju#18173 from barrettj12/hooks-check

juju#18173

When checking if a local charm has hooks, we were assuming that the zip file would always have a separate entry for the directory `hooks/` and for each of its subfiles. However, this is not the case for charms generated by Charmcraft. A better check is that there exists at least one entry with the prefix `hooks/` that is *not* a directory.

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- ~[ ] Go unit tests, with comments saying what you're testing~
- ~[ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

Check unit tests pass.
```
go test ./api/client/charms -check.f charmsMockSuite
```

Pack the `departer` charm (`testcharms/charms/departer`) and verify that it can be deployed.

api/client/charms/client_test.go

@@ -4,6 +4,7 @@
 package charms_test
 
 import (
+	"archive/zip"
 	"os"
 
 	"github.com/juju/charm/v12"
@@ -287,7 +288,7 @@ func (s *addCharmSuite) TestAddCharm(c *gc.C) {
 	c.Assert(got, gc.DeepEquals, origin)
 }
 
-func (s charmsMockSuite) TestCheckCharmPlacement(c *gc.C) {
+func (s *charmsMockSuite) TestCheckCharmPlacement(c *gc.C) {
 	ctrl := gomock.NewController(c)
 	defer ctrl.Finish()
 
@@ -311,7 +312,7 @@ func (s charmsMockSuite) TestCheckCharmPlacement(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
-func (s charmsMockSuite) TestCheckCharmPlacementError(c *gc.C) {
+func (s *charmsMockSuite) TestCheckCharmPlacementError(c *gc.C) {
 	ctrl := gomock.NewController(c)
 	defer ctrl.Finish()
 
@@ -411,7 +412,7 @@ func (s *charmsMockSuite) TestZipHasDispatchFileOnly(c *gc.C) {
 	c.Assert(hasDispatch, jc.IsTrue)
 }
 
-func (s *charmsMockSuite) TestZipHasNoHooksNorDispath(c *gc.C) {
+func (s *charmsMockSuite) TestZipHasNoHooksNorDispatch(c *gc.C) {
 	ch := testcharms.Repo.CharmDir("category") // has no hooks nor dispatch file
 	tempFile, err := os.CreateTemp(c.MkDir(), "charm")
 	c.Assert(err, jc.ErrorIsNil)
@@ -424,3 +425,91 @@ func (s *charmsMockSuite) TestZipHasNoHooksNorDispath(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(hasHooks, jc.IsFalse)
 }
+
+// TestZipHasSingleHook tests that an archive containing only a single hook
+// file (and no zip entry for the hooks directory) is still validated as a
+// charm with hooks.
+func (s *charmsMockSuite) TestZipHasSingleHook(c *gc.C) {
+	tempFile, err := os.CreateTemp(c.MkDir(), "charm")
+	c.Assert(err, jc.ErrorIsNil)
+	defer tempFile.Close()
+
+	zipWriter := zip.NewWriter(tempFile)
+	// add a single install hook
+	_, err = zipWriter.Create("hooks/install")
+	c.Assert(err, jc.ErrorIsNil)
+	err = zipWriter.Close()
+	c.Assert(err, jc.ErrorIsNil)
+
+	// Verify created zip is as expected
+	zipReader, err := zip.OpenReader(tempFile.Name())
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(len(zipReader.File), gc.Equals, 1)
+	c.Assert(zipReader.File[0].Name, gc.Equals, "hooks/install")
+	c.Assert(zipReader.File[0].Mode().IsRegular(), jc.IsTrue)
+
+	// Verify this is validated as having a hook
+	hasHooks, err := (*charms.HasHooksOrDispatch)(tempFile.Name())
+	c.Check(err, jc.ErrorIsNil)
+	c.Check(hasHooks, jc.IsTrue)
+}
+
+// TestZipEmptyHookDir tests that an archive containing only an empty hooks
+// directory is not validated as a charm with hooks.
+func (s *charmsMockSuite) TestZipEmptyHookDir(c *gc.C) {
+	tempFile, err := os.CreateTemp(c.MkDir(), "charm")
+	c.Assert(err, jc.ErrorIsNil)
+	defer tempFile.Close()
+
+	zipWriter := zip.NewWriter(tempFile)
+	// add an empty hooks directory
+	_, err = zipWriter.Create("hooks/")
+	c.Assert(err, jc.ErrorIsNil)
+	err = zipWriter.Close()
+	c.Assert(err, jc.ErrorIsNil)
+
+	// Verify created zip is as expected
+	zipReader, err := zip.OpenReader(tempFile.Name())
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(len(zipReader.File), gc.Equals, 1)
+	c.Assert(zipReader.File[0].Name, gc.Equals, "hooks/")
+	c.Assert(zipReader.File[0].Mode().IsDir(), jc.IsTrue)
+
+	// Verify this is validated as having no hooks
+	hasHooks, err := (*charms.HasHooksOrDispatch)(tempFile.Name())
+	c.Check(err, jc.ErrorIsNil)
+	c.Check(hasHooks, jc.IsFalse)
+}
+
+// TestZipSubfileHook tests that an archive containing nested subfiles inside
+// the hooks directory (i.e. not in the top level) is not validated as a charm
+// with hooks.
+func (s *charmsMockSuite) TestZipSubfileHook(c *gc.C) {
+	tempFile, err := os.CreateTemp(c.MkDir(), "charm")
+	c.Assert(err, jc.ErrorIsNil)
+	defer tempFile.Close()
+
+	zipWriter := zip.NewWriter(tempFile)
+	// add some files inside a subdir of hooks
+	_, err = zipWriter.Create("hooks/foo/bar.sh")
+	c.Assert(err, jc.ErrorIsNil)
+	_, err = zipWriter.Create("hooks/hooks/install")
+	c.Assert(err, jc.ErrorIsNil)
+	_, err = zipWriter.Create("foo/hooks/install")
+	c.Assert(err, jc.ErrorIsNil)
+	err = zipWriter.Close()
+	c.Assert(err, jc.ErrorIsNil)
+
+	// Verify created zip is as expected
+	zipReader, err := zip.OpenReader(tempFile.Name())
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(len(zipReader.File), gc.Equals, 3)
+	for _, f := range zipReader.File {
+		c.Assert(f.Mode().IsRegular(), jc.IsTrue)
+	}
+
+	// Verify this is not validated as having a hook
+	hasHooks, err := (*charms.HasHooksOrDispatch)(tempFile.Name())
+	c.Check(err, jc.ErrorIsNil)
+	c.Check(hasHooks, jc.IsFalse)
+}

api/client/charms/localcharmclient.go

@@ -10,7 +10,9 @@ import (
 	"encoding/hex"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
+	"path/filepath"
 	"strings"
 
 	"github.com/juju/charm/v12"
@@ -149,24 +151,10 @@ func hasHooksFolderOrDispatchFile(name string) (bool, error) {
 		return false, err
 	}
 	defer zipr.Close()
-	count := 0
 	// zip file spec 4.4.17.1 says that separators are always "/" even on Windows.
-	hooksPath := "hooks/"
 	dispatchPath := "dispatch"
 	for _, f := range zipr.File {
-		if strings.Contains(f.Name, hooksPath) {
-			count++
-		}
-		if count > 1 {
-			// 1 is the magic number here.
-			// Charm zip archive is expected to contain several files and folders.
-			// All properly built charms will have a non-empty "hooks" folders OR
-			// a dispatch file.
-			// File names in the archive will be of the form "hooks/" - for hooks folder; and
-			// "hooks/*" for the actual charm hooks implementations.
-			// For example, install hook may have a file with a name "hooks/install".
-			// Once we know that there are, at least, 2 files that have names that start with "hooks/", we
-			// know for sure that the charm has a non-empty hooks folder.
+		if isHook(f) {
 			return true, nil
 		}
 		if strings.Contains(f.Name, dispatchPath) {
@@ -176,6 +164,25 @@ func hasHooksFolderOrDispatchFile(name string) (bool, error) {
 	return false, nil
 }
 
+// isHook verifies whether the given file inside a zip archive is a valid hook
+// file.
+func isHook(f *zip.File) bool {
+	// Hook files must be in the top level of the 'hooks' directory
+	if filepath.Dir(f.Name) != "hooks" {
+		return false
+	}
+	// Valid file modes are regular files or symlinks. All others (directories,
+	// sockets, devices, etc.) are considered invalid file modes.
+	switch mode := f.Mode(); {
+	case mode.IsRegular():
+		return true
+	case mode&fs.ModeSymlink != 0:
+		return true
+	default:
+		return false
+	}
+}
+
 func (c *LocalCharmClient) validateCharmVersion(ch charm.Charm, agentVersion version.Number) error {
 	minver := ch.Meta().MinJujuVersion
 	if minver != version.Zero {