API doesn't allow text

Question

I've just created a simple API for a CAD/MDT I'm working on, I've managed to get it to show the correct information when I do /citations/userid/1. This will then display all the correct values from the SQL database however, if I do /citations/issued_by/kevingorman1000 it will just throw an error. I can't tell what the error is as I'm using Slim php and can't seem to get the errors to display.

Any ideas why it isn't working ? I've added my code below..

$app->get('/citation/issuedby/{issued_by}', function(Request $request, Response $response){

$issued_by = $request->getAttribute('issued_by');

$sql = "SELECT * FROM ncic_citations WHERE issuedby = $issuedby";
try{

    // Get DB Object
    $db = new db();
    // Call Connection to DB
    $db = $db->connect();

    $stmt = $db->query($sql);

    $issby = $stmt->fetchAll(PDO::FETCH_OBJ);
    $db = null;

    echo json_encode($issby);

} catch(PDOExecption $e) {
    echo '{"error"} : {"text": '.$e->getMessage().'}';
}});

Any ideas why this is the case? Does it only allow getting via number or do I need too do something else? First time using this and kinda new to PHP as well.

Thanks for any help.

Thank you for the warning Dharman, too be honest, nothing important is on this, so if it was broken into then doesn't matter what so ever. I'll work out how to do it properly in the future but from what I've found online just doing some research this is the way people say too do it. It won't have any personal / private information in it. All part of a role play community. — Kevin Gorman, Jun 12 '19 at 20:12
My comment was the answer to your problem. It was not a tiny advice you might take into account in the future, but a serious problem in your code. — Dharman, Jun 12 '19 at 20:14

Dharman · Accepted Answer · 2019-06-12T20:16:27.017

0

Your problem is called SQL injection. You can solve it by using prepared statements. Never escape the values with quotes or anything else, as others might have suggested.

$sql = "SELECT * FROM ncic_citations WHERE issuedby = ? ";
$stmt = $db->prepare($sql);
$stmt->execute([$issuedby]);
$issby = $stmt->fetchAll(PDO::FETCH_OBJ);

For a good tutorial on PDO and prepared statements I recommend: https://phpdelusions.net/pdo

edited Jun 12 '19 at 20:16

answered Jun 12 '19 at 20:12

Dharman

30,962
25
85
135

OK, as everyone is saying it, I'll look into doing the prepared statements.. Would you say this information is reliable enough to go by? https://www.w3schools.com/php/php_mysql_prepared_statements.asp i know you posted above but I just like looking for loads of examples so I know roughly what I'm meant to be doing – Kevin Gorman Jun 12 '19 at 20:14
@KevinGorman See updated answer, I added a link to good trustworthy resource on PHP. Do not trust w3schools with PHP, even if this particular article is correct or useful, this website is still full of bad code. – Dharman Jun 12 '19 at 20:20
OK, thank you. I appricate the information and help. I'll take a look and see if I can do it that way – Kevin Gorman Jun 12 '19 at 20:24

score -1 · Answer 2 · answered Jun 12 '19 at 20:08

-1

It's because SQL error (missing quotes around string).

You try to send query

$sql = "SELECT * FROM ncic_citations WHERE issuedby = kevingorman1000";

Correct query has to be

$sql = "SELECT * FROM ncic_citations WHERE issuedby = 'kevingorman1000'";

answered Jun 12 '19 at 20:08

pavel

26,538
10
45
61

Oh, well I feel stupid now... That worked. Thank you. – Kevin Gorman Jun 12 '19 at 20:10
Not even a mention of SQL injection? C'mon. This is dangerous code that'll break as soon as a `'` character is encountered. – ceejayoz Jun 12 '19 at 20:11
@ceejayoz: the problem was in bad query, where you see SQL injection in my query, hm? You're probably happy for giving -1, enjoy (don't do it for reputation). – pavel Jun 12 '19 at 20:15
1

@panther While your answer doesn't have the SQL injection present, you are insinuating that it is ok to just put the quotes around. The down vote is for providing insufficient answer. – Dharman Jun 12 '19 at 20:17
@panther OP's code clearly used a variable `$issuedby`. There's more to fixing their issue *safely* than just putting quote marks around it. – ceejayoz Jun 12 '19 at 20:18
@ceejayoz I know that it's injectable now I've been told like 100 times.. I appraicate panther as they actually answered my question.. I'm looking into the SQL problem now even though like I said on another comment, it doesn't matter atm. – Kevin Gorman Jun 12 '19 at 20:18
@KevinGorman It always matters. Writing code is practice; practicing dangerous things is a good way to accidentally wind up doing them in production somewhere important. – ceejayoz Jun 12 '19 at 20:20
@Dharman "The down vote is providing insufficient answer", considering the fact that the OP _resolved_ their issue due to this answer, you are inherently incorrect in this statement. That being said, the OP has been informed about SQL injection numerous times on this post. It's now up to them to implement it. Not yours or panthers. – Chris Jun 12 '19 at 20:20
1

@Chris No. That is not how this site works. That is not how anything works. This answer provided a *solution*, but a wrong one. Whether or not OP wants to do it properly is up to them, but SO community can't provide poorly written answers, without being down voted. The question clearly was asking about an issue with passing data to SQL and making the data a static part of the query is not a right solution. – Dharman Jun 12 '19 at 20:25
@Dharman “Wrong” solution, again, is _your_ interpretation. Which is what you’re not seeming to understand. Could the answer be improved to implement an SQL injection safe, prepared statement? Yes. Did you see me disagree with that premise? No. Is the answer _wrong_? Again, no. That’s just _your_ interpretation. – Chris Jun 12 '19 at 20:27

API doesn't allow text

2 Answers2