search

vert-x3 / vertx-web

HTTP web applications for Vert.x
Apache License 2.0
1.11k stars 531 forks source link

Allowed characters in path parameters not compliant with OpenAPI specification #2077

Open kiwi-oss opened 2 years ago

kiwi-oss commented 2 years ago

Version

Noticed in 3.6.3, but still exists in current master branch.

Context

As mentioned in https://github.com/vert-x3/vertx-web/issues/1372#issuecomment-671545630, according to the OpenAPI 3.0.3 specification, path parameters have to follow RFC 6570. This means that only alphanumeric characters and -, ., _, ~ are allowed.

However, the OpenAPI 3 path resolver only excludes a set of special characters which allows more characters than defined in RFC 6570.

In addition, there is a comment mentioning reserved characters in RFC 3986 which doesn't seem to be the correct RFC and is even more confusing as it doesn't match the excluded character set in the code below.

Do you have a reproducer?

Run the verticle from https://github.com/vert-x3/vertx-web/issues/1372#issuecomment-567455916 and send the following requests:

❯ curl 'localhost:8888/user/fo\o'
{
  "userId" : "fo\\o"
}
❯ curl 'localhost:8888/user/f,oo'
{
  "userId" : "f,oo"
}

Both requests contain illegal characters according to RFC 6570, so they should result in an error, but they don't.

tsegismont commented 2 years ago

@InfoSec812 would you mind taking care of this one as well?

InfoSec812 commented 2 years ago

The reference in the code to RFC 3986 is about query/post parameters and matching those params. I believe that @kiwi-oss is probably right. This is not what we should be checking for when building a RegEx to be used for a Route. There's still some there which was not properly commented and so I cannot easily be sure as to it's meaning and purpose. Following the usages of the solve() method it is definitely being used to create a Pattern which is then used to add a Route using that pattern.

kiwi-oss commented 2 years ago

I can confirm that the regex is used to add a route with that pattern.

I found that out because one client that I used has sent a request where the value of the path parameter contained an unencoded @ character. That resulted in an unexpected 404 response which made me debug the code.

I wonder if it would be possible to return a 400 response with an error message pointing to the invalid character when the regex doesn't match because the 404 is not very helpful. Should I open a separate issue for that?