Azure / Azurite

A lightweight server clone of Azure Storage that simulates most of the commands supported by it with minimal dependencies
MIT License
1.83k stars 325 forks source link

Invalid "visibilitytimeout" parameters return no error code #2083

Closed Arithmomaniac closed 1 year ago

Arithmomaniac commented 1 year ago

Which service (blob, file, queue, table) does this issue concern?

Queue

Which version of the Azurite was used?

3.2.4

Where do you get Azurite? (npm, DockerHub, NuGet, Visual Studio Code Extension)

Visual Studio

What's the Node.js version?

18.16.0

What problem was encountered?

Passing a too-large visibilitytimeout parameter value to the "Receive Message" endpoint returns a 400 but no error code.

Steps to reproduce the issue?

See log, which tries using visibility timeouts 0 and 691200: azurite.log

The former contains the header x-ms-error-code: OutOfRangeQueryParameterValue, the latter contains no x-ms headers. Compare lines 58-69 to 105-110.

Have you found a mitigation/solution?

I suspect that we should remove the maximum checks here, or handle the errors differently: https://github.com/Azure/Azurite/blob/bb90e2595bf8cf5bd409ed8babdadb2794ea4910/src/queue/generated/artifacts/parameters.ts#L274 https://github.com/Azure/Azurite/blob/bb90e2595bf8cf5bd409ed8babdadb2794ea4910/src/queue/generated/artifacts/parameters.ts#L288

blueww commented 1 year ago

@Arithmomaniac

From Azure Storage Queue rest API doc, you can see visibilitytimeout must "be larger than or equal to 1 second, and it can't be greater than 7 days". And 604800 is just second count of 7 days. As a storage rest API emulator, Azurite should be aligned with Storage rest API doc. So the current azurite behavior is correct and aligned with Azure Storage rest API spec.

Would you please update your program to use a visibilitytimeout to a value between 1-604800 ?

blueww commented 1 year ago

Close as this is not Azurite issue. Azurite behavior is aligned with Azure Storage Queue rest API doc.

Arithmomaniac commented 1 year ago

The problem is not that it returns a 400, it's that it doesn't return the x-ms-error-code header (or any error payload, for that matter)

blueww commented 1 year ago

@Arithmomaniac

As we discussed, the issue is on the error code /message from Azurite is not aligned with azure server. Not on it should/ should not fail.

Would you please give the detail error from real Azure server (the expected error, hide credential)? Then we can look into how to fix it.

BTW, Azurite welcome contribution. It would be great if you could raise a PR to fix it!

Arithmomaniac commented 1 year ago

Here's a payload from actual Azure storage for visibilitytimeout=691200.

210_Full.txt

To get that error from Azurite, the request handling would have to get to here: https://github.com/Azure/Azurite/blob/525f697c813da95c1327053cc6fac27032a436ce/src/queue/handlers/MessagesHandler.ts#L64-L77

But as you can see from L107 from azurite.log, that never happens. I believe the deserialization middleware sees the following from the generated specification for the GET operation:

https://github.com/Azure/Azurite/blob/525f697c813da95c1327053cc6fac27032a436ce/src/queue/generated/artifacts/specifications.ts#L406

Which has this constraint: https://github.com/Azure/Azurite/blob/525f697c813da95c1327053cc6fac27032a436ce/src/queue/generated/artifacts/parameters.ts#L273-L276

Which is violated, throwing an exception and short-circuiting processing.

blueww commented 1 year ago

I have raised PR https://github.com/Azure/Azurite/pull/2087 to fix this issue.

blueww commented 1 year ago

The latest Azurite 3.26.0 contains the fix.