apigee-127 / swagger-tools

A Node.js and browser module that provides tooling around Swagger.
MIT License
702 stars 373 forks source link

content-type validation fails for empty request body #413

Open philippevk opened 8 years ago

philippevk commented 8 years ago

What happens:

It is basically the same thing as #275

But I don't think it's a client error. Consider an operation that consumes application/json. However imagine the body is optional. How would you implement this?

1) Accept both application/json and application/octet-stream This would be wrong. An octet-stream is not a valid body

2) Force client to send 'Content-Type: application/json' header when body is empty It would work but it adds a "gotcha" to the api. http://stackoverflow.com/questions/29784398/should-content-type-header-be-present-when-the-message-body-is-empty

What I would expect is...

Thoughts?

whitlockjc commented 8 years ago

I'm not completely opposed to this. I wonder what @fehguy or @webron have to say about this.

webron commented 8 years ago

spec-wise, the client would be expected to send the content type header as application/json. However, when it comes to validation, I'm fairly indifferent to this case. Seems like a reasonable suggestion.

whitlockjc commented 8 years ago

That's my stance as well. If you say you consume application/json but fail to set Content-Type to application/json, that's on you. But with that being said, I'm not against updating the logic such that if Content-Length is 0, we do not validate it at all. Of course, as I typed that I immediately thought of how this could be used to avoid validation at all which could be an issue.

philippevk commented 8 years ago

I'm pretty sure that http-spec wise sending a request with Content-Length 0 makes the server ignore the body (i.e. The header overrides the actual body length)

Because of this I don't think Content Length 0 can be abused

philippevk commented 8 years ago

Here's the Content Length rfc: https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4

whitlockjc commented 8 years ago

What I meant by abused is if someone wanted to be malicious, all they'd have to do is send Content-Length: 0 and all validation is disabled but that does not guarantee that the web framework will not attempt to process the body. I could be wrong. Thoughts?

philippevk commented 8 years ago

My understanding of the rfc is that a implementation that processes an http request MUST use Content-Length to determine how many bytes it reads to form the body.

Therefore, Content-Length 0 can't be abused because by setting it to 0 the web server MUST ignore the body. In other words, a web framework that follows the spec is guaranteed to give an empty body if Content-Length is 0.

I'll write a test for this just to be sure.

philippevk commented 8 years ago

As I thought! https://gist.github.com/philippevk/303f6bab050a02e64fa0bb8f675fd161

whitlockjc commented 8 years ago

Well, since you've proven the web frameworks are working as expected then my concern of abuse is dissolved. We'll get this supported.

akoskm commented 7 years ago

Any updates on this guys? Do you plan to implement skipping the body validation whenContent-Length is set to 0?

philippevk commented 7 years ago

I just submitted a pull request with a fix. In the meantime you can add the following middleware to your pipeline before swaggerValidator

expressApp.use((req, res, next) => {
  if (req.headers['content-length'] === '0' && req.headers['content-type'] == null) {
    req.headers['content-type'] = 'application/json' // or whatever your api consumes
  }
  next()
})

It's VERY hacky, but it works. Eh.

akoskm commented 7 years ago

@philippevk looks good to me. I'll wait for this to be merged, until then I'll stick to overriding application/octet-stream in the path. IMO not-too-hacky but still meh.