Better Cookie Parsing (#2504)

[gregoryyoung2]

Improves cookie parsing (#2511)

• Directive keys (i.e. cookie names) can now include % characters (fixes Fails to parse cookie if it has % in its name #2504)
• If a directive has no value (i.e. from failing to parse properly) it will be excluded from HTTPCookies
• If some directives have values and others do not, the directives with values will still be included in HTTPCookies

[gwynne]

Please add unit tests for the new behavior.

[gregoryyoung2]

Please add unit tests for the new behavior.

@gwynne done in cc2218e

[Joannis]

Before gwynne gets to it, our release process uses the PR body as release notes.
https://github.com/vapor/vapor/blob/master/.github/contributing.md#releases

[gregoryyoung2]

Quoting old PR body in the discussion:

#2504 brings up an issue where it is valid for a cookie name to include a % character, however Vapor fails to consider this as a valid directive key. This is an issue Vapor developers may run into when sharing a parent domain with other applications.

Additionally, if any cookies fail to parse (due to having a % in the name, for example), this will result in an empty cookie jar. We should only exclude the cookies which fail to parse, and include the ones which parse correctly.

I first extended the expression in Character.isDirectiveKey to return true if self == Character("%"). I then adjusted the HTTPCookies initializer to only include directives that have both a key and value, rather than return nil if any directive is missing a value.

If it is important to keep the API for this HTTPCookies.init stable, we can leave it as an optional initializer (I will update the PR).

[t-ae]

Nice patch!

The implementation does not conform to RFC yet.
As I wrote in #2504, there are some more characters that should be allowed.

!"#$%&'*+^`

It'll be great if all of them are allowed (I'm not 100% sure if all of them should be allowed, though).

By the way I think the name isDirectiveKey is not good.
What it check is if the character is allowed in token. token is a term in RFC, so renaming it to isTokenCharacter and providing the link to RFC will make it clearer.

[gregoryyoung2]

Nice patch!

The implementation does not conform to RFC yet.
As I wrote in #2504, there are some more characters that should be allowed.

!"#$%&'*+^`

It'll be great if all of them are allowed (I'm not 100% sure if all of them should be allowed, though).

By the way I think the name isDirectiveKey is not good.
What it check is if the character is allowed in token. token is a term in RFC, so renaming it to isTokenCharacter and providing the link to RFC will make it clearer.

I agree with all of these points, but due to the lack of documentation for this area of the project I wanted to make this change as small as possible to avoid unintended fallout from the change. Someone who has a more comprehensive understanding of the project might be able to achieve full RFC conformance, but this is sufficient for now.

[0xTim]

@gwynne ping - you happy with these changes?

[gwynne]

I'm not really 100% satisfied with silently dropping cookies that won't parse, but it's the best solution available without further work to improve RFC compliance and provide "permissive" parsing as a configurable option, which can be done in a future PR.

[tanner0101]

These changes are now available in 4.36.1