-1

I am connected to my local host and am trying to use a GET method on line $. I am getting the Notice: Undefined index: deleteid in C:\xampp\htdocs\webd153\delete.php on line 4.

<?php
include 'connection.php';

$deleteid = $_GET['deleteid'];

if (isset($deleteid)) {
$deletesql = $dbh->prepare("DELETE FROM users WHERE id = '$deleteid'");
 $deletesql->execure();
  echo "record has been deleted!<br>";

I am trying to delete names that I have entered in my databases using a form that is connected from my local host to myphpadmin database.

Funk Forty Niner
  • 74,450
  • 15
  • 68
  • 141
tasiaraye
  • 1

1 Answers1

1

Right way is:

<?php
include 'connection.php';

if(isset($_GET['deleteid']) {
 $deleteid = $_GET['deleteid'];

 $deletesql = $dbh->prepare("DELETE FROM users WHERE id = '$deleteid'");
 $deletesql->execute();
 echo "record has been deleted!<br>";
}

But this is VERY INSECURE! When I send request with URL ending ?deleteid=1'+OR+1=1+OR+id=', your database will be deleted all rows. I suggest to change query building as:

 $deletesql = $dbh->prepare("DELETE FROM users WHERE id = (?)");
 $deletesql->bind_param('i', $deleteid);
Jakub Bouček
  • 1,366
  • 10
  • 15
  • 1
    That demonstrates a possible SQL injection in the query. – Jared Farrish Aug 07 '19 at 01:34
  • https://www.php.net/manual/en/mysqli.quickstart.prepared-statements.php – Jared Farrish Aug 07 '19 at 01:38
  • @JaredFarrish You right, thanks. Edited. – Jakub Bouček Aug 07 '19 at 01:39
  • Show it as a prepared statement; it's already demonstrating that. What if it's a UUID or something else? Just prepare it. – Jared Farrish Aug 07 '19 at 01:40
  • `DELETE FROM users WHERE id = (?)` and `$deletesql->bind_param('i', $deleteid);` Note, the type of `'i'` means integer for the type, `s` for string, etc. – Jared Farrish Aug 07 '19 at 01:43
  • using `->prepare` but still directly injecting variables inside the statement. so much for "prepared" statements – Kevin Aug 07 '19 at 01:58
  • `$deletesql->execure();` and at least edit the typo – Kevin Aug 07 '19 at 01:58
  • @JaredFarrish and others, I agree that a prepared statement is by far a much safer method to use. However, I have to disagree on the answer being open to injection, to a certain extent of course. Using `(int)` does help prevent against an sql injection, given that the input will always be that and nothing else being added. – Funk Forty Niner Aug 07 '19 at 03:02
  • @JakubBoucek Your prepare/execute doesn't do anything to help guard against an sql injection. What you wrote there can be modified to read just as `query()`, since there isn't an actual prepared statement here, as Jared gave a link for a reference on it. Just an "FYI" ;-) – Funk Forty Niner Aug 07 '19 at 03:03
  • @FunkFortyNiner That's beside the point. Show how to do it correctly. What was originally given, and commented on, was not the "int" solution, it was literally taking the GET. You could also say somewhere it was getting quoted (not shown here). Who are these "others"? Who cares? – Jared Farrish Aug 07 '19 at 12:51
  • @JaredFarrish I see. Pretend I didn't say anything ;-) – Funk Forty Niner Aug 07 '19 at 12:54
  • @FunkFortyNiner Sometimes it would be nice to have an aside with an answer author to help them without the tyranny of the nitpickers mobbing onto it. Jakub answered the question, however by me mentioning SQL injection, he's now got downvotes. How are we still having this SQL discussion in PHP in 2019? The pile-on still happens too frequently, and well-meaning users get turned off.. – Jared Farrish Aug 07 '19 at 13:02
  • 1
    @JaredFarrish Wasn't my downvote to start with. As far as to why we're still discussing SQL/PHP in 2019; we see a lot of questions asked by many different people and we don't take them as "one" but as a whole and are treated as such by some. This happens day in and day out and have no control over other people's actions. All we can do is try to help, and hopefully to keep on giving people some friendly advice. Stack has gotten way too big for our britches, so the daily grind gets to grow larger every single day. We'll have to stop discussing about this now. It could trigger something. – Funk Forty Niner Aug 07 '19 at 13:14
  • @JakubBoucek I would merge the two parts of your question into one. I see no reason to make the parameter bind a separate part of your answer. Interpolating variables into prepared statements was one of the things wrong with the code in question. Also the parentheses around `?` are unnecessary. – Dharman Aug 07 '19 at 19:19
  • @Dharman No problem, feel free to edit my answer. – Jakub Bouček Aug 08 '19 at 12:18