search

erlyaws / yaws

Yaws webserver
https://erlyaws.github.io
BSD 3-Clause "New" or "Revised" License
1.28k stars 267 forks source link

Make response for OPTIONS * configurable #417

Closed avtobiff closed 3 years ago

avtobiff commented 3 years ago

Previously the response to OPTIONS * was hardcoded to send HTTP 200 and the header Allow: GET, HEAD, OPTIONS, PUT, POST, DELETE.

This change adds the options_asterisk_methods setting in the sconf for a list of custom HTTP methods allowed for OPTIONS . By default the list is empty, and Yaws sends an HTTP 400 Bad Request. If options_asterisk_methods contains HTTP methods, Yaws responds to OPTIONS with HTTP 200 and an Allow header with the contents.

An additional knee-jerk fix was to add the Date header to yaws_server:deliver_xxx/5. According to RFC 7321, Section 7.1.1.2. Date, this header MAY be sent for 1xx and 5xx responses and MUST be sent otherwise.

vinoski commented 3 years ago

Thanks, I'll take a look over the weekend.

vinoski commented 3 years ago

I like the idea of this change. I took a close look over the weekend, and overall it looks good but I've modified your branch locally to address issues that I found, as I figure that's much faster than trying to iterate over writing review comments and then waiting for you to address them.

The biggest issue is that we can't change the default OPTIONS * response from 200 to 400, as users might rely on the current behavior. I know that in a previous job where we used Yaws in production, we used OPTIONS * for something and expected a 200 OK (this was likely why I introduced it in the first place, but I don't remember the details). I've modified the code so that not specifying options_asterisk_methods defaults to the original behavior, specifying it with an empty value results in a 400 Bad Request, and of course specifying a list of methods returns 200 OK with the listed methods.

Other than that, I modified the documentation and tests accordingly, and also modified the yaws.conf.5 man page to document the new setting there too.

I will likely merge this later today.

avtobiff commented 3 years ago

Good point to be backwards compatible. Thanks!

vinoski commented 3 years ago

Ignore the conflicts, which are due to the changes I described previously. This is now on master. Thanks for contributing it!