Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OPTIONS response has invalid blank header with no value #217

Open
ewired opened this issue Feb 21, 2022 · 2 comments
Open

OPTIONS response has invalid blank header with no value #217

ewired opened this issue Feb 21, 2022 · 2 comments

Comments

@ewired
Copy link

ewired commented Feb 21, 2022

I discussed the issue on the Fly help forum regarding the OPTIONS request responding with no CORS headers in #216. Fly's HTTP proxy is returning 502 because the OPTIONS response from Webdis has a blank : header with no name or value.

See two lines above last line:

< HTTP/1.1 200 OK
< Server: Webdis
< Allow: GET,POST,PUT,OPTIONS
< Access-Control-Allow-Methods: GET,POST,PUT,OPTIONS
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Headers: X-Requested-With, Content-Type, Authorization
< Connection: Keep-Alive
< Content-Type: text/html
< Content-Length: 0
< :
<
* Connection #0 to host XYZZY.fly.dev left intact

https://community.fly.io/t/http-handler-502-errors-on-options-request/4160/5?u=ewired

@nicolasff
Copy link
Owner

Thanks for reporting this, I didn't see it yesterday when I ran it with an older version of Webdis but I can reproduce it on the latest. This is likely a regression from #205. /cc @jessie-murray

@nicolasff
Copy link
Owner

@jessie-murray I've tracked it down with git-bisect to your change in dc9d1b6.

This is due to the header replacement code in http_response_set_header, which overwrites the value here:

webdis/src/http.c

Lines 100 to 108 in dc9d1b6

for(i = 0; i < r->header_count; ++i) {
if(strncmp(r->headers[i].key, k, key_sz) == 0) {
pos = i;
/* free old value before replacing it. */
if(r->headers[i].copy == HEADER_COPY_KEY) free(r->headers[i].key);
if(r->headers[i].copy == HEADER_COPY_VALUE) free(r->headers[i].val);
break;
}
}

but later still increments the header count:

r->header_count++;

It needs to avoid this increment if no new value was added. For the case of OPTIONS, somehow the Content-Length header is set twice, both times to 0. The second one causes this increment for which there's no key-value pair to write out.

jessie-murray added a commit to jessie-murray/webdis that referenced this issue Feb 22, 2022
Fix for nicolasff#217, a regression added in nicolasff#205. The "header_count"
field was incremented even when we overwrote a header entry, which
caused Webdis to send a header with no name and no value.
jessie-murray added a commit to jessie-murray/webdis that referenced this issue Feb 22, 2022
nicolasff added a commit that referenced this issue Feb 22, 2022
- Smaller Docker image size
- Bugfix: avoid responding to the wrong client (this could happen in rare cases)
- Better handling of WebSocket frames (details in #212)
- Fix regression introduced in 0.1.19, causing an empty header to be sent (#217)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants