Changes in OAuth2_Storage_Pdo #7
Conversation
Better PDO usage (bindParam in several places, including some really insecure ones) and more user table related customization (most of the time, the user table is already done for the platform).
| ), $config); | ||
| } | ||
|
|
||
| /* ClientCredentialsInterface */ | ||
| public function checkClientCredentials($client_id, $client_secret = null) | ||
| { | ||
| $stmt = $this->db->prepare(sprintf('SELECT * from %s where client_id = "%s"', $this->config['client_table_name'], $client_id)); | ||
| $stmt = $this->db->prepare('SELECT * FROM ' . $this->config['client_table_name'] . ' WHERE client_id = :client_id'); | ||
| $stmt->bindParam(':client_id', $client_id, PDO::PARAM_STR); |
There was a problem hiding this comment.
why is the indentation off here?
There was a problem hiding this comment.
Copy-paste issue (tabs vs. spaces)
|
Thank you for submitting this pull request! the So that being the case, how are these changes more secure? |
|
The real security issues were on statements that had something like In queries where The rest was only for consistency sake. |
|
As stated in my inline comment, there is no security benefit of string concatenation over sprintf. Also, we don't need to worry about sql injection on table names, as that should NEVER be left up to user input. As for username/password sql injection, these are open to sql injection attacks, but as we are binding those in the execute statements, sql injection is not possible. Since this class is NOT for production use (I will add a comment saying as much... and I apologize for the confusion) the Thanks for your support! |
|
As I said, string concatenation was only a panic option, and then my coding style took over; the real security issue was inline insertion of parameters, as in line 45, for example. There's nothing wrong with the This is not production ready; but it could be. Then, all it could take was the override of the password check, and that was it. |
|
Is it possible to change the visibility of properties in "protected" ? To easy extend functionality ? |
|
If there is a convincing reason to do so, then yes. However, any time a property or method goes from private to public, or public to protected, you are adding entrypoints into your model. This makes it very hard to refactor later, as anything might break backwards compatibility depending on how its used. For this reason, it's best for things to default to private and move to public only if there's a reason. Which properties do you think should be protected and why? Also, this should be a new thread... |
|
Thanks for your answer. |
|
sounds reasonable! Will you send me a pull request? |
|
I have sorted out but I will send you a request. Thanks! |
Better PDO usage (bindParam in several places, including some really insecure ones) and more user table related customization (most of the time, the user table is already done for the platform).