Invalid parameter error when calling resize

[roborourke]

Error spotted in some situations when running in lambda:

Object.invalidParameterError (/var/task/node_modules/sharp/lib/is.js:101)
Sharp.<anonymous> (/var/task/node_modules/sharp/lib/resize.js:360)
Array.forEach (<anonymous>:undefined)
Sharp.extract (/var/task/node_modules/sharp/lib/resize.js:355)
Object.module.exports.resizeBuffer (/var/task/index.js:149)

the code should handle whatever is being passed as an invalid parameter before we call Sharp’s resize function.

Acceptance criteria:

• implement argument validation
• run tachyon with invalid arguments without generating errors

[veselala]

Hey team! Please add your planning poker estimate with ZenHub @kovshenin @jerico @wisyhambolu

[yumito]

@ferschubert-hm @mikelittle We have estimated this as a 3, please let us know if anything bothers you

[wisyhambolu]

I tried reproducing the issue with queries like "https://terraform-app-stack.aws.humansmadeus.com/tachyon/2022/07/central-perk.png?position=\u0000,strategy=dsjfje,gravity=dsjf"; but I wasn't able to. The requests return 400 errors which would mean it's not executed by the tachyon lambda function so I would not know if the issue still persists after deploying a fix.

I plan to try again on Monday.

@roborourke I would appreciate if you could provide some guidance on how I can reproduce the error.

[roborourke]

Looking at the parameters that can be passed to Sharp's resize method here specifically:

https://github.com/humanmade/tachyon/blob/master/index.js#L180-L211

So background, lb, fit and resize specifically, you're only testing position, strategy (should be crop_strategy) and gravity in your example URL from the looks of it.

[ferschubert-hm]

Let's see if I can fix it...

Could reproduce it and get a 502 with https://terraform-app-stack.aws.humansmadeus.com/tachyon/2022/07/central-perk.png?resize=100,200&crop_strategy=omg

So crop_strategy should be one of the accepted parameters as well resize values cannot have special chars.

[ferschubert-hm]

We have a lot of vulnerabilities and deprecated warnings:

npm WARN deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.

npm WARN deprecated [email protected]: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
...
12 vulnerabilities (5 moderate, 7 high)

[ferschubert-hm]

In fact the error is thrown for the module.exports.resizeBuffer = async function(buffer, args, callback) { function, not resize itself, nevertheless there are some params in resize that can throw a 5xx as well.

[ferschubert-hm]

Could reproduce it, if you do not specify one of the 4 params for crop it fails, or if you specifiy a non-int value.

Invoke Error 	
{
    "errorType": "Error",
    "errorMessage": "Expected integer for left but received NaN of type number",
    "stack": [
        "Error: Expected integer for left but received NaN of type number",
        "    at Object.invalidParameterError (/var/task/node_modules/sharp/lib/is.js:101:10)",
        "    at Sharp.<anonymous> (/var/task/node_modules/sharp/lib/resize.js:360:16)",
        "    at Array.forEach (<anonymous>)",
        "    at Sharp.extract (/var/task/node_modules/sharp/lib/resize.js:355:38)",
        "    at Object.module.exports.resizeBuffer (/var/task/index.js:149:10)"
    ]
}

[roborourke]

I'm not too worried about the vulns / deprecation warnings just yet, those can be handled separately in another issue. These errors are to be expected too if incorrect values are being provided intentionally.

Additionally looking at the associated spike - the errors reported there are not really related to what this issue is reporting, those were caused by someone intentionally passing incorrect values to the position parameter specifically.

The API docs already indicate what values are expected or available for each query parameter so I don't think we need to keep trying different combinations of things to work that out, see https://docs.altis-dxp.com/media/dynamic-images/#query-args-reference

I think this issue isn't properly scoped yet from when I wrote it originally so here's what I think needs to happen now:

• Create a new empty object for query parameters
• Create a new list for errors
• Ignore unknown query string parameters and pass them through
• Add regex validation of each supported parameter
  • On failure, do not add the parameter to new params object, add error message to errors list
  • On success add parameter to new params object for use as it currently is used
• Return list of errors as response header with name X-Tachyon-Errors separated by semi-colons

Parameters and accepted value regexes:

• w:/^[1-9]\d*$/
• h: /^[1-9]\d*$/
• quality: /^[0-9]{1,3}$/ - also check range, must be >= 0 and <= 100
• resize: /^\d+(px)?,\d+(px)?$/
• crop_strategy: /^(smart|entropy|attention)$/
• gravity: /^(north|northeast|east|southeast|south|southwest|west|northwest|center)$/
• fit: /^\d+(px)?,\d+(px)?$/
• crop: /^\d+(px)?,\d+(px)?,\d+(px)?,\d+(px)?$/
• zoom: /^\d+(\.\d+)?$/
• webp: /^1$/
• lb: /^\d+(px)?,\d+(px)?$/
• background: /^#[a-f0-9]{3}[a-f0-9]{3}?$/ - account for URL encoded hash, should be a string like this at time of processing

[jerico]

@roborourke Opted to delete the object key in arg instead if it's not valid. Let me know if this is not a good idea.

I adjusted webp's regex to /^0|1|true|false$/ because upon testing, server.js and lambda-handler.js both add a value to it (args.webp = !!( request.headers && request.headers['accept'] && request.headers['accept'].match( 'image/webp' ) );) based on the header and validation fails.

https://github.com/humanmade/tachyon/blob/master/lambda-handler.js#L10-L12
https://github.com/humanmade/tachyon/blob/master/server.js#L52-L54