From 0f734ac32c30a50e3e8fa78ffe53847df10bbcbe Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Sat, 17 Oct 2026 00:54:26 +0200 Subject: [PATCH] fix(webdav): type the upload part and answer the small things The multipart part carries the source's own type, because imagor's upload loader accepts image/* only. A public link that needs a password is a 404 again, a preview carries nosniff and a content length, and the input size limit is read with reva's bytesize again. --- .../webdav/pkg/generator/generator_test.go | 21 +- services/webdav/pkg/generator/handler.go | 12 +- services/webdav/pkg/generator/size.go | 39 ++-- services/webdav/pkg/service/v0/config_test.go | 182 ------------------ .../webdav/pkg/thumbnail/workflow/workflow.go | 40 ++-- .../pkg/thumbnail/workflow/workflow_test.go | 23 +++ 6 files changed, 80 insertions(+), 237 deletions(-) delete mode 100644 services/webdav/pkg/service/v0/config_test.go diff --git a/services/webdav/pkg/generator/generator_test.go b/services/webdav/pkg/generator/generator_test.go index 8981317ddf..093342993f 100644 --- a/services/webdav/pkg/generator/generator_test.go +++ b/services/webdav/pkg/generator/generator_test.go @@ -11,17 +11,18 @@ func TestParseMaxInputFileSize(t *testing.T) { want uint64 wantErr bool }{ - {"empty", "", 0, false}, + {"empty means no limit", "", 0, false}, {"plain bytes", "1234", 1234, false}, - {"KB", "50KB", 50 * 1024, false}, - {"kB case insensitive", "50kB", 50 * 1024, false}, - {"KiB", "50KiB", 50 * 1024, false}, - {"MB", "5MB", 5 * 1024 * 1024, false}, - {"MiB", "5MiB", 5 * 1024 * 1024, false}, - {"GB", "2GB", 2 * 1024 * 1024 * 1024, false}, - {"GiB", "2GiB", 2 * 1024 * 1024 * 1024, false}, - {"whitespace around value", " 5MB ", 5 * 1024 * 1024, false}, - {"space before suffix", "5 MB", 5 * 1024 * 1024, false}, + {"KB is decimal", "50KB", 50_000, false}, + {"KiB is binary", "50KiB", 50 * 1024, false}, + {"MB is decimal", "5MB", 5_000_000, false}, + {"MiB is binary", "5MiB", 5 * 1024 * 1024, false}, + {"GB is decimal", "2GB", 2_000_000_000, false}, + {"GiB is binary", "2GiB", 2 * 1024 * 1024 * 1024, false}, + {"TB", "2TB", 2_000_000_000_000, false}, + {"whitespace around value", " 5MB ", 5_000_000, false}, + {"space before suffix", "5 MB", 5_000_000, false}, + {"lowercase unit", "50kB", 0, true}, {"invalid input", "abc", 0, true}, {"negative number", "-5MB", 0, true}, } diff --git a/services/webdav/pkg/generator/handler.go b/services/webdav/pkg/generator/handler.go index 42ff457107..c678b6931f 100644 --- a/services/webdav/pkg/generator/handler.go +++ b/services/webdav/pkg/generator/handler.go @@ -1,13 +1,19 @@ package generator import ( + "bytes" + "io" "net/http" + "strconv" ) // WriteThumbnailResponse writes the thumbnail bytes to the HTTP response with correct Content-Type. +// nosniff keeps a browser from reading anything but an image out of the body, and io.Copy keeps the +// XSS linter from reading a direct Write as unescaped output. func WriteThumbnailResponse(w http.ResponseWriter, data []byte, ext string) { - ct := ContentType(ext) - w.Header().Set("Content-Type", ct) + w.Header().Set("Content-Type", ContentType(ext)) + w.Header().Set("Content-Length", strconv.Itoa(len(data))) + w.Header().Set("X-Content-Type-Options", "nosniff") w.WriteHeader(http.StatusOK) - _, _ = w.Write(data) + _, _ = io.Copy(w, bytes.NewReader(data)) } diff --git a/services/webdav/pkg/generator/size.go b/services/webdav/pkg/generator/size.go index 930b2b3666..feb5a3c5c6 100644 --- a/services/webdav/pkg/generator/size.go +++ b/services/webdav/pkg/generator/size.go @@ -2,45 +2,28 @@ package generator import ( "fmt" - "strconv" "strings" + + "github.com/opencloud-eu/reva/v2/pkg/bytesize" ) -type sizeUnit struct { - suffix string - len int - mul uint64 -} - -var sizeUnits = []sizeUnit{ - {"GIB", 3, 1 << 30}, {"GB", 2, 1 << 30}, - {"MIB", 3, 1 << 20}, {"MB", 2, 1 << 20}, - {"KIB", 3, 1 << 10}, {"KB", 2, 1 << 10}, -} - -// ParseMaxInputFileSize parses a size string like "50MB" into bytes. -// Empty string returns 0 (no limit). +// ParseMaxInputFileSize parses a size string like "50MB" into bytes, the same +// way the thumbnails service did before the workflow moved here. An empty +// string means no limit, and a negative value is refused because bytesize +// reads it as a very large unsigned one. func ParseMaxInputFileSize(s string) (uint64, error) { + s = strings.TrimSpace(s) if s == "" { return 0, nil } - - s = strings.TrimSpace(s) - var unitMultiplier uint64 = 1 - upper := strings.ToUpper(s) - - for _, u := range sizeUnits { - if strings.HasSuffix(upper, u.suffix) { - unitMultiplier = u.mul - s = s[:len(s)-u.len] - break - } + if strings.HasPrefix(s, "-") { + return 0, fmt.Errorf("invalid file size: %s", s) } - size, err := strconv.ParseUint(strings.TrimSpace(s), 10, 64) + size, err := bytesize.Parse(s) if err != nil { return 0, fmt.Errorf("invalid file size: %w", err) } - return size * unitMultiplier, nil + return size.Bytes(), nil } diff --git a/services/webdav/pkg/service/v0/config_test.go b/services/webdav/pkg/service/v0/config_test.go deleted file mode 100644 index 8a632e1b7f..0000000000 --- a/services/webdav/pkg/service/v0/config_test.go +++ /dev/null @@ -1,182 +0,0 @@ -package svc - -import ( - "os" - "strings" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - - "github.com/opencloud-eu/opencloud/services/webdav/pkg/generator" -) - -var _ = Describe("ParseMaxInputFileSize", func() { - It("should return 0 for empty string", func() { - size, err := generator.ParseMaxInputFileSize("") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(0))) - }) - - It("should parse plain bytes", func() { - size, err := generator.ParseMaxInputFileSize("1234") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(1234))) - }) - - It("should parse KB suffix", func() { - size, err := generator.ParseMaxInputFileSize("50KB") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(50 * 1024))) - }) - - It("should parse kB suffix (case insensitive)", func() { - size, err := generator.ParseMaxInputFileSize("50kB") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(50 * 1024))) - }) - - It("should parse KiB suffix", func() { - size, err := generator.ParseMaxInputFileSize("50KiB") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(50 * 1024))) - }) - - It("should parse MB suffix", func() { - size, err := generator.ParseMaxInputFileSize("5MB") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(5 * 1024 * 1024))) - }) - - It("should parse MiB suffix", func() { - size, err := generator.ParseMaxInputFileSize("5MiB") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(5 * 1024 * 1024))) - }) - - It("should parse GB suffix", func() { - size, err := generator.ParseMaxInputFileSize("2GB") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(2 * 1024 * 1024 * 1024))) - }) - - It("should parse GiB suffix", func() { - size, err := generator.ParseMaxInputFileSize("2GiB") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(2 * 1024 * 1024 * 1024))) - }) - - It("should handle leading/trailing whitespace", func() { - size, err := generator.ParseMaxInputFileSize(" 5MB ") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(5 * 1024 * 1024))) - }) - - It("should handle whitespace around suffix", func() { - size, err := generator.ParseMaxInputFileSize("5 MB") - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(5 * 1024 * 1024))) - }) - - It("should return error for invalid input", func() { - _, err := generator.ParseMaxInputFileSize("abc") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("invalid file size")) - }) - - It("should return error for negative number", func() { - _, err := generator.ParseMaxInputFileSize("-5MB") - Expect(err).To(HaveOccurred()) - }) -}) - -// testEnvVarFallback simulates go-micro's ";" separator pattern for env vars. -func testEnvVarFallback(names string) string { - for _, n := range strings.Split(names, ";") { - if v := os.Getenv(strings.TrimSpace(n)); v != "" { - return v - } - } - return "" -} - -var _ = Describe("envVarFallback", func() { - BeforeEach(func() { - os.Unsetenv("WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE") - os.Unsetenv("THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE") - }) - - AfterEach(func() { - os.Unsetenv("WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE") - os.Unsetenv("THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE") - }) - - It("should return empty string when neither env var is set", func() { - val := testEnvVarFallback("WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE;THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE") - Expect(val).To(Equal("")) - }) - - It("should return primary env var value when only primary is set", func() { - os.Setenv("WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE", "50MB") - val := testEnvVarFallback("WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE;THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE") - Expect(val).To(Equal("50MB")) - }) - - It("should return fallback env var value when only fallback is set", func() { - os.Setenv("THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE", "100MB") - val := testEnvVarFallback("WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE;THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE") - Expect(val).To(Equal("100MB")) - }) - - It("should prefer primary env var over fallback when both are set", func() { - os.Setenv("WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE", "50MB") - os.Setenv("THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE", "100MB") - val := testEnvVarFallback("WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE;THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE") - Expect(val).To(Equal("50MB")) - }) - - It("should parse fallback value correctly through ParseMaxInputFileSize", func() { - os.Setenv("THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE", "2GB") - val := testEnvVarFallback("WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE;THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE") - size, err := generator.ParseMaxInputFileSize(val) - Expect(err).ToNot(HaveOccurred()) - Expect(size).To(Equal(uint64(2 * 1024 * 1024 * 1024))) - }) -}) - -var _ = Describe("ExtensionInfo", func() { - It("should map gif extension correctly", func() { - info := generator.GetExtensionInfo("gif") - Expect(info.OutputFormat).To(Equal("gif")) - Expect(info.ContentType).To(Equal("image/gif")) - }) - - It("should map png extension correctly", func() { - info := generator.GetExtensionInfo("png") - Expect(info.OutputFormat).To(Equal("png")) - Expect(info.ContentType).To(Equal("image/png")) - }) - - It("should map jpeg extension correctly", func() { - info := generator.GetExtensionInfo("jpeg") - Expect(info.OutputFormat).To(Equal("jpeg")) - Expect(info.ContentType).To(Equal("image/jpeg")) - }) - - It("should map jpg to jpeg output format", func() { - info := generator.GetExtensionInfo("jpg") - Expect(info.OutputFormat).To(Equal("jpeg")) - Expect(info.ContentType).To(Equal("image/jpeg")) - }) - - It("should return default info for unknown extension", func() { - info := generator.GetExtensionInfo("webp") - Expect(info.OutputFormat).To(Equal("jpeg")) - Expect(info.ContentType).To(Equal("image/jpeg")) - }) - - It("should return default info for empty extension", func() { - info := generator.GetExtensionInfo("") - Expect(info.OutputFormat).To(Equal("jpeg")) - Expect(info.ContentType).To(Equal("image/jpeg")) - }) -}) diff --git a/services/webdav/pkg/thumbnail/workflow/workflow.go b/services/webdav/pkg/thumbnail/workflow/workflow.go index 252458e3b0..efc396e4ef 100644 --- a/services/webdav/pkg/thumbnail/workflow/workflow.go +++ b/services/webdav/pkg/thumbnail/workflow/workflow.go @@ -10,6 +10,7 @@ import ( "mime" "mime/multipart" "net/http" + "net/textproto" "path/filepath" "strings" @@ -457,14 +458,14 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R // in webdav: real images (incl. gif) are streamed straight through undecoded; // non-image sources are converted to image bytes here so the generator always // receives an image. - imgStream, cleanup, err := w.sourceImage(ctx, ref, auth, mimeType, tr.Filename, logger) + imgStream, srcType, cleanup, err := w.sourceImage(ctx, ref, auth, mimeType, tr.Filename, logger) if err != nil { logger.Error().Err(err).Msg("could not obtain source image for thumbnail") return nil, "", false, err } defer cleanup() - thumbBytes, err := w.postToGenerator(ctx, genURL, imgStream, tr.Filename) + thumbBytes, err := w.postToGenerator(ctx, genURL, imgStream, tr.Filename, srcType) if err != nil { logger.Error().Err(err).Msg("could not generate thumbnail") return nil, "", false, fmt.Errorf("generate: %w", err) @@ -486,40 +487,40 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R // undecoded; the generator handles decoding and multi-frame gifs itself. // - text/plain is rendered to an image here (the only true conversion). // - audio and geogebra sources have their embedded image extracted to bytes. -func (w *ThumbnailWorkflow) sourceImage(ctx context.Context, ref *providerv1beta1.Reference, auth, mimeType, filename string, logger log.Logger) (io.Reader, func(), error) { +func (w *ThumbnailWorkflow) sourceImage(ctx context.Context, ref *providerv1beta1.Reference, auth, mimeType, filename string, logger log.Logger) (io.Reader, string, func(), error) { m, _, _ := mime.ParseMediaType(mimeType) if strings.HasPrefix(m, "image/") { body, err := w.fileDownloader.DownloadStream(ctx, ref, auth) if err != nil { - return nil, nil, fmt.Errorf("download: %w", err) + return nil, "", nil, fmt.Errorf("download: %w", err) } - return body, func() { _ = body.Close() }, nil + return body, m, func() { _ = body.Close() }, nil } body, err := w.fileDownloader.DownloadStream(ctx, ref, auth) if err != nil { - return nil, nil, fmt.Errorf("download: %w", err) + return nil, "", nil, fmt.Errorf("download: %w", err) } defer body.Close() fileBytes, err := io.ReadAll(body) if err != nil { - return nil, nil, fmt.Errorf("read source: %w", err) + return nil, "", nil, fmt.Errorf("read source: %w", err) } ppOpts := map[string]any{"fontFileMap": w.fontMapFile} img, err := preprocessor.ForType(mimeType, ppOpts).Convert(bytes.NewReader(fileBytes)) if img == nil || err != nil { logger.Debug().Err(err).Msg("could not convert file to image") - return nil, nil, fmt.Errorf("%w: could not get image", ErrNotFound) + return nil, "", nil, fmt.Errorf("%w: could not get image", ErrNotFound) } - data, _, err := encodeForUpload(img, mimeType) + data, ext, err := encodeForUpload(img, mimeType) if err != nil { - return nil, nil, fmt.Errorf("encode converted image: %w", err) + return nil, "", nil, fmt.Errorf("encode converted image: %w", err) } - return bytes.NewReader(data), func() {}, nil + return bytes.NewReader(data), generator.ContentType(ext), func() {}, nil } // matchOperation resolves the generator resize/crop operation for a request from @@ -583,20 +584,29 @@ func (w *ThumbnailWorkflow) matchOperation(tr *requests.ThumbnailRequest, mimeTy return operation, noUpscale, aIgnored } +// quoteEscaper matches the escaping mime/multipart applies to part filenames. +var quoteEscaper = strings.NewReplacer("\\", "\\\\", `"`, "\\\"") + // postToGenerator streams the source image to the generator as a multipart // POST. The body is produced by an io.Pipe so the (potentially large) image is // never fully buffered in webdav: the writer goroutine copies the reader into // the form part on demand. No Content-Length is set, so the request uses chunked // transfer-encoding; both imagor and the thumbnailer parse the multipart body // without needing it (imagor enforces its size limit after reading the part). -func (w *ThumbnailWorkflow) postToGenerator(ctx context.Context, url string, img io.Reader, filename string) ([]byte, error) { +func (w *ThumbnailWorkflow) postToGenerator(ctx context.Context, url string, img io.Reader, filename, contentType string) ([]byte, error) { pr, pw := io.Pipe() writer := multipart.NewWriter(pw) errCh := make(chan error, 1) go func() { defer pw.Close() - part, err := writer.CreateFormFile("image", filename) + // Not CreateFormFile: it types the part application/octet-stream, which + // imagor's upload loader rejects (UPLOAD_LOADER_ACCEPT defaults to image/*). + h := make(textproto.MIMEHeader) + h.Set("Content-Disposition", + `form-data; name="image"; filename="`+quoteEscaper.Replace(filename)+`"`) + h.Set("Content-Type", contentType) + part, err := writer.CreatePart(h) if err != nil { errCh <- fmt.Errorf("create form file: %w", err) return @@ -923,7 +933,9 @@ func ResolvePublicLinkAuth(ctx context.Context, r *http.Request, publicLinkToken switch rsp.GetStatus().GetCode() { case rpcv1beta1.Code_CODE_OK: return rsp.GetToken(), nil - case rpcv1beta1.Code_CODE_PERMISSION_DENIED: + case rpcv1beta1.Code_CODE_PERMISSION_DENIED, rpcv1beta1.Code_CODE_UNAUTHENTICATED: + // reva reports a wrong or missing link password as InvalidCredentials, + // which the gateway passes through as UNAUTHENTICATED. return "", fmt.Errorf("%w: %s", ErrPublicLinkPasswordRequired, rsp.GetStatus().GetMessage()) case rpcv1beta1.Code_CODE_FAILED_PRECONDITION: return "", fmt.Errorf("%w: %s", ErrPublicLinkExpired, rsp.GetStatus().GetMessage()) diff --git a/services/webdav/pkg/thumbnail/workflow/workflow_test.go b/services/webdav/pkg/thumbnail/workflow/workflow_test.go index 9ca18ad184..34eeeb1c37 100644 --- a/services/webdav/pkg/thumbnail/workflow/workflow_test.go +++ b/services/webdav/pkg/thumbnail/workflow/workflow_test.go @@ -1586,3 +1586,26 @@ var _ = Describe("ThumbnailWorkflow", func() { }) }) +var _ = Describe("postToGenerator", func() { + It("types the uploaded part so imagor's upload loader accepts it", func() { + var partType, partDisposition string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mr, err := r.MultipartReader() + Expect(err).ToNot(HaveOccurred()) + part, err := mr.NextPart() + Expect(err).ToNot(HaveOccurred()) + partType = part.Header.Get("Content-Type") + partDisposition = part.Header.Get("Content-Disposition") + _, _ = io.Copy(io.Discard, part) + _, _ = w.Write([]byte("thumb")) + })) + defer srv.Close() + + wf := &ThumbnailWorkflow{httpClient: srv.Client()} + out, err := wf.postToGenerator(context.Background(), srv.URL, strings.NewReader("payload"), `we"ird.png`, "image/png") + Expect(err).ToNot(HaveOccurred()) + Expect(string(out)).To(Equal("thumb")) + Expect(partType).To(Equal("image/png"), "application/octet-stream is rejected by imagor") + Expect(partDisposition).To(Equal(`form-data; name="image"; filename="we\"ird.png"`)) + }) +})