Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115308
MIME-Version: 1.0
References: <CAFv4g+HRHrvCOTgpbVfCXj06De1kL+1+RwnGgq6tcyLNAtLfQA@mail.gmail.com>
In-Reply-To: <CAFv4g+HRHrvCOTgpbVfCXj06De1kL+1+RwnGgq6tcyLNAtLfQA@mail.gmail.com>
Date: Tue, 6 Jul 2021 08:38:39 +0200
Message-ID: <CAFPFaMJs7qvko=tfBsXuwCqMV3VqXL4TjO2aKWWPjrAicZzM9w@mail.gmail.com>
To: Craig Francis <craig@craigfrancis.co.uk>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000caf41505c66eac3b"
Subject: Re: [PHP-DEV] [RFC] [VOTE] is_literal
From: george.banyard@gmail.com ("G. P. B.")

--000000000000caf41505c66eac3b
Content-Type: text/plain; charset="UTF-8"

On Mon, 5 Jul 2021 at 20:15, Craig Francis <craig@craigfrancis.co.uk> wrote:

> Hi Internals,
>
> I have opened voting on https://wiki.php.net/rfc/is_literal for the
> is-literal function.
>
> The vote closes 2021-07-19
>
> The proposal is to add the function is_literal(), a simple way to identify
> if a string was written by a developer, removing the risk of a variable
> containing an Injection Vulnerability.
>
> This implementation is for literal strings ONLY (after discussion over
> allowing integers) and, thanks to the amazing work of Joe Watkins, now
> works fully with compiler optimisations, interned strings etc.
>
> Craig
>

Hi Craig,

Although I think the idea of the feature is useful,
I'm not so sure about the implementation.

I watched the talk you referenced a couple of times at different points in
time (the first being a couple of years back), and I fail to see how this
RFC is a similar implementation to it. As how they do it at Google is to
have it part of the type systems (arguably in a weird way but nonaless),
and due to the language being compiled the compiler will just flat out
refuse to produce an executable if the types mismatch.

From my understanding, the RFC's implementation is similar to what Google
does, which is to "annotate" the string, but without having the guarantees
of a compiler to back it up. This approach is totally reasonable for static
analysis, as running it is akin to the compilation step in checking the
validity.
However, having this approach built into the language itself seems rather
problematic to me.
Ideally we would want to assign a variable to be of 'literal' type to
ensure none of the actions applied to it demote it from being a literal,
and when such a demotion would occur, for it to TypeError.
Due to PHP's nature we cannot do this (yet?), therefore overloading the
concatenation operation seems rather unwise.
The case where concatenation between a literal and a non-literal happens,
without error, is very similar to passing around a nullable type until one
function/method/property doesn't accept null where it blows up into your
face, and you need to track down where on earth did the null value came
from, which might be multiple calls prior.
And there has been a hell of a lot of talks/articles/etc. about *not* using
nullable types due to this issue.

This is I think the main issue with the current shape of the proposal.
This implementation will detect certain security issues, but finding the
root cause for them is going to be rather complicated, as the concatenation
operation is basically kicking the can down the road about the
responsibility of checking whether or not the result is a literal.
Whereas using a function like concat_literal() which checks that the inputs
are indeed literals provides immediate feedback that the type constraint is
not being violated.

Due to this reason, I'm voting against this proposal.

Best regards,

George P. Banyard

--000000000000caf41505c66eac3b--