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

Issue 2482: accept empty cookies names #2676

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cranberrysoft
Copy link

@cranberrysoft cranberrysoft commented Mar 25, 2025

Description

The change is to skip merging logic for cookies without a name to avoid NPE. Also, we can not merge a cookie if the cookie's name is unavailable.

According to rfc6265, SetCookies processing logic does follow several steps to parse cookies; in general, a client should ignore the cookie when the name is not available If the name-value-pair string lacks a %x3D ("=") character, ignore the set-cookie-string entirely.

There is one edge case related to redirect which this change does not cover:

  1. /path1 'Set-Cookie': '; foo1=bar1' -> redirects to /path2
  2. /path2 'Set-Cookie': 'foo2=bar2'

This will result in

set-cookie: 'foo2=bar2'

instead of

set-cookie: ['; foo1=bar1',  'foo2=bar2']

In this case, because Apache HTTP client correctly implements RFC cookies for /path1, the foo1=bar1 won't be set as the name for the cookie is null.
On the other hand, the Netty ClientCookieDecoder would for the same cookie parse it with the name foo1 and value bar1.

So this

  1. /path1 'Set-Cookie': 'foo1=bar1' -> redirects to /path2
  2. /path2 'Set-Cookie': '; foo2=bar2'

Will result in a different result
['foo1=bar1; Domain=localhost', '; foo2=bar2']

This makes the behaviour inconsistent; however, we would need to use the same cookie parser for the original cookie store and redirect to make the behaviour the same

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

Successfully merging this pull request may close these issues.

1 participant