Skip to content

Conversation

@nitzangoldfeder
Copy link
Contributor

No description provided.

@nitzangoldfeder nitzangoldfeder changed the title Dev cookie v3 risk v2 EN-2988, cookie v3 risk v2 May 9, 2017
@@ -0,0 +1,27 @@
<?xml version="1.0" encoding="UTF-8"?>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this file is necessary in the repository

if not px_cookies:
return None

prefix = px_cookies[0]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it guaranteed that if you have both v1 and v3, the v3 will be the one you get here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, tested it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not what I'm asking. The fact that it once ran and worked, does not mean it's guaranteed to work.
What is the mechanism that enforces it? I didn't see any sorting of the px_cookies list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

return False
data = base64.b64decode(parts[2])
dk = self.pbkdf2_hmac('sha256', self.config['cookie_key'], salt, iterations, dklen=48)
key = dk[:32]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're refactoring, it might be a good idea to move some of these magic numbers to the new px_constants file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which of these magics you refer to?

@yaronschwimmer
Copy link

What about supporting JS challenge?

@nitzangoldfeder
Copy link
Contributor Author

@yaronpx
not in this task

@nitzangoldfeder
Copy link
Contributor Author

@yaronpx please re-review

def build_px_cookie(ctx, config):
config["logger"].debug("PxCookie[build_px_cookie]")
px_cookies = ctx['px_cookies'].keys()
px_cookies.sort(reverse=True)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to sort after you check the list exists.
Actually it's best to sort right before you store it in px_context

@yaronschwimmer yaronschwimmer merged commit 371bd3d into dev May 24, 2017
@yaronschwimmer yaronschwimmer deleted the dev-cookie_v3_risk_v2 branch May 24, 2017 11:40
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

Successfully merging this pull request may close these issues.

3 participants