Repository: lima-vm/lima

Issue:
[v1.1 RC] `limactl info`: `template "default.yaml" not found` (only after installing additional guest agents)
Preparing Homebrew formula for Lima v1.1.0 (RC): https://github.com/AkihiroSuda/homebrew-core/commit/54a4f842f7c3851c67fcffec495899c69c55357e
`limactl info` stops working, only after installing additional guestagents
 
1. Install https://github.com/AkihiroSuda/homebrew-core/blob/lima-1.1.0-rc.0/Formula/l/lima.rb
```
HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source ./lima.rb
```


2. Make sure `limactl info` works
```console
$ limactl info | jq .guestAgents
WARN[0000] Failed to resolve the guest agent binary for "x86_64"  error="guest agent binary could not be found for Linux-x86_64 (Hint: try installing `lima-additional-guestagents` package)"
WARN[0000] Failed to resolve the guest agent binary for "armv7l"  error="guest agent binary could not be found for Linux-armv7l (Hint: try installing `lima-additional-guestagents` package)"
WARN[0000] Failed to resolve the guest agent binary for "ppc64le"  error="guest agent binary could not be found for Linux-ppc64le (Hint: try installing `lima-additional-guestagents` package)"
WARN[0000] Failed to resolve the guest agent binary for "riscv64"  error="guest agent binary could not be found for Linux-riscv64 (Hint: try installing `lima-additional-guestagents` package)"
WARN[0000] Failed to resolve the guest agent binary for "s390x"  error="guest agent binary could not be found for Linux-s390x (Hint: try installing `lima-additional-guestagents` package)"
{
  "aarch64": {
    "location": "/opt/homebrew/share/lima/lima-guestagent.Linux-aarch64.gz"
  }
}
```

3.  Install https://github.com/AkihiroSuda/homebrew-core/blob/lima-1.1.0-rc.0/Formula/l/lima-additional-guestagents.rb
```
HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source ./lima-additional-guestagents.rb
```

4. `limactl info` no longer works
```console
$ limactl info
FATA[0000] template "default.yaml" not found
```

Discussion:
@AkihiroSuda: ```console
$ which limactl
/opt/homebrew/bin/limactl
$ ls -l /opt/homebrew/bin/limactl
lrwxr-xr-x  1 suda  admin  37  5 14 17:50 /opt/homebrew/bin/limactl@ -> ../Cellar/lima/1.1.0-rc.0/bin/limactl
$ ls -l /opt/homebrew/Cellar/lima/1.1.0-rc.0/bin/limactl 
-r-xr-xr-x  1 suda  admin  29205488  5 14 17:50 /opt/homebrew/Cellar/lima/1.1.0-rc.0/bin/limactl*
$ ls -l /opt/homebrew/share/lima/templates/default.yaml 
lrwxr-xr-x  1 suda  admin  65  5 14 17:52 /opt/homebrew/share/lima/templates/default.yaml@ -> ../../../Cellar/lima/1.1.0-rc.0/share/lima/templates/default.yaml
$ ls -l /opt/homebrew/Cellar/lima/1.1.0-rc.0/share/lima/templates/default.yaml 
-rw-r--r--  1 suda  admin  29377  5 14 04:05 /opt/homebrew/Cellar/lima/1.1.0-rc.0/share/lima/templates/default.yaml
```@AkihiroSuda: Something is wrong with
https://github.com/lima-vm/lima/blob/353be898ff96553f1222897aa175c14262bbefed/pkg/templatestore/templatestore.go#L35


It is trying to read `/opt/homebrew/share/lima/templates/Cellar/lima/1.1.0-rc.0/share/lima/templates/default.yaml`@jandubois: This is weird, and I can repo; will take a look if I can figure it out.

~But the additional guest agents won't be found anyways because they are installed into a different directory~

Edit: this doesn't seem to be a problem.
@jandubois: > 1. Install https://github.com/AkihiroSuda/homebrew-core/blob/lima-1.1.0-rc.0/Formula/l/lima.rb
> 
> ```
> HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source ./lima.rb
> ```

After this step the templates are installed as copies:

```
❯ ls -l /opt/homebrew/share/lima/templates/default.yaml
-rw-r--r--@  1 jan  admin  29377 May 13 12:05 default.yaml
```

> 3\. Install https://github.com/AkihiroSuda/homebrew-core/blob/lima-1.1.0-rc.0/Formula/l/lima-additional-guestagents.rb
> 
> ```
> HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source ./lima-additional-guestagents.rb
> ```

After this step the templates are replaced by symlinks:

```
❯ ls -l /opt/homebrew/share/lima/templates/default.yaml
lrwxr-xr-x@ 1 jan  admin  65 May 14 10:51 /opt/homebrew/share/lima/templates/default.yaml -> ../../../Cellar/lima/1.1.0-rc.0/share/lima/templates/default.yaml
```

The problem then becomes that `securejoin.SecureJoin(templatesDir, name)` will strip the `../` parts from the target and the result becomes an invalid path.

I don't understand why installing a different formula will changes the installed files to symlinks (although I had expected them to always be symlinks). Maybe because installing the additional guest agents puts files from 2 different formulas into `/opt/homebrew/share/lima`?

Maybe we could get rid of `securejoin` and make sure the template name doesn't include `../` path fragments?@jandubois: > Maybe we could get rid of `securejoin` and make sure the template name doesn't include `../` path fragments?

Previous discussion that will need to be revisited: https://github.com/lima-vm/lima/pull/805#discussion_r853411702@afbjorklund: > I don't understand why installing a different formula will changes the installed files to symlinks (although I had expected them to always be symlinks).

I think it tries to link the directory if possible, so those files are probably not copies 

Solution patch:
diff --git a/cmd/limactl/start.go b/cmd/limactl/start.go
index 6c65a013..c09aaa85 100644
--- a/cmd/limactl/start.go
+++ b/cmd/limactl/start.go
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"path"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -118,8 +119,7 @@ func loadOrCreateInstance(cmd *cobra.Command, args []string, createOnly bool) (*
 		return nil, err
 	}
 	if isTemplateURL, templateURL := limatmpl.SeemsTemplateURL(arg); isTemplateURL {
-		// No need to use SecureJoin here. https://github.com/lima-vm/lima/pull/805#discussion_r853411702
-		templateName := filepath.Join(templateURL.Host, templateURL.Path)
+		templateName := path.Join(templateURL.Host, templateURL.Path)
 		switch templateName {
 		case "experimental/vz":
 			logrus.Warn("template://experimental/vz was merged into the default template in Lima v1.0. See also <https://lima-vm.io/docs/config/vmtype/>.")
diff --git a/go.mod b/go.mod
index fd1b83b4..ad48c336 100644
--- a/go.mod
+++ b/go.mod
@@ -16,7 +16,6 @@ require (
 	github.com/containers/gvisor-tap-vsock v0.8.6 // gomodjail:unconfined
 	github.com/coreos/go-semver v0.3.1
 	github.com/cpuguy83/go-md2man/v2 v2.0.7
-	github.com/cyphar/filepath-securejoin v0.4.1
 	github.com/digitalocean/go-qemu v0.0.0-20221209210016-f035778c97f7
 	github.com/diskfs/go-diskfs v1.6.0 // gomodjail:unconfined
 	github.com/docker/go-units v0.5.0
diff --git a/go.sum b/go.sum
index f4a546c3..30c2aadf 100644
--- a/go.sum
+++ b/go.sum
@@ -51,8 +51,6 @@ github.com/cpuguy83/go-md2man/v2 v2.0.7 h1:zbFlGlXEAKlwXpmvle3d8Oe3YnkKIK4xSRTd3
 github.com/cpuguy83/go-md2man/v2 v2.0.7/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=
 github.com/creack/pty v1.1.17 h1:QeVUsEDNrLBW4tMgZHvxy18sKtr6VI492kBhUfhDJNI=
 github.com/creack/pty v1.1.17/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4=
-github.com/cyphar/filepath-securejoin v0.4.1 h1:JyxxyPEaktOD+GAnqIqTf9A8tHyAG22rowi7HkoSU1s=
-github.com/cyphar/filepath-securejoin v0.4.1/go.mod h1:Sdj7gXlvMcPZsbhwhQ33GguGLDGQL7h7bg04C/+u9jI=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM=
diff --git a/pkg/limatmpl/locator.go b/pkg/limatmpl/locator.go
index a2f6ca0d..3f2f2674 100644
--- a/pkg/limatmpl/locator.go
+++ b/pkg/limatmpl/locator.go
@@ -40,8 +40,7 @@ func Read(ctx context.Context, name, locator string) (*Template, error) {
 	isTemplateURL, templateURL := SeemsTemplateURL(locator)
 	switch {
 	case isTemplateURL:
-		// No need to use SecureJoin here. https://github.com/lima-vm/lima/pull/805#discussion_r853411702
-		templateName := filepath.Join(templateURL.Host, templateURL.Path)
+		templateName := path.Join(templateURL.Host, templateURL.Path)
 		logrus.Debugf("interpreting argument %q as a template name %q", locator, templateName)
 		if tmpl.Name == "" {
 			// e.g., templateName = "deprecated/centos-7.yaml" , tmpl.Name = "centos-7"



Test patch:
diff --git a/pkg/templatestore/templatestore.go b/pkg/templatestore/templatestore.go
index 4695aa97..f91f2c44 100644
--- a/pkg/templatestore/templatestore.go
+++ b/pkg/templatestore/templatestore.go
@@ -14,7 +14,6 @@ import (
 	"strings"
 	"unicode"
 
-	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/lima-vm/lima/pkg/store/dirnames"
 	"github.com/lima-vm/lima/pkg/usrlocalsharelima"
 )
@@ -42,7 +41,16 @@ func templatesPaths() ([]string, error) {
 	}, nil
 }
 
+// Read searches for template `name` in all template directories and returns the
+// contents of the first one found. Template names cannot contain the substring ".."
+// to make sure they don't reference files outside the template directories. We are
+// not using securejoin.SecureJoin because the actual template may be a symlink to a
+// directory elsewhere (e.g. when installed by Homebrew).
 func Read(name string) ([]byte, error) {
+	doubleDot := ".."
+	if strings.Contains(name, doubleDot) {
+		return nil, fmt.Errorf("template name %q must not contain %q", name, doubleDot)
+	}
 	paths, err := templatesPaths()
 	if err != nil {
 		return nil, err
@@ -54,10 +62,8 @@ func Read(name string) ([]byte, error) {
 		name += ".yaml"
 	}
 	for _, templatesDir := range paths {
-		filePath, err := securejoin.SecureJoin(templatesDir, name)
-		if err != nil {
-			return nil, err
-		}
+		// Normalize filePath for error messages because template names always use forward slashes
+		filePath := filepath.Clean(filepath.Join(templatesDir, name))
 		if b, err := os.ReadFile(filePath); !errors.Is(err, os.ErrNotExist) {
 			return b, err
 		}
@@ -67,6 +73,10 @@ func Read(name string) ([]byte, error) {
 
 const Default = "default"
 
+// Templates returns a list of Template structures containing the Name and Location for each template.
+// It searches all template directories, but only the first template of a given name is recorded.
+// Only non-hidden files with a ".yaml" file extension are considered templates.
+// The final result is sorted alphabetically by template name.
 func Templates() ([]Template, error) {
 	paths, err := templatesPaths()
 	if err != nil {