From 5dcfb3db18ec7d6f2d3a1f006dd9fbedf725697e Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Fri, 25 Sep 2026 23:21:59 +0200 Subject: [PATCH 1/2] packaging: the renderer refuses a file it cannot read instead of crashing, and resolves --out An outside review of #143 found two gaps in the renderer. A --sums file that is missing, unreadable or not UTF-8 printed a Python traceback rather than saying which file and what to do. And --out was checked against the repository by its spelling, so a link or a junction leading into the tree could put rendered packages inside it. Every read now turns a system error or a decoding error into a refusal that names the file. A checksum file over a megabyte is refused before it is read - a release's is under a kilobyte, so that is another file passed by mistake. ROOT and --out are compared as resolved paths. A working folder that cannot be made, and a write that fails, are refusals too, and the working folder is still removed. A guard hands the renderer each of these - a missing file, a file that is not UTF-8, two megabytes of text, a destination through a junction into an empty folder made inside the tree for the purpose, and a destination under a file - and holds each to exit 1 with a sentence and no traceback. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/build_packages.py | 48 +++++++++++--- internal/guard/packaging_test.go | 9 ++- internal/guard/packagingrefusal_test.go | 83 +++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 9 deletions(-) diff --git a/.github/scripts/build_packages.py b/.github/scripts/build_packages.py index 5e32861..6c89fd8 100644 --- a/.github/scripts/build_packages.py +++ b/.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) diff --git a/internal/guard/packaging_test.go b/internal/guard/packaging_test.go index a9b05ff..67527ba 100644 --- a/internal/guard/packaging_test.go +++ b/internal/guard/packaging_test.go @@ -74,6 +74,13 @@ 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 @@ -81,7 +88,7 @@ func renderPackages(t *testing.T, tag string, sums []byte, out string) rendering // 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 diff --git a/internal/guard/packagingrefusal_test.go b/internal/guard/packagingrefusal_test.go index 3929198..42c6e92 100644 --- a/internal/guard/packagingrefusal_test.go +++ b/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 @@ 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"}, + } { + 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 ' ' in text mode and ' *' 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 From 076d6857a545e7731391000eb2a43b71520a083c Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Mon, 28 Sep 2026 18:09:08 +0200 Subject: [PATCH 2/2] packaging: bounded read of the checksum file, nothing left behind by a failed run, every refusal says what to do From the review of #145. - The checksum file is opened once, refused unless it is a regular file, and read at most one byte past the limit. Asking its size first and reading it after could see two different files, and a pipe or a device has no size. read_text, which only ever reads files of this repository, no longer pretends to handle a person's mistakes - read_sums does that. - A folder --out cannot be looked into is refused with a sentence. - A run that fails removes the folders it made to hold --out, deepest first, stopping at the first one that is not empty. - Every new refusal says what to do next. - The junction helper in the guard wraps its error with %w and names both paths (the linter's errorlint, and the review). TestTheRendererLeavesNothingBehindWhenItFailsPartWay makes a refusal after new parent folders were made, a rename that fails after the working folder exists - a name too long for any file system this runs on - and a destination the account cannot list, asked of the system first. The refusal guard also hands the renderer a device as the checksum file. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/build_packages.py | 94 ++++++++++++++++++------- internal/guard/packagingrefusal_test.go | 62 +++++++++++++++- packaging/README.md | 12 ++++ 3 files changed, 143 insertions(+), 25 deletions(-) diff --git a/.github/scripts/build_packages.py b/.github/scripts/build_packages.py index 6c89fd8..cba819c 100644 --- a/.github/scripts/build_packages.py +++ b/.github/scripts/build_packages.py @@ -27,6 +27,7 @@ import os import re import shutil +import stat import sys import tempfile from collections import namedtuple @@ -106,25 +107,20 @@ def refuse(message): raise SystemExit("build_packages: %s" % message) -def read_text(path, what="the file"): - """A text file, or a refusal naming it - never a traceback. +def read_text(path): + """A file of this repository - the templates, go.mod, CNAME, the changelog. - 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). + 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. """ - 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)) + 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 is -# refused by size before a byte of it is read. +# near this is another file passed by mistake, an archive for instance, and +# reading stops one byte past it. SUMS_LIMIT = 1024 * 1024 @@ -170,15 +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. """ - hint = "Download verify-SHA256SUMS.txt from the release you are packaging" + # 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: - size = os.path.getsize(path) + 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 size > SUMS_LIMIT: - refuse("%s is %d bytes, so it is not a release's checksum file. %s" % (path, size, 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, "a checksum file").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()) @@ -354,12 +361,41 @@ def check_out(out): 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) @@ -369,11 +405,15 @@ def build(tag, sums_path, out): refuse("the icon %s is not in the repository" % ICON) parent = os.path.dirname(out) + made = missing_folders(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)) + 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() @@ -396,11 +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. Nothing was left behind" % (out, err.strerror or 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 diff --git a/internal/guard/packagingrefusal_test.go b/internal/guard/packagingrefusal_test.go index 42c6e92..46ed4e7 100644 --- a/internal/guard/packagingrefusal_test.go +++ b/internal/guard/packagingrefusal_test.go @@ -161,6 +161,10 @@ func TestTheRendererRefusesAFileItCannotReadAndAPathThatLeadsIntoTheTree(t *test 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") { @@ -176,6 +180,62 @@ func TestTheRendererRefusesAFileItCannotReadAndAPathThatLeadsIntoTheTree(t *test } } +// 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 { @@ -187,7 +247,7 @@ func makeDirectoryLink(link, target string) error { // 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 fmt.Errorf("making a junction %s to %s: %w (%s)", link, target, err, out) } return nil } diff --git a/packaging/README.md b/packaging/README.md index cc7acb4..9d176d0 100644 --- a/packaging/README.md +++ b/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 +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 |