Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize inputs #1

Closed
wants to merge 2 commits into from
Closed

Sanitize inputs #1

wants to merge 2 commits into from

Conversation

dorelo
Copy link
Collaborator

@dorelo dorelo commented Jan 1, 2017

In the same way we call:

function clean($string) {
   $string = str_replace(' ', '', $string); // removes all spaces
   return preg_replace('/[^A-Za-z0-9\-]/', '', $string);
}

on $credentials to prevent sqli, we should do the same for the other user-controlled inputs, as done in this branch (for api.php).

If it is all taken care of later in the code by the following prepared statement:

$stmt = $mysqli->prepare("INSERT INTO notifications (credentials, title, message, image, link, ip) VALUES (
AES_ENCRYPT(?,'$key'),
AES_ENCRYPT(?,'$key'),
AES_ENCRYPT(?,'$key'),
AES_ENCRYPT(?,'$key'),
AES_ENCRYPT(?,'$key'),
AES_ENCRYPT(?,'$key')
)");

Then this pull request should be rejected, and calling clean() on any input might be useless.

I am not clear on whether it is. Needs discussion.

@dorelo
Copy link
Collaborator Author

dorelo commented Jan 1, 2017

Updated with new commit.

Re-read the function and obviously

function clean($string) {
   $string = str_replace(' ', '', $string); // removes all spaces
   return preg_replace('/[^A-Za-z0-9\-]/', '', $string);
}

Will strip any special chars from URLs... meaning they're not URLs any more. Which breaks the imageURL, link and possibly message fields.

So I think the solution is just prepared statements, and not using the clean function at all. It seems too hacky.

@maxisme maxisme closed this Jan 2, 2017
@dorelo dorelo deleted the input_sanitization branch January 3, 2017 11:33
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.

None yet

2 participants