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.

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

[Bug]: String quoting #10

Open
ineersa opened this issue Jul 8, 2024 · 2 comments
Open

[Bug]: String quoting #10

ineersa opened this issue Jul 8, 2024 · 2 comments
Labels
bug Something isn't working help wanted Extra attention is needed needs to be discussed This issue need to be discussed

Comments

@ineersa
Copy link
Contributor

ineersa commented Jul 8, 2024

What happened?

Unsafe strings quoting.

I think that such features should be implemented inside driver, similar to
https://github.com/php/php-src/blob/master/ext/pdo_sqlite/sqlite_driver.c#L223

This opens security vulnerabilities for this library separate from sqlite3 and libsql.
Also this code is duplicated in 2 classes, which can lead to errors.

How to reproduce the bug

public static function escapeString($value)
    {
        // DISCUSSION: Open PR if you have best approach
        $escaped_value = str_replace(
            ['\\', "\x00", "\n", "\r", "\x1a", "'", '"'],
            ['\\\\', '\\0', '\\n', '\\r', '\\Z', "\\'", '\\"'],
            $value
        );

        return $escaped_value;
    }

    public function quote(string $value): string
    {
        return self::escapeString($value);
    }

Package Version

latest

PHP Version

8.3.8

Laravel Version

11

Which operating systems does with happen with?

Linux

Notes

No response

@ineersa ineersa added the bug Something isn't working label Jul 8, 2024
@darkterminal darkterminal added needs to be discussed This issue need to be discussed help wanted Extra attention is needed labels Jul 9, 2024
@ineersa
Copy link
Contributor Author

ineersa commented Jul 17, 2024

@darkterminal
I've added some basic unit test for your new approach with strtr - https://github.com/tursodatabase/turso-driver-laravel/pull/15/files#diff-66fb0fca9b804fc81e208d29c1eaf187c0e2c7a06aa5934f13871cefdd1b8440R34

This approach is very similar to https://www.php.net/manual/en/mysqli.real-escape-string.php and actually can have problems with some character sets same as built in PHP function.

Also I'm kinda confused why you are rejecting SQLite way of doing it, via using internal SQLite quoting mechanism in extension.

Also from SQLite documentation - https://www.sqlite.org/lang_expr.html

A string constant is formed by enclosing the string in single quotes ('). A single quote within the string can be encoded by putting two single quotes in a row - as in Pascal. C-style escapes using the backslash character are not supported because they are not standard SQL.

So single quotes should be doubled instead of backslash escaping.

@darkterminal
Copy link
Collaborator

It's mentioned here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed needs to be discussed This issue need to be discussed
Projects
None yet
Development

No branches or pull requests

2 participants