TS-4653: esi plugin - disable HTTP_COOKIE variable by default and imp… #798
Conversation
|
Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/322/ for details. |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/428/ for details. |
|
[approve ci] |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/429/ for details. |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/430/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/323/ for details. |
|
Looks like this version of the patch leaves out wildcard support https://github.com/apache/trafficserver/pull/798/files#diff-a6bc69fbcc52f7fff04507b2a650ac4fR370 I think its sane to support that functionality as long as its not the default |
|
@struct , i can add the wildcard support tonight. e.g. whitelistCookie * That will mean we allow all cookies when we are using HTTP_COOKIE esi variables |
|
@zwoop can you take a look at this as well? |
|
wildcard support is added. |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/431/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/324/ for details. |
|
@bryancall / @jpeach / @zwoop any comments here? thanks. |
|
|
||
| bool found = false; | ||
| for (Utils::HeaderValueList::iterator lz = _whitelistCookies.begin(); lz != _whitelistCookies.end(); ++lz) { | ||
| std::string c = *lz; |
There was a problem hiding this comment.
Maybe this is the norm for the esi, but why create this intermediary std::string c? That seems inefficient and unnecessary ? Can't you just use the *lz ?
|
@zwoop , i just did the cleanup. Good to merge? |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/442/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/336/ for details. |
|
@sudheerv can you help to do a review for me? |
| _insert(_dict_data[HTTP_COOKIE], string(iter->name, iter->name_len), string(iter->value, iter->value_len)); | ||
| _debugLog(_debug_tag, "[%s] Inserted cookie with name [%.*s] and value [%.*s]", __FUNCTION__, iter->name_len, iter->name, | ||
| iter->value_len, iter->value); | ||
| std::string v = iter->name; |
There was a problem hiding this comment.
The variable names are not chosen well. Can you please not use 1 or 2 characters for variable names.
|
@shukitchan Overall it looks good. I had one minor comment. |
|
updated just now. will merge after squashing commits into one. thanks. |
…lement a whitelist mechanism to allow the specified cookies for it Original code and idea contributed by Chris Rohlf
shukitchan
force-pushed
the
esicookiepatch
branch
from
dba98b1 to
65972a1
Compare
|
Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/353/ for details. |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/457/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/352/ for details. |
Merge ASF master into 10.0.x
6 participants
…lement a whitelist mechanism to allow the specified cookies for it
Original code and idea contributed by Chris Rohlf (chris.rohlf@gmail.com)
@bryancall / @jpeach , pls help to review. Thanks a lot.