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`)