zf-fr / zfr-oauth2-server

PHP library for creating an OAuth 2 server (currently proof of concept)
BSD 3-Clause "New" or "Revised" License
36 stars 13 forks source link

Add token check in PHP #5

Closed bakura10 closed 10 years ago

bakura10 commented 10 years ago

ping @Ocramius

bakura10 commented 10 years ago

Is it really needed to use the more expensive Crypt comparison here? Or is a simpler $token === $expectedToken enough in this case?

Ocramius commented 10 years ago

@bakura10 the crypt comparison is required to disable timing attacks

bakura10 commented 10 years ago

@mac_nibblet, can you try again? I reverted the fix and instead only left with the checking in PHP.

macnibblet commented 10 years ago

@bakura10 @Ocramius this PR is wrong now that i actually think a bit...

You could have the following tokens Abc abC ? you would have to fetchAll tokens and iterate it no ?

bakura10 commented 10 years ago

Ha yeah you're right........ that's annoying u_u. Iterating seems like the only solution :/.

Ocramius commented 10 years ago

@macnibblet indeed, but the API is using find, assuming that the token is a PK.

I'd actually argue that you cannot have collisions with a case insensitive collation.

bakura10 commented 10 years ago

I'd actually argue that you cannot have collisions with a case insensitive collation.

Probability is extremely small, but could happen :(.

bakura10 commented 10 years ago

But yeah, as token is the PK, you cannot have this problem actually because of the constraint.

Ocramius commented 10 years ago

@bakura10 no, it will just throw an exception when trying to persist the new entity. If your collation is case insensitive, then your unique index will also be.

macnibblet commented 10 years ago

Is a unique constraint case sensitive ?

bakura10 commented 10 years ago

Nope. But it may generate an issue if you have a duplicate. What we may do is adding a check when creating a new token.

I know this is a workaround, but what I've actually done is in my migrations file, forcing the token to be utf8_bin.

Ocramius commented 10 years ago

@bakura10 so there is no check to verify if a token already exists? Because if the check is in place, we don't have the problem with the collation

bakura10 commented 10 years ago

No there is no check currently. It naively assumes that it will always returns a unique string.

Ocramius commented 10 years ago

@bakura10 aaaaaand that's the bug :-) Once that's handled, the entire case sensitivity issue disappears