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

Set-Cookie header not set properly when cookies are modified #5085

Closed
Speedlulu opened this issue Jan 21, 2022 · 7 comments
Closed

Set-Cookie header not set properly when cookies are modified #5085

Speedlulu opened this issue Jan 21, 2022 · 7 comments

Comments

@Speedlulu
Copy link
Contributor

Problem Description

Described here #5084

Steps to reproduce the behavior:

  1. Have a server set a cookie with en empty attributes (Secure, HttpOnly ...)
  2. Edit CookieAttrs object of said object
  3. Assign (value, CookieAttrs) tuple to the flow.response.cookies object
  4. Response header will now have empty attribute in Set-Cookie header set as Secure=; HttpOnly=

System Information

Mitmproxy: 7.0.4
Python: 3.8.10
OpenSSL: OpenSSL 1.1.1l 24 Aug 2021
Platform: Linux-5.11.0-46-generic-x86_64-with-glibc2.29

Pull request

I have created a pull request (#5084) to try to fix the issue, it works well for my use but it might break parts of the app that I'm not aware and/or don't use.

@Speedlulu Speedlulu added the kind/triage Unclassified issues label Jan 21, 2022
@mhils
Copy link
Member

mhils commented Jan 26, 2022

Thank you for the clear report. Your PR looks great on first sight, it just may take me a bit before I find some time to dig into the relevant parts of the codebase and see if this breaks anything. 👍

@mhils mhils added area/protocols kind/bug and removed kind/triage Unclassified issues labels Jan 26, 2022
@Speedlulu
Copy link
Contributor Author

Speedlulu commented Jan 26, 2022

Alrighty let me know if you need anything along the way :)

@Prinzhorn
Copy link
Member

While searching for something entirely unrelated I randomly came across #1512 which I think refs this and can be closed after the PR is merged? It also points towrds HAR dump, so maybe the contrib example addon is affected by this change as well.

@Speedlulu
Copy link
Contributor Author

@Prinzhorn This is indeed while working with SameSite cookies that I ran into this problem.

@Speedlulu
Copy link
Contributor Author

I updated the PR :)

@Prinzhorn
Copy link
Member

@mhils can this issue and #1512 be closed now?

@mhils
Copy link
Member

mhils commented Dec 5, 2023

Yes, but also feel free to just close things if you think they should be closed! 😄

@mhils mhils closed this as completed Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants