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

Cookies with certain keys are ignored #1495

Closed
forrestchang opened this issue Mar 29, 2019 · 6 comments · Fixed by #1496
Closed

Cookies with certain keys are ignored #1495

forrestchang opened this issue Mar 29, 2019 · 6 comments · Fixed by #1496
Labels
Milestone

Comments

@forrestchang
Copy link
Contributor

forrestchang commented Mar 29, 2019

I have a cookie called version=xxx in Headers, but it ignored by flask, using request.cookies.get('version') can't get the cookie.

I check the code then found these:

# _internal.py

_cookie_params = {
    b"expires",
    b"path",
    b"comment",
    b"max-age",
    b"secure",
    b"httponly",
    b"version",
}

# Ignore parameters.  We have no interest in them.
if key.lower() not in _cookie_params:
    yield _cookie_unquote(key), _cookie_unquote(value)

I don't know the comment "We have no interest in them" mean. Is there anyone can explain that for me? I will be appreciate for that.

@ThiefMaster
Copy link
Member

This is about the params of a cookie, not about the cookie name. If a cookie with such a name gets ignored, it's a bug.

@forrestchang
Copy link
Contributor Author

forrestchang commented Mar 29, 2019

@ThiefMaster It seems like a bug,

def _cookie_parse_impl(b):
    """Lowlevel cookie parsing facility that operates on bytes."""
    i = 0
    n = len(b)

    while i < n:
        match = _cookie_re.search(b + b";", i)
        if not match:
            break

        key = match.group("key").strip()
        value = match.group("val") or b""
        i = match.end(0)

        # Ignore parameters.  We have no interest in them.
        if key.lower() not in _cookie_params:
            yield _cookie_unquote(key), _cookie_unquote(value)

Maybe it should be if value.lower() in _cookie_params, if that, I will submit a pull request to fix it.

@forrestchang
Copy link
Contributor Author

I check the RFC 6265, it seems like there are some attributes have different usage. https://tools.ietf.org/html/rfc6265#section-5.2

But I don't find the version attribute.

@davidism
Copy link
Member

This appears to be a bug. I think the intention of the code was to ignore parameters that would only be relevant in a response's Set-Cookie if they were present in a request's Cookie header, but it ended up excluding the keys, not the parameters. Here's an example, this always returns "not set".

from werkzeug.serving import run_simple
from werkzeug.wrappers import Request, Response


@Request.application
def app(request):
    rv = Response("not set")

    if "version" in request.cookies:
        rv.data = request.cookies["version"]
    else:
        rv.set_cookie("version", "2")

    return rv


if __name__ == "__main__":
    run_simple("localhost", 5000, app)

@davidism davidism added the bug label Mar 29, 2019
@davidism davidism added this to the 1.0 milestone Mar 29, 2019
@davidism davidism changed the title Why werkzeug ignore some cookie params? Cookies with certain keys are ignored Mar 29, 2019
@forrestchang
Copy link
Contributor Author

@davidism I have submited a pr, but it had a CI error, it shows:

black....................................................................Failed

I dont't know the black mean.

@davidism
Copy link
Member

Black is a tool we use to enforce a consistent code style. If you look further down the output, it shows what code formatting it wants to change: https://travis-ci.org/pallets/werkzeug/jobs/513098270#L234-L247

Install pre-commit and run it manually to apply our code formatting rules. For future commits, it will run automatically.

pip install pre-commit
pre-commit install
pre-commit run --all-files

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants