add support for additional cookie attributes #27529
Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
| // The name of the cookie attribute. | ||
| string name = 1 [(validate.rules).string = {min_len: 1}]; | ||
|
|
||
| // The optional value of the cookie attribute. | ||
| string value = 2; |
There was a problem hiding this comment.
Should the characters allowed in here be limited by PGV and/or config checking?
There was a problem hiding this comment.
Good point. Not sure what is the max length we can go up to. Would be open to ideas.
There was a problem hiding this comment.
It's not just the length, the actual characters as specified by the HTTP spec. We limit header names, etc. for similar reasons.
There was a problem hiding this comment.
Ok. Changed. See if this makes sense
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
/retest |
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
| string name = 1 | ||
| [(validate.rules).string = | ||
| {min_len: 1 max_bytes: 16384 well_known_regex: HTTP_HEADER_NAME strict: false}]; |
There was a problem hiding this comment.
Not sure if the HTTP header name format is appropriate for the cookie attributes. If there are only some enumatable values, maybe in could be used here rather than well_known_regex.
There was a problem hiding this comment.
The idea of it is make this generic rather than adding the specific list. But If you prefer list, can you please propose what that list would be a good default?
There was a problem hiding this comment.
@wbpcode @mattklein123 any suggestions to take this forward?
There was a problem hiding this comment.
Can you figure out where in the RFC it specifies valid characters in cookie names? cc @RyanTheOptimist
There was a problem hiding this comment.
Cookie names follow token I guess same as Http Header names as per https://www.rfc-editor.org/rfc/rfc6265#section-4.1 and https://www.rfc-editor.org/rfc/rfc2616#section-2.2
There was a problem hiding this comment.
OK sounds good then seems like this is fine for now. Thanks for looking.
|
@wbpcode i think this is waiting on some feedback /wait-any |
|
Can you merge main and I can take a final look? Thanks. /wait |
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
@mattklein123 done. PTAL. |
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: asheryer <asheryer@amazon.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
Commit Message: implement additional attribute support for cookies
Additional Description:
Risk Level: Low
Testing: Updated
Docs Changes: Added
Release Notes: Added
Fixes #27234