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

Cookie needs an update to support HttpOnly (and also Secure) #517

Closed
Hubgut opened this issue May 21, 2020 · 7 comments
Closed

Cookie needs an update to support HttpOnly (and also Secure) #517

Hubgut opened this issue May 21, 2020 · 7 comments

Comments

@Hubgut
Copy link

Hubgut commented May 21, 2020

Hi,
I was happy to see your little Cookie-Implementation but it does not handle tokens without an '=' correctly!
There is only one exception for the "secure" token in the code, even "Secure" or other case-insensitive versions won't work. The standard request field "HttpOnly" will throw an exception like everything else too.
The best solution would be to handle all tokens without an '=' as a Boolean.TRUE.

@johnjaylward
Copy link
Contributor

Is there an RFC or other specification that notes this behavior?

@johnjaylward
Copy link
Contributor

found RFC Here:
https://tools.ietf.org/html/rfc6265

@johnjaylward
Copy link
Contributor

Looks like "User Agents" should ignore unknown attributes:

Unless the cookie's attributes indicate otherwise, the cookie is
returned only to the origin server (and not, for example, to any
subdomains), and it expires at the end of the current session (as
defined by the user agent). User agents ignore unrecognized cookie
attributes (but not the entire cookie).

So it looks like instead of throwing an error, we just set the "true" flag as suggested by the OP and move on, then the caller of our Cookie class is responsible for validating actual attributes.

@johnjaylward
Copy link
Contributor

johnjaylward commented May 22, 2020

Also:

A user agent MUST use an algorithm equivalent to the following
algorithm to parse a "set-cookie-string":

  1. If the set-cookie-string contains a %x3B (";") character:

    The name-value-pair string consists of the characters up to,
    but not including, the first %x3B (";"), and the unparsed-
    attributes consist of the remainder of the set-cookie-string
    (including the %x3B (";") in question).

    Otherwise:

    The name-value-pair string consists of all the characters
    contained in the set-cookie-string, and the unparsed-
    attributes is the empty string.

  2. If the name-value-pair string lacks a %x3D ("=") character,
    ignore the set-cookie-string entirely.

  3. The (possibly empty) name string consists of the characters up
    to, but not including, the first %x3D ("=") character, and the
    (possibly empty) value string consists of the characters after
    the first %x3D ("=") character.

  4. Remove any leading or trailing WSP characters from the name
    string and the value string.

  5. If the name string is empty, ignore the set-cookie-string
    entirely.

  6. The cookie-name is the name string, and the cookie-value is the
    value string.

The user agent MUST use an algorithm equivalent to the following
algorithm to parse the unparsed-attributes:

  1. If the unparsed-attributes string is empty, skip the rest of
    these steps.

  2. Discard the first character of the unparsed-attributes (which
    will be a %x3B (";") character).

  3. If the remaining unparsed-attributes contains a %x3B (";")
    character:

    Consume the characters of the unparsed-attributes up to, but
    not including, the first %x3B (";") character.

Otherwise:

   Consume the remainder of the unparsed-attributes.

   Let the cookie-av string be the characters consumed in this step.
  1. If the cookie-av string contains a %x3D ("=") character:

    The (possibly empty) attribute-name string consists of the
    characters up to, but not including, the first %x3D ("=")
    character, and the (possibly empty) attribute-value string
    consists of the characters after the first %x3D ("=")
    character.
    Otherwise:

    The attribute-name string consists of the entire cookie-av
    string, and the attribute-value string is empty.

  2. Remove any leading or trailing WSP characters from the attribute-
    name string and the attribute-value string.

  3. Process the attribute-name and attribute-value according to the
    requirements in the following subsections. (Notice that
    attributes with unrecognized attribute-names are ignored.)

  4. Return to Step 1 of this algorithm.

When the user agent finishes parsing the set-cookie-string, the user
agent is said to "receive a cookie" from the request-uri with name
cookie-name, value cookie-value, and attributes cookie-attribute-
list. (See Section 5.3 for additional requirements triggered by
receiving a cookie.)

@johnjaylward
Copy link
Contributor

@Hubgut please try out the changes in #521 and let me know how they work for you. I basically removed all attribute validation since User-Agents are instructed to ignore unknown entities anyway.

@Hubgut
Copy link
Author

Hubgut commented May 23, 2020

@johnjaylward I've tested your changes and they work for my simple use case just fine. But I also looked a little bit at the details and it seems that you've forgotten to escape the key in org.json.Cookie.toString(JSONObject). You do unescape it within org.json.Cookie.toJSONObject(String), so I think, you just forgot it the other way around.

Maybe you could also add a little check for the boolean value of an additional attribute in the JSONObject. Someone could have added something like HttpOnly=false to the JSONObject. Parsing this to a cookie string would lead to the addition of ";HttpOnly"!

@stleary
Copy link
Owner

stleary commented May 23, 2020

Please continue this conversation in the pull request #521. @Hubgut comments on the implementation in particular belong there.

@stleary stleary closed this as completed May 23, 2020
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

3 participants