Repository: goreleaser/nfpm
Pull request #290 (2021-01-30 11:37:29)

fix: Prevent destination collisions
This PR makes `ExpandContentGlobs` return an error when multiple contents share the same destination. Otherwise, the destination would just be overwritten.

This leads to particularly nasty broken packages when the symlink convention is misunderstood, so I improved the documentation for that too. If you think that doesn't belong in this PR I will split the PR. But first hear me out:

Everyone I know (including myself) always forgets the order of arguments in `ln -s file1 file2` such that most of the time `ln` calls are preceded by `man ln`. So refering to `ln` in the `nfpm` docs for symlinks might not be the best idea. I learned this the hard way when I added a symlink to a package thinking that

```
- src: /usr/bin/mybinary
  dst: /path/to/my/actual/binary
  type: symlink
```

means that I create a symlink that points from the source ` /usr/bin/mybinary` to the destination `/path/to/my/actual/binary`, but actually it is the other way around. That is definitely confusing but ultimately not a bad idea because this way "dst" always refers to the file created by the package. So this just needed more documentation.

But the end of the story was that my package resulted in a dangling symlink at `/path/to/my/actual/binary` which completely overwrote the actual binary 😄. So this PR addresses both issues that together lead to this broken result.

vercel[bot]: This pull request is being automatically deployed with Vercel ([learn more](https://vercel.link/github-learn-more)).  
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: [https://vercel.com/goreleaser/nfpm/7z0760wbx](https://vercel.com/goreleaser/nfpm/7z0760wbx)  
✅ Preview: *In Progress*


codecov[bot]: # [Codecov](https://codecov.io/gh/goreleaser/nfpm/pull/290?src=pr&el=h1) Report
> Merging [#290](https://codecov.io/gh/goreleaser/nfpm/pull/290?src=pr&el=desc) (3ddf4ea) into [master](https://codecov.io/gh/goreleaser/nfpm/commit/ea1605eaed3e1cad53472fb1ace78f324194ffb0?el=desc) (ea1605e) will **increase** coverage by `0.17%`.
> The diff coverage is `88.88%`.

[![Impacted file tree graph](https://codecov.io/gh/goreleaser/nfpm/pull/290/graphs/tree.svg?width=650&height=150&src=pr&token=9QKGIxn7wg)](https://codecov.io/gh/goreleaser/nfpm/pull/290?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   70.00%   70.17%   +0.17%     
==========================================
  Files           9        9              
  Lines        1090     1103      +13     
==========================================
+ Hits          763      774      +11     
- Misses        203      204       +1     
- Partials      124      125       +1     
```


| [Impacted Files](https://codecov.io/gh/goreleaser/nfpm/pull/290?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [files/files.go](https://codecov.io/gh/goreleaser/nfpm/pull/290/diff?src=pr&el=tree#diff-ZmlsZXMvZmlsZXMuZ28=) | `95.65% <88.88%> (-2.57%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/goreleaser/nfpm/pull/290?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/goreleaser/nfpm/pull/290?src=pr&el=footer). Last update [ea1605e...3ddf4ea](https://codecov.io/gh/goreleaser/nfpm/pull/290?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/files/files.go b/files/files.go
index 838523d..8f81472 100644
--- a/files/files.go
+++ b/files/files.go
@@ -1,6 +1,7 @@
 package files
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"sort"
@@ -51,7 +52,7 @@ func (c Contents) Less(i, j int) bool {
 }
 
 func (c *Content) WithFileInfoDefaults() *Content {
-	var cc = &Content{
+	cc := &Content{
 		Source:      c.Source,
 		Destination: c.Destination,
 		Type:        c.Type,
@@ -110,8 +111,8 @@ func (c *Content) Sys() interface{} {
 }
 
 // ExpandContentGlobs gathers all of the real files to be copied into the package.
-func ExpandContentGlobs(filesSrcDstMap Contents, disableGlobbing bool) (files Contents, err error) {
-	for _, f := range filesSrcDstMap {
+func ExpandContentGlobs(contents Contents, disableGlobbing bool) (files Contents, err error) {
+	for _, f := range contents {
 		var globbed map[string]string
 		if disableGlobbing {
 			f.Source = fileglob.QuoteMeta(f.Source)
@@ -120,7 +121,11 @@ func ExpandContentGlobs(filesSrcDstMap Contents, disableGlobbing bool) (files Co
 		switch f.Type {
 		case "ghost", "symlink":
 			// Ghost and symlink files need to be in the list, but dont glob them because they do not really exist
-			files = append(files, f.WithFileInfoDefaults())
+			files, err = appendWithUniqueDestination(files, f.WithFileInfoDefaults())
+			if err != nil {
+				return nil, err
+			}
+
 			continue
 		}
 
@@ -128,7 +133,11 @@ func ExpandContentGlobs(filesSrcDstMap Contents, disableGlobbing bool) (files Co
 		if err != nil {
 			return nil, err
 		}
-		files = appendGlobbedFiles(globbed, f, files)
+
+		files, err = appendGlobbedFiles(globbed, f, files)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	// sort the files for reproducibility and general cleanliness
@@ -137,7 +146,7 @@ func ExpandContentGlobs(filesSrcDstMap Contents, disableGlobbing bool) (files Co
 	return files, nil
 }
 
-func appendGlobbedFiles(globbed map[string]string, origFile *Content, incFiles Contents) (files Contents) {
+func appendGlobbedFiles(globbed map[string]string, origFile *Content, incFiles Contents) (files Contents, err error) {
 	files = append(files, incFiles...)
 	for src, dst := range globbed {
 		newFile := &Content{
@@ -147,10 +156,36 @@ func appendGlobbedFiles(globbed map[string]string, origFile *Content, incFiles C
 			FileInfo:    origFile.FileInfo,
 			Packager:    origFile.Packager,
 		}
-		files = append(files, newFile.WithFileInfoDefaults())
+
+		files, err = appendWithUniqueDestination(files, newFile.WithFileInfoDefaults())
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return files, nil
+}
+
+var ErrContentCollision = fmt.Errorf("content collision")
+
+func appendWithUniqueDestination(slice []*Content, elems ...*Content) ([]*Content, error) {
+	alreadyPresent := map[string]*Content{}
+
+	for _, presentElem := range slice {
+		alreadyPresent[presentElem.Destination] = presentElem
+	}
+
+	for _, elem := range elems {
+		if present, ok := alreadyPresent[elem.Destination]; ok {
+			return nil, fmt.Errorf(
+				"cannot add %s because %s is already present at the same destination (%s): %w",
+				elem.Source, present.Source, present.Destination, ErrContentCollision)
+		}
+
+		alreadyPresent[elem.Destination] = elem
 	}
 
-	return files
+	return append(slice, elems...), nil
 }
 
 // ToNixPath converts the given path to a nix-style path.
diff --git a/go.sum b/go.sum
index c37d5ba..529bc8b 100644
--- a/go.sum
+++ b/go.sum
@@ -510,6 +510,7 @@ github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81P
 github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
 github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0=
 github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/subosito/gotenv v1.2.0 h1:Slr1R9HxAlEKefgq5jn9U+DnETlIUa6HfgEzj0g5d7s=
 github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw=
diff --git a/www/docs/configuration.md b/www/docs/configuration.md
index 6fe582d..2edbf06 100644
--- a/www/docs/configuration.md
+++ b/www/docs/configuration.md
@@ -100,10 +100,17 @@ contents:
     dst: /etc/foo.conf
     type: config
 
-  # Simple symlink
-  #	Will result in `ln -s /sbin/foo /usr/local/bin/foo`.
-  - src: /sbin/foo
-    dst: /usr/local/bin/foo
+  # Simple symlink at /usr/local/bin/foo which points to /sbin/foo, which is
+  # the same behaviour as `ln -s /sbin/foo /usr/local/bin/foo`.
+  #
+  # This also means that both "src" and "dst" are paths inside the package (or
+  # rather paths in the file system where the package will be installed) and
+  # not in the build environment. This is different from regular files where
+  # "src" is a path in the build environment. However, this convention results
+  # in "dst" always being the file that is created when installing the
+  # package.
+  - src: /actual/path/to/foo
+    dst: /usr/bin/foo
     type: symlink
 
   # Corresponds to `%config(noreplace)` if the packager is rpm, otherwise it is just a config file


Test patch:
diff --git a/files/files_test.go b/files/files_test.go
index 035a911..1aa4d04 100644
--- a/files/files_test.go
+++ b/files/files_test.go
@@ -62,3 +62,13 @@ contents:
 	}
 	wg.Wait()
 }
+
+func TestCollision(t *testing.T) {
+	configuredFiles := []*files.Content{
+		{Source: "../testdata/whatever.conf", Destination: "/samedestination"},
+		{Source: "../testdata/whatever2.conf", Destination: "/samedestination"},
+	}
+
+	_, err := files.ExpandContentGlobs(configuredFiles, true)
+	require.ErrorIs(t, err, files.ErrContentCollision)
+}