Skip to content

Fix bugs#60

Open
DavidVentura wants to merge 2 commits intoGroundApps:masterfrom
DavidVentura:master
Open

Fix bugs#60
DavidVentura wants to merge 2 commits intoGroundApps:masterfrom
DavidVentura:master

Conversation

@DavidVentura
Copy link

Ok, this is in order of the bugs I found:

  • On each request (items/count/anything) this'd throw thousands of errors because not every key is defined in $_POST. the isset calls check for that.
  • You were including the CONSTANTS file twice on api.php, one in mysql_connector, one directly.
  • this if (!hash_equals($authKey, crypt($auth, $authKey))){ is not how you use crypt. There must be something that I'm missing here because I don't know why you'd even do this. You encrypted the variable $auth with the salt $authKey, but you don't have this value stored somewhere to actually check for equality

@DavidVentura
Copy link
Author

The commit 1600b59 fixes a big error that happens if you have an empty list on the phone and press the double-tick button.

@J-8
Copy link
Contributor

J-8 commented Oct 1, 2015

Hey. Thanks for pull request.
However, development takes place on the devel branch which handles a couple of things differently and some things are already fixed.
Also while I agree about the prepared statement, the bind has to happen within the loop or else the value never changes.

@DavidVentura
Copy link
Author

I'll check it out, but the bind value is taken into account on call to
execute().
Check this basic example:
http://www.w3schools.com/php/php_mysql_prepared_statements.asp

I'll pull the other branch and check it out, thanks

On 1 October 2015 at 02:41, Jan Buchta notifications@github.com wrote:

Hey. Thanks for pull request.
However, development takes place on the devel branch which handles a
couple of things differently and some things are already fixed.
Also while I agree about the prepared statement, the bind has to happen
within the loop or else the value never changes.


Reply to this email directly or view it on GitHub
#60 (comment)
.

Stack is the new term for "I have no idea what I'm actually using".

@jklmnn
Copy link
Contributor

jklmnn commented Oct 1, 2015

@J-8 we might have to anounce creating pull requests more prominently in the readme. EDIT: I haven't read #61 before.

  • The index not defined notice is fixed in devel with 1ac8615
  • The double include of CONSTANTS.php is fixed in devel with 604a7eb
  • The fix for prepared statements is obsolete in devel wince we will replace both connectors with pdo, @J-8 do we still want to merge this commit to master?
  • The passwords are hashed with password_hash which generates a secure salt by default. The hash_equals is able to check correctness there.

@ghost
Copy link

ghost commented Oct 13, 2015

I think @jklmnn is right.
The people should only add a pull request to devel.
So they can check if the bug is fixed in the next version.

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