From 029ab1eec9555d7d790d394ef6183bb526b952d0 Mon Sep 17 00:00:00 2001 From: Viktor Sokolov Date: Sun, 7 Sep 2025 23:54:28 +0200 Subject: [PATCH] Another attempt to DRY --- config.go | 8 +- handlers/{processing => }/config.go | 2 +- handlers/context.go | 24 +++ handlers/{processing => }/errors.go | 36 ++-- handlers/{processing => }/path.go | 6 +- handlers/{processing => }/path_test.go | 4 +- handlers/processing/handler.go | 114 ++++--------- handlers/processing/handler_test.go | 1 - handlers/processing/request.go | 81 ++++----- handlers/processing/request_methods.go | 225 +++++++++---------------- handlers/request.go | 178 +++++++++++++++++++ imgproxy.go | 80 ++++++--- integration/processing_handler_test.go | 2 +- 13 files changed, 426 insertions(+), 335 deletions(-) rename handlers/{processing => }/config.go (99%) create mode 100644 handlers/context.go rename handlers/{processing => }/errors.go (58%) rename handlers/{processing => }/path.go (90%) rename handlers/{processing => }/path_test.go (98%) delete mode 100644 handlers/processing/handler_test.go create mode 100644 handlers/request.go diff --git a/config.go b/config.go index 96b2160e..ef9f544c 100644 --- a/config.go +++ b/config.go @@ -4,7 +4,7 @@ import ( "github.com/imgproxy/imgproxy/v3/auximageprovider" "github.com/imgproxy/imgproxy/v3/ensure" "github.com/imgproxy/imgproxy/v3/fetcher" - processinghandler "github.com/imgproxy/imgproxy/v3/handlers/processing" + "github.com/imgproxy/imgproxy/v3/handlers" "github.com/imgproxy/imgproxy/v3/handlers/stream" "github.com/imgproxy/imgproxy/v3/headerwriter" "github.com/imgproxy/imgproxy/v3/semaphores" @@ -20,7 +20,7 @@ type Config struct { WatermarkImage auximageprovider.StaticConfig Transport transport.Config Fetcher fetcher.Config - ProcessingHandler processinghandler.Config + ProcessingHandler handlers.Config StreamHandler stream.Config Server server.Config } @@ -34,7 +34,7 @@ func NewDefaultConfig() Config { WatermarkImage: auximageprovider.NewDefaultStaticConfig(), Transport: transport.NewDefaultConfig(), Fetcher: fetcher.NewDefaultConfig(), - ProcessingHandler: processinghandler.NewDefaultConfig(), + ProcessingHandler: handlers.NewDefaultConfig(), StreamHandler: stream.NewDefaultConfig(), Server: server.NewDefaultConfig(), } @@ -74,7 +74,7 @@ func LoadConfigFromEnv(c *Config) (*Config, error) { return nil, err } - if _, err = processinghandler.LoadConfigFromEnv(&c.ProcessingHandler); err != nil { + if _, err = handlers.LoadConfigFromEnv(&c.ProcessingHandler); err != nil { return nil, err } diff --git a/handlers/processing/config.go b/handlers/config.go similarity index 99% rename from handlers/processing/config.go rename to handlers/config.go index 1569be48..20ba3e91 100644 --- a/handlers/processing/config.go +++ b/handlers/config.go @@ -1,4 +1,4 @@ -package processing +package handlers import ( "errors" diff --git a/handlers/context.go b/handlers/context.go new file mode 100644 index 00000000..f323f190 --- /dev/null +++ b/handlers/context.go @@ -0,0 +1,24 @@ +package handlers + +import ( + "github.com/imgproxy/imgproxy/v3/auximageprovider" + "github.com/imgproxy/imgproxy/v3/fetcher" + "github.com/imgproxy/imgproxy/v3/headerwriter" + "github.com/imgproxy/imgproxy/v3/imagedata" + "github.com/imgproxy/imgproxy/v3/semaphores" +) + +// Context defines the input interface handler needs to operate. +// In a nutshell, this interface strips ImgProxy definition from implementation. +// All the dependent components could share the same global interface. +// +// It might as well be implemented on the Handler struct itself, no matter. +// However, in this case, we'we got to implement it on every Handler struct. +type Context interface { + HeaderWriter() *headerwriter.Writer + Fetcher() *fetcher.Fetcher + Semaphores() *semaphores.Semaphores + FallbackImage() auximageprovider.Provider + WatermarkImage() auximageprovider.Provider + ImageDataFactory() *imagedata.Factory +} diff --git a/handlers/processing/errors.go b/handlers/errors.go similarity index 58% rename from handlers/processing/errors.go rename to handlers/errors.go index bf58d0d8..1c7cd2b1 100644 --- a/handlers/processing/errors.go +++ b/handlers/errors.go @@ -1,4 +1,4 @@ -package processing +package handlers import ( "fmt" @@ -10,15 +10,15 @@ import ( // Monitoring error categories const ( - categoryTimeout = "timeout" - categoryImageDataSize = "image_data_size" - categoryPathParsing = "path_parsing" - categorySecurity = "security" - categoryQueue = "queue" - categoryDownload = "download" - categoryProcessing = "processing" - categoryIO = "IO" - categoryConfig = "config(tmp)" // NOTE: THIS IS TEMPORARY + CategoryTimeout = "timeout" + CategoryImageDataSize = "image_data_size" + CategoryPathParsing = "path_parsing" + CategorySecurity = "security" + CategoryQueue = "queue" + CategoryDownload = "download" + CategoryProcessing = "processing" + CategoryIO = "IO" + CategoryConfig = "config(tmp)" // NOTE: THIS IS TEMPORARY ) type ( @@ -26,7 +26,7 @@ type ( InvalidURLError string ) -func newResponseWriteError(cause error) *ierrors.Error { +func NewResponseWriteError(cause error) *ierrors.Error { return ierrors.Wrap( ResponseWriteError{cause}, 1, @@ -42,7 +42,7 @@ func (e ResponseWriteError) Unwrap() error { return e.error } -func newInvalidURLErrorf(status int, format string, args ...interface{}) error { +func NewInvalidURLErrorf(status int, format string, args ...interface{}) error { return ierrors.Wrap( InvalidURLError(fmt.Sprintf(format, args...)), 1, @@ -55,17 +55,17 @@ func newInvalidURLErrorf(status int, format string, args ...interface{}) error { func (e InvalidURLError) Error() string { return string(e) } // newCantSaveError creates "resulting image not supported" error -func newCantSaveError(format imagetype.Type) error { - return ierrors.Wrap(newInvalidURLErrorf( +func NewCantSaveError(format imagetype.Type) error { + return ierrors.Wrap(NewInvalidURLErrorf( http.StatusUnprocessableEntity, "Resulting image format is not supported: %s", format, - ), 1, ierrors.WithCategory(categoryPathParsing)) + ), 1, ierrors.WithCategory(CategoryPathParsing)) } // newCantLoadError creates "source image not supported" error -func newCantLoadError(format imagetype.Type) error { - return ierrors.Wrap(newInvalidURLErrorf( +func NewCantLoadError(format imagetype.Type) error { + return ierrors.Wrap(NewInvalidURLErrorf( http.StatusUnprocessableEntity, "Source image format is not supported: %s", format, - ), 1, ierrors.WithCategory(categoryProcessing)) + ), 1, ierrors.WithCategory(CategoryProcessing)) } diff --git a/handlers/processing/path.go b/handlers/path.go similarity index 90% rename from handlers/processing/path.go rename to handlers/path.go index be6c5d64..5988ca86 100644 --- a/handlers/processing/path.go +++ b/handlers/path.go @@ -1,4 +1,4 @@ -package processing +package handlers import ( "fmt" @@ -30,8 +30,8 @@ func splitPathSignature(r *http.Request, config *Config) (string, string, error) signature, path, _ := strings.Cut(uri, "/") if len(signature) == 0 || len(path) == 0 { return "", "", ierrors.Wrap( - newInvalidURLErrorf(http.StatusNotFound, "Invalid path: %s", path), 0, - ierrors.WithCategory(categoryPathParsing), + NewInvalidURLErrorf(http.StatusNotFound, "Invalid path: %s", path), 0, + ierrors.WithCategory(CategoryPathParsing), ) } diff --git a/handlers/processing/path_test.go b/handlers/path_test.go similarity index 98% rename from handlers/processing/path_test.go rename to handlers/path_test.go index 12e19f60..ac68aabe 100644 --- a/handlers/processing/path_test.go +++ b/handlers/path_test.go @@ -1,4 +1,4 @@ -package processing +package handlers import ( "net/http" @@ -96,7 +96,7 @@ func (s *PathTestSuite) TestParsePath() { s.Require().Error(err) s.Require().ErrorAs(err, &ierr) - s.Require().Equal(categoryPathParsing, ierr.Category()) + s.Require().Equal(CategoryPathParsing, ierr.Category()) return } diff --git a/handlers/processing/handler.go b/handlers/processing/handler.go index 130db1a2..2f84b102 100644 --- a/handlers/processing/handler.go +++ b/handlers/processing/handler.go @@ -3,54 +3,40 @@ package processing import ( "context" "net/http" - "net/url" - "github.com/imgproxy/imgproxy/v3/auximageprovider" - "github.com/imgproxy/imgproxy/v3/errorreport" + "github.com/imgproxy/imgproxy/v3/handlers" "github.com/imgproxy/imgproxy/v3/handlers/stream" - "github.com/imgproxy/imgproxy/v3/headerwriter" - "github.com/imgproxy/imgproxy/v3/ierrors" - "github.com/imgproxy/imgproxy/v3/imagedata" "github.com/imgproxy/imgproxy/v3/monitoring" "github.com/imgproxy/imgproxy/v3/monitoring/stats" "github.com/imgproxy/imgproxy/v3/options" - "github.com/imgproxy/imgproxy/v3/security" - "github.com/imgproxy/imgproxy/v3/semaphores" ) // Handler handles image processing requests type Handler struct { - hw *headerwriter.Writer // Configured HeaderWriter instance - stream *stream.Handler // Stream handler for raw image streaming - config *Config // Handler configuration - semaphores *semaphores.Semaphores - fallbackImage auximageprovider.Provider - watermarkImage auximageprovider.Provider - idf *imagedata.Factory + hCtx handlers.Context // Input context interface + stream *stream.Handler // Stream handler for raw image streaming + config *handlers.Config // Handler configuration +} + +type request struct { + *handlers.Request + Options *options.ProcessingOptions // Processing options extracted from URL } // New creates new handler object func New( + context handlers.Context, stream *stream.Handler, - hw *headerwriter.Writer, - semaphores *semaphores.Semaphores, - fi auximageprovider.Provider, - wi auximageprovider.Provider, - idf *imagedata.Factory, - config *Config, + config *handlers.Config, ) (*Handler, error) { if err := config.Validate(); err != nil { return nil, err } return &Handler{ - hw: hw, - config: config, - stream: stream, - semaphores: semaphores, - fallbackImage: fi, - watermarkImage: wi, - idf: idf, + hCtx: context, + config: config, + stream: stream, }, nil } @@ -66,57 +52,29 @@ func (h *Handler) Execute( ctx := imageRequest.Context() - // Verify URL signature and extract image url and processing options - imageURL, po, mm, err := h.newRequest(ctx, imageRequest) + r, po, err := handlers.NewRequest(h.hCtx, h, imageRequest, h.config, reqID, rw) if err != nil { return err } // if processing options indicate raw image streaming, stream it and return if po.Raw { - return h.stream.Execute(ctx, imageRequest, imageURL, reqID, po, rw) + return h.stream.Execute(ctx, imageRequest, r.ImageURL, reqID, po, rw) } req := &request{ - handler: h, - imageRequest: imageRequest, - reqID: reqID, - rw: rw, - config: h.config, - po: po, - imageURL: imageURL, - monitoringMeta: mm, - semaphores: h.semaphores, - hwr: h.hw.NewRequest(), - idf: h.idf, + Request: r, + Options: po, } - return req.execute(ctx) + return execute(ctx, req) } -// newRequest extracts image url and processing options from request URL and verifies them -func (h *Handler) newRequest( - ctx context.Context, - imageRequest *http.Request, -) (string, *options.ProcessingOptions, monitoring.Meta, error) { - // let's extract signature and valid request path from a request - path, signature, err := splitPathSignature(imageRequest, h.config) - if err != nil { - return "", nil, nil, err - } +func (h *Handler) ParsePath(path string, headers http.Header) (*options.ProcessingOptions, string, error) { + return options.ParsePath(path, headers) +} - // verify the signature (if any) - if err = security.VerifySignature(signature, path); err != nil { - return "", nil, nil, ierrors.Wrap(err, 0, ierrors.WithCategory(categorySecurity)) - } - - // parse image url and processing options - po, imageURL, err := options.ParsePath(path, imageRequest.Header) - if err != nil { - return "", nil, nil, ierrors.Wrap(err, 0, ierrors.WithCategory(categoryPathParsing)) - } - - // get image origin and create monitoring meta object +func (h *Handler) CreateMeta(ctx context.Context, imageURL string, po *options.ProcessingOptions) monitoring.Meta { imageOrigin := imageOrigin(imageURL) mm := monitoring.Meta{ @@ -125,27 +83,13 @@ func (h *Handler) newRequest( monitoring.MetaProcessingOptions: po.Diff().Flatten(), } - // set error reporting and monitoring context - errorreport.SetMetadata(imageRequest, "Source Image URL", imageURL) - errorreport.SetMetadata(imageRequest, "Source Image Origin", imageOrigin) - errorreport.SetMetadata(imageRequest, "Processing Options", po) - monitoring.SetMetadata(ctx, mm) - // verify that image URL came from the valid source - err = security.VerifySourceURL(imageURL) - if err != nil { - return "", nil, mm, ierrors.Wrap(err, 0, ierrors.WithCategory(categorySecurity)) - } + // NOTE: errorreport needs to be patched (just not in the context of this PR) + // set error reporting and monitoring context + // errorreport.SetMetadata(ctx, "Source Image URL", imageURL) + // errorreport.SetMetadata(ctx, "Source Image Origin", imageOrigin) + // errorreport.SetMetadata(ctx, "Processing Options", po) - return imageURL, po, mm, nil -} - -// imageOrigin extracts image origin from URL -func imageOrigin(imageURL string) string { - if u, uerr := url.Parse(imageURL); uerr == nil { - return u.Scheme + "://" + u.Host - } - - return "" + return mm } diff --git a/handlers/processing/handler_test.go b/handlers/processing/handler_test.go deleted file mode 100644 index ce50a100..00000000 --- a/handlers/processing/handler_test.go +++ /dev/null @@ -1 +0,0 @@ -package processing diff --git a/handlers/processing/request.go b/handlers/processing/request.go index ae35ae8a..61cffb06 100644 --- a/handlers/processing/request.go +++ b/handlers/processing/request.go @@ -4,55 +4,29 @@ import ( "context" "errors" "net/http" + "net/url" "github.com/imgproxy/imgproxy/v3/fetcher" - "github.com/imgproxy/imgproxy/v3/headerwriter" + "github.com/imgproxy/imgproxy/v3/handlers" "github.com/imgproxy/imgproxy/v3/ierrors" - "github.com/imgproxy/imgproxy/v3/imagedata" "github.com/imgproxy/imgproxy/v3/imagetype" - "github.com/imgproxy/imgproxy/v3/monitoring" "github.com/imgproxy/imgproxy/v3/monitoring/stats" - "github.com/imgproxy/imgproxy/v3/options" - "github.com/imgproxy/imgproxy/v3/semaphores" "github.com/imgproxy/imgproxy/v3/server" "github.com/imgproxy/imgproxy/v3/vips" ) -// request holds the parameters and state for a single request request -type request struct { - handler *Handler - imageRequest *http.Request - reqID string - rw http.ResponseWriter - config *Config - po *options.ProcessingOptions - imageURL string - monitoringMeta monitoring.Meta - semaphores *semaphores.Semaphores - hwr *headerwriter.Request - idf *imagedata.Factory -} - -// execute handles the actual processing logic -func (r *request) execute(ctx context.Context) error { +func execute(ctx context.Context, r *request) error { // Check if we can save the resulting image - canSave := vips.SupportsSave(r.po.Format) || - r.po.Format == imagetype.Unknown || - r.po.Format == imagetype.SVG + canSave := vips.SupportsSave(r.Options.Format) || + r.Options.Format == imagetype.Unknown || + r.Options.Format == imagetype.SVG if !canSave { - return newCantSaveError(r.po.Format) + return handlers.NewCantSaveError(r.Options.Format) } - // Acquire queue semaphore (if enabled) - releaseQueueSem, err := r.semaphores.AcquireQueue() - if err != nil { - return err - } - defer releaseQueueSem() - // Acquire processing semaphore - releaseProcessingSem, err := r.acquireProcessingSem(ctx) + releaseProcessingSem, err := r.AcquireProcessingSem(ctx) if err != nil { return err } @@ -66,37 +40,37 @@ func (r *request) execute(ctx context.Context) error { statusCode := http.StatusOK // Request headers - imgRequestHeaders := r.makeImageRequestHeaders() + imgRequestHeaders := r.MakeImageRequestHeaders() // create download options - do := r.makeDownloadOptions(ctx, imgRequestHeaders) + do := r.MakeDownloadOptions(imgRequestHeaders, r.Options.SecurityOptions) // Fetch image actual - originData, originHeaders, err := r.fetchImage(ctx, do) + originData, originHeaders, err := r.FetchImage(ctx, do) if err == nil { defer originData.Close() // if any originData has been opened, we need to close it } // Check that image detection didn't take too long if terr := server.CheckTimeout(ctx); terr != nil { - return ierrors.Wrap(terr, 0, ierrors.WithCategory(categoryTimeout)) + return ierrors.Wrap(terr, 0, ierrors.WithCategory(handlers.CategoryTimeout)) } // Respond with NotModified if image was not modified var nmErr fetcher.NotModifiedError if errors.As(err, &nmErr) { - r.hwr.SetOriginHeaders(nmErr.Headers()) + r.HeaderWriter.SetOriginHeaders(nmErr.Headers()) - return r.respondWithNotModified() + return respondWithNotModified(r) } // Prepare to write image response headers - r.hwr.SetOriginHeaders(originHeaders) + r.HeaderWriter.SetOriginHeaders(originHeaders) // If error is not related to NotModified, respond with fallback image and replace image data if err != nil { - originData, statusCode, err = r.handleDownloadError(ctx, err) + originData, statusCode, err = handleDownloadError(ctx, r, err) if err != nil { return err } @@ -104,11 +78,11 @@ func (r *request) execute(ctx context.Context) error { // Check if image supports load from origin format if !vips.SupportsLoad(originData.Format()) { - return newCantLoadError(originData.Format()) + return handlers.NewCantLoadError(originData.Format()) } // Actually process the image - result, err := r.processImage(ctx, originData) + result, err := processImage(ctx, r, originData) // Let's close resulting image data only if it differs from the source image data if result != nil && result.OutData != nil && result.OutData != originData { @@ -117,26 +91,35 @@ func (r *request) execute(ctx context.Context) error { // First, check if the processing error wasn't caused by an image data error if derr := originData.Error(); derr != nil { - return ierrors.Wrap(derr, 0, ierrors.WithCategory(categoryDownload)) + return ierrors.Wrap(derr, 0, ierrors.WithCategory(handlers.CategoryDownload)) } // If it wasn't, than it was a processing error if err != nil { - return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryProcessing)) + return ierrors.Wrap(err, 0, ierrors.WithCategory(handlers.CategoryProcessing)) } // Write debug headers. It seems unlogical to move they to headerwriter since they're // not used anywhere else. - err = r.writeDebugHeaders(result, originData) + err = writeDebugHeaders(r, result, originData) if err != nil { - return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryImageDataSize)) + return ierrors.Wrap(err, 0, ierrors.WithCategory(handlers.CategoryImageDataSize)) } // Responde with actual image - err = r.respondWithImage(statusCode, result.OutData) + err = respondWithImage(r, statusCode, result.OutData) if err != nil { return err } return nil } + +// imageOrigin extracts image origin from URL +func imageOrigin(imageURL string) string { + if u, uerr := url.Parse(imageURL); uerr == nil { + return u.Scheme + "://" + u.Host + } + + return "" +} diff --git a/handlers/processing/request_methods.go b/handlers/processing/request_methods.go index b692e981..e3bf9aea 100644 --- a/handlers/processing/request_methods.go +++ b/handlers/processing/request_methods.go @@ -6,114 +6,48 @@ import ( "net/http" "strconv" - "github.com/imgproxy/imgproxy/v3/cookies" "github.com/imgproxy/imgproxy/v3/errorreport" + "github.com/imgproxy/imgproxy/v3/handlers" "github.com/imgproxy/imgproxy/v3/httpheaders" "github.com/imgproxy/imgproxy/v3/ierrors" "github.com/imgproxy/imgproxy/v3/imagedata" "github.com/imgproxy/imgproxy/v3/monitoring" - "github.com/imgproxy/imgproxy/v3/options" "github.com/imgproxy/imgproxy/v3/processing" "github.com/imgproxy/imgproxy/v3/server" log "github.com/sirupsen/logrus" ) -// makeImageRequestHeaders creates headers for the image request -func (r *request) makeImageRequestHeaders() http.Header { - h := make(http.Header) - - // If ETag is enabled, we forward If-None-Match header - if r.config.ETagEnabled { - h.Set(httpheaders.IfNoneMatch, r.imageRequest.Header.Get(httpheaders.IfNoneMatch)) - } - - // If LastModified is enabled, we forward If-Modified-Since header - if r.config.LastModifiedEnabled { - h.Set(httpheaders.IfModifiedSince, r.imageRequest.Header.Get(httpheaders.IfModifiedSince)) - } - - return h -} - -// acquireProcessingSem acquires the processing semaphore -func (r *request) acquireProcessingSem(ctx context.Context) (context.CancelFunc, error) { - defer monitoring.StartQueueSegment(ctx)() - - fn, err := r.semaphores.AcquireProcessing(ctx) - if err != nil { - // We don't actually need to check timeout here, - // but it's an easy way to check if this is an actual timeout - // or the request was canceled - if terr := server.CheckTimeout(ctx); terr != nil { - return nil, ierrors.Wrap(terr, 0, ierrors.WithCategory(categoryTimeout)) - } - - // We should never reach this line as err could be only ctx.Err() - // and we've already checked for it. But beter safe than sorry - return nil, ierrors.Wrap(err, 0, ierrors.WithCategory(categoryQueue)) - } - - return fn, nil -} - -// makeDownloadOptions creates a new default download options -func (r *request) makeDownloadOptions(ctx context.Context, h http.Header) imagedata.DownloadOptions { - downloadFinished := monitoring.StartDownloadingSegment(ctx, r.monitoringMeta.Filter( - monitoring.MetaSourceImageURL, - monitoring.MetaSourceImageOrigin, - )) - - return imagedata.DownloadOptions{ - Header: h, - MaxSrcFileSize: r.po.SecurityOptions.MaxSrcFileSize, - DownloadFinished: downloadFinished, - } -} - -// fetchImage downloads the source image asynchronously -func (r *request) fetchImage(ctx context.Context, do imagedata.DownloadOptions) (imagedata.ImageData, http.Header, error) { - var err error - - if r.config.CookiePassthrough { - do.CookieJar, err = cookies.JarFromRequest(r.imageRequest) - if err != nil { - return nil, nil, ierrors.Wrap(err, 0, ierrors.WithCategory(categoryDownload)) - } - } - - return r.idf.DownloadAsync(ctx, r.imageURL, "source image", do) -} - // handleDownloadError replaces the image data with fallback image if needed -func (r *request) handleDownloadError( +func handleDownloadError( ctx context.Context, + r *request, originalErr error, ) (imagedata.ImageData, int, error) { - err := r.wrapDownloadingErr(originalErr) + err := r.WrapDownloadingErr(originalErr) // If there is no fallback image configured, just return the error - data, headers := r.getFallbackImage(ctx, r.po) + data, headers := getFallbackImage(ctx, r) if data == nil { return nil, 0, err } // Just send error - monitoring.SendError(ctx, categoryDownload, err) + monitoring.SendError(ctx, handlers.CategoryDownload, err) // We didn't return, so we have to report error if err.ShouldReport() { - errorreport.Report(err, r.imageRequest) + errorreport.Report(err, r.Req) } log. - WithField("request_id", r.reqID). - Warningf("Could not load image %s. Using fallback image. %s", r.imageURL, err.Error()) + WithField("request_id", r.ID). + Warningf("Could not load image %s. Using fallback image. %s", r.ImageURL, err.Error()) var statusCode int // Set status code if needed - if r.config.FallbackImageHTTPCode > 0 { - statusCode = r.config.FallbackImageHTTPCode + if r.Config.FallbackImageHTTPCode > 0 { + statusCode = r.Config.FallbackImageHTTPCode } else { statusCode = err.StatusCode() } @@ -122,27 +56,27 @@ func (r *request) handleDownloadError( headers.Del(httpheaders.Expires) headers.Del(httpheaders.LastModified) - r.hwr.SetOriginHeaders(headers) - r.hwr.SetIsFallbackImage() + r.HeaderWriter.SetOriginHeaders(headers) + r.HeaderWriter.SetIsFallbackImage() return data, statusCode, nil } // getFallbackImage returns fallback image if any -func (r *request) getFallbackImage( +func getFallbackImage( ctx context.Context, - po *options.ProcessingOptions, + r *request, ) (imagedata.ImageData, http.Header) { - if r.handler.fallbackImage == nil { + if r.Context.FallbackImage() == nil { return nil, nil } - data, h, err := r.handler.fallbackImage.Get(ctx, po) + data, h, err := r.Context.FallbackImage().Get(ctx, r.Options) if err != nil { log.Warning(err.Error()) - if ierr := r.wrapDownloadingErr(err); ierr.ShouldReport() { - errorreport.Report(ierr, r.imageRequest) + if ierr := r.WrapDownloadingErr(err); ierr.ShouldReport() { + errorreport.Report(ierr, r.Req) } return nil, nil @@ -152,127 +86,130 @@ func (r *request) getFallbackImage( } // processImage calls actual image processing -func (r *request) processImage(ctx context.Context, originData imagedata.ImageData) (*processing.Result, error) { - defer monitoring.StartProcessingSegment(ctx, r.monitoringMeta.Filter(monitoring.MetaProcessingOptions))() - return processing.ProcessImage(ctx, originData, r.po, r.handler.watermarkImage, r.handler.idf) +func processImage( + ctx context.Context, + r *request, + originData imagedata.ImageData, +) (*processing.Result, error) { + defer monitoring.StartProcessingSegment( + ctx, + r.MonitoringMeta.Filter(monitoring.MetaProcessingOptions), + )() + return processing.ProcessImage(ctx, originData, r.Options, r.Context.WatermarkImage(), r.Context.ImageDataFactory()) } // writeDebugHeaders writes debug headers (X-Origin-*, X-Result-*) to the response -func (r *request) writeDebugHeaders(result *processing.Result, originData imagedata.ImageData) error { - if !r.config.EnableDebugHeaders { +func writeDebugHeaders( + r *request, + result *processing.Result, + originData imagedata.ImageData, +) error { + if !r.Config.EnableDebugHeaders { return nil } if result != nil { - r.rw.Header().Set(httpheaders.XOriginWidth, strconv.Itoa(result.OriginWidth)) - r.rw.Header().Set(httpheaders.XOriginHeight, strconv.Itoa(result.OriginHeight)) - r.rw.Header().Set(httpheaders.XResultWidth, strconv.Itoa(result.ResultWidth)) - r.rw.Header().Set(httpheaders.XResultHeight, strconv.Itoa(result.ResultHeight)) + r.ResponseWriter.Header().Set(httpheaders.XOriginWidth, strconv.Itoa(result.OriginWidth)) + r.ResponseWriter.Header().Set(httpheaders.XOriginHeight, strconv.Itoa(result.OriginHeight)) + r.ResponseWriter.Header().Set(httpheaders.XResultWidth, strconv.Itoa(result.ResultWidth)) + r.ResponseWriter.Header().Set(httpheaders.XResultHeight, strconv.Itoa(result.ResultHeight)) } // Try to read origin image size size, err := originData.Size() if err != nil { - return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryImageDataSize)) + return ierrors.Wrap(err, 0, ierrors.WithCategory(handlers.CategoryImageDataSize)) } - r.rw.Header().Set(httpheaders.XOriginContentLength, strconv.Itoa(size)) + r.ResponseWriter.Header().Set(httpheaders.XOriginContentLength, strconv.Itoa(size)) return nil } // respondWithNotModified writes not-modified response -func (r *request) respondWithNotModified() error { - r.hwr.SetExpires(r.po.Expires) - r.hwr.SetVary() +func respondWithNotModified(r *request) error { + r.HeaderWriter.SetExpires(r.Options.Expires) + r.HeaderWriter.SetVary() - if r.config.LastModifiedEnabled { - r.hwr.Passthrough(httpheaders.LastModified) + if r.Config.LastModifiedEnabled { + r.HeaderWriter.Passthrough(httpheaders.LastModified) } - if r.config.ETagEnabled { - r.hwr.Passthrough(httpheaders.Etag) + if r.Config.ETagEnabled { + r.HeaderWriter.Passthrough(httpheaders.Etag) } - r.hwr.Write(r.rw) + r.HeaderWriter.Write(r.ResponseWriter) - r.rw.WriteHeader(http.StatusNotModified) + r.ResponseWriter.WriteHeader(http.StatusNotModified) server.LogResponse( - r.reqID, r.imageRequest, http.StatusNotModified, nil, + r.ID, r.Req, http.StatusNotModified, nil, log.Fields{ - "image_url": r.imageURL, - "processing_options": r.po, + "image_url": r.ImageURL, + "processing_options": r.Options, }, ) return nil } -func (r *request) respondWithImage(statusCode int, resultData imagedata.ImageData) error { +func respondWithImage(r *request, statusCode int, resultData imagedata.ImageData) error { // We read the size of the image data here, so we can set Content-Length header. // This indireclty ensures that the image data is fully read from the source, no // errors happened. resultSize, err := resultData.Size() if err != nil { - return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryImageDataSize)) + return ierrors.Wrap(err, 0, ierrors.WithCategory(handlers.CategoryImageDataSize)) } - r.hwr.SetContentType(resultData.Format().Mime()) - r.hwr.SetContentLength(resultSize) - r.hwr.SetContentDisposition( - r.imageURL, - r.po.Filename, + r.HeaderWriter.SetContentType(resultData.Format().Mime()) + r.HeaderWriter.SetContentLength(resultSize) + r.HeaderWriter.SetContentDisposition( + r.ImageURL, + r.Options.Filename, resultData.Format().Ext(), "", - r.po.ReturnAttachment, + r.Options.ReturnAttachment, ) - r.hwr.SetExpires(r.po.Expires) - r.hwr.SetVary() - r.hwr.SetCanonical(r.imageURL) + r.HeaderWriter.SetExpires(r.Options.Expires) + r.HeaderWriter.SetVary() + r.HeaderWriter.SetCanonical(r.ImageURL) - if r.config.LastModifiedEnabled { - r.hwr.Passthrough(httpheaders.LastModified) + if r.Config.LastModifiedEnabled { + r.HeaderWriter.Passthrough(httpheaders.LastModified) } - if r.config.ETagEnabled { - r.hwr.Passthrough(httpheaders.Etag) + if r.Config.ETagEnabled { + r.HeaderWriter.Passthrough(httpheaders.Etag) } - r.hwr.Write(r.rw) + r.HeaderWriter.Write(r.ResponseWriter) - r.rw.WriteHeader(statusCode) + r.ResponseWriter.WriteHeader(statusCode) - _, err = io.Copy(r.rw, resultData.Reader()) + _, err = io.Copy(r.ResponseWriter, resultData.Reader()) var ierr *ierrors.Error if err != nil { - ierr = newResponseWriteError(err) + ierr = handlers.NewResponseWriteError(err) - if r.config.ReportIOErrors { - return ierrors.Wrap(ierr, 0, ierrors.WithCategory(categoryIO), ierrors.WithShouldReport(true)) + if r.Config.ReportIOErrors { + return ierrors.Wrap( + ierr, 0, + ierrors.WithCategory(handlers.CategoryIO), + ierrors.WithShouldReport(true), + ) } } server.LogResponse( - r.reqID, r.imageRequest, statusCode, ierr, + r.ID, r.Req, statusCode, ierr, log.Fields{ - "image_url": r.imageURL, - "processing_options": r.po, + "image_url": r.ImageURL, + "processing_options": r.Options, }, ) return nil } - -// wrapDownloadingErr wraps original error to download error -func (r *request) wrapDownloadingErr(originalErr error) *ierrors.Error { - err := ierrors.Wrap(originalErr, 0, ierrors.WithCategory(categoryDownload)) - - // we report this error only if enabled - if r.config.ReportDownloadingErrors { - err = ierrors.Wrap(err, 0, ierrors.WithShouldReport(true)) - } - - return err -} diff --git a/handlers/request.go b/handlers/request.go new file mode 100644 index 00000000..091ddeaf --- /dev/null +++ b/handlers/request.go @@ -0,0 +1,178 @@ +package handlers + +import ( + "context" + "net/http" + + "github.com/imgproxy/imgproxy/v3/cookies" + "github.com/imgproxy/imgproxy/v3/headerwriter" + "github.com/imgproxy/imgproxy/v3/httpheaders" + "github.com/imgproxy/imgproxy/v3/ierrors" + "github.com/imgproxy/imgproxy/v3/imagedata" + "github.com/imgproxy/imgproxy/v3/monitoring" + "github.com/imgproxy/imgproxy/v3/security" + "github.com/imgproxy/imgproxy/v3/server" + "github.com/imgproxy/imgproxy/v3/structdiff" +) + +// Options is an object of URL options extracted from the URL +type Options = structdiff.Diffable + +// PathPaser is an interface for URL path parser: it extracts processing options and image path +type Constructor[O Options] interface { + ParsePath(path string, headers http.Header) (O, string, error) + CreateMeta(ctx context.Context, imageURL string, po O) monitoring.Meta +} + +type Request struct { + Context Context // Input context interface + Config *Config // Handler configuration + ID string // Request ID + Req *http.Request // Original HTTP request + ResponseWriter http.ResponseWriter // HTTP response writer + HeaderWriter *headerwriter.Request // Header writer request + ImageURL string // Image URL to process + MonitoringMeta monitoring.Meta // Monitoring metadata +} + +// PrepareRequest extracts image url and processing options from request URL and verifies them +func NewRequest[P Constructor[O], O Options]( + handler Context, // or, essentially, instance + constructor P, + imageRequest *http.Request, + config *Config, + reqID string, + rw http.ResponseWriter, +) (*Request, O, error) { + // let's extract signature and valid request path from a request + path, signature, err := splitPathSignature(imageRequest, config) + if err != nil { + return nil, *new(O), err + } + + // verify the signature (if any) + if err = security.VerifySignature(signature, path); err != nil { + return nil, *new(O), ierrors.Wrap(err, 0, ierrors.WithCategory(CategorySecurity)) + } + + // parse image url and processing options + po, imageURL, err := constructor.ParsePath(path, imageRequest.Header) + if err != nil { + return nil, *new(O), ierrors.Wrap(err, 0, ierrors.WithCategory(CategoryPathParsing)) + } + + mm := constructor.CreateMeta(imageRequest.Context(), imageURL, po) + + // verify that image URL came from the valid source + err = security.VerifySourceURL(imageURL) + if err != nil { + return nil, *new(O), ierrors.Wrap(err, 0, ierrors.WithCategory(CategorySecurity)) + } + + return &Request{ + Context: handler, + Config: config, + ID: reqID, + Req: imageRequest, + ResponseWriter: rw, + HeaderWriter: handler.HeaderWriter().NewRequest(), + ImageURL: imageURL, + MonitoringMeta: mm, + }, po, nil +} + +// MakeDownloadOptions creates [imagedata.DownloadOptions] +// from image request headers and security options. +func (r *Request) MakeDownloadOptions( + h http.Header, + secops security.Options, +) imagedata.DownloadOptions { + return imagedata.DownloadOptions{ + Header: h, + MaxSrcFileSize: secops.MaxSrcFileSize, + } +} + +// AcquireProcessingSem acquires the processing semaphore. +// It allows as many concurrent processing requests as workers are configured. +func (r *Request) AcquireProcessingSem(ctx context.Context) (context.CancelFunc, error) { + defer monitoring.StartQueueSegment(ctx)() + + sem := r.Context.Semaphores() + + // Acquire queue semaphore (if enabled) + releaseQueueSem, err := sem.AcquireQueue() + if err != nil { + return nil, err + } + // Defer releasing the queue semaphore since we'll exit the queue on return + defer releaseQueueSem() + + // Acquire processing semaphore + releaseProcessingSem, err := sem.AcquireProcessing(ctx) + if err != nil { + // We don't actually need to check timeout here, + // but it's an easy way to check if this is an actual timeout + // or the request was canceled + if terr := server.CheckTimeout(ctx); terr != nil { + return nil, ierrors.Wrap(terr, 0, ierrors.WithCategory(CategoryTimeout)) + } + + // We should never reach this line as err could be only ctx.Err() + // and we've already checked for it. But beter safe than sorry + return nil, ierrors.Wrap(err, 0, ierrors.WithCategory(CategoryQueue)) + } + + return releaseProcessingSem, nil +} + +// MakeImageRequestHeaders creates headers for the image request +func (r *Request) MakeImageRequestHeaders() http.Header { + h := make(http.Header) + + // If ETag is enabled, we forward If-None-Match header + if r.Config.ETagEnabled { + h.Set(httpheaders.IfNoneMatch, r.Req.Header.Get(httpheaders.IfNoneMatch)) + } + + // If LastModified is enabled, we forward If-Modified-Since header + if r.Config.LastModifiedEnabled { + h.Set(httpheaders.IfModifiedSince, r.Req.Header.Get(httpheaders.IfModifiedSince)) + } + + return h +} + +// FetchImage downloads the source image asynchronously +func (r *Request) FetchImage( + ctx context.Context, + do imagedata.DownloadOptions, +) (imagedata.ImageData, http.Header, error) { + do.DownloadFinished = monitoring.StartDownloadingSegment(ctx, r.MonitoringMeta.Filter( + monitoring.MetaSourceImageURL, + monitoring.MetaSourceImageOrigin, + )) + + var err error + + if r.Config.CookiePassthrough { + do.CookieJar, err = cookies.JarFromRequest(r.Req) + if err != nil { + return nil, nil, ierrors.Wrap(err, 0, ierrors.WithCategory(CategoryDownload)) + } + } + + return r.Context.ImageDataFactory().DownloadAsync(ctx, r.ImageURL, "source image", do) +} + +// WrapDownloadingErr wraps original error to download error +func (r *Request) WrapDownloadingErr(originalErr error) *ierrors.Error { + err := ierrors.Wrap(originalErr, 0, ierrors.WithCategory(CategoryDownload)) + + // we report this error only if enabled + if r.Config.ReportDownloadingErrors { + err = ierrors.Wrap(err, 0, ierrors.WithShouldReport(true)) + } + + return err +} diff --git a/imgproxy.go b/imgproxy.go index dcd5db06..bf149a64 100644 --- a/imgproxy.go +++ b/imgproxy.go @@ -31,19 +31,21 @@ const ( // ImgProxy holds all the components needed for imgproxy to function type ImgProxy struct { - HeaderWriter *headerwriter.Writer - Semaphores *semaphores.Semaphores - FallbackImage auximageprovider.Provider - WatermarkImage auximageprovider.Provider - Fetcher *fetcher.Fetcher - ProcessingHandler *processinghandler.Handler - StreamHandler *stream.Handler - ImageDataFactory *imagedata.Factory - Config *Config + headerWriter *headerwriter.Writer + semaphores *semaphores.Semaphores + fallbackImage auximageprovider.Provider + watermarkImage auximageprovider.Provider + fetcher *fetcher.Fetcher + processingHandler *processinghandler.Handler + streamHandler *stream.Handler + imageDataFactory *imagedata.Factory + config *Config } // New creates a new imgproxy instance func New(ctx context.Context, config *Config) (*ImgProxy, error) { + i := &ImgProxy{} + headerWriter, err := headerwriter.New(&config.HeaderWriter) if err != nil { return nil, err @@ -82,7 +84,7 @@ func New(ctx context.Context, config *Config) (*ImgProxy, error) { } ph, err := processinghandler.New( - streamHandler, headerWriter, semaphores, fallbackImage, watermarkImage, idf, &config.ProcessingHandler, + i, streamHandler, &config.ProcessingHandler, ) if err != nil { return nil, err @@ -103,22 +105,22 @@ func New(ctx context.Context, config *Config) (*ImgProxy, error) { return nil, err } - return &ImgProxy{ - HeaderWriter: headerWriter, - Semaphores: semaphores, - FallbackImage: fallbackImage, - WatermarkImage: watermarkImage, - Fetcher: fetcher, - StreamHandler: streamHandler, - ProcessingHandler: ph, - ImageDataFactory: idf, - Config: config, - }, nil + i.headerWriter = headerWriter + i.semaphores = semaphores + i.fallbackImage = fallbackImage + i.watermarkImage = watermarkImage + i.fetcher = fetcher + i.processingHandler = ph + i.streamHandler = streamHandler + i.imageDataFactory = idf + i.config = config + + return i, nil } // BuildRouter sets up the HTTP routes and middleware func (i *ImgProxy) BuildRouter() (*server.Router, error) { - r, err := server.NewRouter(&i.Config.Server) + r, err := server.NewRouter(&i.config.Server) if err != nil { return nil, err } @@ -128,12 +130,12 @@ func (i *ImgProxy) BuildRouter() (*server.Router, error) { r.GET(faviconPath, r.NotFoundHandler).Silent() r.GET(healthPath, handlers.HealthHandler).Silent() - if i.Config.Server.HealthCheckPath != "" { - r.GET(i.Config.Server.HealthCheckPath, handlers.HealthHandler).Silent() + if i.config.Server.HealthCheckPath != "" { + r.GET(i.config.Server.HealthCheckPath, handlers.HealthHandler).Silent() } r.GET( - "/*", i.ProcessingHandler.Execute, + "/*", i.processingHandler.Execute, r.WithSecret, r.WithCORS, r.WithPanic, r.WithReportError, r.WithMonitoring, ) @@ -171,7 +173,7 @@ func (i *ImgProxy) StartServer(ctx context.Context) error { // startMemoryTicker starts a ticker that periodically frees memory and optionally logs memory stats func (i *ImgProxy) startMemoryTicker(ctx context.Context) { - ticker := time.NewTicker(i.Config.Server.FreeMemoryInterval) + ticker := time.NewTicker(i.config.Server.FreeMemoryInterval) defer ticker.Stop() for { @@ -181,9 +183,33 @@ func (i *ImgProxy) startMemoryTicker(ctx context.Context) { case <-ticker.C: memory.Free() - if i.Config.Server.LogMemStats { + if i.config.Server.LogMemStats { memory.LogStats() } } } } + +func (i *ImgProxy) HeaderWriter() *headerwriter.Writer { + return i.headerWriter +} + +func (i *ImgProxy) Semaphores() *semaphores.Semaphores { + return i.semaphores +} + +func (i *ImgProxy) FallbackImage() auximageprovider.Provider { + return i.fallbackImage +} + +func (i *ImgProxy) WatermarkImage() auximageprovider.Provider { + return i.watermarkImage +} + +func (i *ImgProxy) Fetcher() *fetcher.Fetcher { + return i.fetcher +} + +func (i *ImgProxy) ImageDataFactory() *imagedata.Factory { + return i.imageDataFactory +} diff --git a/integration/processing_handler_test.go b/integration/processing_handler_test.go index b3329708..b0c1eb87 100644 --- a/integration/processing_handler_test.go +++ b/integration/processing_handler_test.go @@ -267,7 +267,7 @@ func (s *ProcessingHandlerTestSuite) TestSkipProcessingSVG() { s.Require().Equal(http.StatusOK, res.StatusCode) - data, err := s.imgproxy().ImageDataFactory.NewFromBytes(s.testData.Read("test1.svg")) + data, err := s.imgproxy().ImageDataFactory().NewFromBytes(s.testData.Read("test1.svg")) s.Require().NoError(err) expected, err := svg.Sanitize(data)