From ea10e20bd07afd65cd10bfe8f42cefcf93e59d0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Fri, 4 Sep 2026 09:26:43 +0200 Subject: [PATCH] address review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/thumbnails/README.md | 7 +- services/thumbnails/pkg/config/config.go | 5 + .../thumbnails/pkg/service/http/v0/push.go | 126 +++++-- .../pkg/service/http/v0/push_imaging.go | 42 ++- .../pkg/service/http/v0/push_test.go | 106 ++++++ .../pkg/service/http/v0/push_vips.go | 55 +-- .../thumbnails/pkg/service/http/v0/service.go | 2 + services/webdav/pkg/config/config.go | 9 +- .../pkg/config/defaults/defaultconfig.go | 3 +- services/webdav/pkg/dav/requests/thumbnail.go | 43 ++- services/webdav/pkg/generator/extensions.go | 20 +- .../webdav/pkg/generator/generator_test.go | 19 +- services/webdav/pkg/generator/url.go | 19 +- .../webdav/pkg/preprocessor/preprocessor.go | 24 +- .../pkg/preprocessor/preprocessor_vips.go | 17 +- services/webdav/pkg/service/v0/service.go | 30 +- .../webdav/pkg/thumbnail/cache/filecache.go | 4 +- .../webdav/pkg/thumbnail/cache/s3cache.go | 9 +- .../pkg/thumbnail/workflow/encode_imaging.go | 2 +- .../pkg/thumbnail/workflow/encode_vips.go | 2 +- .../webdav/pkg/thumbnail/workflow/workflow.go | 318 ++++++++++++------ .../pkg/thumbnail/workflow/workflow_test.go | 114 ++++++- 22 files changed, 722 insertions(+), 254 deletions(-) diff --git a/services/thumbnails/README.md b/services/thumbnails/README.md index 8bfaeafb9a..94f0d765e4 100644 --- a/services/thumbnails/README.md +++ b/services/thumbnails/README.md @@ -2,6 +2,8 @@ The thumbnails service is a stateless image resizer. It exposes an imagor-compatible push endpoint that accepts an original image as a multipart upload and returns the resized thumbnail encoded in the requested format. +> **Warning:** This service performs no authentication or authorization. By default it binds to `127.0.0.1:9186` so only local processes (the webdav service) can reach it. If you change `THUMBNAILS_HTTP_ADDR` to a non-loopback address, put it behind your reverse proxy's auth layer or an internal network boundary — anyone who can reach the endpoint can upload and retrieve arbitrary images through it. + ## Push Endpoint The webdav service (which owns the complete thumbnail workflow) POSTs the source file to this endpoint and receives the processed image back. @@ -12,7 +14,9 @@ The webdav service (which owns the complete thumbnail workflow) POSTs the source | `POST /unsafe/fit-in/{width}x{height}` (optionally `/filters:format({format})`) | Scale to fit within width x height, preserving aspect ratio and never upscaling (letterboxed) | | `POST /unsafe/stretch/{width}x{height}` (optionally `/filters:format({format})`) | Resize to the exact width x height without preserving aspect ratio (distorts) | -The filters segment is captured whole and parsed; only the `format` filter is meaningful to this executor, other filters are ignored. The `format` filter is optional: when absent the input's own format is preserved (detected via the imaging library), matching imagor. Inputs we cannot re-encode (e.g. webp, tiff, bmp) fall back to JPEG, mirroring imagor's default for unsavable sources. +The filters segment is captured whole and parsed; the `format` and `no_upscale` filters are meaningful to this executor, other filters are ignored. The `format` filter is optional: when absent the input's own format is preserved (detected from the image header), matching imagor. Inputs we cannot re-encode (e.g. webp, tiff, bmp) fall back to JPEG, mirroring imagor's default for unsavable sources. + +Like real imagor, the default fill resize **upscales** small sources to fill the box exactly (a 100x100 source requested at `320x320` returns a 320x320 image). Adding the `no_upscale()` filter caps the result at the source size instead (the same request returns 100x100). The `fit-in` route never upscales regardless of filters. The webdav service selects the route based on the requested processor. The full mapping is: @@ -38,6 +42,7 @@ The request body is a `multipart/form-data` upload with a single file field name |---|---| | `THUMBNAILS_HTTP_ADDR` | Bind address of the HTTP service (default `127.0.0.1:9186`) | | `THUMBNAILS_LOG_LEVEL` | Log level (`panic`, `fatal`, `error`, `warn`, `info`, `debug`, `trace`) | +| `THUMBNAILS_MAX_CONCURRENT_REQUESTS` | Maximum number of thumbnail requests decoded and resized in parallel. Default is 0 (unlimited). Requests arriving while the limit is reached get HTTP 429 with a `Retry-After` header | ## Using libvips for Image Processing diff --git a/services/thumbnails/pkg/config/config.go b/services/thumbnails/pkg/config/config.go index 0f8d8d4c87..6d39e03900 100644 --- a/services/thumbnails/pkg/config/config.go +++ b/services/thumbnails/pkg/config/config.go @@ -32,4 +32,9 @@ type Thumbnail struct { MaxInputWidth int `yaml:"max_input_width" env:"THUMBNAILS_MAX_INPUT_WIDTH" desc:"The maximum width of an input image which is being processed." introductionVersion:"1.0.0"` // MaxInputHeight is the maximum height of an input image which is being processed. MaxInputHeight int `yaml:"max_input_height" env:"THUMBNAILS_MAX_INPUT_HEIGHT" desc:"The maximum height of an input image which is being processed." introductionVersion:"1.0.0"` + // MaxConcurrentRequests limits the number of thumbnail requests that are + // decoded and resized in parallel. 0 (the default) means unlimited, matching + // main's THUMBNAILS_MAX_CONCURRENT_REQUESTS. Requests arriving while the limit + // is reached get HTTP 429 Too Many Requests. + MaxConcurrentRequests int `yaml:"max_concurrent_requests" env:"THUMBNAILS_MAX_CONCURRENT_REQUESTS" desc:"Maximum number of concurrent thumbnail generation requests. Default is 0 which is unlimited." introductionVersion:"1.0.0"` } diff --git a/services/thumbnails/pkg/service/http/v0/push.go b/services/thumbnails/pkg/service/http/v0/push.go index f2e6523e0f..9e5c7490f0 100644 --- a/services/thumbnails/pkg/service/http/v0/push.go +++ b/services/thumbnails/pkg/service/http/v0/push.go @@ -11,7 +11,6 @@ import ( "strings" "github.com/go-chi/chi/v5" - "github.com/kovidgoyal/imaging" ) // gifMagic is the leading signature of a GIF file (GIF87a or GIF89a). It guards @@ -40,6 +39,13 @@ var errInvalid = fmt.Errorf("invalid") // imagor. It returns only imagor's defined status codes: 400 for invalid requests or a // file exceeding the max size, 422 for an image exceeding the max resolution. func (s Thumbnails) pushHandler(w http.ResponseWriter, r *http.Request) { + ok, retryAfter := s.limiter.acquire() + if !ok { + writeTooManyRequests(w, retryAfter) + return + } + defer s.limiter.release() + width, err := parseDim(r, "width") if err != nil { writeInvalid(w, "invalid width") @@ -80,11 +86,13 @@ func (s Thumbnails) pushHandler(w http.ResponseWriter, r *http.Request) { return } - ext, hasFormat, err := outputFormatFromFilters(chi.URLParam(r, "filters")) + filtersSegment := chi.URLParam(r, "filters") + ext, hasFormat, err := outputFormatFromFilters(filtersSegment) if err != nil { writeInvalid(w, err.Error()) return } + noUpscale := filterPresent(filtersSegment, "no_upscale") if !hasFormat { // No format filter: preserve the input's own format (imagor default), // detected via imaging. Only runs on this path, so the common webdav @@ -92,25 +100,29 @@ func (s Thumbnails) pushHandler(w http.ResponseWriter, r *http.Request) { ext = inputFormatViaImaging(imgData) } + // imagor ErrMaxResolutionExceeded: the declared input exceeds the configured + // maximum width or height. Checked against the image header BEFORE any + // decoding so a dimension bomb (a tiny file declaring huge dimensions) is + // rejected without allocating the pixel buffer. webdav translates the + // resulting 422 into its legacy 403 response. + if s.maxWidth > 0 || s.maxHeight > 0 { + cfg, _, err := image.DecodeConfig(bytes.NewReader(imgData)) + if err == nil { + if (s.maxWidth > 0 && cfg.Width > s.maxWidth) || (s.maxHeight > 0 && cfg.Height > s.maxHeight) { + writeMaxResolution(w) + return + } + } + } + // The box and operation are chosen by webdav (the sizing brain); this service // is a dumb, imagor-like executor that just fits within the given box. - processed, srcBounds, err := processImage(bytes.NewReader(imgData), width, height, operation) + processed, err := processImage(bytes.NewReader(imgData), width, height, operation, noUpscale) if err != nil { writeInvalid(w, "failed to process image") return } - // imagor ErrMaxResolutionExceeded: the decoded input exceeds the configured - // maximum width/height. Checked against the source (pre-resize) bounds because - // this service owns the image; webdav translates the resulting 422 into its - // legacy 403 response. - if s.maxWidth > 0 || s.maxHeight > 0 { - if (s.maxWidth == 0 || srcBounds.Dx() > s.maxWidth) && (s.maxHeight == 0 || srcBounds.Dy() > s.maxHeight) { - writeMaxResolution(w) - return - } - } - var ( buf bytes.Buffer contentType string @@ -165,6 +177,51 @@ func writeMaxResolution(w http.ResponseWriter) { fmt.Fprintln(w, "maximum resolution exceeded") } +// concurrencyLimiter bounds the number of requests processed in parallel. It is +// nil when no limit is configured (THUMBNAILS_MAX_CONCURRENT_REQUESTS = 0). +type concurrencyLimiter struct { + tokens chan struct{} +} + +func newConcurrencyLimiter(limit int) *concurrencyLimiter { + if limit <= 0 { + return nil + } + tokens := make(chan struct{}, limit) + for i := 0; i < limit; i++ { + tokens <- struct{}{} + } + return &concurrencyLimiter{tokens: tokens} +} + +// acquire tries to take a slot without blocking. It reports whether the request +// may proceed and, when not, how many seconds to wait before retrying. +func (l *concurrencyLimiter) acquire() (bool, int) { + if l == nil { + return true, 0 + } + select { + case <-l.tokens: + return true, 0 + default: + return false, 1 + } +} + +func (l *concurrencyLimiter) release() { + if l == nil { + return + } + l.tokens <- struct{}{} +} + +// writeTooManyRequests responds with 429 and a short Retry-After header. +func writeTooManyRequests(w http.ResponseWriter, retryAfter int) { + w.Header().Set("Retry-After", strconv.Itoa(retryAfter)) + w.WriteHeader(http.StatusTooManyRequests) + fmt.Fprintln(w, "too many concurrent requests") +} + // mapFormatToExt maps imagor format names to the internal extension used by encoders. func mapFormatToExt(format string) string { switch strings.ToLower(format) { @@ -198,20 +255,22 @@ func outputFormatFromFilters(segment string) (ext string, hasFormat bool, err er return "", false, nil } -// inputFormatViaImaging detects the uploaded image's format with imaging and maps it -// to our output extension. jpeg/png/gif are kept; anything else (webp, tiff, bmp, or -// an undecodable body) falls back to jpeg, mirroring imagor's unsavable-source default. +// inputFormatViaImaging detects the uploaded image's format with the stdlib +// decoder registry (image.DecodeConfig reads only the header) and maps it to our +// output extension. jpeg/png/gif are kept; anything else (webp, tiff, bmp, or an +// undecodable body) falls back to jpeg, mirroring imagor's unsavable-source +// default. func inputFormatViaImaging(imgData []byte) string { - img, _, err := imaging.DecodeAll(bytes.NewReader(imgData)) - if err != nil || img == nil || img.Metadata == nil { + _, format, err := image.DecodeConfig(bytes.NewReader(imgData)) + if err != nil { return "jpg" } - switch img.Metadata.Format { - case imaging.PNG: + switch format { + case "png": return "png" - case imaging.GIF: + case "gif": return "gif" - default: // JPEG, WEBP, TIFF, BMP, UNKNOWN, ... + default: // jpeg, webp, tiff, bmp, ... return "jpg" } } @@ -226,6 +285,17 @@ func splitFilter(token string) (name, args string, ok bool) { return token[:i], token[i+1 : len(token)-1], true } +// filterPresent reports whether a named filter occurs in an imagor filters +// segment (e.g. "no_upscale():format(jpeg)"). +func filterPresent(segment, name string) bool { + for _, token := range strings.Split(segment, ":") { + if n, _, ok := splitFilter(token); ok && n == name { + return true + } + } + return false +} + // isGifReader reports whether the reader holds GIF data, based on the GIF magic // bytes. It peeks at the first 4 bytes and rewinds the stream so it can be read // again. This guard is required because gif.DecodeAll panics (nil dereference) @@ -252,11 +322,11 @@ func isGifReader(r io.Reader) bool { // Backend-specific function set in init() by push_imaging.go or push_vips.go. // For gif input it returns a *gif.GIF with every frame resized (preserving // animation); for other inputs it returns a single image.Image. The operation -// selects the resize/crop mode (fill, fit-in, stretch). The -// second return value is the decoded source's pixel bounds, reported before any -// resize so the max-resolution limit can be enforced against the input rather -// than the output. -var processImage func(io.Reader, int, int, string) (any, image.Rectangle, error) +// selects the resize/crop mode (fill, fit-in, stretch). noUpscale mirrors +// imagor's no_upscale() filter: when set, the default fill resize must not +// enlarge the source (fit-in already never upscales; stretch is exact and +// ignores it). +var processImage func(io.Reader, int, int, string, bool) (any, error) // encodeJPEG and encodePNG write the processed image to w in the requested // format. Set by init() in push_imaging.go / push_vips.go. The vips backend diff --git a/services/thumbnails/pkg/service/http/v0/push_imaging.go b/services/thumbnails/pkg/service/http/v0/push_imaging.go index 69c90be59e..083739c0c5 100644 --- a/services/thumbnails/pkg/service/http/v0/push_imaging.go +++ b/services/thumbnails/pkg/service/http/v0/push_imaging.go @@ -22,34 +22,39 @@ func init() { } // processImageImaging resizes the input using the imaging backend. The operation -// selects the resize/crop mode: fill (center-crop to the box), fit-in (fit within -// the box without cropping, never upscaling), or stretch (resize to the exact box). -func processImageImaging(r io.Reader, width, height int, operation string) (any, image.Rectangle, error) { +// selects the resize/crop mode: fill (center-crop to the box, upscaling by default +// like real imagor's default resize), fit-in (fit within the box without cropping, +// never upscaling), or stretch (resize to the exact box). noUpscale caps the +// default fill at the source size, mirroring imagor's no_upscale() filter. +func processImageImaging(r io.Reader, width, height int, operation string, noUpscale bool) (any, error) { if isGifReader(r) { g, err := gif.DecodeAll(r) if err == nil && len(g.Image) > 0 { - srcBounds := g.Image[0].Bounds() - return resizeGIF(g, width, height, operation), srcBounds, nil + return resizeGIF(g, width, height, operation, noUpscale), nil } } img, err := imaging.Decode(r, imaging.AutoOrientation(true)) if err != nil { - return nil, image.Rectangle{}, err + return nil, err } srcBounds := img.Bounds() switch operation { case OpStretch: - return imaging.Resize(img, width, height, imaging.Lanczos), srcBounds, nil + return imaging.Resize(img, width, height, imaging.Lanczos), nil case OpFitIn: if srcBounds.Dx() > width || srcBounds.Dy() > height { - return imaging.Fit(img, width, height, imaging.Lanczos), srcBounds, nil + return imaging.Fit(img, width, height, imaging.Lanczos), nil } - return img, srcBounds, nil + return img, nil default: // OpFill - return imaging.Thumbnail(img, width, height, imaging.Lanczos), srcBounds, nil + if noUpscale && srcBounds.Dx() <= width && srcBounds.Dy() <= height { + // imagor no_upscale(): never enlarge the source. + return img, nil + } + return imaging.Thumbnail(img, width, height, imaging.Lanczos), nil } } @@ -57,8 +62,9 @@ func processImageImaging(r io.Reader, width, height int, operation string) (any, // animation. It composites each frame onto a running canvas honoring the gif // disposal method, resizes with the requested processor, and re-pallettes the // result using Floyd-Steinberg dithering. Code adapted from -// https://github.com/willnorris/gifresize. -func resizeGIF(m *gif.GIF, width, height int, operation string) *gif.GIF { +// https://github.com/willnorris/gifresize. noUpscale caps the default fill at +// the source size, mirroring imagor's no_upscale() filter. +func resizeGIF(m *gif.GIF, width, height int, operation string, noUpscale bool) *gif.GIF { srcX, srcY := m.Config.Width, m.Config.Height b := image.Rect(0, 0, srcX, srcY) tmp := image.NewRGBA(b) @@ -79,7 +85,11 @@ func resizeGIF(m *gif.GIF, width, height int, operation string) *gif.GIF { processed = tmp } default: // OpFill - processed = imaging.Fill(tmp, width, height, imaging.Center, imaging.Lanczos) + if noUpscale && srcX <= width && srcY <= height { + processed = tmp + } else { + processed = imaging.Fill(tmp, width, height, imaging.Center, imaging.Lanczos) + } } m.Image[i] = imageToPaletted(processed, frame.Palette) @@ -92,8 +102,10 @@ func resizeGIF(m *gif.GIF, width, height int, operation string) *gif.GIF { } } - m.Config.Width = width - m.Config.Height = height + if !noUpscale || srcX > width || srcY > height { + m.Config.Width = width + m.Config.Height = height + } return m } diff --git a/services/thumbnails/pkg/service/http/v0/push_test.go b/services/thumbnails/pkg/service/http/v0/push_test.go index 4b80aa9d84..65c234c0d6 100644 --- a/services/thumbnails/pkg/service/http/v0/push_test.go +++ b/services/thumbnails/pkg/service/http/v0/push_test.go @@ -338,6 +338,35 @@ func TestPushEndpoint_Default_Upscales(t *testing.T) { } } +// TestPushEndpoint_Default_NoUpscaleFilter pins imagor's no_upscale() filter: +// with it, the default resize must not enlarge a small source. A 50x50 source +// requested at 200x200 comes back as 50x50 instead of 200x200. +func TestPushEndpoint_Default_NoUpscaleFilter(t *testing.T) { + mux := newTestMux() + + imgBytes := createTestImage(50, 50) + body, contentType := createMultipartBody(imgBytes) + + req := httptest.NewRequest(http.MethodPost, "/unsafe/200x200/filters:no_upscale():format(png)/", body) + req.Header.Set("Content-Type", contentType) + rec := httptest.NewRecorder() + + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected status 200, got %d: %s", rec.Code, rec.Body.String()) + } + + img, err := png.Decode(rec.Body) + if err != nil { + t.Fatalf("failed to decode response image: %v", err) + } + bounds := img.Bounds() + if bounds.Dx() != 50 || bounds.Dy() != 50 { + t.Errorf("no_upscale should keep the source at 50x50, got %dx%d", bounds.Dx(), bounds.Dy()) + } +} + // TestPushEndpoint_FitIn_LetterboxOutcome pins the opt-in "fit" processor's // outcome: aspect-preserving fit with no upscaling. A 200x100 source requested // at 100x100 must come back as exactly 100x50 — centered/letterboxed in the box, @@ -586,6 +615,26 @@ func TestPushEndpoint_MaxResolutionExceeded(t *testing.T) { } } +// TestPushEndpoint_MaxResolutionExceeded_OneAxis pins the per-axis check: a source +// that exceeds only the max width (but is well under the max height) must still be +// rejected with 422. This catches an inverted "both axes" condition. +func TestPushEndpoint_MaxResolutionExceeded_OneAxis(t *testing.T) { + mux := newTestMuxWithLimits(100, 100) + + imgBytes := createTestImage(500, 10) + body, contentType := createMultipartBody(imgBytes) + + req := httptest.NewRequest(http.MethodPost, "/unsafe/64x64/filters:format(png)/", body) + req.Header.Set("Content-Type", contentType) + rec := httptest.NewRecorder() + + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusUnprocessableEntity { + t.Errorf("expected status 422 for width-oversized input, got %d: %s", rec.Code, rec.Body.String()) + } +} + // TestPushEndpoint_WithinMaxResolution confirms an input within the configured // limits still succeeds (the 422 check must not over-trigger). func TestPushEndpoint_WithinMaxResolution(t *testing.T) { @@ -605,6 +654,63 @@ func TestPushEndpoint_WithinMaxResolution(t *testing.T) { } } +// TestConcurrencyLimiter pins the THUMBNAILS_MAX_CONCURRENT_REQUESTS behavior: +// up to N requests may proceed in parallel, the (N+1)th is rejected non-blocking +// with a 429. A limit of 0 disables the limiter entirely. +func TestConcurrencyLimiter(t *testing.T) { + t.Run("limit 2 rejects the third concurrent request", func(t *testing.T) { + l := newConcurrencyLimiter(2) + + if ok, _ := l.acquire(); !ok { + t.Fatal("first acquire should succeed") + } + if ok, _ := l.acquire(); !ok { + t.Fatal("second acquire should succeed") + } + ok, retryAfter := l.acquire() + if ok { + t.Fatal("third acquire should fail while the limit is reached") + } + if retryAfter <= 0 { + t.Errorf("expected a positive Retry-After hint, got %d", retryAfter) + } + + l.release() + if ok, _ := l.acquire(); !ok { + t.Fatal("acquire after release should succeed") + } + }) + + t.Run("zero limit is unlimited", func(t *testing.T) { + var l *concurrencyLimiter + for i := 0; i < 100; i++ { + if ok, _ := l.acquire(); !ok { + t.Fatalf("acquire %d should succeed without a limiter", i) + } + } + l.release() + }) + + t.Run("429 is returned when the handler is saturated", func(t *testing.T) { + s := Thumbnails{limiter: newConcurrencyLimiter(1)} + if ok, _ := s.limiter.acquire(); !ok { + t.Fatal("could not saturate the limiter") + } + defer s.limiter.release() + + req := httptest.NewRequest(http.MethodPost, "/unsafe/64x64/filters:format(png)/", nil) + rec := httptest.NewRecorder() + s.pushHandler(rec, req) + + if rec.Code != http.StatusTooManyRequests { + t.Errorf("expected 429 from a saturated limiter, got %d", rec.Code) + } + if rec.Header().Get("Retry-After") == "" { + t.Error("expected a Retry-After header on 429") + } + }) +} + func createTestJPEG(width, height int) []byte { img := image.NewRGBA(image.Rect(0, 0, width, height)) for y := 0; y < height; y++ { diff --git a/services/thumbnails/pkg/service/http/v0/push_vips.go b/services/thumbnails/pkg/service/http/v0/push_vips.go index dbf7431017..821ee2905f 100644 --- a/services/thumbnails/pkg/service/http/v0/push_vips.go +++ b/services/thumbnails/pkg/service/http/v0/push_vips.go @@ -21,25 +21,26 @@ func init() { } // processImageVips resizes the input using libvips. The operation selects the -// resize/crop mode: fill (center-crop to the box), fit-in (fit within the box -// without cropping, never upscaling), or stretch (resize to the exact box). -func processImageVips(r io.Reader, width, height int, operation string) (any, image.Rectangle, error) { +// resize/crop mode: fill (center-crop to the box, upscaling by default like real +// imagor's default resize), fit-in (fit within the box without cropping, never +// upscaling), or stretch (resize to the exact box). noUpscale caps the default +// fill at the source size, mirroring imagor's no_upscale() filter. +func processImageVips(r io.Reader, width, height int, operation string, noUpscale bool) (any, error) { if isGifReader(r) { g, err := gif.DecodeAll(r) if err == nil && len(g.Image) > 0 { - srcBounds := g.Image[0].Bounds() - return resizeGIFVips(g, width, height, operation), srcBounds, nil + return resizeGIFVips(g, width, height, operation, noUpscale), nil } } imgData, err := io.ReadAll(r) if err != nil { - return nil, image.Rectangle{}, err + return nil, err } m, err := vips.NewImageFromBuffer(imgData) if err != nil { - return nil, image.Rectangle{}, err + return nil, err } // Note: no defer m.Close() here. The ImageRef is returned to the caller and @@ -47,42 +48,48 @@ func processImageVips(r io.Reader, width, height int, operation string) (any, im // after exporting. Closing it here would free the underlying C image before the // export runs (use-after-free). - srcBounds := image.Rect(0, 0, m.Width(), m.Height()) - switch operation { case OpStretch: // Resize to the exact box without preserving aspect ratio. hScale := float64(width) / float64(m.Width()) vScale := float64(height) / float64(m.Height()) if err := m.ResizeWithVScale(hScale, vScale, vips.KernelLanczos3); err != nil { - return nil, image.Rectangle{}, err + return nil, err } case OpFitIn: // Fit within the box without cropping and never upscale (SizeDown). if err := m.ThumbnailWithSize(width, height, vips.InterestingNone, vips.SizeDown); err != nil { - return nil, image.Rectangle{}, err + return nil, err } default: // OpFill - // Center-crop to fill the box exactly. - if err := m.ThumbnailWithSize(width, height, vips.InterestingAttention, vips.SizeBoth); err != nil { - return nil, image.Rectangle{}, err + // Center-crop to fill the box exactly. SizeBoth matches real imagor's + // default resize (which upscales small sources); no_upscale() caps it + // at the source size via SizeDown. + size := vips.SizeBoth + if noUpscale { + size = vips.SizeDown + } + if err := m.ThumbnailWithSize(width, height, vips.InterestingAttention, size); err != nil { + return nil, err } } if err := m.RemoveMetadata(); err != nil { - return nil, image.Rectangle{}, err + return nil, err } // Keep the image in libvips form so the encode hooks can export it via // ExportJpeg/ExportPng (matching the legacy pipeline's progressive JPEG and // quality-80 output) instead of a lossy PNG round-trip + stdlib re-encode. - return m, srcBounds, nil + return m, nil } // resizeGIFVips resizes every frame of an animated gif while preserving the // animation, compositing each frame onto a running canvas honoring the gif -// disposal method and re-palletting with Floyd-Steinberg dithering. -func resizeGIFVips(m *gif.GIF, width, height int, operation string) *gif.GIF { +// disposal method and re-palletting with Floyd-Steinberg dithering. noUpscale +// caps the default fill at the source size, mirroring imagor's no_upscale() +// filter. +func resizeGIFVips(m *gif.GIF, width, height int, operation string, noUpscale bool) *gif.GIF { srcX, srcY := m.Config.Width, m.Config.Height b := image.Rect(0, 0, srcX, srcY) tmp := image.NewRGBA(b) @@ -103,7 +110,11 @@ func resizeGIFVips(m *gif.GIF, width, height int, operation string) *gif.GIF { processed = tmp } default: // OpFill - processed = imaging.Fill(tmp, width, height, imaging.Center, imaging.Lanczos) + if noUpscale && srcX <= width && srcY <= height { + processed = tmp + } else { + processed = imaging.Fill(tmp, width, height, imaging.Center, imaging.Lanczos) + } } m.Image[i] = imageToPalettedVips(processed, frame.Palette) @@ -116,8 +127,10 @@ func resizeGIFVips(m *gif.GIF, width, height int, operation string) *gif.GIF { } } - m.Config.Width = width - m.Config.Height = height + if !noUpscale || srcX > width || srcY > height { + m.Config.Width = width + m.Config.Height = height + } return m } diff --git a/services/thumbnails/pkg/service/http/v0/service.go b/services/thumbnails/pkg/service/http/v0/service.go index 99ce58379a..066c76288f 100644 --- a/services/thumbnails/pkg/service/http/v0/service.go +++ b/services/thumbnails/pkg/service/http/v0/service.go @@ -38,6 +38,7 @@ func NewService(opts ...Option) Service { mux: m, maxWidth: limits.MaxInputWidth, maxHeight: limits.MaxInputHeight, + limiter: newConcurrencyLimiter(limits.MaxConcurrentRequests), } // Push-based thumbnail generation endpoint (imagor-compatible). The optional @@ -64,6 +65,7 @@ type Thumbnails struct { mux *chi.Mux maxWidth int maxHeight int + limiter *concurrencyLimiter } // ServeHTTP implements the Service interface. diff --git a/services/webdav/pkg/config/config.go b/services/webdav/pkg/config/config.go index d7be0a48d5..f071bcd187 100644 --- a/services/webdav/pkg/config/config.go +++ b/services/webdav/pkg/config/config.go @@ -25,12 +25,11 @@ type Config struct { WebdavNamespace string `yaml:"webdav_namespace" env:"WEBDAV_WEBDAV_NAMESPACE" desc:"CS3 path layout to use when forwarding /webdav requests" introductionVersion:"1.0.0"` RevaGateway string `yaml:"reva_gateway" env:"OC_REVA_GATEWAY" desc:"CS3 gateway used to look up user metadata" introductionVersion:"1.0.0"` - ThumbnailGeneratorURL string `yaml:"thumbnail_generator_url" env:"WEBDAV_THUMBNAIL_GENERATOR_URL" desc:"Base URL of the thumbnail generator service, e.g. http://thumbnails:9130" introductionVersion:"1.0.0"` - ThumbnailGeneratorTimeout string `yaml:"thumbnail_generator_timeout" env:"WEBDAV_THUMBNAIL_GENERATOR_TIMEOUT" desc:"HTTP timeout for requests to the thumbnail generator, e.g. 30s" introductionVersion:"1.0.0"` - ThumbnailGeneratorAuthHeader string `yaml:"thumbnail_generator_auth_header" env:"WEBDAV_THUMBNAIL_GENERATOR_AUTH_HEADER" desc:"Optional auth header value sent to the generator (for external generators behind a reverse proxy)" introductionVersion:"1.0.0"` - MaxInputFileSize string `yaml:"max_input_file_size" env:"WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE;THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE" desc:"Maximum file size of an input image for thumbnail generation. Usable common abbreviations: [KB, KiB, MB, MiB, GB, GiB], example: 50MB" introductionVersion:"1.0.0"` + ThumbnailGeneratorURL string `yaml:"thumbnail_generator_url" env:"WEBDAV_THUMBNAIL_GENERATOR_URL" desc:"Base URL of the thumbnail generator service, e.g. http://thumbnails:9130" introductionVersion:"1.0.0"` + ThumbnailGeneratorTimeout string `yaml:"thumbnail_generator_timeout" env:"WEBDAV_THUMBNAIL_GENERATOR_TIMEOUT" desc:"HTTP timeout for requests to the thumbnail generator, e.g. 30s" introductionVersion:"1.0.0"` + MaxInputFileSize string `yaml:"max_input_file_size" env:"WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE;THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE" desc:"Maximum file size of an input image for thumbnail generation. Usable common abbreviations: [KB, KiB, MB, MiB, GB, GiB], example: 50MB" introductionVersion:"1.0.0"` - ThumbnailCacheBackend string `yaml:"thumbnail_cache_backend" env:"WEBDAV_THUMBNAIL_CACHE_BACKEND" desc:"Cache backend for imagor thumbnails: 'none', 'memory', 'file', or 's3'. Default: none." introductionVersion:"1.0.0"` + ThumbnailCacheBackend string `yaml:"thumbnail_cache_backend" env:"WEBDAV_THUMBNAIL_CACHE_BACKEND" desc:"Cache backend for imagor thumbnails: 'none', 'memory', 'file', or 's3'. Default: file." introductionVersion:"1.0.0"` ThumbnailCacheDir string `yaml:"thumbnail_cache_dir" env:"WEBDAV_THUMBNAIL_CACHE_DIR" desc:"Directory for file-based thumbnail cache (Default: $OC_BASE_DATA_PATH/thumbnails/files). Only used when ThumbnailCacheBackend is 'file'." introductionVersion:"1.0.0"` ThumbnailResolutions []string `yaml:"thumbnail_resolutions" env:"THUMBNAILS_RESOLUTIONS;WEBDAV_THUMBNAIL_RESOLUTIONS" desc:"Supported target resolutions in the format WidthxHeight like 32x32. The requested size is snapped onto one of these (orientation-aware) before being sent to the generator." introductionVersion:"1.0.0"` ThumbnailCacheS3Bucket string `yaml:"thumbnail_cache_s3_bucket" env:"WEBDAV_THUMBNAIL_CACHE_S3_BUCKET" desc:"S3 bucket name for thumbnail cache when ThumbnailCacheBackend is 's3'." introductionVersion:"1.0.0"` diff --git a/services/webdav/pkg/config/defaults/defaultconfig.go b/services/webdav/pkg/config/defaults/defaultconfig.go index a0fc57cfdc..fa852eddde 100644 --- a/services/webdav/pkg/config/defaults/defaultconfig.go +++ b/services/webdav/pkg/config/defaults/defaultconfig.go @@ -47,7 +47,8 @@ func DefaultConfig() *config.Config { ThumbnailGeneratorURL: "http://127.0.0.1:9186", ThumbnailGeneratorTimeout: "30s", - ThumbnailCacheBackend: "none", + MaxInputFileSize: "50MB", + ThumbnailCacheBackend: "file", ThumbnailCacheDir: path.Join(cored.BaseDataPath(), "thumbnails", "files"), ThumbnailResolutions: []string{ "16x16", "32x32", "64x64", "128x128", "320x320", "1024x1024", // square diff --git a/services/webdav/pkg/dav/requests/thumbnail.go b/services/webdav/pkg/dav/requests/thumbnail.go index f1a4574767..af926b9d0f 100644 --- a/services/webdav/pkg/dav/requests/thumbnail.go +++ b/services/webdav/pkg/dav/requests/thumbnail.go @@ -48,6 +48,10 @@ type ThumbnailRequest struct { // Aspect reports whether the client wants the aspect ratio preserved (the // legacy ownCloud "a" flag: a=1/absent -> preserve, a=0 -> fill the box). Aspect bool + // Identifier is the username from /dav/files/{user}/... when present. The + // workflow resolves it via GetUserByClaim so the file is looked up in that + // user's home; empty for space-ID, /webdav/ and public-link requests. + Identifier string } // ParseThumbnailRequest extracts all required parameters from a http request. @@ -56,7 +60,10 @@ func ParseThumbnailRequest(r *http.Request) (*ThumbnailRequest, error) { fp := ctx.Value(constants.ContextKeyPath).(string) - var ref *providerv1beta1.Reference + var ( + ref *providerv1beta1.Reference + identifier string + ) if v := ctx.Value(constants.ContextKeyID); v != nil { id := v.(string) if strings.Contains(id, "$") { @@ -72,19 +79,20 @@ func ParseThumbnailRequest(r *http.Request) (*ThumbnailRequest, error) { } } else { // The identifier is a username (dav/files/{username}/...); the workflow - // resolves it to the user's home path. - ref = &providerv1beta1.Reference{Path: fp} - } - } else { - if token := chi.URLParam(r, "token"); token != "" { - ref = &providerv1beta1.Reference{Path: path.Join("/public", token, strings.TrimLeft(fp, "/"))} - } else { - // Path-only request (/webdav/...): the absolute CS3 path is carried in - // ContextKeyPath; the workflow resolves the space root. + // resolves it to that user's home path via GetUserByClaim. ref = &providerv1beta1.Reference{Path: fp} + identifier = id } } + if token := chi.URLParam(r, "token"); token != "" { + ref = &providerv1beta1.Reference{Path: path.Join("/public", token, strings.TrimLeft(fp, "/"))} + } else if ref == nil { + // Path-only request (/webdav/...): the absolute CS3 path is carried in + // ContextKeyPath; the workflow resolves the space root. + ref = &providerv1beta1.Reference{Path: fp} + } + q := r.URL.Query() width, height, err := parseDimensions(q) if err != nil { @@ -92,13 +100,14 @@ func ParseThumbnailRequest(r *http.Request) (*ThumbnailRequest, error) { } return &ThumbnailRequest{ - Ref: ref, - Filename: filepath.Base(fp), - Extension: filepath.Ext(fp), - Width: int32(width), - Height: int32(height), - Processor: q.Get("processor"), - Aspect: q.Get("a") != "0", + Ref: ref, + Filename: filepath.Base(fp), + Extension: filepath.Ext(fp), + Width: int32(width), + Height: int32(height), + Processor: q.Get("processor"), + Aspect: q.Get("a") != "0", + Identifier: identifier, }, nil } diff --git a/services/webdav/pkg/generator/extensions.go b/services/webdav/pkg/generator/extensions.go index 812302e64d..3cfbf16db2 100644 --- a/services/webdav/pkg/generator/extensions.go +++ b/services/webdav/pkg/generator/extensions.go @@ -38,20 +38,14 @@ func ContentType(ext string) string { return GetExtensionInfo(ext).ContentType } -// GuessExtension guesses a file extension from a MIME type. -func GuessExtension(mimeType string) string { - exts := []string{".jpg", ".png", ".gif"} - for _, ext := range exts { - if strings.HasSuffix(mimeType, "/"+strings.TrimLeft(ext, ".")) || - strings.Contains(mimeType, "jpeg") && ext == ".jpg" { - return ext[1:] - } - } - - switch { - case strings.Contains(mimeType, "png"): +// MimeToExt maps a resource mime type to the file extension used for the on-disk +// thumbnail cache key. Unknown image types fall back to jpg, matching main's +// behavior (only png/gif are treated specially). +func MimeToExt(mimeType string) string { + switch mimeType { + case "image/png": return "png" - case strings.Contains(mimeType, "gif"): + case "image/gif": return "gif" default: return "jpg" diff --git a/services/webdav/pkg/generator/generator_test.go b/services/webdav/pkg/generator/generator_test.go index 848ad9665c..8981317ddf 100644 --- a/services/webdav/pkg/generator/generator_test.go +++ b/services/webdav/pkg/generator/generator_test.go @@ -86,7 +86,7 @@ func TestContentType(t *testing.T) { } } -func TestGuessExtension(t *testing.T) { +func TestMimeToExt(t *testing.T) { tests := []struct { mime string want string @@ -99,8 +99,8 @@ func TestGuessExtension(t *testing.T) { for _, tt := range tests { t.Run(tt.mime, func(t *testing.T) { - if got := GuessExtension(tt.mime); got != tt.want { - t.Errorf("GuessExtension(%q) = %q, want %q", tt.mime, got, tt.want) + if got := MimeToExt(tt.mime); got != tt.want { + t.Errorf("MimeToExt(%q) = %q, want %q", tt.mime, got, tt.want) } }) } @@ -114,6 +114,7 @@ func TestBuildURL(t *testing.T) { height int32 operation string ext string + noUpscale bool want string }{ { @@ -125,6 +126,16 @@ func TestBuildURL(t *testing.T) { ext: "jpeg", want: "http://generator/unsafe/128x128/filters:format(jpeg)/", }, + { + name: "fill with no_upscale filter caps the resize at source size", + base: "http://generator", + width: 128, + height: 128, + operation: OpFill, + ext: "jpeg", + noUpscale: true, + want: "http://generator/unsafe/128x128/filters:no_upscale():format(jpeg)/", + }, { name: "fit-in preserves aspect ratio within the box without upscaling", base: "http://generator", @@ -147,7 +158,7 @@ func TestBuildURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := BuildURL(tt.base, tt.width, tt.height, tt.operation, tt.ext) + got := BuildURL(tt.base, tt.width, tt.height, tt.operation, tt.ext, tt.noUpscale) if got != tt.want { t.Errorf("BuildURL() = %q, want %q", got, tt.want) } diff --git a/services/webdav/pkg/generator/url.go b/services/webdav/pkg/generator/url.go index ae745dd0db..810a7abe9a 100644 --- a/services/webdav/pkg/generator/url.go +++ b/services/webdav/pkg/generator/url.go @@ -18,7 +18,9 @@ const ( // operationSegment maps an operation to its imagor URL path segment. The fill // operation is the bare WxH resize (imagor's default), so it contributes no -// leading segment. +// leading segment; stretch and fit-in use their imagor names. Note that real +// imagor has no /fill/ route — "fill" is only our internal name for the default +// center-crop resize. func operationSegment(op string) string { switch op { case OpFitIn: @@ -32,13 +34,20 @@ func operationSegment(op string) string { // BuildURL constructs the thumbnail generator processing URL for a matched box // and operation. The operation segment selects the resize/crop mode (which also -// governs upscaling); the image is re-encoded to the requested output format via a -// single imagor filters segment. -func BuildURL(baseURL string, width, height int32, operation, outputExt string) string { +// governs upscaling); the image is re-encoded to the requested output format via +// an imagor filters segment. Like real imagor, the default (fill) resize upscales +// small sources to fill the box exactly; pass noUpscale=true to emit the +// no_upscale() filter so the generator caps the result at the source size. +func BuildURL(baseURL string, width, height int32, operation, outputExt string, noUpscale bool) string { base := strings.TrimRight(baseURL, "/") box := fmt.Sprintf("%dx%d", width, height) - filterSegment := fmt.Sprintf("filters:format(%s)", outputExt) + + filters := "format(" + outputExt + ")" + if noUpscale { + filters = "no_upscale():format(" + outputExt + ")" + } + filterSegment := "filters:" + filters segment := operationSegment(operation) if segment == "" { diff --git a/services/webdav/pkg/preprocessor/preprocessor.go b/services/webdav/pkg/preprocessor/preprocessor.go index 5f9799d690..0bc562f68b 100644 --- a/services/webdav/pkg/preprocessor/preprocessor.go +++ b/services/webdav/pkg/preprocessor/preprocessor.go @@ -42,7 +42,13 @@ func (i GifDecoder) Convert(r io.Reader) (any, error) { // GgsDecoder is a converter for the geogebra slides file type GgsDecoder struct{ thumbnailpath string } -// Convert reads the ggs file and returns the thumbnail image +// Convert reads the ggs file and returns the thumbnail image. +// +// TODO: this parses user-provided bytes in-process (archive/zip streams the +// embedded thumbnail out of the .ggs container, which is then decoded and +// re-encoded here). Tika is the intended out-of-process extractor for embedded +// images; reading/re-encoding user-supplied embedded images in webdav is a +// separate attack vector not addressed by this architecture change. func (g GgsDecoder) Convert(r io.Reader) (any, error) { var buf bytes.Buffer _, err := io.Copy(&buf, r) @@ -76,7 +82,13 @@ func (g GgsDecoder) Convert(r io.Reader) (any, error) { // AudioDecoder is a converter for the audio file type AudioDecoder struct{} -// Convert reads the audio file and extracts the thumbnail image from the id3 tag +// Convert reads the audio file and extracts the thumbnail image from the id3 tag. +// +// TODO: this parses user-provided bytes in-process (dhowden/tag reads the audio +// metadata and pulls the embedded APIC picture, which is then decoded and +// re-encoded here). Tika is the intended out-of-process extractor for embedded +// images; reading/re-encoding user-supplied embedded images in webdav is a +// separate attack vector not addressed by this architecture change. func (i AudioDecoder) Convert(r io.Reader) (any, error) { b, err := io.ReadAll(r) if err != nil { @@ -200,7 +212,13 @@ type GGPStruct struct { // GgpDecoder is a converter for the geogebra pinboard file type GgpDecoder struct{} -// Convert reads the ggp file and returns the first thumbnail image +// Convert reads the ggp file and returns the first thumbnail image. +// +// TODO: this parses user-provided bytes in-process (JSON/base64 extraction of +// the embedded image, which is then decoded here). Tika is the intended +// out-of-process extractor for embedded images; reading/re-encoding user-supplied +// embedded images in webdav is a separate attack vector not addressed by this +// architecture change. func (j GgpDecoder) Convert(r io.Reader) (any, error) { ggp := &GGPStruct{} err := json.NewDecoder(r).Decode(ggp) diff --git a/services/webdav/pkg/preprocessor/preprocessor_vips.go b/services/webdav/pkg/preprocessor/preprocessor_vips.go index 087bafb1b7..15af62e0b7 100644 --- a/services/webdav/pkg/preprocessor/preprocessor_vips.go +++ b/services/webdav/pkg/preprocessor/preprocessor_vips.go @@ -5,16 +5,25 @@ package preprocessor import ( "io" - "github.com/davidbyttow/govips/v2/vips" + vips "github.com/davidbyttow/govips/v2/vips" + "github.com/kovidgoyal/imaging" + "github.com/pkg/errors" ) func init() { vips.LoggingSettings(nil, vips.LogLevelError) } +// ImageDecoder is a converter for the image file. It decodes with imaging so the +// downstream encode step always receives an image.Image (the vips-backed +// encoder re-wraps it into libvips itself). Returning a *vips.ImageRef here +// would leak a C image past the workflow's ownership boundary. type ImageDecoder struct{} -func (v ImageDecoder) Convert(r io.Reader) (interface{}, error) { - img, err := vips.NewImageFromReader(r) - return img, err +func (i ImageDecoder) Convert(r io.Reader) (any, error) { + img, err := imaging.Decode(r, imaging.AutoOrientation(true)) + if err != nil { + return nil, errors.Wrap(err, `could not decode the image`) + } + return img, nil } diff --git a/services/webdav/pkg/service/v0/service.go b/services/webdav/pkg/service/v0/service.go index 969d44c7c2..11f6ff4d03 100644 --- a/services/webdav/pkg/service/v0/service.go +++ b/services/webdav/pkg/service/v0/service.go @@ -113,7 +113,6 @@ func NewService(opts ...Option) (Service, error) { wf, err := workflow.NewWorkflow( workflow.WithGeneratorURL(conf.ThumbnailGeneratorURL), - workflow.WithAuthHeader(conf.ThumbnailGeneratorAuthHeader), workflow.WithCache(c), workflow.WithHTTPClient(httpClient), workflow.WithMaxInputSize(maxInputSize), @@ -124,6 +123,7 @@ func NewService(opts ...Option) (Service, error) { workflow.WithStater(workflow.NewGatewayStater(gatewaySelector)), workflow.WithFileDownloader(workflow.NewGatewayFileDownloader(gatewaySelector, httpClient)), workflow.WithSpaceLookup(workflow.NewGatewaySpaceLookup(gatewaySelector)), + workflow.WithUserResolver(workflow.NewGatewayUserResolver(gatewaySelector)), ) if err != nil { return nil, fmt.Errorf("create thumbnail workflow: %w", err) @@ -395,8 +395,15 @@ func (g Webdav) handleWorkflowError(w http.ResponseWriter, r *http.Request, err renderError(w, r, errBadRequest("Unsupported file type")) return } - logger.Debug().Err(err).Msg("thumbnail workflow failed") - renderError(w, r, errNotFound(notFoundMsg(tr.Filename))) + if errors.Is(err, workflow.ErrNotFound) { + logger.Debug().Err(err).Msg("thumbnail source could not be located") + renderError(w, r, errNotFound(notFoundMsg(tr.Filename))) + return + } + // Anything else (generator down, download failure, timeout, ...) is a server + // error: clients must not cache it as "no preview" the way they do 404s. + logger.Error().Err(err).Msg("thumbnail workflow failed") + renderError(w, r, errInternalError("could not generate thumbnail")) } func (g Webdav) handleHeadError(w http.ResponseWriter, r *http.Request, err error, tr *requests.ThumbnailRequest, logger log.Logger) { @@ -420,13 +427,17 @@ func (g Webdav) handleHeadError(w http.ResponseWriter, r *http.Request, err erro renderError(w, r, errBadRequest("Unsupported file type")) return } - logger.Debug().Err(err).Msg("thumbnail head check failed") - renderError(w, r, errNotFound(notFoundMsg(tr.Filename))) + if errors.Is(err, workflow.ErrNotFound) { + logger.Debug().Err(err).Msg("thumbnail source could not be located") + renderError(w, r, errNotFound(notFoundMsg(tr.Filename))) + return + } + logger.Error().Err(err).Msg("thumbnail head check failed") + renderError(w, r, errInternalError("could not check thumbnail")) } func (g Webdav) handlePublicLinkAuthError(w http.ResponseWriter, r *http.Request, err error, filename string, logger log.Logger) { - errMsg := err.Error() - if strings.Contains(errMsg, "PERMISSION_DENIED") || strings.Contains(errMsg, "password required") { + if errors.Is(err, workflow.ErrPublicLinkPasswordRequired) { // A password-protected public link accessed without (or with the wrong) // password must not reveal that the resource exists, so it is hidden // behind a 404 rather than a 403. @@ -434,11 +445,12 @@ func (g Webdav) handlePublicLinkAuthError(w http.ResponseWriter, r *http.Request renderError(w, r, errNotFound(notFoundMsg(filename))) return } - if strings.Contains(errMsg, "FAILED_PRECONDITION") || strings.Contains(errMsg, "expired") { + if errors.Is(err, workflow.ErrPublicLinkExpired) { + logger.Debug().Err(err).Msg("public link has expired") renderError(w, r, newErrResponse(http.StatusGone, "public link has expired")) return } - logger.Debug().Err(err).Msg("could not authenticate public link") + logger.Error().Err(err).Msg("could not authenticate public link") renderError(w, r, errInternalError("could not authenticate public link")) } diff --git a/services/webdav/pkg/thumbnail/cache/filecache.go b/services/webdav/pkg/thumbnail/cache/filecache.go index 4701903774..ab8ee18c3e 100644 --- a/services/webdav/pkg/thumbnail/cache/filecache.go +++ b/services/webdav/pkg/thumbnail/cache/filecache.go @@ -29,9 +29,7 @@ func (c *FileCache) Get(key string) ([]byte, error) { return nil, err } - result := make([]byte, len(data)) - copy(result, data) - return result, nil + return data, nil } func (c *FileCache) Put(key string, data []byte) error { diff --git a/services/webdav/pkg/thumbnail/cache/s3cache.go b/services/webdav/pkg/thumbnail/cache/s3cache.go index 687d882f11..9c1fedf14c 100644 --- a/services/webdav/pkg/thumbnail/cache/s3cache.go +++ b/services/webdav/pkg/thumbnail/cache/s3cache.go @@ -99,14 +99,7 @@ func (c *S3Cache) Get(key string) ([]byte, error) { } defer obj.Close() - data, err := io.ReadAll(obj) - if err != nil { - return nil, err - } - - result := make([]byte, len(data)) - copy(result, data) - return result, nil + return io.ReadAll(obj) } func (c *S3Cache) Put(key string, data []byte) error { diff --git a/services/webdav/pkg/thumbnail/workflow/encode_imaging.go b/services/webdav/pkg/thumbnail/workflow/encode_imaging.go index 267322c7de..e732dd9d25 100644 --- a/services/webdav/pkg/thumbnail/workflow/encode_imaging.go +++ b/services/webdav/pkg/thumbnail/workflow/encode_imaging.go @@ -19,7 +19,7 @@ import ( func encodeForUpload(v any, mimeType string) ([]byte, string, error) { switch data := v.(type) { case []byte: - return data, generator.GuessExtension(mimeType), nil + return data, generator.MimeToExt(mimeType), nil case image.Image: var ( buf bytes.Buffer diff --git a/services/webdav/pkg/thumbnail/workflow/encode_vips.go b/services/webdav/pkg/thumbnail/workflow/encode_vips.go index a830c9b902..d3bce98a54 100644 --- a/services/webdav/pkg/thumbnail/workflow/encode_vips.go +++ b/services/webdav/pkg/thumbnail/workflow/encode_vips.go @@ -18,7 +18,7 @@ import ( func encodeForUpload(v any, mimeType string) ([]byte, string, error) { switch data := v.(type) { case []byte: - return data, generator.GuessExtension(mimeType), nil + return data, generator.MimeToExt(mimeType), nil case image.Image: img, err := vips.NewImageFromGoImage(data) if err != nil { diff --git a/services/webdav/pkg/thumbnail/workflow/workflow.go b/services/webdav/pkg/thumbnail/workflow/workflow.go index 5b104b22a1..4833cad25e 100644 --- a/services/webdav/pkg/thumbnail/workflow/workflow.go +++ b/services/webdav/pkg/thumbnail/workflow/workflow.go @@ -18,6 +18,7 @@ import ( rpcv1beta1 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" providerv1beta1 "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + chimiddleware "github.com/go-chi/chi/v5/middleware" revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" "github.com/opencloud-eu/reva/v2/pkg/utils" @@ -38,9 +39,11 @@ type Stater interface { Stat(ctx context.Context, ref *providerv1beta1.Reference, auth string) (*providerv1beta1.StatResponse, error) } -// FileDownloader downloads file bytes from storage. +// FileDownloader opens a read stream on a file in storage. The returned reader +// must be consumed (and closed by the caller) exactly once; it is the raw +// response body and is never buffered or decoded here. type FileDownloader interface { - Download(ctx context.Context, ref *providerv1beta1.Reference, auth string) ([]byte, error) + DownloadStream(ctx context.Context, ref *providerv1beta1.Reference, auth string) (io.ReadCloser, error) } // SpaceLookup resolves a path-only reference (ResourceId == nil) into a full @@ -52,6 +55,15 @@ type SpaceLookup interface { Resolve(ctx context.Context, ref *providerv1beta1.Reference, auth string) (*providerv1beta1.Reference, error) } +// UserResolver resolves users for path-only thumbnail requests. WhoAmI maps an +// auth token to its user (used for /webdav/... where the URL carries no +// username); GetUserByClaim maps a username from /dav/files/{user}/... to its +// user, mirroring main's behavior. +type UserResolver interface { + WhoAmI(ctx context.Context, auth string) (*userv1beta1.User, error) + GetUserByClaim(ctx context.Context, claim, value string) (*userv1beta1.User, error) +} + // ErrFileProcessing is returned when the file is still being processed by the // storage backend (e.g. a virus scan or conversion in flight). Callers should // surface this as HTTP 425 Too Early with a Retry-After header. @@ -72,6 +84,11 @@ var ErrNotAFile = errors.New("thumbnails: resource is not a file") // surface this as HTTP 403 Forbidden, matching the legacy thumbnail service. var ErrPermissionDenied = errors.New("thumbnails: no download permission") +// ErrNotFound is returned when the requested file could not be located or has no +// usable checksum. Callers should surface this as HTTP 404 Not Found with a +// "File could not be located" message, matching the legacy thumbnail service. +var ErrNotFound = errors.New("thumbnails: file could not be located") + // fileIsProcessing reports whether the resource carries the "processing" status // in its opaque map, as set by the storage backend while a file is being handled. func fileIsProcessing(info *providerv1beta1.ResourceInfo) bool { @@ -105,7 +122,6 @@ func thumbnailStorageKey(checksum string, w, h int, ext, processor string) strin // stat → validate → cache check → download → preprocess → generate → cache → respond. type ThumbnailWorkflow struct { generatorURL string - authHeader string cache cache.ThumbnailCache httpClient *http.Client maxInputSize uint64 @@ -116,6 +132,7 @@ type ThumbnailWorkflow struct { stater Stater fileDownloader FileDownloader spaceLookup SpaceLookup + userResolver UserResolver } // NewWorkflow creates a ThumbnailWorkflow. generatorURL must be non-empty. @@ -151,11 +168,6 @@ func WithGeneratorURL(url string) Option { return func(w *ThumbnailWorkflow) { w.generatorURL = url } } -// WithAuthHeader sets an optional auth header value sent to the generator (for external generators behind a reverse proxy). -func WithAuthHeader(header string) Option { - return func(w *ThumbnailWorkflow) { w.authHeader = header } -} - // WithCache sets the thumbnail cache. func WithCache(c cache.ThumbnailCache) Option { return func(w *ThumbnailWorkflow) { w.cache = c } @@ -209,6 +221,13 @@ func WithSpaceLookup(l SpaceLookup) Option { return func(w *ThumbnailWorkflow) { w.spaceLookup = l } } +// WithUserResolver sets the resolver used to map an auth token to its user for +// path-only requests (/dav/files/{user}/..., /webdav/...). When unset, such +// requests cannot be absolutized and fall back to the raw path. +func WithUserResolver(r UserResolver) Option { + return func(w *ThumbnailWorkflow) { w.userResolver = r } +} + // Execute handles a user thumbnail request (GET). func (w *ThumbnailWorkflow) Execute(ctx context.Context, tr *requests.ThumbnailRequest, auth string, logger log.Logger) (data []byte, ext string, aIgnored bool, err error) { ref, err := w.resolveReference(ctx, tr, auth) @@ -287,9 +306,11 @@ func (w *ThumbnailWorkflow) resolveReference(ctx context.Context, tr *requests.T return ref, nil } - // Path-only request: absolutize the path relative to the requesting user's - // home (the legacy webdav behavior) before resolving the owning space. - absPath := w.absolutizeUserPath(ctx, ref.GetPath(), auth) + // Path-only request: absolutize the path relative to a user's home (the + // legacy webdav behavior) before resolving the owning space. The user is the + // one named in the URL (/dav/files/{user}/...) when present, otherwise the + // token owner (/webdav/...). + absPath := w.absolutizeUserPath(ctx, ref.GetPath(), tr.Identifier, auth) if w.spaceLookup == nil { return &providerv1beta1.Reference{Path: absPath}, nil @@ -302,16 +323,34 @@ func (w *ThumbnailWorkflow) resolveReference(ctx context.Context, tr *requests.T return resolved, nil } -// absolutizeUserPath maps a path-only reference to an absolute CS3 path under the -// requesting user's home, matching the legacy webdav handler. Public link paths -// (already absolute under /public) and any path that is not relative to the -// caller are returned unchanged. -func (w *ThumbnailWorkflow) absolutizeUserPath(ctx context.Context, p string, auth string) string { +// absolutizeUserPath maps a path-only reference to an absolute CS3 path under a +// user's home, matching the legacy webdav handler. The user is resolved from the +// username in the URL (identifier, via GetUserByClaim) when present, otherwise +// from the auth token (via WhoAmI). Public link paths (already absolute under +// /public) and any path that is not relative to the caller are returned +// unchanged. +func (w *ThumbnailWorkflow) absolutizeUserPath(ctx context.Context, p, identifier, auth string) string { if strings.HasPrefix(p, "/") { return p } - user, err := w.resolveUser(ctx, auth) + if w.userResolver == nil { + // No resolver configured: we cannot absolutize the path. + w.log.Warn().Msg("no user resolver configured, using raw path for thumbnail") + return p + } + + var ( + user *userv1beta1.User + err error + ) + if identifier != "" { + // /dav/files/{user}/...: honor the username from the URL. + user, err = w.userResolver.GetUserByClaim(ctx, "username", identifier) + } else { + // /webdav/...: resolve the token owner. + user, err = w.userResolver.WhoAmI(ctx, auth) + } if err != nil { // Without a resolved user we cannot absolutize; fall back to the raw path. w.log.Warn().Err(err).Msg("could not resolve user for thumbnail path") @@ -347,6 +386,12 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R } checksum := info.GetChecksum().GetSum() + if checksum == "" { + // Without a checksum the cache key would be shared across all files, so + // the resource is treated as not found (matching main's grpc service). + logger.Debug().Msg("resource info is missing a checksum") + return nil, "", false, ErrNotFound + } mimeType := info.GetMimeType() // The output type follows the source mime (like main's GetExtForMime); when @@ -374,27 +419,6 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R } } - fileBytes, err := w.fileDownloader.Download(ctx, ref, auth) - if err != nil { - logger.Error().Err(err).Msg("could not download file for thumbnail") - return nil, "", false, fmt.Errorf("download: %w", err) - } - - // Direct image types are handed to the generator as-is; everything else is - // converted to an image first (audio cover art, geogebra, text, gif). - var toSend any = fileBytes - if !isDirectImageMime(mimeType) { - 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, "", false, fmt.Errorf("could not get image") - } - toSend = img - } - // webdav is the sizing brain: it snaps the requested size onto a configured // resolution (orientation-aware). It never inspects the image bytes; the // generator is a dumb executor that just fits within the given box. @@ -403,8 +427,25 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R if w.resolutions != nil { box = w.resolutions.Match(reqBox) } - genURL := generator.BuildURL(w.generatorURL, int32(box.Dx()), int32(box.Dy()), operation, outputExt) - thumbBytes, err := w.postToGenerator(ctx, genURL, toSend, mimeType) + + // webdav never asks the generator to upscale: it snaps onto a resolution at + // least as large as requested, so the box is always >= the source intent. + // The no_upscale() filter makes the generator behave like real imagor for + // any other client that posts smaller boxes. + genURL := generator.BuildURL(w.generatorURL, int32(box.Dx()), int32(box.Dy()), operation, outputExt, true) + + // Produce the source image to send to the generator, never decoding an image + // 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) + 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) if err != nil { logger.Error().Err(err).Msg("could not generate thumbnail") return nil, "", false, fmt.Errorf("generate: %w", err) @@ -419,6 +460,49 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R return thumbBytes, outputExt, aIgnored, nil } +// sourceImage returns a reader over the source image to send to the generator, +// plus a cleanup func to release any resources (download body, temp buffer). It +// never decodes an image in webdav: +// - real images (image/*, incl. gif) are streamed straight through from storage +// 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) { + 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 body, func() { _ = body.Close() }, nil + } + + body, err := w.fileDownloader.DownloadStream(ctx, ref, auth) + if err != nil { + 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) + } + + 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("could not get image") + } + + data, _, err := encodeForUpload(img, mimeType) + if err != nil { + return nil, nil, fmt.Errorf("encode converted image: %w", err) + } + return bytes.NewReader(data), func() {}, nil +} + // matchOperation resolves the generator resize/crop operation for a request from // the client's processor and the legacy "a" flag. An explicit processor always // wins; when no processor is given the default depends on the source type (gifs @@ -474,42 +558,60 @@ func (w *ThumbnailWorkflow) matchOperation(tr *requests.ThumbnailRequest, mimeTy return operation, aIgnored } -func (w *ThumbnailWorkflow) postToGenerator(ctx context.Context, url string, file any, mimeType string) ([]byte, error) { - data, ext, err := encodeForUpload(file, mimeType) - if err != nil { - return nil, fmt.Errorf("encode for upload: %w", err) - } - - body := &bytes.Buffer{} - writer := multipart.NewWriter(body) - - part, err := writer.CreateFormFile("image", "file."+ext) - if err != nil { - return nil, fmt.Errorf("create form file: %w", err) - } - if _, err := part.Write(data); err != nil { - return nil, fmt.Errorf("write form data: %w", err) - } - - if err := writer.Close(); err != nil { - return nil, fmt.Errorf("close multipart writer: %w", err) - } - - req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, body) +// 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) { + pr, pw := io.Pipe() + writer := multipart.NewWriter(pw) + + errCh := make(chan error, 1) + go func() { + defer pw.Close() + part, err := writer.CreateFormFile("image", filename) + if err != nil { + errCh <- fmt.Errorf("create form file: %w", err) + return + } + if _, err := io.Copy(part, img); err != nil { + errCh <- fmt.Errorf("write form data: %w", err) + return + } + if err := writer.Close(); err != nil { + errCh <- fmt.Errorf("close multipart writer: %w", err) + return + } + errCh <- nil + }() + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, pr) if err != nil { + pr.Close() return nil, fmt.Errorf("create generator request: %w", err) } req.Header.Set("Content-Type", writer.FormDataContentType()) - if w.authHeader != "" { - req.Header.Set(revactx.TokenHeader, w.authHeader) + // Forward the request ID so the generator's logs can be correlated with this + // request. No auth token is sent: the generator is an unauthenticated + // internal service (bound to loopback by default). + if reqID := chimiddleware.GetReqID(ctx); reqID != "" { + req.Header.Set("X-Request-ID", reqID) } httpRsp, err := w.httpClient.Do(req) if err != nil { + pr.Close() return nil, fmt.Errorf("generator request: %w", err) } defer httpRsp.Body.Close() + if writeErr := <-errCh; writeErr != nil { + io.Copy(io.Discard, httpRsp.Body) + return nil, writeErr + } + if httpRsp.StatusCode < 200 || httpRsp.StatusCode >= 300 { respBody, _ := io.ReadAll(httpRsp.Body) // The generator reports ErrMaxResolutionExceeded (422) when the decoded @@ -530,32 +632,18 @@ func (w *ThumbnailWorkflow) postToGenerator(ctx context.Context, url string, fil return rspData, nil } -// directImageMimes are mime types the generator can decode directly; their -// bytes are passed through without preprocessing. -var directImageMimes = map[string]struct{}{ - "image/png": {}, - "image/jpg": {}, - "image/jpeg": {}, - "image/tiff": {}, - "image/bmp": {}, - "image/x-ms-bmp": {}, - "image/webp": {}, -} - -func isDirectImageMime(mimeType string) bool { - m, _, _ := mime.ParseMediaType(mimeType) - _, ok := directImageMimes[m] - return ok -} - // gatewaySelector is the interface for selecting a gateway client. type gatewaySelector interface { Next(...pool.Option) (gatewayv1beta1.GatewayAPIClient, error) } -func (w *ThumbnailWorkflow) resolveUser(ctx context.Context, auth string) (*userv1beta1.User, error) { - gs := w.stater.(*gatewayStater) - client, err := gs.selector.Next() +// gatewayUserResolver implements UserResolver using the CS3 gateway's WhoAmI. +type gatewayUserResolver struct { + selector gatewaySelector +} + +func (g *gatewayUserResolver) WhoAmI(ctx context.Context, auth string) (*userv1beta1.User, error) { + client, err := g.selector.Next() if err != nil { return nil, fmt.Errorf("get gateway client: %w", err) } @@ -571,6 +659,28 @@ func (w *ThumbnailWorkflow) resolveUser(ctx context.Context, auth string) (*user return userRes.GetUser(), nil } +func (g *gatewayUserResolver) GetUserByClaim(ctx context.Context, claim, value string) (*userv1beta1.User, error) { + client, err := g.selector.Next() + if err != nil { + return nil, fmt.Errorf("get gateway client: %w", err) + } + + userRes, err := client.GetUserByClaim(ctx, &userv1beta1.GetUserByClaimRequest{Claim: claim, Value: value}) + if err != nil { + return nil, fmt.Errorf("get user by claim: %w", err) + } + if userRes.GetStatus().GetCode() != rpcv1beta1.Code_CODE_OK { + return nil, fmt.Errorf("get user by claim: %s", userRes.GetStatus().GetMessage()) + } + + return userRes.GetUser(), nil +} + +// NewGatewayUserResolver creates a UserResolver backed by the CS3 gateway. +func NewGatewayUserResolver(selector gatewaySelector) UserResolver { + return &gatewayUserResolver{selector: selector} +} + // gatewayStater implements Stater using the CS3 gateway. type gatewayStater struct { selector gatewaySelector @@ -606,7 +716,7 @@ type gatewayFileDownloader struct { httpClient *http.Client } -func (g *gatewayFileDownloader) Download(ctx context.Context, ref *providerv1beta1.Reference, auth string) ([]byte, error) { +func (g *gatewayFileDownloader) DownloadStream(ctx context.Context, ref *providerv1beta1.Reference, auth string) (io.ReadCloser, error) { client, err := g.selector.Next() if err != nil { return nil, fmt.Errorf("get gateway client: %w", err) @@ -636,18 +746,15 @@ func (g *gatewayFileDownloader) Download(ctx context.Context, ref *providerv1bet if err != nil { return nil, fmt.Errorf("download request: %w", err) } - defer httpRsp.Body.Close() if httpRsp.StatusCode != http.StatusOK { + httpRsp.Body.Close() return nil, fmt.Errorf("download failed with status %d", httpRsp.StatusCode) } - data, err := io.ReadAll(httpRsp.Body) - if err != nil { - return nil, fmt.Errorf("read download body: %w", err) - } - - return data, nil + // Return the live response body; the caller owns and closes it. The bytes are + // never buffered or decoded in webdav. + return httpRsp.Body, nil } func extractProtocol(protocols []*gatewayv1beta1.FileDownloadProtocol) (endpoint, token string) { @@ -738,7 +845,19 @@ func NewGatewaySpaceLookup(selector gatewaySelector) SpaceLookup { return &gatewaySpaceLookup{selector: selector} } -// ResolvePublicLinkAuth authenticates a public link token via the gateway. +// ErrPublicLinkPasswordRequired is returned when a password-protected public +// link is accessed without (or with the wrong) password. Callers should surface +// this as HTTP 404 Not Found so the resource's existence is not revealed. +var ErrPublicLinkPasswordRequired = errors.New("public link requires a password") + +// ErrPublicLinkExpired is returned when a public link token has expired. +// Callers should surface this as HTTP 410 Gone. +var ErrPublicLinkExpired = errors.New("public link has expired") + +// ResolvePublicLinkAuth authenticates a public link token via the gateway. It +// returns ErrPublicLinkPasswordRequired or ErrPublicLinkExpired (wrapped) when +// the gateway reports the corresponding gRPC status code, so callers can branch +// on the error type instead of matching error text. func ResolvePublicLinkAuth(ctx context.Context, r *http.Request, publicLinkToken string, selector gatewaySelector) (string, error) { gatewayClient, err := selector.Next() if err != nil { @@ -768,9 +887,14 @@ func ResolvePublicLinkAuth(ctx context.Context, r *http.Request, publicLinkToken return "", fmt.Errorf("could not authenticate public link: %w", err) } - if rsp.GetStatus().GetCode() != rpcv1beta1.Code_CODE_OK { + switch rsp.GetStatus().GetCode() { + case rpcv1beta1.Code_CODE_OK: + return rsp.GetToken(), nil + case rpcv1beta1.Code_CODE_PERMISSION_DENIED: + return "", fmt.Errorf("%w: %s", ErrPublicLinkPasswordRequired, rsp.GetStatus().GetMessage()) + case rpcv1beta1.Code_CODE_FAILED_PRECONDITION: + return "", fmt.Errorf("%w: %s", ErrPublicLinkExpired, rsp.GetStatus().GetMessage()) + default: return "", fmt.Errorf("public link authentication failed: code=%s message=%s", rsp.GetStatus().GetCode(), rsp.GetStatus().GetMessage()) } - - return rsp.GetToken(), nil } diff --git a/services/webdav/pkg/thumbnail/workflow/workflow_test.go b/services/webdav/pkg/thumbnail/workflow/workflow_test.go index cc37926c5c..1a4f393f82 100644 --- a/services/webdav/pkg/thumbnail/workflow/workflow_test.go +++ b/services/webdav/pkg/thumbnail/workflow/workflow_test.go @@ -17,14 +17,14 @@ import ( userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" providerv1beta1 "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" - "github.com/opencloud-eu/reva/v2/pkg/storagespace" - "github.com/opencloud-eu/reva/v2/pkg/utils" "github.com/go-chi/chi/v5" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/status" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" + "github.com/opencloud-eu/reva/v2/pkg/storagespace" + "github.com/opencloud-eu/reva/v2/pkg/utils" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -292,6 +292,7 @@ var _ = Describe("ThumbnailWorkflow", func() { WithStater(NewGatewayStater(gatewaySelector)), WithFileDownloader(NewGatewayFileDownloader(gatewaySelector, &http.Client{})), WithSpaceLookup(NewGatewaySpaceLookup(gatewaySelector)), + WithUserResolver(NewGatewayUserResolver(gatewaySelector)), ) Expect(err).ToNot(HaveOccurred()) @@ -398,6 +399,85 @@ var _ = Describe("ThumbnailWorkflow", func() { }) }) + Describe("path-only /webdav/ reference", func() { + It("resolves the token owner via WhoAmI when no username is in the URL", func() { + var statRef *providerv1beta1.Reference + + gatewayClient.On("WhoAmI", mock.Anything, mock.MatchedBy(func(req *gatewayv1beta1.WhoAmIRequest) bool { + return req.GetToken() == testToken + })).Return(&gatewayv1beta1.WhoAmIResponse{ + Status: status.NewOK(context.Background()), + User: &userv1beta1.User{ + Id: &userv1beta1.UserId{ + Idp: "https://opencloud-server:9200", + OpaqueId: "test-opaque", + Type: userv1beta1.UserType_USER_TYPE_PRIMARY, + }, + Username: "test", + }, + }, nil) + + gatewayClient.On("Stat", mock.Anything, mock.MatchedBy(func(req *providerv1beta1.StatRequest) bool { + statRef = req.GetRef() + return strings.Contains(req.GetRef().GetPath(), "photo.jpeg") + })).Return(&providerv1beta1.StatResponse{ + Status: status.NewOK(context.Background()), + Info: &providerv1beta1.ResourceInfo{ + Type: providerv1beta1.ResourceType_RESOURCE_TYPE_FILE, + PermissionSet: &providerv1beta1.ResourcePermissions{InitiateFileDownload: true}, + MimeType: "image/jpeg", + Size: 1024, + Checksum: &providerv1beta1.ResourceChecksum{Sum: "webdavchecksum"}, + }, + }, nil) + + downloadBody := testJPEG(800, 600) + var downloadRef *providerv1beta1.Reference + + storageServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write(downloadBody) + })) + defer storageServer.Close() + + gatewayClient.On("InitiateFileDownload", mock.Anything, mock.MatchedBy(func(req *providerv1beta1.InitiateFileDownloadRequest) bool { + downloadRef = req.GetRef() + return strings.Contains(req.GetRef().GetPath(), "photo.jpeg") + })).Return(&gatewayv1beta1.InitiateFileDownloadResponse{ + Status: status.NewOK(context.Background()), + Protocols: []*gatewayv1beta1.FileDownloadProtocol{ + {Protocol: "spaces", DownloadEndpoint: storageServer.URL, Token: "download-token"}, + }, + }, nil) + + generatorSrv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte("webdav-thumbnail")) + })) + defer generatorSrv.Close() + wf.generatorURL = generatorSrv.URL + + tr := &requests.ThumbnailRequest{ + Ref: &providerv1beta1.Reference{Path: "photo.jpeg"}, + Filename: "photo.jpeg", + Extension: ".jpeg", + Width: 36, + Height: 36, + } + + data, ext, _, err := wf.Execute(context.Background(), tr, testToken, logger) + Expect(err).ToNot(HaveOccurred()) + Expect(ext).To(Equal("jpg")) + Expect(data).To(Equal([]byte("webdav-thumbnail"))) + + // No username in the URL: the token owner (test) is resolved via WhoAmI. + Expect(statRef.GetResourceId()).ToNot(BeNil()) + Expect(statRef.GetPath()).To(Equal("./users/test-opaque/photo.jpeg")) + Expect(downloadRef.GetResourceId()).ToNot(BeNil()) + Expect(downloadRef.GetPath()).To(Equal("./users/test-opaque/photo.jpeg")) + }) + }) + Describe("space-scoped reference", func() { It("anchors stat and download at the space ResourceId, not a path mount", func() { const ( @@ -611,11 +691,12 @@ var _ = Describe("ThumbnailWorkflow", func() { wf.generatorURL = generatorSrv.URL tr := &requests.ThumbnailRequest{ - Ref: &providerv1beta1.Reference{Path: "photo.jpeg"}, - Filename: "photo.jpeg", - Extension: ".jpeg", - Width: 36, - Height: 36, + Ref: &providerv1beta1.Reference{Path: "photo.jpeg"}, + Filename: "photo.jpeg", + Extension: ".jpeg", + Width: 36, + Height: 36, + Identifier: "alice", } data, ext, _, err := wf.Execute(context.Background(), tr, testToken, logger) @@ -623,12 +704,12 @@ var _ = Describe("ThumbnailWorkflow", func() { Expect(ext).To(Equal("jpg")) Expect(data).To(Equal([]byte("user-thumbnail"))) - // The path-only reference is resolved to a space-anchored reference by - // the space lookup (the users storage provider root from the mock). + // The username from the URL (alice) is resolved via GetUserByClaim and + // the path is absolutized under alice's home, not the token owner's. Expect(statRef.GetResourceId()).ToNot(BeNil()) - Expect(statRef.GetPath()).To(Equal("./users/test-opaque/photo.jpeg")) + Expect(statRef.GetPath()).To(Equal("./users/alice-opaque/photo.jpeg")) Expect(downloadRef.GetResourceId()).ToNot(BeNil()) - Expect(downloadRef.GetPath()).To(Equal("./users/test-opaque/photo.jpeg")) + Expect(downloadRef.GetPath()).To(Equal("./users/alice-opaque/photo.jpeg")) }) }) @@ -711,7 +792,7 @@ var _ = Describe("ThumbnailWorkflow", func() { Expect(captured.data).ToNot(Equal(text)) }) - It("re-encodes gif files to gif bytes before posting", func() { + It("passes gif files through to the generator undecoded", func() { gifBytes, err := os.ReadFile(filepath.Join("..", "..", "preprocessor", "test_assets", "noise.gif")) Expect(err).ToNot(HaveOccurred()) @@ -727,12 +808,9 @@ var _ = Describe("ThumbnailWorkflow", func() { _, _, _, err = wf2.Execute(context.Background(), tr, testToken, logger) Expect(err).ToNot(HaveOccurred()) - Expect(captured.data).ToNot(BeEmpty()) - // The generator must receive valid gif bytes (GIF87a/GIF89a magic). - Expect(string(captured.data[:6])).To(SatisfyAny( - Equal("GIF89a"), - Equal("GIF87a"), - )) + // Gifs are image/* and must reach the generator byte-for-byte; the + // generator (imagor or thumbnailer) handles multi-frame resizing. + Expect(captured.data).To(Equal(gifBytes)) }) It("returns an error when the file cannot be converted to an image", func() {