From caa5b5d8ffb69e08fa68205d2443a8d038ca1a3a Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 15 Oct 2026 22:30:00 +0200 Subject: [PATCH] fix(19.3-06): refuse protected uploads into a public_url bucket (WR-07) - attach.Store returns attach.ErrProtectedNeedsPrivateBucket for an is_public false upload while storage.uploads.public_url is set, before any blob is written - OpenBucket's public_url line becomes a warning naming the refusal --- docs/services/storage.md | 2 +- modules/lagoon/README.md | 5 +++-- modules/lagoon/attach/bucket.go | 8 +++++--- modules/lagoon/attach/store.go | 12 ++++++++++++ modules/lagoon/attach/url_test.go | 25 +++++++++++++++++++++++++ 5 files changed, 46 insertions(+), 6 deletions(-) diff --git a/docs/services/storage.md b/docs/services/storage.md index 17805f8..3e6518f 100644 --- a/docs/services/storage.md +++ b/docs/services/storage.md @@ -91,7 +91,7 @@ uploads: `attach.PublicURL`, `attach.File.URL` and thumbnail URLs then are that base, one slash and the blob key, which keeps the partition layout: `https://cdn.example.com/uploads/5f1/d0c/2e9/5f1d0c2e9a7b4c3d8e6f.jpg`. `attach.PublicURLBase` returns the base. The value must be an absolute `http` or `https` URL with a host and without user info, query or fragment; anything else stops the start-up with an error naming the key. Thumbnails are written to the bucket when they are first requested through `attach.File.Thumb`, so their URLs work behind the base as well. -The base applies to every URL the framework builds, private files included. The `/storage/uploads` route checks `is_public` before it serves a file; a public bucket or a CDN serves any key it holds. Use `public_url` only for a bucket that holds public files alone, or knowingly rely on the 22 random hexadecimal characters of each disk name. `serve` logs one line at start-up naming the host and this rule. +The `/storage/uploads` route checks `is_public` before it serves a file; a public bucket or a CDN serves any key it holds. A bucket behind `public_url` therefore holds public files only: while `public_url` is set, `attach.Store` refuses a protected (`is_public` false) upload with `attach.ErrProtectedNeedsPrivateBucket` before it writes anything. Keep protected relations on an application whose bucket is private, with `public_url` unset, or make the relation public. `serve` logs one warning at start-up naming the host and this rule. Leave `public_url` unset to keep the URLs under `public_path_prefix`. With `storage.uploads.serve` on, those URLs go through the [Serving files](#serving-files) route, which works for an S3 bucket too, and private files stay protected. diff --git a/modules/lagoon/README.md b/modules/lagoon/README.md index 797c348..c32d627 100644 --- a/modules/lagoon/README.md +++ b/modules/lagoon/README.md @@ -38,7 +38,7 @@ Use cases: - Lifecycle and relations: hook interfaces matching GORM's native method names (`lagoon.HasBeforeCreate`, `lagoon.HasBeforeSave`, `lagoon.HasBeforeDelete`, `lagoon.HasAfterDelete`) plus `lagoon.HasBeforeValidate`; `lagoon.WithSoftDeleteCascade` runs a cascade inside the parent delete; `lagoon.RegisterJoinTable` wires pivot models with business columns. - Deferred binding: WinterCMS's `deferred_bindings` table holds the uploads and related-record changes of a form whose record is not saved yet. Every operation takes a `lagoon.DeferredKey` (the form's session key, the owning backend admin's id and the master record's morph type from `lagoon.MorphType`) and never reads or changes another admin's rows, since each row stores `backend_user_id`. `lagoon.DeferredBind` and `lagoon.DeferredUnbind` port WinterCMS's duplicate and cancel rules: a repeated bind writes nothing, and an unbind of a slave with a pending bind deletes that bind and returns it so the caller can remove what it created. `lagoon.DeferredBindings` reads and locks a session's bindings for the save that commits them, `lagoon.DeferredForget` deletes them once applied, and `lagoon.DeferredSlaves` is the subquery a list uses to include pending rows. A child created under deferral carries the `lagoon.DeferredEnvelope` (`{"created":true,"pivot":{...},"upload_id":"..."}`) in `pivot_data`. `lagoon.PurgeDeferred` removes expired bindings: it deletes an unattached `system_files` row a bind points at, and its blobs only after the commit, deletes a child only when its binding carries the created envelope, keeps records that were only linked, and locks each batch with `FOR UPDATE SKIP LOCKED`. - Imports from Laravel: `lagoon.DecryptLaravelPayload` decrypts Laravel `encrypted` payloads with the old application key, for one-off data imports. -- Attachments (`attach`): the `attach.File` model for `system_files` rows, WinterCMS-compatible partitioned storage keys (`attach.BlobKey`, `attach.PartitionDirectory`), an S3 bucket option behind the `attach.S3BuildTag` build tag (an untagged binary refuses an `s3://` bucket URL at boot with an error naming the tag), public URLs (`attach.PublicURL` for any key, under `storage.uploads.public_url` when a public bucket or CDN serves the files (`attach.PublicURLBase`), `attach.File.URL` for an original, matching WinterCMS's `File::getPath()` under the WinterCMS layout), on-demand thumbnails through `attach.File.Thumb` for JPEG, PNG, GIF and WebP originals (a WebP original's thumbnail is JPEG bytes under its `.webp` name, since WebP cannot be encoded; a missing, undecodable or oversized original gets WinterCMS's broken-image picture, `attach.BrokenImagePNG`, as its thumbnail, as `File::makeThumb` does), storing uploads through `attach.Store` (a server-generated disk name whose extension follows the sniffed content through `attach.DiskExtension`, so a PNG named `x.html` is stored as `.png`, an extension allow-list with `attach.DefaultImageExtensions` and `attach.DefaultFileExtensions` as defaults, a MIME filter, a size limit enforced while streaming and, in image mode, the `attach.IsAllowedImage` content guard), attachment relation declarations (`attach.Relation`, `attach.HasRelations`), static serving with an optional `is_public` gate (`attach.StaticHandlerPublic`), and a two-phase delete that removes blobs only after the database transaction commits (`attach.DeleteForOwner`, `attach.DeleteKeys`). +- Attachments (`attach`): the `attach.File` model for `system_files` rows, WinterCMS-compatible partitioned storage keys (`attach.BlobKey`, `attach.PartitionDirectory`), an S3 bucket option behind the `attach.S3BuildTag` build tag (an untagged binary refuses an `s3://` bucket URL at boot with an error naming the tag), public URLs (`attach.PublicURL` for any key, under `storage.uploads.public_url` when a public bucket or CDN serves the files (`attach.PublicURLBase`), `attach.File.URL` for an original, matching WinterCMS's `File::getPath()` under the WinterCMS layout), on-demand thumbnails through `attach.File.Thumb` for JPEG, PNG, GIF and WebP originals (a WebP original's thumbnail is JPEG bytes under its `.webp` name, since WebP cannot be encoded; a missing, undecodable or oversized original gets WinterCMS's broken-image picture, `attach.BrokenImagePNG`, as its thumbnail, as `File::makeThumb` does), storing uploads through `attach.Store` (a server-generated disk name whose extension follows the sniffed content through `attach.DiskExtension`, so a PNG named `x.html` is stored as `.png`, an extension allow-list with `attach.DefaultImageExtensions` and `attach.DefaultFileExtensions` as defaults, a MIME filter, a size limit enforced while streaming, in image mode the `attach.IsAllowedImage` content guard, and the refusal of protected uploads while `storage.uploads.public_url` serves the bucket publicly, `attach.ErrProtectedNeedsPrivateBucket`), attachment relation declarations (`attach.Relation`, `attach.HasRelations`), static serving with an optional `is_public` gate (`attach.StaticHandlerPublic`), and a two-phase delete that removes blobs only after the database transaction commits (`attach.DeleteForOwner`, `attach.DeleteKeys`). ## Usage @@ -211,6 +211,7 @@ func (p *Plugin) Migrations() []*gormigrate.Migration { | `attach.ErrFileType` | Returned by `attach.Store` for a missing, malformed or disallowed extension. | | `attach.ErrMIMEType` | Returned by `attach.Store` when the content type matches no `MIMETypes` entry. | | `attach.ErrNotImage` | Returned by `attach.Store` in image mode for content that is not an allowed image. | +| `attach.ErrProtectedNeedsPrivateBucket` | Returned by `attach.Store` for a protected (`Public` false) upload while `storage.uploads.public_url` is set, before any blob is written: that bucket is served publicly. | | `attach.DefaultImageExtensions` | Image-mode extensions when none are given: jpg, jpeg, png, gif, webp. | | `attach.DefaultFileExtensions` | File-mode extensions when none are given: WinterCMS's default list without the script-capable types. | | `attach.AllowedImageMIMEs` | The sniffed content types the image guard accepts. | @@ -232,7 +233,7 @@ Keys are read from the compass config; the `SUMMER_` environment overlay maps a | `app.previous_keys` | empty | List of earlier base64 keys still accepted when decrypting, for key rotation. | | `storage.uploads.bucket_url` | none (required by `attach.OpenBucket`) | Uploads bucket URL, `file://`, `mem://` or `s3://` (with `attach.S3BuildTag`; query parameters such as `region`, `endpoint` and `use_path_style` per gocloud.dev, credentials from the AWS environment). | | `storage.uploads.public_path_prefix` | `/storage/uploads` | URL prefix used when building public file and thumbnail URLs. | -| `storage.uploads.public_url` | empty | Absolute `http` or `https` base of a public bucket or CDN; when set, file and thumbnail URLs are this base plus the partition path. A malformed value fails `attach.OpenBucket`; a set value logs that the bucket must hold only public files. | +| `storage.uploads.public_url` | empty | Absolute `http` or `https` base of a public bucket or CDN; when set, file and thumbnail URLs are this base plus the partition path. A malformed value fails `attach.OpenBucket`; a set value logs a warning that the bucket must hold only public files, and `attach.Store` refuses protected uploads (`attach.ErrProtectedNeedsPrivateBucket`). | | `database.deferred_bindings.purge_days` | `5` | Age in days after which `deferred:purge` removes a deferred binding, WinterCMS's `cleanUp(5)`; the `--days` flag overrides it. | | `database.deferred_bindings.purge_at` | `03:00` | Daily `HH:MM` (in `app.timezone`) at which the scheduler runs `deferred:purge`; an empty value disables the framework entry. | diff --git a/modules/lagoon/attach/bucket.go b/modules/lagoon/attach/bucket.go index bc19bf8..e473fde 100644 --- a/modules/lagoon/attach/bucket.go +++ b/modules/lagoon/attach/bucket.go @@ -115,7 +115,8 @@ func checkS3Driver(bucketURL string) error { // binary built with S3BuildTag) and records // storage.uploads.public_path_prefix and storage.uploads.public_url. An // empty bucket_url or a malformed public_url fails boot; a set public_url -// logs one info line saying the bucket behind it must hold only public files. +// logs one warning saying the bucket behind it must hold only public files +// and that Store refuses protected uploads (ErrProtectedNeedsPrivateBucket). func OpenBucket(ctx context.Context, cfg *compass.Config) (*blob.Bucket, error) { if cfg == nil { return nil, fmt.Errorf("attach: config is nil") @@ -149,8 +150,9 @@ func OpenBucket(ctx context.Context, cfg *compass.Config) (*blob.Bucket, error) return bucket, nil } setPublicURLBase(publicURL.String()) - slog.Default().InfoContext(ctx, "attach: storage.uploads.public_url is set; every file and thumbnail URL points at this host, "+ - "which serves any key without the is_public check, so the bucket behind it must hold only public files", + slog.Default().WarnContext(ctx, "attach: storage.uploads.public_url is set; every file and thumbnail URL points at this host, "+ + "which serves any key without the is_public check, so the bucket behind it must hold only public files: "+ + "protected (is_public false) uploads are refused", slog.String("host", publicURL.Host)) return bucket, nil } diff --git a/modules/lagoon/attach/store.go b/modules/lagoon/attach/store.go index 78d8dc3..42294cd 100644 --- a/modules/lagoon/attach/store.go +++ b/modules/lagoon/attach/store.go @@ -35,6 +35,11 @@ var ( // ErrNotImage is returned by Store in image mode when the bytes are not // an image IsAllowedImage accepts. ErrNotImage = errors.New("attach: file is not an allowed image") + // ErrProtectedNeedsPrivateBucket is returned by Store for a protected + // (Upload.Public false) upload while storage.uploads.public_url is set: + // the host behind public_url serves every key of the bucket without the + // is_public check, so a protected file needs a private bucket. + ErrProtectedNeedsPrivateBucket = errors.New("attach: storage.uploads.public_url is set, so the bucket is served publicly and a protected (is_public false) upload needs a private bucket: unset storage.uploads.public_url or make the relation public") ) // DefaultImageExtensions is the extension list of an image upload when @@ -88,6 +93,10 @@ type Limits struct { // Store saves an upload as an unattached system_files row. // +// A protected upload (Upload.Public false) while storage.uploads.public_url +// is set returns ErrProtectedNeedsPrivateBucket before the body is read or +// any blob is written. +// // It accepts the client extension, lower-cased, only when it matches // [a-z0-9]{1,10} and is allowed by Limits (else ErrFileType). It reads up to // 1 MiB ahead to sniff the content type from the bytes; in image mode those @@ -123,6 +132,9 @@ func Store(ctx context.Context, db *gorm.DB, bucket *blob.Bucket, in Upload, lim if lim.MaxBytes < 0 { return nil, fmt.Errorf("attach: negative size limit %d", lim.MaxBytes) } + if !in.Public && PublicURLBase() != "" { + return nil, ErrProtectedNeedsPrivateBucket + } name := clientBaseName(in.FileName) ext := strings.ToLower(strings.TrimPrefix(path.Ext(name), ".")) if !extPattern.MatchString(ext) || !slices.Contains(allowedExtensions(lim), ext) { diff --git a/modules/lagoon/attach/url_test.go b/modules/lagoon/attach/url_test.go index 45d44ea..f241cf5 100644 --- a/modules/lagoon/attach/url_test.go +++ b/modules/lagoon/attach/url_test.go @@ -3,6 +3,7 @@ package attach import ( "bytes" "encoding/binary" + "errors" "fmt" "hash/crc32" "image" @@ -19,6 +20,7 @@ import ( "git.golem15.com/golem15/summercms/modules/compass" "gocloud.dev/blob" "gocloud.dev/blob/memblob" + "gorm.io/gorm" ) // winterLayoutBucket opens a mem:// bucket with the WinterCMS public prefix @@ -466,3 +468,26 @@ func TestPublicURLValidation(t *testing.T) { } } } + +func TestStoreProtectedNeedsPrivateBucket(t *testing.T) { + // Not parallel: the public URL base is package state. + t.Cleanup(func() { setPublicURLBase("") }) + bucket := memblob.OpenBucket(nil) + t.Cleanup(func() { _ = bucket.Close() }) + db := &gorm.DB{} + + setPublicURLBase("https://cdn.example.com/uploads/") + _, err := Store(t.Context(), db, bucket, Upload{FileName: "notes.txt", Body: bytes.NewReader([]byte("secret"))}, Limits{}) + if !errors.Is(err, ErrProtectedNeedsPrivateBucket) { + t.Fatalf("protected upload with public_url: error = %v, want ErrProtectedNeedsPrivateBucket", err) + } + for _, want := range []string{"storage.uploads.public_url", "private bucket"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q lacks %q", err, want) + } + } + iter := bucket.List(nil) + if obj, err := iter.Next(t.Context()); err == nil { + t.Fatalf("a blob was written: %s", obj.Key) + } +}