packaging: the renderer refuses a file it cannot read instead of crashing, and resolves --out

.github/scripts/build_packages.py

@@ -31,7 +31,10 @@
 import tempfile
 from collections import namedtuple
 
-ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+# realpath, not abspath: the check that keeps --out outside the repository
+# compares resolved paths, so a symbolic link or a junction pointing into the
+# tree cannot walk the packages into it (outside review of #143).
+ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))
 TEMPLATES = os.path.join(ROOT, "packaging")
 TEMPLATE_SUFFIX = ".in"
 PLACEHOLDER = re.compile(r"\{\{([A-Z0-9_]+)\}\}")
@@ -103,9 +106,26 @@ def refuse(message):
     raise SystemExit("build_packages: %s" % message)
 
 
-def read_text(path):
-    with open(path, encoding="utf-8-sig") as handle:
-        return handle.read().replace("\r\n", "\n")
+def read_text(path, what="the file"):
+    """A text file, or a refusal naming it - never a traceback.
+
+    A file that is missing, unreadable or not UTF-8 is an input a person got
+    wrong, and the answer has to say which file and what to do, not print a
+    Python exception (outside review of #143).
+    """
+    try:
+        with open(path, encoding="utf-8-sig") as handle:
+            return handle.read().replace("\r\n", "\n")
+    except UnicodeDecodeError:
+        refuse("%s is not UTF-8 text, so it is not %s" % (path, what))
+    except OSError as err:
+        refuse("cannot read %s %s: %s" % (what, path, err.strerror or err))
+
+
+# A release's checksum file is under a kilobyte - 970 bytes for v0.4.0. Anything
+# near this is another file passed by mistake, an archive for instance, and is
+# refused by size before a byte of it is read.
+SUMS_LIMIT = 1024 * 1024
 
 
 def repository():
@@ -150,8 +170,15 @@ def read_sums(path):
     mode, and that star is not part of the name. A file saved on Windows may
     carry a byte order mark and CRLF. None of that may reach an address.
     """
+    hint = "Download verify-SHA256SUMS.txt from the release you are packaging"
+    try:
+        size = os.path.getsize(path)
+    except OSError as err:
+        refuse("cannot read the checksum file %s: %s. %s" % (path, err.strerror or err, hint))
+    if size > SUMS_LIMIT:
+        refuse("%s is %d bytes, so it is not a release's checksum file. %s" % (path, size, hint))
     sums = {}
-    for number, line in enumerate(read_text(path).split("\n"), 1):
+    for number, line in enumerate(read_text(path, "a checksum file").split("\n"), 1):
         if not line.strip():
             continue
         found = re.fullmatch(r"([0-9A-Fa-f]{64}) [ *](\S.*)", line.strip())
@@ -322,7 +349,7 @@ def destination(package, relative, version):
 
 def check_out(out):
     """--out is outside the repository and empty or absent, or a refusal."""
-    out = os.path.abspath(out)
+    out = os.path.realpath(out)
     root = os.path.normcase(ROOT)
     if os.path.normcase(out) == root or os.path.normcase(out).startswith(root + os.sep):
         refuse("--out %s is inside the repository. Rendered packages are not source - put "
@@ -342,8 +369,11 @@ def build(tag, sums_path, out):
         refuse("the icon %s is not in the repository" % ICON)
 
     parent = os.path.dirname(out)
-    os.makedirs(parent, exist_ok=True)
-    work = tempfile.mkdtemp(prefix=".packages-", dir=parent)
+    try:
+        os.makedirs(parent, exist_ok=True)
+        work = tempfile.mkdtemp(prefix=".packages-", dir=parent)
+    except OSError as err:
+        refuse("cannot create a working folder in %s: %s" % (parent, err.strerror or err))
     try:
         used = set()
         known = set()
@@ -366,6 +396,8 @@ def build(tag, sums_path, out):
         if os.path.isdir(out):
             os.rmdir(out)
         os.rename(work, out)
+    except OSError as err:
+        refuse("cannot write the packages to %s: %s. Nothing was left behind" % (out, err.strerror or err))
     finally:
         if os.path.isdir(work):
             shutil.rmtree(work)

internal/guard/packaging_test.go

@@ -74,14 +74,21 @@ func fixtureSums() []byte {
 
 // renderPackages runs the renderer the way a person does.
 func renderPackages(t *testing.T, tag string, sums []byte, out string) rendering {
+	t.Helper()
+	return renderFrom(t, tag, sumsFile(t, sums), out)
+}
+
+// renderFrom is renderPackages with the checksum file named rather than
+// written, so a guard can hand it a path to a file that is not there.
+func renderFrom(t *testing.T, tag, sumsPath, out string) rendering {
 	t.Helper()
 	python := pythonForGate(t)
 	// The interpreter is the one found on PATH, the script is a file of this
 	// repository, and every argument is a value this guard chose or a file it
 	// just wrote - nothing here comes from anything a person typed.
 	// nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command
 	cmd := exec.Command(python, packagingScript(t),
-		"--tag", tag, "--sums", sumsFile(t, sums), "--out", out)
+		"--tag", tag, "--sums", sumsPath, "--out", out)
 	cmd.Dir = repoRoot(t)
 	said, err := cmd.CombinedOutput()
 	code := 0

internal/guard/packagingrefusal_test.go

@@ -1,10 +1,13 @@
 package guard
 
 import (
+	"bytes"
+	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"regexp"
+	"runtime"
 	"strings"
 	"testing"
 )
@@ -109,6 +112,86 @@
 	return strings.Join(names, ", ")
 }
 
+// A file the renderer cannot read, and a destination that leads into the
+// repository by another name, are refused with a sentence rather than a Python
+// traceback or a write into the tree. An outside review of #143 found both: a
+// missing --sums printed an exception, and --out was checked by how it was
+// spelled rather than by where it leads.
+func TestTheRendererRefusesAFileItCannotReadAndAPathThatLeadsIntoTheTree(t *testing.T) {
+	dir := t.TempDir()
+	notUTF8 := filepath.Join(dir, "latin1.txt")
+	huge := filepath.Join(dir, "huge.txt")
+	aFile := filepath.Join(dir, "a-file-not-a-folder")
+	for path, body := range map[string][]byte{
+		notUTF8: {0xff, 0xfe, 0x41, 0x0a},
+		huge:    bytes.Repeat([]byte("a"), 2<<20),
+		aFile:   []byte("x"),
+	} {
+		if err := os.WriteFile(path, body, 0o600); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// The link points at an EMPTY folder made for this guard inside the tree,
+	// not at the tree itself, so no cleanup that followed it could reach
+	// anything else. The link is removed before the temporary directory that
+	// holds it - cleanups run last registered first.
+	target := filepath.Join(repoRoot(t), "packaging-guard-link-target")
+	if _, err := os.Stat(target); err == nil {
+		t.Fatalf("%s already exists, so this guard cannot tell what the renderer wrote there", target)
+	}
+	if err := os.Mkdir(target, 0o700); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.RemoveAll(target) })
+	link := filepath.Join(t.TempDir(), "into-the-tree")
+	if err := makeDirectoryLink(link, target); err != nil {
+		t.Fatalf("making a link to %s: %v", target, err)
+	}
+	t.Cleanup(func() { _ = os.Remove(link) })
+
+	for _, c := range []struct{ what, sums, out, says string }{
+		{"a checksum file that is not there", filepath.Join(dir, "missing.txt"),
+			filepath.Join(t.TempDir(), "packages"), "cannot read the checksum file"},
+		{"a checksum file that is not UTF-8", notUTF8,
+			filepath.Join(t.TempDir(), "packages"), "is not UTF-8 text"},
+		{"a file far too big to be a checksum file", huge,
+			filepath.Join(t.TempDir(), "packages"), "is not a release's checksum file"},
+		{"a destination that leads into the tree through a link", sumsFile(t, fixtureSums()),
+			filepath.Join(link, "packages"), "inside the repository"},
+		{"a destination under something that is a file", sumsFile(t, fixtureSums()),
+			filepath.Join(aFile, "packages"), "cannot create a working folder"},
+	} {
+		r := renderFrom(t, packagingTag, c.sums, c.out)
+		if r.code != 1 || !strings.HasPrefix(r.said, "build_packages: ") || strings.Contains(r.said, "Traceback") {
+			t.Errorf("%s: exit %d, and a refusal is exit 1 with a sentence, not a crash:\n%s", c.what, r.code, r.said)
+			continue
+		}
+		if !strings.Contains(r.said, c.says) {
+			t.Errorf("%s: the refusal does not say %q:\n%s", c.what, c.says, r.said)
+		}
+	}
+	if left := entriesOf(t, target); left != "" {
+		t.Errorf("the renderer wrote into the tree through the link: %s", left)
+	}
+}
+
+// makeDirectoryLink makes link lead to target: a junction on Windows, which
+// needs no privilege where a symbolic link does, and a symbolic link elsewhere.
+func makeDirectoryLink(link, target string) error {
+	if runtime.GOOS != "windows" {
+		return os.Symlink(target, link)
+	}
+	// Both paths are ones this guard just chose, under its own temporary
+	// directory and the repository - nothing a person typed.
+	// nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command
+	out, err := exec.Command("cmd", "/c", "mklink", "/J", link, target).CombinedOutput()
+	if err != nil {
+		return fmt.Errorf("%v: %s", err, out)
+	}
+	return nil
+}
+
 // sha256sum writes '<hash>  <name>' in text mode and '<hash> *<name>' in
 // binary mode, and a file saved on Windows may carry a byte order mark and
 // CRLF. The star is not part of the name, and none of it may reach an address