search

mozilla / PyFxA

Python library for interacting with the Firefox Accounts ecosystem
Other
31 stars 20 forks source link

OAuth client did not check the 'state' value for /authorization using… #78

Closed ghost closed 4 years ago

ghost commented 4 years ago

… code-challenge - fixes #77

Also adapted tests to respond with the state correctly using a callback.

ghost commented 4 years ago

TBH, I didn't think very much about the actual implications in terms of security, but found the static value a bit too static and not checking the redirect URL for it a bit too relaxed. But I agree, doing this where we are in a single method with a single HTTP call doesn't make much sense from a security perspective.

OTOH, it gives an interested reader an idea of what may be important in a real OAuth flow, just as you described.

So overall, the changes are a bit opinionated, but I thought they wouldn't hurt, or?

rfk commented 4 years ago

OTOH, it gives an interested reader an idea of what may be important in a real OAuth flow, just as you described.

This is a fair point 👍

So overall, the changes are a bit opinionated, but I thought they wouldn't hurt, or?

Indeed, they won't hurt! I just wanted to check whether there weren't any mismatched expectations going on here about what this method does and doesn't do.

What do you think about adding a brief comment at the point where we generate the state parameter, saying basically "Checking state parameter is usually very important, it doesn't actually provide security in this case because the flow can't be injected half-way through, but we're doing it for completeness"?

ghost commented 4 years ago

What do you think about adding a brief comment at the point where we generate the state parameter, saying basically "Checking state parameter is usually very important, it doesn't actually provide security in this case because the flow can't be injected half-way through, but we're doing it for completeness"?

Done.