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

[5.8] Correctly escape single quotes in json paths #28160

Merged
merged 3 commits into from
Apr 10, 2019
Merged

[5.8] Correctly escape single quotes in json paths #28160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be1896c
Correctly escape single quotes in json paths
brendt Apr 10, 2019
ea41e90
Improve naming
brendt Apr 10, 2019
93f59c4
Extra test
brendt Apr 10, 2019
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
2 changes: 2 additions & 0 deletions src/Illuminate/Database/Query/Grammars/Grammar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,8 @@ protected function wrapJsonFieldAndPath($column)
*/
protected function wrapJsonPath($value, $delimiter = '->')
{
$value = preg_replace("/([\\\\]+)?\\'/", "\\'", $value);
Copy link
Contributor

Choose a reason for hiding this comment

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

With preg_replace("/(\\\\)*'/", ...), the tests still pass. Is there a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You sure? Because it won't match \', this would cause only the ' to be escaped. Resulting in \\' as the end result, which in turn allows for the attack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't the tests check this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do: https://github.com/laravel/framework/pull/28160/files#diff-132870d3cbe60fc402aa1a0a8b47387cR2266

Copy link
Contributor

Choose a reason for hiding this comment

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

Same thoughts as staudenmeir, preg_replace("/\\\\*'/", ...) appears to be a strictly equivalent code. But simpler thus less error-prone.

I guess you got a bit confused between PHP and regex escaping ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Describing the code:

  • replace: quote preceded by 0 or more backslashes
  • with: quote preceded by one backslash


return '\'$."'.str_replace($delimiter, '"."', $value).'"\'';
}

Expand Down
23 changes: 23 additions & 0 deletions tests/Database/DatabaseQueryBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2252,6 +2252,29 @@ public function testMySqlWrappingJsonWithBooleanAndIntegerThatLooksLikeOne()
$this->assertEquals('select * from `users` where json_extract(`items`, \'$."available"\') = true and json_extract(`items`, \'$."active"\') = false and json_unquote(json_extract(`items`, \'$."number_available"\')) = ?', $builder->toSql());
}

public function testJsonPathEscaping()
{
$expectedWithJsonEscaped = <<<SQL
select json_unquote(json_extract(`json`, '$."\'))#"'))
SQL;

$builder = $this->getMySqlBuilder();
$builder->select("json->'))#");
$this->assertEquals($expectedWithJsonEscaped, $builder->toSql());

$builder = $this->getMySqlBuilder();
$builder->select("json->\'))#");
$this->assertEquals($expectedWithJsonEscaped, $builder->toSql());

$builder = $this->getMySqlBuilder();
$builder->select("json->\\'))#");
$this->assertEquals($expectedWithJsonEscaped, $builder->toSql());

$builder = $this->getMySqlBuilder();
$builder->select("json->\\\'))#");
$this->assertEquals($expectedWithJsonEscaped, $builder->toSql());
}

public function testMySqlWrappingJson()
{
$builder = $this->getMySqlBuilder();
Expand Down