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.

Sign up for GitHub

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 tag, item, and mod inputs #54

Merged
merged 2 commits into from
Aug 14, 2017
Merged

Sanitize tag, item, and mod inputs #54

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
000a591
Sanitize tag, item, and mod inputs
elifoster Aug 11, 2017
4820122
Fix loose false check, use ternary
elifoster Aug 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions OreDict.body.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,10 @@ static public function deleteEntry($id, $user) {
* @return bool|int False if it failed to add, or the new ID.
*/
static public function addEntry($mod, $name, $tag, $user, $params = '') {
if (!self::isStrValid($mod) || !self::isStrValid($name) || !self::isStrValid($tag)) {
return false;
}

$dbw = wfGetDB(DB_MASTER);

$result = $dbw->insert(
Expand Down Expand Up @@ -305,6 +309,14 @@ static public function addEntry($mod, $name, $tag, $user, $params = '') {
return $lastInsert;
}

/**
* @param $str string The string to check
* @return bool Whether this string is non-empty and does not contain invalid values.
*/
public static function isStrValid($str) {
return !empty($str) && Title::newFromText($str) !== null;
}

/**
* Edits an entry based on the data given in the first parameter.
* @param $update array An array essentially identical to a row. This contains the new data.
Expand All @@ -322,6 +334,17 @@ static public function editEntry($update, $id, $user) {
__METHOD__
);
$row = $stuff->current();

$tag = empty($update['tag_name']) ? $row->tag_name : $update['tag_name'];
$item = empty($update['item_name']) ? $row->item_name : $update['item_name'];
$mod = empty($update['mod_name']) ? $row->mod_name : $update['mod_name'];
$params = empty($update['grid_params']) ? $row->grid_params : $update['grid_params'];

// Sanitize input
if (!self::isStrValid($tag) || !self::isStrValid($item) || !self::isStrValid($mod)) {
return 1;
}

$result = $dbw->update(
'ext_oredict_items',
$update,
Expand All @@ -333,11 +356,6 @@ static public function editEntry($update, $id, $user) {
return 1;
}

$tag = empty($update['tag_name']) ? $row->tag_name : $update['tag_name'];
$item = empty($update['item_name']) ? $row->item_name : $update['item_name'];
$mod = empty($update['mod_name']) ? $row->mod_name : $update['mod_name'];
$params = empty($update['grid_params']) ? $row->grid_params : $update['grid_params'];

// Prepare log vars
$target = empty($mod) ? "$tag - $item" : "$tag - $item ($mod)";

Expand Down
2 changes: 2 additions & 0 deletions i18n/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"oredict-import-fail-nochange": "Entry already exists and no changes were made!",
"oredict-import-fail-exists": "Entry already exists!",
"oredict-import-fail-insert": "Insert failed!",
"oredict-import-fail-chars": "Input contains invalid values (empty or invalid characters)!",
"oredict-import-comment": "Importing entries.",
"oredict-tag-name": "Ore dictionary name",
"oredict-item-name": "Item name",
Expand Down Expand Up @@ -60,6 +61,7 @@
"oredict-manager-submit": "Go",
"oredict-manager-fail-insert": "Insert failed!",
"oredict-manager-fail-norows": "Query returned empty set (i.e., zero rows).",
"oredict-manager-fail-general": "Failed to edit entry. Double check inputs.",
"log-name-oredict": "OreDict log",
"log-description-oredict": "Here you can find a list of changes made to OreDict entries.",
"logentry-oredict-editentry": "$1 edited entry #$15 ($7{{#if:$8| from $8}}). Changes: $4;",
Expand Down
7 changes: 7 additions & 0 deletions special/ImportOreDict.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ public function execute($par) {
$tag = $tagName;
$fItem = $itemName;
$mod = $modName;

// Sanitize inputs
if (!OreDict::isStrValid($tag) || !OreDict::isStrValid($fItem) || !OreDict::isStrValid($mod)) {
$out->addHTML($this->returnMessage(false, $this->msg('oredict-import-fail-chars')->text()));
continue;
}

$item = $stuff->current();

// Prepare log vars
Expand Down
38 changes: 33 additions & 5 deletions special/OreDictEntryManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,24 @@ public function execute($par) {
return;
}

$this->updateEntry($opts);
$result = $this->updateEntry($opts);
// It's obvious if a deletion was successful
if ($opts->getValue('delete') != 1) {
switch ($result) {
case 0: {
// Success: no op
break;
}
case 1: {
$out->addWikiText($this->msg('oredict-manager-fail-general')->text());
break;
}
case 2: {
$out->addWikiText($this->msg('oredict-import-fail-nochange')->text());
break;
}
}
}
}

// Load data
Expand Down Expand Up @@ -99,9 +116,20 @@ private function createEntry(FormOptions $opts) {
return -2;
}

return OreDict::addEntry($mod, $item, $tag, $this->getUser(), $params);
$result = OreDict::addEntry($mod, $item, $tag, $this->getUser(), $params);
if ($result == false) {
return -2;
}
return $result;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ? : a thing in PHP?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also loosely checking against false in PHP, while rare, could end in disaster.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come to think about it, in Java the syntax would be !$results although I suspect that's not a possible thing in PHP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

return ($result === false ? -2 : $result);

Copy link
Member Author

@elifoster elifoster Aug 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alexia Is that check loose? I thought === false would be "loose" while == false would be strictly checking against false.

OreDict#addEntry returns either false or the entry ID. Is this a poor way to handle this?

Edit: Never mind, Peter corrected me. I had it flipped around in my head.

}

/**
* @param FormOptions $opts
* @return bool|int False if it was a deletion or there was nothing to update. These cases are not important for the
* purposes of this special page.
* Returns the integer returned by OreDict#editEntry denoting success of the update. This is
* actually useful.
*/
private function updateEntry(FormOptions $opts) {
$dbw = wfGetDB(DB_MASTER);
$entryId = intval($opts->getValue('entry_id'));
Expand All @@ -113,16 +141,16 @@ private function updateEntry(FormOptions $opts) {
'grid_params' => $opts->getValue('grid_params'),
);
if ($stuff->numRows() == 0) {
return;
return false;
}

// Try to delete before doing any other processing
if ($opts->getValue('delete') == 1) {
OreDict::deleteEntry($entryId, $this->getUser());
return;
return false;
}

OreDict::editEntry($ary, $entryId, $this->getUser());
return OreDict::editEntry($ary, $entryId, $this->getUser());
}

private function outputUpdateForm(stdClass $opts = NULL) {
Expand Down