From 5a8b7f816fff28d0a175bfcf8d57f07096ed650a Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Sun, 4 Oct 2026 07:59:59 +0200 Subject: [PATCH] fix(14): WR-02 derive the screenshot extension from the sniffed image type The public screenshot kept the client file's extension, so a valid image named x.html or x.svg was stored and served under that type. The disk name now carries the client extension only when it names the sniffed type, else the type's own; file_name keeps only the client's base name. --- classes/submission_store.go | 63 ++++++++++++++++++++++++++++++------- feedback_test.go | 24 ++++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-) diff --git a/classes/submission_store.go b/classes/submission_store.go index 321f33a..1bd324d 100644 --- a/classes/submission_store.go +++ b/classes/submission_store.go @@ -7,6 +7,7 @@ import ( "errors" "mime" "path/filepath" + "slices" "strconv" "strings" @@ -71,31 +72,69 @@ func StoreSubmission(ctx context.Context, app *backpack.App, gdb *gorm.DB, bucke return err } +// screenshotExtensions are the disk-name extensions each allowed image +// type may carry; the first is used when the client's is not one of them. +var screenshotExtensions = map[string][]string{ + "image/jpeg": {"jpg", "jpeg"}, + "image/png": {"png"}, + "image/gif": {"gif"}, + "image/webp": {"webp"}, +} + +// screenshotExtension is the disk-name extension for a screenshot whose +// bytes sniffed as contentType: the client file's extension, lower-cased, +// when it names that type, else the type's own. The public uploads +// directory is served by extension, so a valid image named x.html or x.svg +// must never keep its client extension. +func screenshotExtension(contentType, fileName string) (string, error) { + allowed, ok := screenshotExtensions[contentType] + if !ok { + return "", errors.New("feedback: screenshot is not an allowed image") + } + ext := strings.ToLower(strings.TrimPrefix(filepath.Ext(fileName), ".")) + if slices.Contains(allowed, ext) { + return ext, nil + } + return allowed[0], nil +} + +// clientBaseName is the last path segment of a client file name, trimmed +// (attach.Store's cleaning of Upload.FileName). +func clientBaseName(name string) string { + name = strings.ReplaceAll(name, `\`, "/") + if i := strings.LastIndex(name, "/"); i >= 0 { + name = name[i+1:] + } + return strings.TrimSpace(name) +} + // storeScreenshot writes the screenshot under a fresh Winter-style disk -// name (22 hex characters plus the client file's extension, lower-cased) -// and inserts its system_files row attached to the submission, public and -// sorted by its own id as WinterCMS's Sortable trait does. The returned +// name (22 hex characters plus an extension that matches the sniffed image +// type) and inserts its system_files row attached to the submission, public +// and sorted by its own id as WinterCMS's Sortable trait does. The returned // file is set once the blob is written, so a failed row can be cleaned up. func storeScreenshot(ctx context.Context, db *gorm.DB, bucket *blob.Bucket, submissionID uint, shot *Screenshot) (*attach.File, error) { - raw := make([]byte, 11) - if _, err := rand.Read(raw); err != nil { - return nil, err - } - diskName := hex.EncodeToString(raw) - if ext := strings.ToLower(strings.TrimPrefix(filepath.Ext(shot.Filename), ".")); ext != "" { - diskName += "." + ext - } contentType := SniffImageMIME(shot.Data) if mt, _, err := mime.ParseMediaType(contentType); err == nil { contentType = mt } + fileName := clientBaseName(shot.Filename) + ext, err := screenshotExtension(contentType, fileName) + if err != nil { + return nil, err + } + raw := make([]byte, 11) + if _, err := rand.Read(raw); err != nil { + return nil, err + } + diskName := hex.EncodeToString(raw) + "." + ext if err := bucket.WriteAll(ctx, attach.BlobKey(diskName), shot.Data, &blob.WriterOptions{ContentType: contentType}); err != nil { return nil, err } public := true f := &attach.File{ DiskName: diskName, - FileName: shot.Filename, + FileName: fileName, FileSize: int64(len(shot.Data)), ContentType: contentType, IsPublic: &public, diff --git a/feedback_test.go b/feedback_test.go index 74dfbe6..f98488b 100644 --- a/feedback_test.go +++ b/feedback_test.go @@ -617,6 +617,30 @@ func TestFeedbackSubmit(t *testing.T) { } }) + t.Run("client-extension-never-names-the-public-file", func(t *testing.T) { + _, app, gdb := newHandler(t) + bucket, err := blob.OpenBucket(t.Context(), "mem://") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = bucket.Close() }) + for name, want := range map[string]string{ + "x.html": ".png", "../../x.svg": ".png", "noext": ".png", "Shot.PNG": ".png", + } { + sub := &models.Submission{Message: "x", Type: "bug", PageURL: "https://widget.example.test/"} + if err := classes.StoreSubmission(t.Context(), app, gdb, bucket, sub, &classes.Screenshot{Filename: name, Data: pngBytes(t, 0)}); err != nil { + t.Fatal(err) + } + file, err := classes.ScreenshotOf(t.Context(), gdb, sub.ID) + if err != nil || file == nil { + t.Fatalf("screenshot %v %v", file, err) + } + if !strings.HasSuffix(file.DiskName, want) || strings.Contains(file.FileName, "/") { + t.Fatalf("%q stored as %q (file_name %q)", name, file.DiskName, file.FileName) + } + } + }) + t.Run("rolled-back-write-queues-nothing", func(t *testing.T) { _, app, gdb := newHandler(t) subsBefore := countRows(t, gdb, `SELECT COUNT(*) FROM golem15_feedback_submissions`)