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

cookie_key is rewriten in ApiBasketMiddleWare.process_response() #205

Open
twil opened this issue Nov 27, 2019 · 4 comments
Open

cookie_key is rewriten in ApiBasketMiddleWare.process_response() #205

twil opened this issue Nov 27, 2019 · 4 comments
Labels

Comments

@twil
Copy link

twil commented Nov 27, 2019

This line https://github.com/django-oscar/django-oscar-api/blob/master/oscarapi/middleware.py#L202

for cookie_key in cookies_to_delete:
    response.delete_cookie(cookie_key)

overwrites line 196 if cookies_to_delete is not empty

cookie_key = self.get_cookie_key(request)
@maerteijn maerteijn added the bug label Nov 28, 2019
@maerteijn
Copy link
Member

maerteijn commented Nov 28, 2019

You are right and this can be considered a bug, thanks for reporting.

Need to fix this and probably write a test for it.

@maerteijn
Copy link
Member

maerteijn commented Nov 28, 2019

@twil
I expected the following test to fail, see 658090e#diff-f35e3692dec538e7c17d3cb43da39e2eR141

But it isn't, probably due to https://github.com/django-oscar/django-oscar-api/blob/master/oscarapi/middleware.py#L163.

So yes, the code needs to be changed, and now I think it's even not needed. But I'm not sure so let's ask @specialunderwear

I'm not 100% confident here, as this middleware is a bit spooky 😄

@twil
Copy link
Author

twil commented Nov 28, 2019

@maerteijn yep, spooky indeed:)

The test doesn't fail because cookies_to_delete is empty. I see how it can be used but can't find where it is used.

@maerteijn
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants