Probable Bug in Deck.getCardsOfType

[BitBlitz]

We are using the card name as the card_type. In this case, some names have embedded apostrophe's, such as "Shipbuilder's Guild".

When calling $this->cards->getCardsOfType( "Shipbuilder's Guild" ), I hit a SQL injection error:

Unexpected error: Propagating error from GS 1 (method: createGame): Fatal error during harbourjc setup: Error while processing SQL request (leto.boardgamearena.com via TCP/IP): SELECT card_id id, card_type type, card_type_arg type_arg, card_location location, card_location_arg location_arg FROM card WHERE card_type='Shipbuilder's Guild' You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 's Guild'' at line 1

[paramesis]

The error you are seeing is indeed due to the apostrophe in the name. As the error is hinting, the SQL query sent by the deck component defines the card_type as "Shipbuilder", with some junk at the end, followed by an extra single quote.

The string you use in the "card_type" column shouldn't be the same as the translatable string used in the interface. This string is used for SQL lookups, so it should be a unique identifier that differentiates it from other card_type values in your game and interface logic. It should not have any apostrophes, and I would recommend keeping it short, all lowercase, and using underscores instead of spaces (or come up with your own consistent standard).

Also, bear in mind if you are copying the standard 'card' database schema for use in your deck, the card_type has a maximum length of 16 characters, unless you've changed it. So even if the apostrophe doesn't give you an error, you will get errors down the line if you're trying to use longer strings (It might just trim the string to "Shipbuilders Gui" if you take out the apostrophe).

You can use the material.inc.php to associate the card_type identifier with the translatable "name", as well as other parameters as needed:

Example

Code: Select all

$this->card_info = array(
  'shipbuilders' => array(
    "name" => clienttranslate("Shipbuilder's Guild"),
    "tooltip_description" => clienttranslate("Converts something into something else..."),
    //...
  ),
  //...
);

[BitBlitz]

The length limit is well taken, but this still seems like a SQL injection bug in the Deck API that should be fixed. If a field is a string, it should encode any special characters consistently.

[tchobello]

Ever heard about Netiquette, coding habits ?

[BitBlitz]

tchobello,

Not sure if that's directed at me, but this seems like a secure API question, not one of coding habits especially since the length of the field is customizable. SQL injection in an API is a potential security issue.

Regarding Netiquette, I don't see how my reply was rude.

B

[cpasbanal]

Hi,

I eventually made it work (starting from what BitBlitz had begun, thanks to him) with this:

Code: Select all

$this->cards->getCardsOfType( self::escapeStringForDB($card_name) )

I began by escaping the string when creating the Deck but then I had to escapeStringForDB twice, which was useless.

----

To explain a little bit what we wanted to achieve with BitBlitz, the point of this is to have db easily readable when debugging by keeping the "Building names" easy to read in phpmyadmin and there is no real risk of SQL injection since all the stuff is stored in material.php.