From 8bec65459542fb9c0e412e7901b1279a3803a181 Mon Sep 17 00:00:00 2001 From: kolaente Date: Fri, 12 Jun 2026 11:48:41 +0200 Subject: [PATCH] refactor(background): extract download + unsplash-proxy logic for reuse Split the HTTP plumbing from the business logic in the v1 project-background download and Unsplash image proxy handlers so /api/v2 can reuse it without duplicating it: - LoadProjectBackgroundForDownload (background/handler) loads the bg file + modtime and fires the Unsplash pingback; GetProjectBackground now calls it. - WriteProjectBackground (web/files) writes v1's exact background wire shape (image/jpg, no-cache, stat-modtime Last-Modified, If-Modified-Since 304). - FetchUnsplashImageByID / FetchUnsplashThumbByID (background/unsplash) return the open upstream body for the caller to stream; the v1 proxy handlers now call them. A typed ErrUnsplashImageDoesNotExist maps to 404 on both APIs. - ErrProjectHasNoBackground (models) gives the no-background case a domain error; v1 keeps its verbatim 404 message. v1 responses are unchanged on the wire. --- pkg/models/error.go | 28 ++++++ pkg/modules/background/handler/background.go | 65 +++++++------- pkg/modules/background/unsplash/proxy.go | 93 ++++++++++++++++---- pkg/web/files/project_background.go | 55 ++++++++++++ 4 files changed, 187 insertions(+), 54 deletions(-) create mode 100644 pkg/web/files/project_background.go diff --git a/pkg/models/error.go b/pkg/models/error.go index 2f9d652c9..0b793aecc 100644 --- a/pkg/models/error.go +++ b/pkg/models/error.go @@ -535,6 +535,34 @@ func (err *ErrProjectViewDoesNotExist) HTTPError() web.HTTPError { } } +// ErrProjectHasNoBackground represents an error where a project has no background set. +type ErrProjectHasNoBackground struct { + ProjectID int64 +} + +// IsErrProjectHasNoBackground checks if an error is ErrProjectHasNoBackground. +func IsErrProjectHasNoBackground(err error) bool { + _, ok := err.(*ErrProjectHasNoBackground) + return ok +} + +func (err *ErrProjectHasNoBackground) Error() string { + return fmt.Sprintf("Project has no background [ProjectID: %d]", err.ProjectID) +} + +// ErrCodeProjectHasNoBackground holds the unique world-error code of this error +const ErrCodeProjectHasNoBackground = 3015 + +// HTTPError holds the http error description +func (err *ErrProjectHasNoBackground) HTTPError() web.HTTPError { + return web.HTTPError{ + HTTPCode: http.StatusNotFound, + Code: ErrCodeProjectHasNoBackground, + // Message kept verbatim from v1's inline handler error so the wire body is unchanged. + Message: "Project background not found", + } +} + // ============== // Task errors // ============== diff --git a/pkg/modules/background/handler/background.go b/pkg/modules/background/handler/background.go index afe7901e5..da9d0e522 100644 --- a/pkg/modules/background/handler/background.go +++ b/pkg/modules/background/handler/background.go @@ -31,6 +31,7 @@ import ( "image" "io" "net/http" + "os" "strconv" "strings" @@ -43,6 +44,7 @@ import ( "code.vikunja.io/api/pkg/modules/background/unsplash" "code.vikunja.io/api/pkg/modules/background/upload" "code.vikunja.io/api/pkg/web" + webfiles "code.vikunja.io/api/pkg/web/files" "github.com/bbrks/go-blurhash" "github.com/gabriel-vasile/mimetype" @@ -385,54 +387,47 @@ func GetProjectBackground(c *echo.Context) error { return err } - if project.BackgroundFileID == 0 { - _ = s.Rollback() - return echo.NewHTTPError(http.StatusNotFound, "Project background not found") - } - - // Get the file - bgFile := &files.File{ - ID: project.BackgroundFileID, - } - if err := bgFile.LoadFileByID(); err != nil { - _ = s.Rollback() - return err - } - stat, err := files.FileStat(bgFile) + bgFile, stat, err := LoadProjectBackgroundForDownload(s, project) if err != nil { _ = s.Rollback() + if models.IsErrProjectHasNoBackground(err) { + return echo.NewHTTPError(http.StatusNotFound, "Project background not found") + } return err } - // Unsplash requires pingbacks as per their api usage guidelines. - // To do this in a privacy-preserving manner, we do the ping from inside of Vikunja to not expose any user details. - // FIXME: This should use an event once we have events - unsplash.Pingback(s, bgFile) - if err := s.Commit(); err != nil { _ = s.Rollback() return err } - // Override the global no-store directive so browsers can cache background images. - // no-cache allows caching but requires revalidation via If-Modified-Since. - c.Response().Header().Set("Cache-Control", "no-cache") + webfiles.WriteProjectBackground(c.Response(), c.Request(), bgFile, stat) + return nil +} - // Set Last-Modified header if we have the file stat, so clients can decide whether to use cached files - if stat != nil { - modTime := stat.ModTime().UTC() - c.Response().Header().Set(echo.HeaderLastModified, modTime.Format(http.TimeFormat)) - - // Check If-Modified-Since and return 304 if the file hasn't changed - if ifModSince := c.Request().Header.Get("If-Modified-Since"); ifModSince != "" { - if t, err := http.ParseTime(ifModSince); err == nil && !modTime.After(t) { - return c.NoContent(http.StatusNotModified) - } - } +// LoadProjectBackgroundForDownload opens the project's background file (bytes ready to +// read) and stats it for the modtime the download uses for caching. It also fires the +// Unsplash pingback side effect, required by Unsplash's API guidelines and done +// server-side so no user details are exposed. Returns ErrProjectHasNoBackground when the +// project has none; the caller owns committing the session and closing bgFile.File. +func LoadProjectBackgroundForDownload(s *xorm.Session, project *models.Project) (bgFile *files.File, stat os.FileInfo, err error) { + if project.BackgroundFileID == 0 { + return nil, nil, &models.ErrProjectHasNoBackground{ProjectID: project.ID} } - // Serve the file - return c.Stream(http.StatusOK, "image/jpg", bgFile.File) + bgFile = &files.File{ID: project.BackgroundFileID} + if err := bgFile.LoadFileByID(); err != nil { + return nil, nil, err + } + stat, err = files.FileStat(bgFile) + if err != nil { + return nil, nil, err + } + + // FIXME: This should use an event once we have events + unsplash.Pingback(s, bgFile) + + return bgFile, stat, nil } // RemoveProjectBackground removes a project background, no matter the background provider diff --git a/pkg/modules/background/unsplash/proxy.go b/pkg/modules/background/unsplash/proxy.go index 69fb23716..1d0b11607 100644 --- a/pkg/modules/background/unsplash/proxy.go +++ b/pkg/modules/background/unsplash/proxy.go @@ -18,32 +18,95 @@ package unsplash import ( "context" + "errors" + "io" "net/http" "strings" "code.vikunja.io/api/pkg/utils" + "code.vikunja.io/api/pkg/web" "github.com/labstack/echo/v5" ) -func unsplashImage(url string, c *echo.Context) error { +// ErrUnsplashImageDoesNotExist is returned when Unsplash answers an image proxy fetch +// with a non-success status, mirroring v1's echo.ErrNotFound. It satisfies +// web.HTTPErrorProcessor so the v2 error bridge maps it to a 404. +type ErrUnsplashImageDoesNotExist struct{} + +// IsErrUnsplashImageDoesNotExist checks if an error is ErrUnsplashImageDoesNotExist. +func IsErrUnsplashImageDoesNotExist(err error) bool { + var target *ErrUnsplashImageDoesNotExist + return errors.As(err, &target) +} + +func (err *ErrUnsplashImageDoesNotExist) Error() string { + return "Unsplash image does not exist" +} + +// HTTPError holds the http error description. +func (err *ErrUnsplashImageDoesNotExist) HTTPError() web.HTTPError { + return web.HTTPError{HTTPCode: http.StatusNotFound, Message: "Not Found"} +} + +// fetchUnsplashImage fetches an image from Unsplash through the SSRF-safe client and +// returns its still-open response body for the caller to stream and close. The url is +// rebased onto the hardcoded images.unsplash.com host (stripping any client-supplied +// host) so the proxy can only ever reach Unsplash. It returns +// ErrUnsplashImageDoesNotExist on a non-success upstream status. +func fetchUnsplashImage(url string) (io.ReadCloser, error) { // Replacing and appending the url for security reasons req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://images.unsplash.com/"+strings.Replace(url, "https://images.unsplash.com/", "", 1), nil) if err != nil { - return err + return nil, err } resp, err := utils.NewSSRFSafeHTTPClient().Do(req) //nolint:gosec // SSRF protection is handled by the SSRF-safe client if err != nil { - return err + return nil, err } - defer resp.Body.Close() if resp.StatusCode > 399 { - return echo.ErrNotFound + _ = resp.Body.Close() + return nil, &ErrUnsplashImageDoesNotExist{} } - return c.Stream(http.StatusOK, "image/jpg", resp.Body) + return resp.Body, nil } -// ProxyUnsplashImage proxies a thumbnail from unsplash for privacy reasons. +// FetchUnsplashImageByID resolves an Unsplash image by id, fires the required pingback, +// and returns the full-resolution image body for the caller to stream and close. +func FetchUnsplashImageByID(imageID string) (io.ReadCloser, error) { + photo, err := getUnsplashPhotoInfoByID(imageID) + if err != nil { + return nil, err + } + pingbackByPhotoID(photo.ID) + return fetchUnsplashImage(photo.Urls.Raw) +} + +// FetchUnsplashThumbByID resolves an Unsplash image by id, fires the required pingback, +// and returns a thumbnail (max width 200px) body for the caller to stream and close. +func FetchUnsplashThumbByID(imageID string) (io.ReadCloser, error) { + photo, err := getUnsplashPhotoInfoByID(imageID) + if err != nil { + return nil, err + } + pingbackByPhotoID(photo.ID) + return fetchUnsplashImage("https://images.unsplash.com/" + getImageID(photo.Urls.Raw) + "?ixlib=rb-1.2.1&q=80&fm=jpg&crop=entropy&cs=tinysrgb&w=200&fit=max&ixid=eyJhcHBfaWQiOjcyODAwfQ") +} + +// streamUnsplashImage streams a fetched image body to the v1 echo response, mapping the +// not-found sentinel back to echo.ErrNotFound so v1's wire response is unchanged. +func streamUnsplashImage(body io.ReadCloser, err error, c *echo.Context) error { + if err != nil { + if IsErrUnsplashImageDoesNotExist(err) { + return echo.ErrNotFound + } + return err + } + defer body.Close() + return c.Stream(http.StatusOK, "image/jpg", body) +} + +// ProxyUnsplashImage proxies an image from unsplash for privacy reasons. // @Summary Get an unsplash image // @Description Get an unsplash image. **Returns json on error.** // @tags project @@ -55,12 +118,8 @@ func unsplashImage(url string, c *echo.Context) error { // @Failure 500 {object} models.Message "Internal error" // @Router /backgrounds/unsplash/image/{image} [get] func ProxyUnsplashImage(c *echo.Context) error { - photo, err := getUnsplashPhotoInfoByID(c.Param("image")) - if err != nil { - return err - } - pingbackByPhotoID(photo.ID) - return unsplashImage(photo.Urls.Raw, c) + body, err := FetchUnsplashImageByID(c.Param("image")) + return streamUnsplashImage(body, err, c) } // ProxyUnsplashThumb proxies a thumbnail from unsplash for privacy reasons. @@ -75,10 +134,6 @@ func ProxyUnsplashImage(c *echo.Context) error { // @Failure 500 {object} models.Message "Internal error" // @Router /backgrounds/unsplash/image/{image}/thumb [get] func ProxyUnsplashThumb(c *echo.Context) error { - photo, err := getUnsplashPhotoInfoByID(c.Param("image")) - if err != nil { - return err - } - pingbackByPhotoID(photo.ID) - return unsplashImage("https://images.unsplash.com/"+getImageID(photo.Urls.Raw)+"?ixlib=rb-1.2.1&q=80&fm=jpg&crop=entropy&cs=tinysrgb&w=200&fit=max&ixid=eyJhcHBfaWQiOjcyODAwfQ", c) + body, err := FetchUnsplashThumbByID(c.Param("image")) + return streamUnsplashImage(body, err, c) } diff --git a/pkg/web/files/project_background.go b/pkg/web/files/project_background.go new file mode 100644 index 000000000..aeda725a7 --- /dev/null +++ b/pkg/web/files/project_background.go @@ -0,0 +1,55 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package files + +import ( + "io" + "net/http" + "os" + + "code.vikunja.io/api/pkg/files" +) + +// WriteProjectBackground streams a project's background file (its .File reader must be +// open) to the response, shared by the v1 and v2 background handlers. It does not close +// the reader; the caller owns it. +// +// The wire shape differs from WriteFileDownload on purpose and must stay byte-identical +// to v1: backgrounds are always served as image/jpg (no Content-Disposition, no +// Content-Length), with a cache-revalidation Last-Modified from the storage modtime +// rather than the file's DB Created timestamp. +func WriteProjectBackground(w http.ResponseWriter, r *http.Request, bgFile *files.File, stat os.FileInfo) { + // Override the global no-store directive so browsers can cache background images. + // no-cache allows caching but requires revalidation via If-Modified-Since. + w.Header().Set("Cache-Control", "no-cache") + + if stat != nil { + modTime := stat.ModTime().UTC() + w.Header().Set("Last-Modified", modTime.Format(http.TimeFormat)) + + if ifModSince := r.Header.Get("If-Modified-Since"); ifModSince != "" { + if t, err := http.ParseTime(ifModSince); err == nil && !modTime.After(t) { + w.WriteHeader(http.StatusNotModified) + return + } + } + } + + w.Header().Set("Content-Type", "image/jpg") + w.WriteHeader(http.StatusOK) + _, _ = io.Copy(w, bgFile.File) +}