PHP inserting Apostrophes where it shouldn't

Not too sure what's going on here as this doesn't seem like standard practise to me. But basically I have a basic database thingy going on that lets users submit code snippets. They can provide up to 5 tags for their submission.

Now I'm still learning so please forgive me if this is obvious!

Here's the PHP script that makes it all work (note there may be some CodeIgniter specific functions in there):

function submitform()
  {
    $this->load->helper(array('form', 'url'));

    $this->load->library('form_validation');
    $this->load->database();

    $this->form_validation->set_error_delimiters('<p style="color:#FF0000;">', '</p>');

      $this->form_validation->set_rules('title', 'Title', 'trim|required|min_length[5]|max_length[255]|xss_clean');
      $this->form_validation->set_rules('summary', 'Summary', 'trim|required|min_length[5]|max_length[255]|xss_clean');
      $this->form_validation->set_rules('bbcode', 'Code', 'required|min_length[5]'); // No XSS clean (or <script> tags etc. are gone)
      $this->form_validation->set_rules('tags', 'Tags', 'trim|xss_clean|required|max_length[254]');

    if ($this->form_validation->run() == FALSE)
    {
        // Do some stuff if it fails
    }
    else
    {  
      // User's input values
      $title   = $this->db->escape(set_value('title'));
      $summary = $this->db->escape(set_value('summary'));
      $code    = $this->db->escape(set_value('bbcode'));
      $tags    = $this->db->escape(set_value('tags'));

      // Stop things like <script> tags working
      $codesanitised    = htmlspecialchars($code);

      // Other values to be entered
      $author = $this->tank_auth->get_user_id();

       $bi1 = "";
       $bi2 = "";

       // This long messy bit basically sees which browsers the code is compatible with.
       if (isset($_POST['IE6'])) {$bi1 .= "IE6, "; $bi2 .= "1, ";} else {$bi1 .= "IE6, "; $bi2 .= "NULL, ";}
       if (isset($_POST['IE7'])) {$bi1 .= "IE7, "; $bi2 .= "1, ";} else {$bi1 .= "IE7, "; $bi2 .= "NULL, ";}
       if (isset($_POST['IE8'])) {$bi1 .= "IE8, "; $bi2 .= "1, ";} else {$bi1 .= "IE8, "; $bi2 .= "NULL, ";}
       if (isset($_POST['FF2'])) {$bi1 .= "FF2, "; $bi2 .= "1, ";} else {$bi1 .= "FF2, "; $bi2 .= "NULL, ";}
       if (isset($_POST['FF3'])) {$bi1 .= "FF3, "; $bi2 .= "1, ";} else {$bi1 .= "FF3, "; $bi2 .= "NULL, ";}
       if (isset($_POST['SA3'])) {$bi1 .= "SA3, "; $bi2 .= "1, ";} else {$bi1 .= "SA3, "; $bi2 .= "NULL, ";}
       if (isset($_POST['SA4'])) {$bi1 .= "SA4, "; $bi2 .= "1, ";} else {$bi1 .= "SA4, "; $bi2 .= "NULL, ";}
       if (isset($_POST['CHR'])) {$bi1 .= "CHR, "; $bi2 .= "1, ";} else {$bi1 .= "CHR, "; $bi2 .= "NULL, ";}
       if (isset($_POST['OPE'])) {$bi1 .= "OPE, "; $bi2 .= "1, ";} else {$bi1 .= "OPE, "; $bi2 .= "NULL, ";}
       if (isset($_POST['OTH'])) {$bi1 .= "OTH, "; $bi2 .= "1, ";} else {$bi1 .= "OTH, "; $bi2 .= "NULL, ";}

       // $b1 is $bi1 without the last two characters (, ) which would cause a query error
       $b1 = substr($bi1, 0, -2);
       $b2 = substr($bi2, 0, -2);

// :::::::::::THIS IS WHERE THE IMPORTANT STUFF IS, STACKOVERFLOW READERS::::::::::

       // Split up all the words in $tags into individual variables - each tag is seperated with a space
      $pieces = explode(" ", $tags);
      // Usage:
      // echo $pieces[0]; // piece1 etc

      $ti1 = "";
      $ti2 = "";

      // Now we'll do similar to what we did with the compatible browsers to generate a bit of a query string
      if ($pieces[0]!=NULL) {$ti1 .= "tag1, "; $ti2 .= "$pieces[0], ";} else {$ti1 .= "tag1, "; $ti2 .= "NULL, ";}
      if ($pieces[1]!=NULL) {$ti1 .= "tag2, "; $ti2 .= "$pieces[1], ";} else {$ti1 .= "tag2, "; $ti2 .= "NULL, ";}
      if ($pieces[2]!=NULL) {$ti1 .= "tag3, "; $ti2 .= "$pieces[2], ";} else {$ti1 .= "tag3, "; $ti2 .= "NULL, ";}
      if ($pieces[3]!=NULL) {$ti1 .= "tag4, "; $ti2 .= "$pieces[3], ";} else {$ti1 .= "tag4, "; $t开发者_Go百科i2 .= "NULL, ";}
      if ($pieces[4]!=NULL) {$ti1 .= "tag5, "; $ti2 .= "$pieces[4], ";} else {$ti1 .= "tag5, "; $ti2 .= "NULL, ";} 

       $t1 = substr($ti1, 0, -2);
       $t2 = substr($ti2, 0, -2); 

       $sql = "INSERT INTO code (id, title, author, summary, code, date, $t1, $b1) VALUES ('', $title, $author, $summary, $codesanitised, NOW(), $t2, $b2)";
       $this->db->query($sql); 

          $this->load->view('subviews/template/headerview');
          $this->load->view('subviews/template/menuview');
          $this->load->view('subviews/template/sidebar');

          $this->load->view('thanksforsubmission');
          $this->load->view('subviews/template/footerview');
    }
  }

Sorry about that boring drivel of code there. I realise I probably have a few bad practises in there - please point them out if so.

This is what the outputted query looks like (it results in an error and isn't queried at all):

A Database Error Occurred
Error Number: 1136

Column count doesn't match value count at row 1

INSERT INTO code (id, title, author, summary, code, date, tag1, tag2, tag3, tag4, tag5, IE6, IE7, IE8, FF2, FF3, SA3, SA4, CHR, OPE, OTH) VALUES ('', 'test2', 1, 'test2', 'test2   ', NOW(), 'test2, test2, test2, test2, test2', NULL, NULL, 1, 1, 1, 1, 1, 1, 1, NULL)

You'll see at the bit after NOW(), 'test2, test2, test2, test2, test2' - I never asked it to put all that in apostrophes. Did I?

What I could do is put each of those lines like this:

 if ($pieces[0]!=NULL) {$ti1 .= "tag1, "; $ti2 .= "'$pieces[0]', ";} else {$ti1 .= "tag1, "; $ti2 .= "NULL, ";}

With single quotes around $pieces[0] etc. - but then my problem is that this kinda fails when the user only enters 4 tags, or 3, or whatever.

Sorry if that's the worst phrased question in history, I tried, but my brain has turned to mush.

Thanks for your help!

Jack

---

It's hard to be certain, but I believe this is the relevant line

$tags = $this->db->escape( set_value( 'tags' ) );

It's hard to say since I don't know what set_value() does

I'm going to guess that this turns the string test2 test2 test2 test2 test2 into 'test2 test2 test2 test2 test2' - those single-quotes stay in the string, even after you explode it and place it all together again.

But, I have to say, all of this code is really messy. In fact, the problem you're having is one you shouldn't even have to worry about (dynamically building an INSERT with variable columns). It shows me a weakness in your schema - tags should be in a N:M (many-to-many) relationship to the code table, not a hard limit of columns. The way you've done it here breaks the 2nd normal form of database normalization.

So, you can definitely go for the quick-fix and change how you're escaping these tag values, but I'd recommend updating your schema.

---

Actually you tell to do put quotes around it by doing this:

 $tags  = $this->db->escape(set_value('tags'));

Your DB class does not know that you provide multiple values (how should it?). It treats the value getting from set_value('tags') as string and strings need to be escaped.

Later you explode that string:

 $pieces = explode(" ", $tags);

which should give you e.g.

$pieces[0] = "'test1";
$pieces[1] = "test2";
$pieces[2] = "test3'";

You are then concatenating the pieces again which in the end gives you: "'test1, test2, test3'".

The only thing that looks strange to me is the ending single quotation as you remove the last to characters from the string.

To solve the problem you can do this: Don't escape set_value('tags') beforehand. Escape the single values instead:

if ($pieces[0]!=NULL) {$ti1 .= "tag1, "; $ti2 .= $this->db->escape($pieces[0]) . ',';}

---

Another issue: I have no insight in your DB design but it looks like that tags should go into an own table (and then relating either via one-to-many or many-to-many relationship). Otherwise, you will be limited to only six tags. But that depends on the actual purpose.

Same for the browsers: What if a new browser gets on the market? You would have to extend your table. Better is to have a table with all browsers, e.g.

   id   |  browser
-------------------
   0    |    IE6
   1    |    IE7
   2    |    IE8
   3    |    FF2
etc.

and relate them via an intermediate table to the codes:

 --table code_browser

 code_id | browser_id
 --------------------
    0    |    0
    0    |    1
    0    |    3
    1    |    2
    2    |    1

 etc.

---

A little confused by the question but from my understanding I think you have a couple of problems

a) For your ID column I would recommend using 0 and not '' (makes it a little more readable in my opinion.

b) You define your empty strings $ti1 = "";

Then you add to them

if ($pieces[0]!=NULL) {$ti1 .= "tag1, "; $ti2 .= "$pieces[0], ";} else {$ti1 .= "tag1, "; $ti2 .= "NULL, ";}

$ti1 is now equal to tag1,

  if ($pieces[1]!=NULL) {$ti1 .= "tag2, "; $ti2 .= "$pieces[1], ";} else {$ti1 .= "tag2, "; $ti2 .= "NULL, ";}

$ti1 is now equal to tag1, tag2,

What I think you should be doing is

  if ($pieces[0]!=NULL) {$ti1 .= "'tag1', "; $ti2 .= "$pieces[0], ";} else {$ti1 .= "'tag1', "; $ti2 .= "'NULL, ";}

(note the addition of the single quotes inside the double quotes, so instead of having $ti1 is now equal to tag1, tag2, you will have $ti1 is now equal to 'tag1', 'tag2', etc

c) I not entirely sure what these tags are meant to be representing but I would create a joining table with the id of the column and ONE tag. This means each id will match up to 5 rows in the joining table. This will allow you to construct better queries for counting etc on these tags.

---

A single variable is being given, so it's being treated as such.

You could use the if statements to build out your $sql query directly.

   $sql = "INSERT INTO code (id, title, author, summary, code, date";
        if ($pieces[0]!=NULL) {$sql .= ", tag1"; }
        if ($pieces[1]!=NULL) {$sql .= ", tag2"; }
    ...

etc.