Should explode on literal-string returns a array<literal-string>

[VincentLanglet]

Hi @craigfrancis

I don't know a good place for such discussion.
Since PHPStan enforcing literal-string and you already participated on some topics, I think here can be ok.

Since implode on array<literal-string> returns a literal-string.
Couldn't we consider that the opposite on explode is true ?

Currently it's annoying for static analysis that explode(',', implode(',', $literalString)) lose the literal-string type.

I know there is currently nothing about explode in the is_literal RFC https://wiki.php.net/rfc/is_literal
but, unless I misunderstood this one was refused/postponed and is still not implemented in PHP 8.3.

So we should consider that

• The RFC could/should evolved.
• At worst, the RFC will never be accepted.

WDYT about adding explode in the RFC ? Then PHPStan could update the DynamicReturnTypeExtension.

In the same way, there is the trim method which can be consider as the opposite of concatenation.

[craigfrancis]

Hi Vincent,

Thanks for looking at these things, I really appreciate it.

As to explode() being able to return a literal-string, it's something I'd like to avoid.

In short, I'm trying to avoid the problem of trying to work out which functions should be allowed or not (each function added to the list could have unintended consequences).

If you look at the Python LiteralString implementation, it has a fairly long list of methods that preserve LiteralString, and as far as I'm aware, this has been the only complaint against PEP 675 - i.e. it's too complicated, and we don't know that they are all safe, e.g. format()... whereas the PHP implementation only allows str_repeat(), str_pad(), implode(), and join().

---

While string concatenation is nearly always fine (joining trusted strings together, as often happens with HTML and SQL) there is a very small risk with the security guarantees (e.g. a developer could create an array of single character values, and allow user input to create a literal-string based on those values), but the developer knows they are doing this on purpose. So, considering concatenation is used is nearly all existing code, and it does not really cause a problem, it's allowed... whereas other string manipulations aren't as clear; while explode() might be fine, substr() and similar are not:

$url    = substr('https://example.com/js/a.js?v=55', 0, $length);
$html   = substr('<a href="#">#</a>', 0, $length);

i.e. If $url was used in a Content-Security-Policy, the query string needs to be removed; but, if we assume $length was somehow controlled by an attacker, as more of the string is removed, the more resources are allowed (“https:” basically allows resources from anywhere). And, with the HTML example, moving from the tag content to the attribute can be a problem (while HTML Templating Engines should be fine, unfortunately libraries like Twig are not currently context aware, so you need to change from the default 'html' encoding to 'html_attr' encoding).

https://wiki.php.net/rfc/literal_string#string_splitting

And that's why I'm trying to avoid any functions from being able to return a literal-string, the only reason I included str_repeat(), str_pad(), implode(), and join() is because they are simpler ways of doing string concatenation.

That said, if there is a really good case to break this simple rule (concatenation only), I'd be happy to discuss it.

Craig

[VincentLanglet]

While string concatenation is nearly always fine (joining trusted strings together, as often happens with HTML and SQL) there is a very small risk with the security guarantees (e.g. a developer could create an array of single character values, and allow user input to create a literal-string based on those values), but the developer knows they are doing this on purpose. So, considering concatenation is used is nearly all existing code, and it does not really cause a problem, it's allowed... whereas other string manipulations aren't as clear; while explode() might be fine, substr() and similar are not:

$url = substr('https://example.com/js/a.js?v=55', 0, $length);
$html = substr('<a href="#">#</a>', 0, $length);

You're quoting complain about Python-LiteralString being "too complicated" but on the opposite side we already have developer complaining about the sprintf issue https://github.com/phpstan/phpstan/issues?q=is%3Aissue+sprintf+literal+is%3Aclosed

I think it could help people to accept the "literal-string" feature if it could be easier to use. It's not easy currently, "Yes there is no risk of injection with your codebase using tons of sprintf but please rewrite everything to use standard concatenation in order to please static analysis please" is not easily listened in a developer team.

I understand the case with substring is not safe. But it can be explained because one of parameter is not a literal.
(The limitation here is the fact that a literal-int doesn't exist in PHP (But it does exist in Psalm ! https://psalm.dev/r/bba8c1f23a)

On other side, things like

• explode($literalString1, $literalString2)
• sprintf($literalString1, $literalString2, ...)
• trim($literalString1, $literalString2)

Seems safe to me. We could have a rule saying that if every parameters are literals, the result is a literal too. Do you have a counter example ?

And I dont think it's an issue if all the "literal" functions are not listed in the first RFC, we could have some which are added in the later RFC/PHP versions.

[craigfrancis]

sprintf() is a good counter example, while in theory it might be fine, we know it gets more complicated when a value is being coerced from one type to another (static analysis and php will not want to have to interrogate the format string to decide if coercion happened), we know that the locale can have an effect (example), and it's formatting features can affect the input in unexpected ways. It's why I'm hoping to work with Juliette (of PHPCompatibility fame, when we both aren't distracted with client work) so we can make a tool that can auto replace sprintf(), when it's only being used for string concatenation, to use standard concatenation (which is faster, and can be easier to read).

I really want literal-string to be easy as possible to use, but without introducing features that could break it, either by introducing a security problem, or by making it complicated (i.e. I don't want everyone to have to remember which functions are allowed or not; there is still a bit of me which is wondering if I should have removed all functions from the list).

I think trim() is a good example to explain the complexity problem, while in theory it's safe (I can't be 100% sure on that, some system are white-space sensitive, and it can take a second argument to remove other characters), it breaks the clear "developer defined string" definition, into something that's more "developer defined, but can be modified"... with functions like ltrim() and rtrim() also needing to be accepted? then there is strtoupper() and mb_strtoupper()? how about str_replace()? and should str_split() work if explode() works? etc, etc.

[VincentLanglet]

sprintf() is a good counter example, while in theory it might be fine, we know it gets more complicated when a value is being coerced from one type to another (static analysis and php will not want to have to interrogate the format string to decide if coercion happened), we know that the locale can have an effect (example), and it's formatting features can affect the input in unexpected ways.

Yes, that why I was only considering case when all the param where literal string.

sprintf($literalString1, $literalString2, ...);

I really want literal-string to be easy as possible to use, but without introducing features that could break it, either by introducing a security problem, or by making it complicated (i.e. I don't want everyone to have to remember which functions are allowed or not; there is still a bit of me which is wondering if I should have removed all functions from the list).

I think trim() is a good example to explain the complexity problem, while in theory it's safe (I can't be 100% sure on that, some system are white-space sensitive, and it can take a second argument to remove other characters), it breaks the clear "developer defined string" definition, into something that's more "developer defined, but can be modified"... with functions like ltrim() and rtrim() also needing to be accepted? then there is strtoupper() and mb_strtoupper()? how about str_replace()? and should str_split() work if explode() works? etc, etc.

I really do understand the idea @craigfrancis.

I was just trying to share some "user-experience" from different code base I had where I was trying to play with PHPStan and literal-string. While the current definition of literal-string might be easier to define and implements, I think in most of codebase it's harder to use because it will report a lot of query executed with non-literal-string which are still perfectly safe.

For instance I had a function like

/**
 * @param literal-string $url
 */
public function insertUrl(string $url) {
     $this->executeQuery('INSERT INTO urls (value, hash) VALUES ('.$url.', '.md5($url).');');
}

which was complaining about the fact executeQuery require a literal-string. I fixed the issue this way

/**
 * @param literal-string $url
 */
public function insertUrl(string $url) {
    /** @var literal-string $hash */
     $hash = md5($url);
     $this->executeQuery('INSERT INTO urls (value, hash) VALUES ('.$url.', '.$hash.');');
}

and I know $hash is not a real literal-string, but it's kinda because when stub were introduced for methods like executeQuery to ask for a literal-string as parameter we were "too strict" and instead of a pure literal-string we just need something like a safe-string.

In the same way, we sometimes need in our codebase to write things like

/** @var literal-string $limit */
$limit = (string) $someLimiteralInteger;
$this->executeQuery('SELECT * from urls limit '. $limit);

because the literal-int notion doesn't exist.

Hopefully, the literal-string phpdoc solve all the encountered issue so far. (Even if I understand it can be considered hacky)

[craigfrancis]

Out of interest, why (and how) are you adding those values directly into the SQL without any quoting or escaping? Normally I'd recommend using parameterised queries so you cannot make escaping mistakes (it also helps the database keep user values separate from the SQL, so there cannot be any parsing issues)... but I'm not sure what's happening in your examples (I'm also intrigued by the use of md5, but I best keep on topic).

And I did originally try using the word "safe" in an earlier version, and that did not go down well; the example I now give is execute_command('rm -rf ?', [$path]) where the first argument, as a literal-string, is safe in terms of an injection vulnerability, but I wouldn't like to see what happens if the attacker can provide any $path :-)

[VincentLanglet]

Out of interest, why (and how) are you adding those values directly into the SQL without any quoting or escaping? Normally I'd recommend using parameterised queries so you cannot make escaping mistakes (it also helps the database keep user values separate from the SQL, so there cannot be any parsing issues)... but I'm not sure what's happening in your examples (I'm also intrigued by the use of md5, but I best keep on topic).

First, you cannot use everything as a parameter in doctrine queries.
See doctrine/dbal#3459
So the example $this->executeQuery('SELECT * from urls limit '. $limit); cannot be solved.

And our codebase provide a method which does insert by batch.

    /**
     * @param array<literal-string> $fields
     * @param array<literal-string> $rows
     * @param positive-int          $chunkSize
     */
    public function chunkBatchAndExecute(array $fields, array $rows, int $chunkSize = 100): void
    {
        $tableName = $this->getClassMetadata()->getTableName();
        $fieldsString = implode(',', $fields);

        $chunks = array_chunk($rows, $chunkSize);
        foreach ($chunks as $chunk) {
            $rowsString = implode(',', $chunk);
            $sql = "INSERT IGNORE INTO {$tableName}({$fieldsString}) VALUES {$rowsString}";
            $this->getEntityManager()->getConnection()->executeStatement($sql);
        }
    }

It kinda seems overkill to refactor everthing to support parameters where all the existing query are

/** @param array<int> $translationIds */
public function batchInsert(int $projectId, int $urlId, array $translationIds): void
    {
        /** This could basically be considered as array<literal-string> */
        $values = array_map(static fn ($translationId) => "({$projectId}, {$urlId}, {$translationId})", $translationIds);

        $this->chunkBatchAndExecute(
            ['project_id', 'url_id', 'translation_id'],
            $values,
            1000
        );
    }

Would you expect something like

/** @param array<int> $translationIds */
public function batchInsert(int $projectId, int $urlId, array $translationIds): void
    {
        $values = array_map(static fn ($translationId, key) => [
            'row' => "(:projectId{$key}, :urlId{$key}, :translationId{$key})",
            'param' => ['projectId{$key}' => $projectId, 'urlId{$key}' => $urlId, 'translationId{$key}' => $translationId],
        ], $translationIds, array_keys($translationIds));

        $this->chunkBatchAndExecute(
            ['project_id', 'url_id', 'translation_id'],
            $values,
            1000
        );
    }

And modifying chunkBatchAndExecute in consequence ?