psf / psf/requests

Setting Session Cookies

Open
#3,669 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
54.3k
Forks
10.4k
Avg merge
16h 43m
Merged PRs (30d)
3

Description

This is a continuation of the original PR (#3595) to address how we set Session cookies in Requests. The PR had enough disagreement on how this should be addressed that I felt it was better to move this into a discussion.

As noted in #3595, Requests currently allows the user to provide cookies as a dictionary. While we've allowed this for individual requests, because their domain is (mostly) scoped, there are some real security concerns for supporting this for Sessions.

The issues

  1. If the user sets Session's cookies attribute to a dict, it will happily accept it, but crashes when you try to send anything. To correct this issue, the cookies need to be supplied as a CookieJar instance as noted in the documentation. This is unideal because we allow users to use an idiom supported everywhere else in the API, and then fail later with a fairly unhelpful exception.

  2. #3595 automated this suggested process from the documentation, as we do elsewhere in Requests, but it allows you to easily set session-wide cookies that aren't bounded by a domain. This means it's easy to unintentionally send sensitive cookie information to unintended recipients.

  3. The currently proposed solution of using add_dict_to_cookiejar or cookiejar_from_dict provides no extra protection against the security issues raised in #3595.

Solutions(?)

  1. I think this is a two step process. For Requests 2.x, I'm suggesting we add a warning to the user when they set Session cookies attribute to a dict. This points them at the documentation so they can at least try to do the right things. Otherwise, they're none the wiser until their program crashes with an error in a separate module and only minor clues on the cause. In 3.0.0, I think this warning should be raised an actual exception since we don't support cookies as dicts for Sessions.

  2. (and also 3.) If we're going to require the extra step of making the user explicitly use add_dict_to_cookiejar or cookie_from_dict, then let's make them useful. They currently don't provide any benefit over the dictionary approach in #3595. We can emulate some of the functionality of how a browser handles cookies by allowing users to supply cookie parameters on a per-dictionary basis. This is similar to cookies set by individual page requests.

    cookies = {'SessionID': '1234',
               'SecretToken': 'efe077444d17b2de7feee356601d'}

    s = Session()
    add_dict_to_cookiejar(
        s.cookies, cookies, domain='auth.myauthsite.com', 
        secure=True, expires='123456789'
    )

This is a relatively simple changed as shown in an initial mockup in e2a4f9f & 4454849. This would need to be accompanied by a documentation update (something like 3e4e5b7) to explain how to do this properly.


@sigmavirus24, when you've got a moment, could you confirm this issue properly encapsulates your concerns and I didn't leave anything out? As for the possible solutions, are any of these amenable? I know your work in #2714 will help this a little bit with the default cookie policy change, but we still need to provide a way for the user to easily adhere to the safeguards we're putting in place.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing the Session cookie behavior described in #3595 and the linked advanced documentation, then compare the mockups in e2a4f9f and 4454849. Clarify whether the goal is warning or rejecting dictionary cookies, and whether cookie parameters should be supported by add_dict_to_cookiejar or cookiejar_from_dict. Done requires an agreed design, implementation, tests, and corresponding documentation.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend, security
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.