HTTP: allow "header;" to replace an internal header with a blank one#2362
HTTP: allow "header;" to replace an internal header with a blank one#2362
Conversation
Reported-by: Michael Kaufmann Fixes #2357
| char *cptr = Curl_checkheaders(conn, "Connection:"); | ||
| char *cptr = Curl_checkheaders(conn, "Connection"); | ||
| #define TE_HEADER "TE: gzip\r\n" | ||
|
|
There was a problem hiding this comment.
Before creating an updated "Connection" header, a check is necessary that the header is not empty, e.g.
char *cptr = Curl_checkheaders(conn, "Connection");
if(cptr && cptr[10] != ':')
cptr = NULL;
if(cptr) {
while(*cptr && ISSPACE(*cptr))
cptr++;
if (!*cptr)
cptr = NULL;
}
There was a problem hiding this comment.
I'm not sure I follow.
For transfer-encoding it must send a Connection: header which is what this ensures. It will override the user's (conflicting) decision to remove or blank it, if there is one. This is actually a bug already in the current code, as passing -H Connection: and --tr-encoding will send the somewhat odd-looking header Connection:, TE.
Are you suggesting the order to remove or blank the Connection: header should be stronger than the request to ask for transfer-encoding? I'm proposing the other way around since a user can't get transfer-encoding without the header, and if a user really doesn't want the Connection: header it makes no sense to ask for transfer-encoding!
There was a problem hiding this comment.
Are you suggesting the order to remove or blank the Connection: header should be stronger than the request to ask for transfer-encoding?
Yes, that was my intention. But now I think you're right, we should not change the existing behavior. So a "TE" should be added to "Connection", even if the user specified "Connection;" in the header list.
The existing code needs to be tweaked a bit to avoid adding a "Connection; TE" (with semicolon instead of colon).
There was a problem hiding this comment.
Agreed. I'll add a test case doing exactly that to make sure it gets right...
|
@mkauf, can you think of or detect any other problems? |
|
@bagder : In the function When adding the "TE" to "Connection", anything after "Connection;" is not ignored, I think "Connection; some-future-feature" would be changed to "Connection; some-future-feature, TE" I think it would be a good idea to use |
lib/http.c
Outdated
|
|
||
| Curl_safefree(conn->allocptr.te); | ||
|
|
||
| cptr = Curl_copy_header_value(cptr); |
There was a problem hiding this comment.
cptr is NULL if there's no custom "Connection" header in the list.
Fix:
if(cptr) {
cptr = Curl_copy_header_value(cptr);
if(!cptr)
return CURLE_OUT_OF_MEMORY;
}
lib/http.c
Outdated
| /* Create the (updated) Connection: header */ | ||
| conn->allocptr.te = cptr? aprintf("%s, TE\r\n" TE_HEADER, cptr): | ||
| conn->allocptr.te = (cptr && *cptr) ? | ||
| aprintf("%s, TE\r\n" TE_HEADER, cptr) : |
There was a problem hiding this comment.
Now cptr contains only the header value.
"%s, TE\r\n" must be changed to "Connection: %s, TE\r\n"
There was a problem hiding this comment.
ack, and it even broke some test cases because of this...
|
Thanks! |
Reported-by: Michael Kaufmann
Fixes #2357