Catch libvips errors

+ more reliable way to check if the image is too big to download.

app/config.example.lua

@@ -21,7 +21,7 @@ return {
             read = 15000,
         },
         -- Number describing the max image size to receive (in bytes). Use 0 for no limits.
-        max_image_size = 0,
+        max_image_size = 104857600, -- 100 MB
         -- Number describing the maximum number of allowed redirects.
         max_redirects = 10,
         -- Allowed mime types. Use empty table to allow all mime types

spec/api_spec.lua

@@ -154,5 +154,29 @@ describe("api", function()
             -- Log not readable errors
             assert.equal(1, #ngx._logs)
         end)
+
+        it("libvips error", function()
+            api:add_manipulator(require "weserv.manipulators.thumbnail")
+
+            -- Create a unique file (starting with 'imo_') in our shared memory.
+            local tmpfile = utils.tempname("/dev/shm", "imo_")
+
+            -- Write a 1x1 svg image
+            local f = assert(io.open(tmpfile, "w"))
+            f:write("<svg width=\"1\" height=\"1\"></svg>")
+            f:close()
+
+            local image, api_err = api:process(tmpfile, {
+                url = "http://example.org/1x1.svg",
+                w = '10000001', -- VIPS_MAX_COORD + 1
+            })
+            assert.falsy(image)
+            assert.equal(404, api_err.status)
+            assert.truthy(api_err.message:find("parameter width not set"))
+            assert(os.remove(tmpfile))
+
+            -- Log libvips errors
+            assert.equal(1, #ngx._logs)
+        end)
     end)
 end)

spec/client_spec.lua

@@ -14,32 +14,27 @@ describe("client", function()
             send = 5000,
             read = 10000,
         },
-        max_image_size = 0,
+        max_image_size = 24,
         max_redirects = 10,
         allowed_mime_types = {}
     }
 
     local errors = {}
 
-    local reader_co = coroutine.create(function(max_chunk_size)
-        coroutine.yield("Chunk 1 " .. max_chunk_size .. "\n")
-        coroutine.yield("Chunk 2 " .. max_chunk_size .. "\n")
-        coroutine.yield("Chunk 3 " .. max_chunk_size .. "\n")
-        coroutine.yield(nil)
-    end)
-
-    local body_reader = function(...)
-        if coroutine.status(reader_co) == "suspended" then
-            return select(2, coroutine.resume(reader_co, ...))
-        else
-            return nil, "can't resume a " .. coroutine.status(reader_co) .. " coroutine"
-        end
+    local body_reader = function(file_chunks)
+        return coroutine.wrap(function()
+            for i = 1, file_chunks do
+                coroutine.yield("Chunk " .. i .. "\n")
+            end
+            coroutine.yield(nil)
+        end)
     end
 
+
     local response = {
         status = 200,
         headers = {},
-        body_reader = body_reader
+        body_reader = body_reader(3)
     }
 
     local tempname = true
@@ -133,7 +128,7 @@ describe("client", function()
         response = {
             status = 200,
             headers = {},
-            body_reader = body_reader
+            body_reader = body_reader(3)
         }
     end)
 
@@ -165,7 +160,7 @@ describe("client", function()
             local t = f:read("*all")
             f:close()
 
-            assert.equal("Chunk 1 65536\nChunk 2 65536\nChunk 3 65536\n", t)
+            assert.equal("Chunk 1\nChunk 2\nChunk 3\n", t)
             assert(os.remove(res.tmpfile))
         end)
 
@@ -176,7 +171,7 @@ describe("client", function()
             assert.equal("Unable to parse URL", err.message)
             assert.falsy(res)
 
-            -- Don"t log invalid uris
+            -- Don't log invalid uris
             assert.equal(0, #ngx._logs)
         end)
 
@@ -194,7 +189,7 @@ describe("client", function()
             assert.spy(stubbed_http.request).was_not_called()
             assert.spy(stubbed_http.set_keepalive).was_not_called()
 
-            -- Don"t log connect errors
+            -- Don't log connect errors
             assert.equal(0, #ngx._logs)
         end)
 
@@ -234,10 +229,28 @@ describe("client", function()
             -- in buffer` error from set_keepalive for the next request.
             assert.spy(stubbed_http.close).was.called()
 
-            -- Don"t log request errors
+            -- Don't log request errors
             assert.equal(0, #ngx._logs)
         end)
 
+        it("max image size error", function()
+            response.body_reader = body_reader(4)
+
+            local valid, invalid_err = client:request("https://ory.weserv.nl/big_image.jpg")
+
+            assert.spy(stubbed_http.connect).was.called()
+            assert.spy(stubbed_http.ssl_handshake).was.called()
+            assert.spy(stubbed_http.request).was.called()
+            assert.spy(stubbed_http.set_keepalive).was_not_called()
+
+            assert.falsy(valid)
+            assert.equal(404, invalid_err.status)
+            assert.equal([[The image is too big to be downloaded. Max image size: 24 B]], invalid_err.message)
+
+            -- Log images that are too big
+            assert.equal(1, #ngx._logs)
+        end)
+
         it("keepalive error", function()
             errors.keepalive = "timeout"
 
@@ -371,35 +384,10 @@ describe("client", function()
                 },
             })
 
-            assert.equal(400, invalid_err.status)
+            assert.falsy(valid)
+            assert.equal(404, invalid_err.status)
             assert.equal([[The request image is not a valid (supported) image.
 Allowed mime types: image/jpeg]], invalid_err.message)
-            assert.falsy(valid)
-        end)
-
-        it("max image size", function()
-            local new_client = require("weserv.client").new({
-                user_agent = "Mozilla/5.0 (compatible; ImageFetcher/8.0; +http://images.weserv.nl/)",
-                timeouts = {
-                    connect = 5000,
-                    send = 5000,
-                    read = 10000,
-                },
-                max_image_size = 1024,
-                max_redirects = 10,
-                allowed_mime_types = {}
-            })
-            local valid, invalid_err = new_client:is_valid_response({
-                headers = {
-                    ["Content-Length"] = "2048"
-                },
-            })
-
-            assert.equal(400, invalid_err.status)
-            assert.equal([[The image is too big to be downloaded.
-Image size: 2 KB
-Max image size: 1 KB]], invalid_err.message)
-            assert.falsy(valid)
         end)
     end)
 end)

spec/helpers/utils_spec.lua

@@ -49,9 +49,6 @@ describe("utils", function()
 
         tmpfile = utils.tempname("/path/does/not/exist", "imo_")
         assert.falsy(tmpfile)
-
-        -- Log unique file errors
-        assert.equal(1, #ngx._logs)
     end)
 
     it("test clean uri", function()

spec/manipulators/thumbnail_spec.lua

@@ -70,7 +70,7 @@ describe("thumbnail manipulator", function()
             local image, api_err = api:process(test_image, {})
 
             assert.falsy(image)
-            assert.equal(400, api_err.status)
+            assert.equal(404, api_err.status)
             assert.truthy(api_err.message:find("Image is too large for processing"))
         end)
 
@@ -83,7 +83,7 @@ describe("thumbnail manipulator", function()
             })
 
             assert.falsy(image)
-            assert.equal(400, api_err.status)
+            assert.equal(404, api_err.status)
             assert.truthy(api_err.message:find("Requested image dimensions are too large"))
         end)
     end)

src/weserv/api.lua

@@ -91,7 +91,8 @@ function api:next(image, args)
         -- Call the manipulator, which may itself call next().
         return manipulator.process(self, image, args)
     else
-        -- Reverse premultiplication after all transformations:
+        -- The last image manipulation was completed, reverse
+        -- premultiplication after all transformations:
         if args.is_premultiplied then
             -- Unpremultiply image alpha and cast pixel values to integer
             image = image:unpremultiply():cast("uchar")
@@ -141,13 +142,13 @@ function api:process(tmpfile, args)
     args.string_options = string_options
 
     local image
-    local success, image_err = pcall(function()
+    local read_success, read_err = pcall(function()
         image = vips.Image.new_from_file(args.tmp_file_name .. args.string_options, load_options)
     end)
 
-    if not success then
+    if not read_success then
         -- Log image not readable errors
-        ngx.log(ngx.ERR, "Image not readable: ", image_err)
+        ngx.log(ngx.ERR, "Image not readable: ", read_err)
 
         return nil, {
             status = ngx.HTTP_NOT_FOUND,
@@ -167,8 +168,38 @@ function api:process(tmpfile, args)
     -- Fill working queue
     for k, v in ipairs(self.manipulators) do self.manipulators_queue[k] = v end
 
-    -- Run the next manipulator.
-    return self:next(image, args)
+    -- Wrap it into pcall because we may throw errors.
+    local success, image_err = pcall(function()
+        -- Run the next manipulator.
+        image = self:next(image, args)
+    end)
+
+    if not success then
+        local api_err
+
+        -- lua-vips will throw errors as string types
+        if type(image_err) == "string" then
+            -- Log libvips errors
+            ngx.log(ngx.ERR, "libvips error: ", image_err)
+
+            api_err = {
+                status = ngx.HTTP_NOT_FOUND,
+                message = "libvips error: " .. image_err,
+            }
+        else
+            -- Otherwise we may throw it as a table type which
+            -- contains a status and message.
+            api_err = image_err
+        end
+
+        -- A image manipulation has caused an error, return
+        -- the error which may contain information to resolve it.
+        return nil, api_err
+    else
+        -- All image manipulations were successful, return
+        -- the image without errors
+        return image, nil
+    end
 end
 
 return {

src/weserv/client.lua

@@ -9,7 +9,6 @@ local table = table
 local string = string
 local assert = assert
 local unpack = unpack
-local tonumber = tonumber
 local setmetatable = setmetatable
 
 --- Client module.
@@ -64,27 +63,11 @@ function client:is_valid_response(res)
 Allowed mime types: %s]]
 
         return nil, {
-            status = ngx.HTTP_BAD_REQUEST,
+            status = ngx.HTTP_NOT_FOUND,
             message = string.format(error_template, table.concat(supported_images, ", "))
         }
     end
 
-    if self.config.max_image_size ~= 0 then
-        local length = tonumber(res.headers["Content-Length"])
-
-        if length ~= nil and length > self.config.max_image_size then
-            local error_template = [[The image is too big to be downloaded.
-Image size: %s
-Max image size: %s]]
-
-            return nil, {
-                status = ngx.HTTP_BAD_REQUEST,
-                message = string.format(error_template, utils.format_bytes(length),
-                    utils.format_bytes(self.config.max_image_size))
-            }
-        end
-    end
-
     if res.status ~= ngx.HTTP_OK then
         return nil, {
             status = ngx.HTTP_NOT_FOUND,
@@ -223,18 +206,43 @@ function client:request(uri, addl_headers, redirect_nr)
 
     local f = assert(io.open(res.tmpfile, "wb"))
 
+    local max_image_size = self.config.max_image_size
+    local current_size = 0
+
+    local image_too_big = false
+
     repeat
         local chunk, read_err = reader(65536)
         if read_err then
             ngx.log(ngx.ERR, read_err)
         end
 
         if chunk then
+            current_size = current_size + #chunk
+            image_too_big = max_image_size ~= 0 and current_size > max_image_size
+
+            if image_too_big then
+                break
+            end
+
             f:write(chunk)
         end
     until not chunk
     f:close()
 
+    if image_too_big then
+        httpc:close()
+        os.remove(res.tmpfile)
+
+        ngx.log(ngx.ERR, "Image is too big to be downloaded.")
+        local error_template = "The image is too big to be downloaded. Max image size: %s"
+
+        return nil, {
+            status = ngx.HTTP_NOT_FOUND,
+            message = string.format(error_template, utils.format_bytes(self.config.max_image_size))
+        }
+    end
+
     local ok, keepalive_err = httpc:set_keepalive()
     if not ok then
         ngx.log(ngx.ERR, "Failed to set keepalive: ", keepalive_err)

src/weserv/helpers/punycode.lua

@@ -16,7 +16,7 @@ local initial_n = 128 -- 0x80
 local delimiter = "-" -- 0x2D
 
 -- Highest positive signed 32-bit float value
-local max_int = 2147483647; -- aka. 0x7FFFFFFF or 2^31-1
+local max_int = 2147483647 -- aka. 0x7FFFFFFF or 2^31-1
 
 --- Punycode module
 -- @module punycode
@@ -74,7 +74,7 @@ function punycode.adapt(delta, numPoints, firstTime)
         k = k + 1
     end
 
-    return base * k + math.floor((base - t_min + 1) * delta / (delta + skew));
+    return base * k + math.floor((base - t_min + 1) * delta / (delta + skew))
 end
 
 --- Encoding procedure as per section 6.3 of RFC 3492.