fix(client): upload an attachment through the root that checked it

_plans/052_upload-through-the-root.md

@@ -0,0 +1,103 @@
+# 052: upload an attachment through the root that checked it
+
+Answers #186. The converter checks an image through the documentation root's
+`os.Root`, but the client computes its checksum and uploads it through a plain
+path. So the check and the two reads are three separate lookups, and only the
+first is bounded. A directory on the path replaced by a symbolic link between
+the check and the reads makes markfluence upload a file from outside the root,
+which **S2** (`no-read-outside-root`) rules out. It is a race, not a static
+hole: no layout of files on disk triggers it by itself.
+
+The same shape has a second, milder consequence, which this plan fixes too:
+the checksum and the upload are two opens, so the bytes uploaded can differ
+from the bytes whose checksum is recorded in the attachment's comment.
+
+## Decisions
+
+**D1. A `LocalAttachment` carries the root, and opens through it.** It gains
+`Root *os.Root` (tagged `json:"-"`, so `check --show-html --json` and the
+schema are unchanged) and an `Open() (*os.File, error)` method:
+
+- with a `Root`, it opens `Source`, the root-relative path the converter
+  checked, through `Root.Open`, which refuses an escape through a symbolic
+  link anywhere on the path;
+- without one, it opens `Path` with `os.Open`.
+
+`Path` stays: it is what error messages and `check --show-html` show.
+
+**D2. The converter sets `Root`.** `images.go` already holds `r.root.FS`; it
+sets it on every attachment it records.
+
+**D3. `attachment-upload` sets no `Root`.** Its files are named on the command
+line by the person running it, not by a reference in a Markdown file, so S2
+does not govern them, and a file given with `--name` need not be under any
+root at all. It still gets D4, which removes its own check-then-open race
+(`os.Stat` and a later open).
+
+**D4. Whatever is opened must be a regular file, checked on the handle.**
+`Open` calls `Stat` on the opened file and refuses anything that is not
+regular. The converter's `Lstat` and `attachment-upload`'s `os.Stat` checked a
+path; this checks the thing actually read, so a FIFO swapped in cannot hang
+the upload and a directory cannot be "uploaded".
+
+**D5. The upload verifies the checksum it recorded.** `planAttachments`
+computes the checksum through `Open`; `uploadAttachment` hashes the bytes as
+it copies them into the request, and before sending compares the result with
+the planned checksum. On a mismatch it sends nothing and fails with an error
+naming the file, `changed while publishing`. The upload already reads the whole
+file into memory, so this costs one hash and no extra read. The error is local
+(not a `requestError`), so `--json` reports it as a local failure, not
+`NETWORK`.
+
+## Files
+
+| file | change |
+|---|---|
+| `internal/attachref/attachref.go` | `Root` field, `Open` method |
+| `internal/convert/images.go` | set `Root` |
+| `internal/client/client.go` | `planAttachments` and `uploadAttachment` open through `Open`; `uploadAttachment` takes the planned checksum and verifies it; `fileChecksum` takes the attachment |
+| `cmd/attachmentupload/attachmentupload.go` | nothing required; the `os.Stat` check stays for its early, friendly error |
+| `CLAUDE.md` | `internal/attachref` has no bullet; the client bullet notes that attachments open through the root |
+
+## Tests
+
+- `attachref`: `Open` with a root opens the file; refuses an escape through a
+  symlinked directory (built after the root is opened, which is the race);
+  refuses a non-regular file (a FIFO, a directory); without a root opens
+  `Path`.
+- `client`: `SyncAttachments` with an attachment whose root-relative path
+  escapes through a symlinked directory fails and uploads nothing; a file
+  whose content changes between plan and upload fails with `changed while
+  publishing` and uploads nothing (a test hook between the two, or a plan
+  built by hand with a wrong checksum).
+- `convert`: an image attachment carries the root.
+
+## Not in scope
+
+- **Holding one file handle from plan to upload.** It would make D5
+  unnecessary, but a page with many images would hold many descriptors open
+  across network calls. Reopening through the root and verifying the checksum
+  gives the same guarantee.
+- **Bounding `attachment-upload` by a root** (D3).
+
+## Amended after code review
+
+- **D4: a symbolic link inside the root is refused too.** `os.Root` refuses
+  an escape but follows a link that stays inside the root, and markfluence
+  follows none (docs/design-principles.md, Symlinks). A link swapped in at an
+  image's own name since the converter's `Lstat` would publish another file in
+  the project, a `.env` say, under the image's name. After opening through the
+  root, `Open` re-`Lstat`s the name, refuses a link, and requires `os.SameFile`
+  between the name and the handle.
+- **D4: an escape reads as "outside the documentation root"**, the converter's
+  wording, not `os.Root`'s bare `path escapes from parent` under a generic
+  "opening" wrapper, which read as a disk failure.
+- **D5 is replaced.** Refusing a file that changed between planning and
+  upload made an autosave or a build step fail a whole publish, half done.
+  Instead `uploadAttachment` reads the file whole and takes the comment's
+  checksum from the bytes it sends, which keeps the comment honest with no new
+  way to fail; the upload buffered the whole form already. The planned
+  checksum and comment no longer ride on the plan.
+- **Not changed:** a batch that stops partway still reports none of the
+  uploads that landed before the failure. That predates this plan (any upload
+  error does it) and is left for its own change.

internal/attachref/attachref.go

@@ -5,6 +5,14 @@
 // instead of being owned by either.
 package attachref
 
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+	"syscall"
+)
+
 // LocalAttachment is a local file to be uploaded as one page's attachment.
 // Path is absolute. Filename is the attachment name, which is Source's base
 // name (convert.AttachmentFilename). Source is the normalized root-relative
@@ -21,4 +29,80 @@ type LocalAttachment struct {
 	Filename string `json:"filename"`
 	Path     string `json:"path"`
 	Source   string `json:"source"`
+
+	// Root, when set, is the documentation root Source is relative to, and
+	// Open reads through it (#186, S2). The converter sets it; a file named on
+	// attachment-upload's command line has none, since no Markdown reference
+	// chose it and --name lets it live anywhere.
+	Root *os.Root `json:"-"`
+}
+
+// Open opens the file to upload. With a Root it opens Source through the
+// root, the path the converter checked, so a directory on the way that became
+// a symbolic link since the check cannot lead the read outside the root: the
+// check and the read would otherwise be two separate lookups. Without one it
+// opens Path.
+//
+// os.Root refuses an escape but follows a symbolic link that stays inside the
+// root, and markfluence follows none (docs/design-principles.md, Symlinks): a
+// link swapped in at the file's own name since the check would publish some
+// other file in the project -- a .env, say -- under this attachment's name. So
+// after opening, the name is looked at again through the root, and it must not
+// be a link and must be the very file the handle holds.
+//
+// Whatever is opened must be a regular file, checked on the handle rather than
+// the path, so the check describes the thing actually read. The open is
+// non-blocking, so a FIFO put in the file's place is refused instead of
+// hanging the upload until something writes to it.
+func (a LocalAttachment) Open() (*os.File, error) {
+	const flags = os.O_RDONLY | syscall.O_NONBLOCK
+	var f *os.File
+	var err error
+	if a.Root != nil {
+		f, err = a.Root.OpenFile(filepath.FromSlash(a.Source), flags, 0)
+	} else {
+		f, err = os.OpenFile(a.Path, flags, 0)
+	}
+	if err != nil {
+		return nil, a.openError(err)
+	}
+	if err := a.check(f); err != nil {
+		_ = f.Close()
+		return nil, err
+	}
+	return f, nil
+}
+
+// check refuses a handle that is not a regular file, or, through a Root, a
+// name that is a symbolic link or no longer names the file the handle holds.
+func (a LocalAttachment) check(f *os.File) error {
+	fi, err := f.Stat()
+	if err != nil {
+		return err
+	}
+	if !fi.Mode().IsRegular() {
+		return fmt.Errorf("%s is not a regular file", a.Path)
+	}
+	if a.Root == nil {
+		return nil
+	}
+	named, err := a.Root.Lstat(filepath.FromSlash(a.Source))
+	switch {
+	case err != nil:
+		return a.openError(err)
+	case named.Mode()&os.ModeSymlink != 0:
+		return fmt.Errorf("%s is a symbolic link, not a regular file", a.Path)
+	case !os.SameFile(fi, named):
+		return fmt.Errorf("%s changed while it was being opened", a.Path)
+	}
+	return nil
+}
+
+// openError reports an escape the way the converter does, since os.Root's own
+// error names neither the file nor the reason and reads as a disk failure.
+func (a LocalAttachment) openError(err error) error {
+	if strings.Contains(err.Error(), "escapes from parent") {
+		return fmt.Errorf("%s is outside the documentation root", a.Path)
+	}
+	return fmt.Errorf("opening %s: %w", a.Path, err)
 }

internal/attachref/attachref_test.go

@@ -0,0 +1,140 @@
+package attachref
+
+import (
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+	"syscall"
+	"testing"
+)
+
+// rootWith makes a documentation root holding d/x.png and opens it.
+func rootWith(t *testing.T) (dir string, root *os.Root) {
+	t.Helper()
+	dir = t.TempDir()
+	if err := os.MkdirAll(filepath.Join(dir, "d"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(dir, "d", "x.png"), []byte("inside"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	root, err := os.OpenRoot(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = root.Close() })
+	return dir, root
+}
+
+func read(t *testing.T, a LocalAttachment) string {
+	t.Helper()
+	f, err := a.Open()
+	if err != nil {
+		t.Fatalf("Open: %v", err)
+	}
+	defer func() { _ = f.Close() }()
+	b, err := io.ReadAll(f)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return string(b)
+}
+
+func TestOpenThroughTheRoot(t *testing.T) {
+	dir, root := rootWith(t)
+	a := LocalAttachment{Path: filepath.Join(dir, "d", "x.png"), Source: "d/x.png", Root: root}
+	if got := read(t, a); got != "inside" {
+		t.Errorf("read %q, want inside", got)
+	}
+}
+
+// TestOpenRefusesAnEscapeMadeAfterTheCheck is #186: the directory on the path
+// becomes a symbolic link out of the root after the root was opened (and the
+// converter checked the file). Opening by Path would follow it; opening
+// through the root refuses.
+func TestOpenRefusesAnEscapeMadeAfterTheCheck(t *testing.T) {
+	dir, root := rootWith(t)
+	outside := t.TempDir()
+	if err := os.WriteFile(filepath.Join(outside, "x.png"), []byte("secret"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.RemoveAll(filepath.Join(dir, "d")); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(outside, filepath.Join(dir, "d")); err != nil {
+		t.Fatal(err)
+	}
+
+	a := LocalAttachment{Path: filepath.Join(dir, "d", "x.png"), Source: "d/x.png", Root: root}
+	f, err := a.Open()
+	if err == nil {
+		b, _ := io.ReadAll(f)
+		_ = f.Close()
+		t.Fatalf("Open read %q from outside the root, want an error", b)
+	}
+	if want := a.Path + " is outside the documentation root"; err.Error() != want {
+		t.Errorf("err = %v, want %q", err, want)
+	}
+}
+
+// TestOpenRefusesWhatIsNotAFile: checked on the handle, and a FIFO must be
+// refused rather than block the open until something writes to it.
+func TestOpenRefusesWhatIsNotAFile(t *testing.T) {
+	dir, root := rootWith(t)
+	if err := syscall.Mkfifo(filepath.Join(dir, "d", "pipe.png"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	for _, source := range []string{"d/pipe.png", "d"} {
+		a := LocalAttachment{Path: filepath.Join(dir, source), Source: source, Root: root}
+		if f, err := a.Open(); err == nil {
+			_ = f.Close()
+			t.Errorf("%s: Open succeeded, want it refused", source)
+		} else if !strings.Contains(err.Error(), "not a regular file") {
+			t.Errorf("%s: err = %v", source, err)
+		}
+		a.Root = nil
+		if f, err := a.Open(); err == nil {
+			_ = f.Close()
+			t.Errorf("%s without a root: Open succeeded, want it refused", source)
+		}
+	}
+}
+
+// TestOpenWithoutARootUsesPath: attachment-upload's files, which no root
+// bounds.
+func TestOpenWithoutARootUsesPath(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "anywhere.png")
+	if err := os.WriteFile(path, []byte("given"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if got := read(t, LocalAttachment{Path: path, Source: "renamed.png"}); got != "given" {
+		t.Errorf("read %q, want given", got)
+	}
+}
+
+// TestOpenRefusesASymlinkInsideTheRoot: os.Root would follow a link that stays
+// inside the root, and markfluence follows none, so a link swapped in at the
+// file's own name cannot publish another file in the project under its name.
+func TestOpenRefusesASymlinkInsideTheRoot(t *testing.T) {
+	dir, root := rootWith(t)
+	if err := os.WriteFile(filepath.Join(dir, ".env"), []byte("CONFLUENCE_TOKEN=x"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Remove(filepath.Join(dir, "d", "x.png")); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink("../.env", filepath.Join(dir, "d", "x.png")); err != nil {
+		t.Fatal(err)
+	}
+	a := LocalAttachment{Path: filepath.Join(dir, "d", "x.png"), Source: "d/x.png", Root: root}
+	f, err := a.Open()
+	if err == nil {
+		b, _ := io.ReadAll(f)
+		_ = f.Close()
+		t.Fatalf("Open read %q through an in-root symlink, want an error", b)
+	}
+	if !strings.Contains(err.Error(), "symbolic link") {
+		t.Errorf("err = %v", err)
+	}
+}