fix(lagoon/attach): store non-inline uploads with Content-Disposition attachment (quick-261028-rqe)

- attach.IsInlineContentType: the shared inline raster list (JPEG, PNG,
  GIF, WebP, AVIF), parameters and case ignored, false for empty or
  unparsable values
- writeBlob stores Content-Disposition: attachment for every other type,
  so a public bucket or CDN makes the browser download HTML, SVG, PDF or
  plain text; content_type and thumbnails are unchanged
- driver tests on memblob and fileblob, Postgres-backed Store assertion,
  lagoon README and the storage and attachments pages

modules/lagoon/attach/inline.go

@@ -0,0 +1,44 @@
+package attach
+
+import (
+	"mime"
+	"strings"
+
+	"gocloud.dev/blob"
+)
+
+// inlineContentTypes are the raster image types a browser may show inline
+// from an upload.
+var inlineContentTypes = map[string]bool{
+	"image/avif": true,
+	"image/gif":  true,
+	"image/jpeg": true,
+	"image/png":  true,
+	"image/webp": true,
+}
+
+// attachmentDisposition is the Content-Disposition stored with a non-inline
+// upload. It is the bare value with no filename parameter: the client's file
+// name never reaches blob metadata or a response header.
+const attachmentDisposition = "attachment"
+
+// IsInlineContentType reports whether contentType, with parameters and case
+// ignored, is a raster image a browser may show inline from an upload (JPEG,
+// PNG, GIF, WebP, AVIF). An empty or unparsable value is not. Store stores
+// every other type, SVG, PDF, HTML and plain text included, with
+// Content-Disposition: attachment, and the surf uploads route serves it as a
+// download.
+func IsInlineContentType(contentType string) bool {
+	mediaType, _, err := mime.ParseMediaType(contentType)
+	return err == nil && inlineContentTypes[strings.ToLower(mediaType)]
+}
+
+// uploadWriterOptions are the blob writer options of an upload: the content
+// type, plus the attachment disposition when the type is not inline.
+func uploadWriterOptions(contentType string) *blob.WriterOptions {
+	opts := &blob.WriterOptions{ContentType: contentType}
+	if !IsInlineContentType(contentType) {
+		opts.ContentDisposition = attachmentDisposition
+	}
+	return opts
+}

modules/lagoon/attach/inline_test.go

@@ -0,0 +1,116 @@
+package attach
+
+import (
+	"strings"
+	"testing"
+
+	"gocloud.dev/blob"
+	"gocloud.dev/blob/fileblob"
+	"gocloud.dev/blob/memblob"
+)
+
+func TestIsInlineContentType(t *testing.T) {
+	cases := []struct {
+		in   string
+		want bool
+	}{
+		{"image/jpeg", true},
+		{"image/png", true},
+		{"image/gif", true},
+		{"image/webp", true},
+		{"image/avif", true},
+		{"IMAGE/WEBP; charset=binary", true},
+		{"text/html; charset=utf-8", false},
+		{"image/svg+xml", false},
+		{"application/pdf", false},
+		{"text/plain", false},
+		{"application/octet-stream", false},
+		{"application/zip", false},
+		{"image/bmp", false},
+		{"image/x-icon", false},
+		{"video/mp4", false},
+		{"audio/mpeg", false},
+		{"", false},
+		{"not a media type;;", false},
+	}
+	for _, tc := range cases {
+		if got := IsInlineContentType(tc.in); got != tc.want {
+			t.Errorf("IsInlineContentType(%q) = %v, want %v", tc.in, got, tc.want)
+		}
+	}
+}
+
+// TestWriteBlobDisposition is the driver half of the public-bucket fix: a
+// public bucket serves the headers Store stores with the object, so the
+// attachment disposition must survive the write on the real drivers. The
+// metadata-skip fileblob bucket shows that a driver which does not keep
+// attributes still accepts the option without an error.
+func TestWriteBlobDisposition(t *testing.T) {
+	cases := []struct {
+		name, ct, disposition string
+	}{
+		{"html", "text/html", "attachment"},
+		{"text", "text/plain", "attachment"},
+		{"pdf", "application/pdf", "attachment"},
+		{"svg", "image/svg+xml", "attachment"},
+		{"zip", "application/zip", "attachment"},
+		{"png", "image/png", ""},
+		{"jpeg", "image/jpeg", ""},
+		{"avif", "image/avif", ""},
+	}
+	sidecar, err := fileblob.OpenBucket(t.TempDir(), nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	noMeta, err := fileblob.OpenBucket(t.TempDir(), &fileblob.Options{Metadata: fileblob.MetadataDontWrite})
+	if err != nil {
+		t.Fatal(err)
+	}
+	buckets := []struct {
+		name       string
+		bucket     *blob.Bucket
+		keepsAttrs bool
+	}{
+		{"memblob", memblob.OpenBucket(nil), true},
+		{"fileblob sidecar", sidecar, true},
+		{"fileblob metadata skipped", noMeta, false},
+	}
+	const body = "<html><script>alert(1)</script></html>"
+	for _, b := range buckets {
+		bucket := b.bucket
+		t.Cleanup(func() { _ = bucket.Close() })
+		t.Run(b.name, func(t *testing.T) {
+			ctx := t.Context()
+			for _, tc := range cases {
+				key := "abc/def/012/" + tc.name + "-file.bin"
+				n, err := writeBlob(ctx, bucket, key, strings.NewReader(body), tc.ct, 0)
+				if err != nil {
+					t.Fatalf("%s: writeBlob: %v", tc.name, err)
+				}
+				if n != int64(len(body)) {
+					t.Fatalf("%s: wrote %d bytes, want %d", tc.name, n, len(body))
+				}
+				got, err := bucket.ReadAll(ctx, key)
+				if err != nil {
+					t.Fatalf("%s: ReadAll: %v", tc.name, err)
+				}
+				if string(got) != body {
+					t.Fatalf("%s: body = %q, want %q", tc.name, got, body)
+				}
+				if !b.keepsAttrs {
+					continue
+				}
+				attrs, err := bucket.Attributes(ctx, key)
+				if err != nil {
+					t.Fatalf("%s: Attributes: %v", tc.name, err)
+				}
+				if attrs.ContentType != tc.ct {
+					t.Errorf("%s: ContentType = %q, want %q", tc.name, attrs.ContentType, tc.ct)
+				}
+				if attrs.ContentDisposition != tc.disposition {
+					t.Errorf("%s: ContentDisposition = %q, want %q", tc.name, attrs.ContentDisposition, tc.disposition)
+				}
+			}
+		})
+	}
+}

modules/lagoon/attach/store.go

@@ -106,9 +106,13 @@ type Limits struct {
 // is then streamed into bucket at BlobKey of a server-generated disk name (22
 // random lowercase hex characters, a dot and the extension DiskExtension
 // derives from the sniffed type and the client extension, so a PNG named
-// x.html is stored as .png; file_name keeps the client's name); a body longer
-// than Limits.MaxBytes aborts the write, deletes the key and returns
-// ErrTooLarge. Finally it inserts the row with empty attachment columns,
+// x.html is stored as .png; file_name keeps the client's name). The blob's
+// Content-Type is the stored content type; a type IsInlineContentType does
+// not accept also gets Content-Disposition: attachment, so a public bucket or
+// CDN that sends the stored headers makes a browser download the file instead
+// of rendering it. The row's content_type holds the same type either way. A
+// body longer than Limits.MaxBytes aborts the write, deletes the key and
+// returns ErrTooLarge. Finally it inserts the row with empty attachment columns,
 // is_public from Upload.Public, the byte size and the content type, and sets
 // sort_order to the new id as WinterCMS's Sortable trait does. When the row
 // cannot be written the blob is deleted again.
@@ -201,7 +205,7 @@ func Store(ctx context.Context, db *gorm.DB, bucket *blob.Bucket, in Upload, lim
 func writeBlob(ctx context.Context, bucket *blob.Bucket, key string, r io.Reader, contentType string, limit int64) (int64, error) {
 	writeCtx, cancel := context.WithCancel(ctx)
 	defer cancel()
-	w, err := bucket.NewWriter(writeCtx, key, &blob.WriterOptions{ContentType: contentType})
+	w, err := bucket.NewWriter(writeCtx, key, uploadWriterOptions(contentType))
 	if err != nil {
 		return 0, fmt.Errorf("attach: blob writer: %w", err)
 	}

modules/lagoon/attach/store_test.go

@@ -506,13 +506,31 @@ func TestStoreDiskExtension(t *testing.T) {
 			if err := gdb.First(&stored, f.ID).Error; err != nil {
 				t.Fatal(err)
 			}
-			if stored.DiskName != f.DiskName || stored.FileName != tc.fileName {
+			if stored.DiskName != f.DiskName || stored.FileName != tc.fileName || stored.ContentType != f.ContentType {
 				t.Fatalf("stored row %+v", stored)
 			}
 			got, err := bucket.ReadAll(ctx, attach.BlobKey(f.DiskName))
 			if err != nil || !bytes.Equal(got, tc.body) {
 				t.Fatalf("blob under %s: %v", attach.BlobKey(f.DiskName), err)
 			}
+			attrs, err := bucket.Attributes(ctx, attach.BlobKey(f.DiskName))
+			if err != nil {
+				t.Fatalf("blob attributes: %v", err)
+			}
+			if attrs.ContentType != f.ContentType {
+				t.Fatalf("blob Content-Type %q, want the stored %q", attrs.ContentType, f.ContentType)
+			}
+			// An HTML body uploaded as note.txt keeps text/html as its type
+			// but is stored with an attachment disposition, so a public
+			// bucket makes the browser download it. Only raster images stay
+			// inline.
+			wantDisposition := "attachment"
+			if tc.ct == "image/png" || tc.ct == "image/jpeg" {
+				wantDisposition = ""
+			}
+			if attrs.ContentDisposition != wantDisposition {
+				t.Fatalf("blob Content-Disposition %q, want %q", attrs.ContentDisposition, wantDisposition)
+			}
 		})
 	}
 }

modules/lagoon/attach/thumb_test.go

@@ -128,6 +128,14 @@ func TestFileThumbResizesOnce(t *testing.T) {
 	if url1 != want || url2 != want {
 		t.Fatalf("urls = %q %q, want %q", url1, url2, want)
 	}
+	// Thumbnails stay inline: an image type and no disposition.
+	attrs, err := bucket.Attributes(ctx, strings.TrimPrefix(url1, "/storage/uploads/"))
+	if err != nil {
+		t.Fatalf("thumbnail Attributes: %v", err)
+	}
+	if attrs.ContentType != "image/jpeg" || attrs.ContentDisposition != "" {
+		t.Fatalf("thumbnail attrs = %q %q, want image/jpeg and no disposition", attrs.ContentType, attrs.ContentDisposition)
+	}
 }
 
 func TestFileThumbEncodeFailureDoesNotCache(t *testing.T) {