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

HTTP2 apparently allows multiple "Cookie:" header lines which seems to not work here #8

Closed
charlesdaniel opened this issue Feb 6, 2022 · 3 comments

Comments

@charlesdaniel
Copy link

I believe this line https://github.com/imbolc/tower-cookies/blob/main/src/service/mod.rs#L39 is only processing the first occurrence of the Cookie: header but apparently HTTP2 supports multiple Cookie headers (for better compression) (See https://www.rfc-editor.org/rfc/rfc7540#section-8.1.2.5 ). Both Chrome and Safari seem to be sending them as separate cookie headers.

Switching that line https://github.com/imbolc/tower-cookies/blob/main/src/service/mod.rs#L39 out to this hideous block seems to fix it as it just recreates the giant cookie blob and lets the rest split like normal.

        // let value = req.headers().get(header::COOKIE).cloned();

        let value: Vec<_> = req
            .headers()
            .get_all(header::COOKIE)
            .iter()
            .map(|a| a.to_str().unwrap())
            .collect();
        let value = value.join("; ");
        let value = Some(HeaderValue::from_str(&*value).unwrap());
@imbolc
Copy link
Owner

imbolc commented Feb 6, 2022

I've just checked it, and everything works exactly as you said. On the implementation though, I'd prefer these manipulations to be lazy, to avoid unnecessary allocations for requests without cookies access. Let me know if you'd provide a PR, or I can look into it myself in a few days. Thank you 🙏

@charlesdaniel
Copy link
Author

Totally agree a lazy implementation would be better. Unfortunately I won't be able to provide a PR. Thank you for the great repo.

@imbolc
Copy link
Owner

imbolc commented Feb 23, 2022

It's finally published, sorry for the delay :)

@imbolc imbolc closed this as completed Feb 23, 2022
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

No branches or pull requests

2 participants