Bump version

.clinerules

@@ -71,8 +71,9 @@ This service runs in Kubernetes behind a TLS-terminating ingress.
 - All upload code paths must enforce `maxUploadSize` via
   `http.MaxBytesReader`, and clean up any partially written file/directory
   on failure.
-- New HTTP handlers must call `logRequest` with an accurate status code for
+- New HTTP handlers must write an explicit status code on every response
-  every response path.
+  path. The `withLogging` middleware records what `responseRecorder` actually
+  saw, so a handler that returns without writing is logged as a 200.
 - Keep the JSON error shape for `/upload` consistent with
   `writeMultipartError` (`{"error": "..."}`).
 

.gitea/workflows/docker-build.yml

@@ -23,4 +23,5 @@ jobs:
           password: ${{ secrets.LOCAL_REGISTRY_PASSWORD }}
 
       - name: Push image
+        if: github.ref == 'refs/heads/main'
         run: ./build.sh push

README.md

@@ -45,6 +45,10 @@ Downloads support range requests, so an interrupted transfer can be resumed:
 curl -C - -O https://beam.example.com/x123abcx/myfile.txt
 ```
 
+Uploaded content is served with `X-Content-Type-Options: nosniff` and
+`Content-Security-Policy: sandbox`, so an uploaded HTML or SVG file opens as a
+sandboxed, script-free document instead of running on the beam origin.
+
 ### Endpoints
 
 | Method       | Path                 | Purpose                                  |
@@ -105,7 +109,7 @@ to write to whatever is mounted at `/uploads`.
 ## Development
 
 ```sh
-go build ./... && go vet ./... && gofmt -l . && go test ./...
+go build ./... && go vet ./... && gofmt -l .
 ```
 
 Run locally:

build.sh

@@ -1,7 +1,7 @@
 #!/bin/sh
 set -eu
 
-VERSION=1.2.0
+VERSION=1.3.0
 GIT_SHA=$(git rev-parse --short HEAD)
 IMAGE="docker.sdal.tech/beam:$VERSION-$GIT_SHA"
 IMAGE_LATEST="docker.sdal.tech/beam:latest"

main.go

@@ -262,14 +262,20 @@ func handleUpload(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "Failed to create file", http.StatusInternalServerError)
 		return
 	}
-	defer file.Close()
 
 	// MaxBytesReader needs the ResponseWriter the stdlib handed us so it can
 	// mark the connection for close; the logging wrapper would hide it.
 	limitedBody := http.MaxBytesReader(baseWriter(w), r.Body, maxUploadSize)
 	written, err := io.Copy(file, limitedBody)
+	// Closed here rather than in a defer: nothing above returns early, and a
+	// write-back failure only surfacing at close would otherwise arrive after
+	// the 200 and the URL had gone out, leaving the client believing a
+	// truncated file was stored.
+	closeErr := file.Close()
+	if err == nil {
+		err = closeErr
+	}
 	if err != nil {
-		file.Close()
 		os.Remove(targetPath)
 		os.Remove(targetDir)
 		if isMaxBytesError(err) {
@@ -466,6 +472,14 @@ func handleDownload(w http.ResponseWriter, r *http.Request) {
 	// The extension lookup relies on the system MIME database, which the
 	// Dockerfile provides via the mailcap package.
 	w.Header().Set("Content-Type", detectContentType(filename))
+	// Uploads are anonymous and served back from this origin, so an uploaded
+	// .html or .svg opened inline would otherwise be stored XSS. The sandbox
+	// directive puts the response in an opaque origin with scripting, forms and
+	// top-level navigation disabled; nosniff stops a mislabeled file from being
+	// re-typed into something executable. Both are set before ServeContent so
+	// they apply to its 206, 304 and HEAD responses too.
+	w.Header().Set("X-Content-Type-Options", "nosniff")
+	w.Header().Set("Content-Security-Policy", "sandbox")
 	// FormatMediaType returns "" for names it cannot encode; don't emit an
 	// empty header in that case.
 	if disposition := mime.FormatMediaType("inline", map[string]string{"filename": filename}); disposition != "" {
@@ -496,17 +510,34 @@ func generateUniqueID() (string, error) {
 	return "", fmt.Errorf("failed to generate unique ID after %d retries", maxRetries)
 }
 
+// generateID returns a random ID drawn uniformly from idCharset. Bytes landing
+// in the last, partial repetition of the charset within a byte's range are
+// discarded rather than folded in with a bare modulo: 256 = 7*36 + 4, so
+// folding would make "a" through "d" about 14% more likely than the other 32
+// characters. Only 4 of 256 values are rejected, so a second read is rarely
+// needed.
 func generateID() string {
-	bytes := make([]byte, idLength)
+	const unbiasedLimit = byte(256 - 256%len(idCharset))
-	if _, err := rand.Read(bytes); err != nil {
+
+	id := make([]byte, 0, idLength)
+	buf := make([]byte, idLength)
+	for len(id) < idLength {
+		if _, err := rand.Read(buf); err != nil {
 			panic(err)
 		}
 
-	for i, b := range bytes {
+		for _, b := range buf {
-		bytes[i] = idCharset[int(b)%len(idCharset)]
+			if b >= unbiasedLimit {
+				continue
+			}
+			id = append(id, idCharset[int(b)%len(idCharset)])
+			if len(id) == idLength {
+				break
+			}
+		}
 	}
 
-	return string(bytes)
+	return string(id)
 }
 
 func getHost(r *http.Request) string {
@@ -601,6 +632,29 @@ func (rr *responseRecorder) Write(b []byte) (int, error) {
 	return n, err
 }
 
+// ReadFrom keeps downloads on the kernel's sendfile path. io.Copy — which is
+// where http.ServeContent ends up — only lets the destination take over the
+// copy when it implements io.ReaderFrom, so wrapping the ResponseWriter would
+// otherwise hide net/http's own implementation and push every byte through a
+// userspace buffer.
+func (rr *responseRecorder) ReadFrom(src io.Reader) (int64, error) {
+	if rr.status == 0 {
+		rr.status = http.StatusOK
+	}
+
+	var n int64
+	var err error
+	if rf, ok := rr.ResponseWriter.(io.ReaderFrom); ok {
+		n, err = rf.ReadFrom(src)
+	} else {
+		// Copy into the wrapped writer, not into rr, or this recurses.
+		n, err = io.Copy(rr.ResponseWriter, src)
+	}
+
+	rr.written += n
+	return n, err
+}
+
 // Unwrap lets http.ResponseController and baseWriter reach the original
 // ResponseWriter.
 func (rr *responseRecorder) Unwrap() http.ResponseWriter { return rr.ResponseWriter }
@@ -742,6 +796,12 @@ func collectExpiredFiles(root string, duration time.Duration) ([]string, error)
 
 	err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
+			// A failure on root itself means the sweep saw nothing at all — an
+			// unmounted or unreadable volume — so let it reach the caller
+			// instead of logging it as one more skipped path.
+			if path == root {
+				return err
+			}
 			log.Printf("Cleanup: skipping %q: %v", path, err)
 			return nil
 		}
@@ -771,8 +831,13 @@ func collectEmptyDirs(root string) ([]string, error) {
 
 	err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
 		// d is nil when the root itself cannot be read, so this check must
-		// come first.
+		// come first. That case also means the sweep saw nothing at all — an
+		// unmounted or unreadable volume — so let it reach the caller instead
+		// of logging it as one more skipped path.
 		if err != nil {
+			if path == root {
+				return err
+			}
 			log.Printf("Cleanup: skipping %q: %v", path, err)
 			return nil
 		}