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

.github/scripts/build_packages.py

@@ -27,11 +27,15 @@
 import os
 import re
 import shutil
+import stat
 import sys
 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_]+)\}\}")
@@ -104,10 +108,22 @@ def refuse(message):
 
 
 def read_text(path):
+    """A file of this repository - the templates, go.mod, CNAME, the changelog.
+
+    These are not inputs a person chooses, so a failure here is a broken
+    checkout and a traceback says so. What a person DOES choose - the checksum
+    file - is read by read_sums, which refuses with a sentence instead.
+    """
     with open(path, encoding="utf-8-sig") as handle:
         return handle.read().replace("\r\n", "\n")
 
 
+# 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
+# reading stops one byte past it.
+SUMS_LIMIT = 1024 * 1024
+
+
 def repository():
     """owner/name, from the module path - the one place the address is written."""
     found = re.search(r"^module github\.com/([^/\s]+/[^/\s]+)\s*$",
@@ -150,8 +166,26 @@ 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.
     """
+    # Opened once and read at most one byte past the limit, from a regular file
+    # only: a size asked for first and a read made after could see two different
+    # files, and a pipe or a device has no size to ask (outside review of #145).
+    hint = "Download verify-SHA256SUMS.txt from the release you are packaging and pass that"
+    try:
+        with open(path, "rb") as handle:
+            if not stat.S_ISREG(os.fstat(handle.fileno()).st_mode):
+                refuse("%s is not a file. %s" % (path, hint))
+            data = handle.read(SUMS_LIMIT + 1)
+    except OSError as err:
+        refuse("cannot read the checksum file %s: %s. %s" % (path, err.strerror or err, hint))
+    if len(data) > SUMS_LIMIT:
+        refuse("%s is larger than %d bytes, so it is not a release's checksum file. %s"
+               % (path, SUMS_LIMIT, hint))
+    try:
+        text = data.decode("utf-8-sig").replace("\r\n", "\n")
+    except UnicodeDecodeError:
+        refuse("%s is not UTF-8 text, so it is not a release's checksum file. %s" % (path, hint))
     sums = {}
-    for number, line in enumerate(read_text(path).split("\n"), 1):
+    for number, line in enumerate(text.split("\n"), 1):
         if not line.strip():
             continue
         found = re.fullmatch(r"([0-9A-Fa-f]{64}) [ *](\S.*)", line.strip())
@@ -322,17 +356,46 @@ 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 "
                "them somewhere outside it" % out)
-    if os.path.exists(out) and (not os.path.isdir(out) or os.listdir(out)):
-        refuse("--out %s already holds something. Nothing is overwritten - pass an empty "
-               "or new directory" % out)
+    if os.path.exists(out):
+        try:
+            holds = not os.path.isdir(out) or bool(os.listdir(out))
+        except OSError as err:
+            refuse("cannot look inside --out %s: %s. Pass a new directory, or one you can "
+                   "read and write" % (out, err.strerror or err))
+        if holds:
+            refuse("--out %s already holds something. Nothing is overwritten - pass an empty "
+                   "or new directory" % out)
     return out
 
 
+def missing_folders(path):
+    """The folders of path that do not exist yet, deepest first - what this run
+    will have made, and the most a failed run may take away again."""
+    made = []
+    while not os.path.exists(path):
+        made.append(path)
+        up = os.path.dirname(path)
+        if up == path:
+            break
+        path = up
+    return made
+
+
+def remove_empty(folders):
+    """Remove the folders this run made, deepest first, stopping at the first one
+    that is not empty - it then holds something that is not ours."""
+    for folder in folders:
+        try:
+            os.rmdir(folder)
+        except OSError:
+            return
+
+
 def build(tag, sums_path, out):
     """Render every package into out, all or nothing."""
     version = parse_tag(tag)
@@ -342,8 +405,15 @@ 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)
+    made = missing_folders(parent)
+    try:
+        os.makedirs(parent, exist_ok=True)
+        work = tempfile.mkdtemp(prefix=".packages-", dir=parent)
+    except OSError as err:
+        remove_empty(made)
+        refuse("cannot create a working folder in %s: %s. Check that you can write there, "
+               "or pass another --out" % (parent, err.strerror or err))
+    finished = False
     try:
         used = set()
         known = set()
@@ -366,9 +436,17 @@ def build(tag, sums_path, out):
         if os.path.isdir(out):
             os.rmdir(out)
         os.rename(work, out)
+        finished = True
+    except OSError as err:
+        refuse("cannot write the packages to %s: %s. Check that you can write there, or "
+               "pass another --out. Nothing was left behind" % (out, err.strerror or err))
     finally:
+        # A refusal is a SystemExit, so it reaches here too: nothing half
+        # rendered, and no folder this run made, outlives a run that failed.
         if os.path.isdir(work):
             shutil.rmtree(work)
+        if not finished:
+            remove_empty(made)
     return out
 
 

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,146 @@ func entriesOf(t *testing.T, dir string) string {
 	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"},
+		// A device opens and reads like a file and has no size worth asking:
+		// NUL answers nothing, /dev/zero answers forever.
+		{"a checksum file that is a device", map[bool]string{true: "NUL", false: "/dev/zero"}[runtime.GOOS == "windows"],
+			filepath.Join(t.TempDir(), "packages"), "is not a file"},
+	} {
+		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)
+	}
+}
+
+// A run that fails part way leaves nothing behind - neither its working
+// folder nor the folders it made to hold --out - and a destination it cannot
+// look into is refused with a sentence. An outside review of #145 found all
+// three: the parents made by makedirs outlived a refusal that said "Nothing was
+// left behind", no case reached the handler for a write that fails after the
+// working folder exists, and os.listdir on --out could still raise.
+func TestTheRendererLeavesNothingBehindWhenItFailsPartWay(t *testing.T) {
+	unreleased := []byte(strings.ReplaceAll(string(fixtureSums()), "0.4.0", "9.9.9"))
+	nested := t.TempDir()
+	long := t.TempDir()
+	for _, c := range []struct {
+		what, tag string
+		sums      []byte
+		out, base string
+		says      string
+	}{
+		// The refusal comes from the changelog, after the parents are made.
+		{"a refusal after new parent folders were made", "v9.9.9", unreleased,
+			filepath.Join(nested, "new", "deeper", "packages"), nested, "CHANGELOG.md"},
+		// Every file is written into the working folder first, and the name is
+		// too long for any file system this runs on only at the last rename.
+		{"a write that fails after the working folder exists", packagingTag, fixtureSums(),
+			filepath.Join(long, strings.Repeat("x", 300)), long, "cannot write the packages"},
+	} {
+		r := renderPackages(t, c.tag, 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, c.base); left != "" {
+			t.Errorf("%s: the failed run left %s behind in %s", c.what, left, c.base)
+		}
+	}
+
+	// A folder this account cannot list, asked of the system first rather than
+	// assumed - an administrator can list some of these, and then the case says
+	// so instead of passing on nothing.
+	denied := map[string]string{"windows": `C:\System Volume Information`, "darwin": "/private/var/root"}[runtime.GOOS]
+	if denied == "" {
+		denied = "/root"
+	}
+	if _, err := os.ReadDir(denied); !os.IsPermission(err) {
+		t.Logf("NOT ASKED: %s answered %v rather than a refusal to list it, so there is no folder here "+
+			"this account cannot look into", denied, err)
+		return
+	}
+	r := renderPackages(t, packagingTag, fixtureSums(), denied)
+	if r.code != 1 || !strings.Contains(r.said, "cannot look inside --out") || strings.Contains(r.said, "Traceback") {
+		t.Errorf("a destination this account cannot list: exit %d, and it has to be refused with a sentence:\n%s",
+			r.code, r.said)
+	}
+}
+
+// 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("making a junction %s to %s: %w (%s)", link, target, 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

packaging/README.md

@@ -10,6 +10,18 @@ the product name and the licence, are held to their Go originals by a guard.
     python .github/scripts/build_packages.py --tag v0.4.0 \
         --sums verify-SHA256SUMS.txt --out <a directory outside the repository>
 
+The renderer refuses, with a sentence that names the input and says what to do,
+and never a traceback: a tag that is a release candidate or not a tag, a
+checksum file of another release or missing an archive, a version the changelog
+never dated, a checksum file that is missing, unreadable, not a regular file,
+not UTF-8 or over a megabyte (a release's is under a kilobyte), a destination
+inside the repository - compared as a resolved path, so a link or a junction
+does not get round it - or one that already holds something or cannot be looked
+into, and a value that would break the file it lands in. It renders into a
+working folder beside the destination and renames it at the end, and a run that
+fails removes the working folder and any folder it made to hold the
+destination.
+
 ## Four packages, two per feed
 
 | | the window | the command line |