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

Do not include cookies with empty key #31

Merged
merged 3 commits into from
May 20, 2024
Merged

Conversation

jvdboog
Copy link
Contributor

@jvdboog jvdboog commented May 8, 2024

I got the following error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/asgiref/sync.py", line 518, in thread_handler
    raise exc_info[1]
  File "/usr/local/lib/python3.11/site-packages/django/core/handlers/exception.py", line 42, in inner
    response = await get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/asgiref/sync.py", line 518, in thread_handler
    raise exc_info[1]
  File "/usr/local/lib/python3.11/site-packages/django/core/handlers/base.py", line 253, in _get_response_async
    response = await wrapped_callback(
               ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/views/decorators/clickjacking.py", line 50, in _view_wrapper
    response = await view_func(*args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/website/app_next/views.py", line 37, in get
    return await nextjs_request(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/website/app_next/functions.py", line 8, in nextjs_request
    return await render_nextjs_page(
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django_nextjs/render.py", line 149, in render_nextjs_page
    content, status, response_headers = await _render_nextjs_page_to_string(
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django_nextjs/render.py", line 102, in _render_nextjs_page_to_string
    async with aiohttp.ClientSession(
               ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/aiohttp/client.py", line 305, in __init__
    self._cookie_jar.update_cookies(cookies)
  File "/usr/local/lib/python3.11/site-packages/aiohttp/cookiejar.py", line 179, in update_cookies
    tmp[name] = cookie  # type: ignore[assignment]
    ~~~^^^^^^
  File "/usr/local/lib/python3.11/http/cookies.py", line 498, in __setitem__
    self.__set(key, rval, cval)
  File "/usr/local/lib/python3.11/http/cookies.py", line 488, in __set
    M.set(key, real_value, coded_value)
  File "/usr/local/lib/python3.11/http/cookies.py", line 353, in set
    raise CookieError('Illegal key %r' % (key,))
http.cookies.CookieError: Illegal key ''

To fix this I added an extra check that the cookie is not passed through when the value of the cookie is empty.

@danialkeimasi
Copy link
Member

The error appears to be with an empty cookie key, not the value:

  File "/usr/local/lib/python3.11/http/cookies.py", line 498, in __setitem__
    self.__set(key, rval, cval)
  File "/usr/local/lib/python3.11/http/cookies.py", line 488, in __set
    M.set(key, real_value, coded_value)
  File "/usr/local/lib/python3.11/http/cookies.py", line 353, in set
    raise CookieError('Illegal key %r' % (key,))
http.cookies.CookieError: Illegal key ''

@danialkeimasi
Copy link
Member

Empty value works:

curl 'http://localhost:8000/magnet/jobs' -H 'Cookie: key='

However, supplying an empty key would result in the same error as mentioned:

curl 'http://localhost:8000/magnet/jobs' -H 'Cookie: =value'

@jvdboog
Copy link
Contributor Author

jvdboog commented May 13, 2024

My bad, it has been fixed with the new commit.

@danialkeimasi
Copy link
Member

No problem 🙏🏼
Since the value itself isn't an issue, we can skip filtering cookies based on their values. We only need to filter if the key is empty.

@danialkeimasi danialkeimasi merged commit d64fc16 into QueraTeam:main May 20, 2024
9 checks passed
@danialkeimasi
Copy link
Member

I have also allowed passing cookies with empty value in 4fb93a9 since it's not causing exceptions.

@danialkeimasi danialkeimasi changed the title Do not include cookies with empty value Do not include cookies with empty key May 20, 2024
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.

2 participants