Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114858
MIME-Version: 1.0
References: <CAFv4g+F-7F7o6va5wCrojKe+Uysx2Ec3H4yV_mUZYEyNm3MkdQ@mail.gmail.com>
 <CAOwTYAt3rztsZ1BTjEbsonqPR_thgG7UqEUO2pKeN=dEg1jqgg@mail.gmail.com>
In-Reply-To: <CAOwTYAt3rztsZ1BTjEbsonqPR_thgG7UqEUO2pKeN=dEg1jqgg@mail.gmail.com>
Date: Mon, 14 Jun 2021 13:29:53 +0100
Message-ID: <CA+kxMuSJvk2bDM1dQnZHxgiO+YrYhbjS8BziAy+YGSSW8cR1nw@mail.gmail.com>
To: Joe Watkins <krakjoe@gmail.com>
Cc: Craig Francis <craig@craigfrancis.co.uk>, PHP internals <internals@lists.php.net>, 
	Matthew Brown <matthewmatthew@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] [RFC] is_literal
From: Danack@basereality.com (Dan Ackroyd)

Hi Craig, Joe,

While I think the idea behind this RFC is great, the current
implementation is terrible, as it will cause some fatal errors in
production.

# The problem

Imagine we have this code:

function getInfoPanel(UserPreferences $up): string
{
    $html = "<div style='color: " . $up->getColor() . "'>";
    $html .= // more html here
    $html .= "</div>"

    return $html;
}

class UserPreferences {
    private DB $db;

    function getColor(): string {
        $preferredColor = $this->db->getColor();

        if ($preferredColor === 'light') {
            return '#fff';
        }

        if ($preferredColor === 'dark') {
            return '#000';
        }
        return '#fff'; // default is light
    }
}

And then the string returned by getInfoPanel() is used elsewhere that
does a check for is_literal. That check will pass.

Now imagine a feature request comes in, to allow people to customise
the color of their UI, so the UserPreferences code is changed to:

class UserPreferences {
    private DB $db;

    function getColor(): string {
        $preferredColor = $this->db->getColor();

        if ($preferredColor === 'light') {
            return '#fff';
        }

        if ($preferredColor === 'dark') {
            return '#000';
        }
        return $preferredColor; // user has set custom color.
    }
}

Assume that both UserPreferences and getInfoPanel code is covered by
unit tests. The developer who made that change would run the tests,
see the tests pass and then push their code live. At which point, it
would cause an error in production, as the string returned would not
pass the is_literal check.

This would be a complete pain in the butt to fix.

The only information would be that the html string is not
literal....but there wouldn't be any info about which bit was the
problem. Someone would have to manually check which code had been
called to produce the non-literal string.

That could mean  the site would be unavailable for users that had set
a custom color. Or more likely, the developers would just turn off the
is_literal check or at least downgrade it to a mere logging error aka
something to be ignored.


# How to fix this in the RFC

By not carrying the literal flag on string concatenation, but
requiring developers to use functions like literal_concat() and
literal_implode(), the 'false affordance'* goes away:

function getInfoPanel(UserPreferences $up): string
{
    $html[] = "<div style='color: " . $up->getColor() . "'>";
    $html[] = // more html here
    $html[] = "</div>"

    return literal_concat(...$html);
}


Now, when the feature request comes in to allow users to set custom
colors, the code will fail on the literal_concat. When that function
checks internally that all the strings passed to it are literal, it
will throw an exception from the function that has made the bad
assumption about whether a string is user input or not.

Yes, it's slightly ironic that making errors be louder and happen
earlier in the program is better than making them happen later.

# Wouldn't forcing people to use literal_concat() and
literal_implode() be annoying

I don't think so.

The code change required is pretty small, and can be done either by a
junior developer, or could be automated by things like
https://github.com/rectorphp/rector. Even if there are 100 places
where an existing series of string concatenations needs to be
converted to chucking in an array, and then using literal_concat() and
each of those takes 10 minutes... that's 2 days to do the conversion.
In return for which the application becomes a lot more secure than it
was previously.

But additionally, I don't think many people are actually going to be
using literal_concat() in many places. My guess is that the vast
majority of places where you are combining bits of strings together,
you would realise that you can switch to using type-specific escaping,
as you're going to need to do the escaping anyway.


# Is this a problem in other languages?

The similar implementation used in Google is used for Java. As Java is
statically compiled, any attempt to pass a non-literal string to a
function that expects a literal string fails to compile. And if it
won't compile, it won't run, and so won't have errors in production.

The only thing that needs to be changed in my opinion is not carrying
the literal flag through string concatenation. Although that would be
slightly more work to use the feature, avoiding having inevitable and
hard to debug errors in production is the trade-off.

cheers
Dan
Ack


* false affordance - something that works, except when it doesn't.