Non-value attributes in Set-Cookie header #1171

Closed
nnposter opened this Issue Apr 2, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@nnposter

nnposter commented Apr 2, 2018

Parser for Set-Cookie header (function parse_set_cookie in http.lua) is currently assigning attributes without a value (such as Secure or HttpOnly) an artificial Boolean value of true (not string "true"). In contrast, attributes with explicitly empty values (...; attr=;...) are assigned a zero-length string as the value.

According to RFC 6265, section 5.2 these two cases should have the same outcome: empty value.

I am proposing to change the parser to assign a zero-length string to value-less attributes, which would have the following effect:

Pro

  • A parsed attribute always has a value of type string
  • Zero-length string is still interpreted as true in logical tests so this change should have negligible impact on existing code
  • There is no distinction between ...; attr;... and ...; attr=;..., which is what the RFC prescribes.

Con

  • There is no distinction between ...; attr;... and ...; attr=;..., so response member rawheader would have to be used if somebody truly needs to differentiate between the two cases.

I will implement the change in a few weeks unless anybody brings up concerns.

@cldrn

This comment has been minimized.

Show comment Hide comment
@cldrn

cldrn Apr 3, 2018

Member

The change seems correct. I'm just wondering if we can incorporate all the changes you've made to be more rfc complaint into @vinamrabhatia work too.

@vinamrabhatia Have you looked into nnposter's commits related to http and cookies? It would be great for you to consider them in your final revision.

Member

cldrn commented Apr 3, 2018

The change seems correct. I'm just wondering if we can incorporate all the changes you've made to be more rfc complaint into @vinamrabhatia work too.

@vinamrabhatia Have you looked into nnposter's commits related to http and cookies? It would be great for you to consider them in your final revision.

@nnposter

This comment has been minimized.

Show comment Hide comment
@nnposter

nnposter Apr 25, 2018

Resolved with r37235 (23d61f5)

Resolved with r37235 (23d61f5)

@nnposter nnposter closed this Apr 25, 2018

@vinamrabhatia

This comment has been minimized.

Show comment Hide comment
@vinamrabhatia

vinamrabhatia Apr 26, 2018

Contributor

@nnposter I will make the similar changes in the cookies library work too.

Contributor

vinamrabhatia commented Apr 26, 2018

@nnposter I will make the similar changes in the cookies library work too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment