Optionally escape output of devel/php form [#2999049]

Problem/Motivation

If the output of the code executed on what a user submits on the devel/php form is html, for example when making a curl request to a site, then the html is currently rendered by the browser, which is not only an inconvenience, but could result in malicious JS being executed, etc.

Example:

$curl = curl_init('https://google.com');
$result = curl_exec($curl);
print($result);
curl_close($curl);

Proposed resolution

Provide a checkbox to allow the user to optionally escape all output.

Remaining tasks

Do it.

User interface changes

New checkbox on the devel/php form.

API changes

None.

Data model changes

None.

Comment	File	Size	Author
#4	2999049-4.patch	1.56 KB	manuel garcia
#4	interdiff-2999049-2-4.txt	1.34 KB	manuel garcia
#4	Screenshot from 2018-09-30 11-05-45.png	38.77 KB	manuel garcia
#4	Screenshot from 2018-09-30 11-05-30.png	24.77 KB	manuel garcia
#2	2999049-2.patch	640 bytes	manuel garcia

Comments

Comment #1

12 September 2018 at 13:45

Manuel Garcia created an issue. See original summary.

Comment #2

manuel garcia commented 12 September 2018 at 13:46

Status:

Active

» Needs review

Status	File	Size
new	2999049-2.patch	640 bytes

Kindly review :)

Comment #3

salvis

he/his

commented 29 September 2018 at 15:23

Status:

Needs review

» Needs work

I see your point, but making such a change may make people unhappy who have been fine with the current behavior for many years. Personally, I can't remember ever outputting HTML, but you can do it if you try hard enough, of course.

I could see having a persistent "Escape HTML output" checkbox on the devel/php page that would let you choose which way to go. This would at the same time alert users to the potential risk. The initial state (default) should be OFF, as it is now.

Comment #4

manuel garcia commented 30 September 2018 at 15:35

Title:	Escape output of devel/php form	» Optionally escape output of devel/php form
Issue summary:	View changes

Status	File	Size
new	Screenshot from 2018-09-30 11-05-30.png	24.77 KB
new	Screenshot from 2018-09-30 11-05-45.png	38.77 KB
new	interdiff-2999049-2-4.txt	1.34 KB
new	2999049-4.patch	1.56 KB

Thanks @salvis for taking the time to have a look at this. I agree this would be better if it was optional, doing so in this patch.

You can check the difference that having the checkbox on or off makes using this example:

$curl = curl_init('https://google.com');
$result = curl_exec($curl);
print($result);
curl_close($curl);

Checkbox off:
Escape HTML disabled

Checkbox on:
Escape HTML enabled

Another place where security isn’t a concern but it would still be useful is when using Drupal's HTTP client. If you execute this for example:

$client = \Drupal::httpClient();
$result = $client->get('https://google.com');
print($result->getBody()->getContents());

Without the checkbox, you will not see any output displayed.
With the checkbox enabled, you will see the source code of the page returned.

I've included a description on the checkbox but not very convinced about it, please let me know what you think :)

Comment #5

manuel garcia commented 30 September 2018 at 15:32

Status:

Needs work

» Needs review

Comment #6

salvis

he/his

commented 2 October 2018 at 00:52

2 files were hidden/shown/deleted

Status	File	Size
hidden	Screenshot from 2018-09-30 11-05-30.png	24.77 KB
hidden	Screenshot from 2018-09-30 11-05-45.png	38.77 KB

Very nice, thank you for the good illustrations!

Maybe it would be easier to understand if we invert the checkbox, something like

[ ] Allow interpreting HTML
If the output is HTML, turning this ON will allow your browser to interpret it, including possibly malicious content. The safe choice is OFF.

Originally, I leaned towards making it completely persistent, in order to preserve the current behavior as much as possible, but now I start to think we could turn escaping on whenever we initially show the form, and just preserve the state across executions.

Producing and wanting to see interpreted HTML is probably quite rare, and it typically makes more sense to see the HTML. Getting it rendered would just be a matter of activating the checkbox and hitting [Execute] again, after you've seen what it produces. This would also make the behavior more predictable, especially if more than one person is using it.

Alternatively, we could just have a second button, like [Execute and render]...

What do you and others think?

Comment #7

manuel garcia commented 2 October 2018 at 08:36

I am a bit puzzled by why when using curl the HTML is not escaped by drupal_set_message() to be honest. This seems to be the only situation where this happens:

1. If you do just return '<bold>test</bold>';, what gets printed is only <pre>bold</pre>.
2. If you do the following the output is actually empty, unless you use the checkbox:

$client = \Drupal::httpClient();
$result = $client->get('https://google.com');
print($result->getBody()->getContents());

3. However if you use the curl code in the description:

$curl = curl_init('https://google.com');
$result = curl_exec($curl);
print($result);
curl_close($curl);

Then the output is actually rendered html, unless you use the checkbox.

So as it stands right now, having the checkbox say Allow interpreting HTML could be misleading. No matter if the checkbox is on or not, we're actually not interpreting HTML in nearly all situations (except when using curl apparently).

Therefore giving the option to display escaped html instead would make sense in my opinion. Say have the checkbox read Display escaped HTML output, or Escape HTML output?