Sanitize annotations #52

nlisgo · 2018-01-23T16:19:01Z

No description provided.

nlisgo · 2018-01-23T16:27:08Z

I believe that I am experiencing this issue: ezyang/htmlpurifier#71

nlisgo · 2018-01-23T21:24:32Z

src/Annotations/AppKernel.php

@@ -103,6 +104,7 @@ public function __construct(string $environment = 'dev')
                'authority' => '',
                'group' => '',
            ],
+            'html_purifier.cache_dir' => $config['html_purifier']['cache_dir'] ?? __DIR__.'/../../var/html_purifier/cache',


We need to create the /srv/annotations/var/html_purifier/cache in all environments in order for this to pass.

nlisgo · 2018-01-23T21:32:46Z

src/Annotations/AppKernel.php

@@ -103,6 +104,12 @@ public function __construct(string $environment = 'dev')
                'authority' => '',
                'group' => '',
            ],
+            'html_purifier' => ($config['html_purifier'] ?? []) + [
+                'Cache.SerializerPath' => __DIR__.'/../../var/cache/html_purifier',


We need to create this directory in all environments: /srv/annotations/var/cache/html_purifier.

Is the Directory /srv/annotations/vendor/ezyang/htmlpurifier/library/HTMLPurifier/DefinitionCache/Serializer not writable, please chmod to 777 error related to this?

It's related to that. I will work on MarkdownSanitizerTest so that it doesn't attempt to write to cache.

giorgiosironi · 2018-01-24T10:07:35Z

src/Annotations/Serializer/CommonMark/Block/Renderer/LatexRenderer.php

@@ -1,26 +0,0 @@
-<?php


nice to see these classes simplified as function callbacks

giorgiosironi · 2018-01-24T10:10:09Z

src/Annotations/Serializer/CommonMark/MarkdownSanitizer.php

+        $this->htmlPurifier = $htmlPurifier;
+    }
+
+    public function parse(string $input) : string


This API conversion pipeline needs its step to be listed as documentation in the README:

1. Hypothesis API to `Model` objects 2. Denormalization 2.1 Conversion to HTML (uses library X) 2.2 HTML purification (uses library Y) ...

(ADRs are not a good format for that)

The obvious questions that needs to be answered there is why we sanitize intermediate HTML and convert back to Markdown rather than sanitizing the final HTML

thewilkybarkid · 2018-01-24T14:39:00Z

composer.json

@@ -29,7 +29,6 @@
        "guzzlehttp/guzzle": "^6.3",
        "knplabs/console-service-provider": "^2.1",
        "league/commonmark": "^0.17.0",
-        "psr/container": "^1.0",


Unrelated to this PR, but it should be included since AppKernel implements ContainerInterface.

thewilkybarkid · 2018-01-24T14:39:51Z

src/Annotations/AppKernel.php

+                'allow_unsafe_links' => false,
+            ],
+            'html_purifier' => ($config['html_purifier'] ?? []) + [
+                'Cache.SerializerPath' => __DIR__.'/../../var/cache/html_purifier',


Could we extract the base cache path as its own parameter?

You mean var/cache/ or /srv/annotations/var/cache?

Something like

annotations/src/Annotations/AppKernel.php

Line 81 in 83c91e5

'cache.path' => $config['cache']['path'] ?? __DIR__.'/../../var/cache',

giorgiosironi · 2018-01-24T14:43:23Z

README.md

+## Journey of a request to the annotations api
+1. Query received via the `annotations` api.
+1. Retrieve annotation listing from `hypothes.is/api` based on query parameters.
+1. Denormalize the annotations into an object model.


[[HypothesisClient\Model]`?

thewilkybarkid · 2018-01-24T14:43:39Z

src/Annotations/Serializer/CommonMark/functions.php

 function clean_paragraph($text)
 {
    $allowed_tags = '<i><sub><sup><span><del><math><a><br><caption>';

    return strip_tags($text, $allowed_tags);
 }
+
+function escape_math($text)


These should have types. (function escape_math(string $text) : string.)

giorgiosironi · 2018-01-24T14:44:23Z

README.md

+1. Query received via the `annotations` api.
+1. Retrieve annotation listing from `hypothes.is/api` based on query parameters.
+1. Denormalize the annotations into an object model.
+1. Normalize the annotations to prepare the response which conforms to the eLife json schema. [HTMLPurifier](http://htmlpurifier.org/) is used to sanitise content values. We rely on [CommonMark](http://commonmark.thephpleague.com/) to step through the Markdown content and prepare the structure that we need.


would invert the two sentences: first we step through, and while stepping through we can sanitise?

giorgiosironi · 2018-01-24T14:48:04Z

src/Annotations/AppKernel.php

+            'html_purifier' => ($config['html_purifier'] ?? []) + [
+                'Cache.SerializerPath' => __DIR__.'/../../var/cache/html_purifier',
+            ],
+            'html_converter' => ($config['html_converter'] ?? []) + [


I think this is a leftover to be removed?

Will remove

giorgiosironi · 2018-01-24T14:53:35Z

src/Annotations/Serializer/CommonMark/functions.php

+function escape_math(string $text) : string
+{
+    // Escape MathML.
+    $escaped = preg_replace_callback('~(?P<before><math[^>]*>)(?P<mathml>.*)(?P<after></math>)~s', function ($match) {


I wonder if the regex is greedy and what happens with multiple tags. I'll ensure there is a test for that.

I will add a test.

Named groups aren't needed? Could be simplified to:

function escape_math(string $text) : string { $escape = function (array $matches) { return Xml::escape($matches[0]); }; return preg_replace_callback_array([ '~(<math[^>]*>.*?</math>)~s' => $escape, '~(\$\$.+?\$\$)~s' => $escape, ], $text); }

giorgiosironi · 2018-01-24T14:58:22Z

src/Annotations/Serializer/CommonMark/functions.php

+    }, $escaped);
+
+    return $escaped;
+}


also, I'm not sure whether an "unescaping" is necessary

unescaping is not necessary as LaTeX and MathML are being presented as escaped in the api response.

thewilkybarkid · 2018-01-24T15:01:12Z

src/Annotations/Serializer/HypothesisClientAnnotationNormalizer.php

 use League\CommonMark\Block\Element;
 use League\CommonMark\DocParser;
 use League\CommonMark\ElementRendererInterface;
 use Psr\Log\LoggerInterface;
 use Symfony\Component\Serializer\Normalizer\NormalizerAwareInterface;
 use Symfony\Component\Serializer\Normalizer\NormalizerAwareTrait;
 use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
+use function eLife\Annotations\Serializer\CommonMark\escape_math;

 final class HypothesisClientAnnotationNormalizer implements NormalizerInterface, NormalizerAwareInterface


NormalizerAwareInterface not needed anymore?

Will remove

thewilkybarkid · 2018-01-24T15:03:19Z

src/Annotations/Serializer/HypothesisClientAnnotationNormalizer.php

+
+    private function purify(string $text) : string
+    {
+        return $this->htmlPurifier->purify($text);


I suspect we'll want to cache the results of these (IIRC the library doesn't do this).

This class is feeling a bit bloated now, knowing how to convert all the bits and also purify them. Can purification happen before/after?

Purification happening after is a possibility. I implemented a solution where it was happening before but it wasn't optimal.

In order for it to happen after the HypothesisClientAnnotationNormalizer would return an unsanitized response and then we would need a class so step through each item in the content array. So, I would need 2 places where the structure of the response was known. Perhaps we can have a chat about this tomorrow.

I've done a refactor here which means I can remove purify from this class. See what you think.

thewilkybarkid · 2018-01-25T08:38:31Z

src/Annotations/Serializer/HypothesisClientAnnotationNormalizer.php

@@ -145,8 +143,8 @@ private function processListBlock(Element\ListBlock $block)
                foreach ($item->children() as $child) {
                    if ($child instanceof Element\ListBlock) {
                        $items[] = [$render($child)];
-                    } else {
-                        $items[] = $this->htmlRenderer->renderBlock($child);
+                    } elseif ($item = $this->htmlRenderer->renderBlock($child)) {


Not introduced by this PR, but processText() has already rendered the whole list then thrown it away (in processText()) in favour of doing each item here?

The render of the list converts the whole list to HTML. We need to step through the list to separate out each item. I will see if I can improve this.

You just need to stop it rendering the whole thing if it's a list, it's wasted effort.

thewilkybarkid · 2018-01-25T08:43:22Z

src/Annotations/Serializer/CommonMark/HtmlRenderer.php

+use League\CommonMark\Environment;
+use League\CommonMark\HtmlRenderer as CommonMarkHtmlRenderer;
+
+class HtmlRenderer extends CommonMarkHtmlRenderer


The name doesn't describe what it's doing (HtmlPurifierRenderer?). Better to use composition too.

Misses out renderInlines() too? Edit: or are 'inlines' always included inside blocks?

I was working under that assumption. I will verify it by introducing a test.

thewilkybarkid · 2018-01-25T08:45:45Z

src/Annotations/Serializer/HypothesisClientAnnotationNormalizer.php


-final class HypothesisClientAnnotationNormalizer implements NormalizerInterface, NormalizerAwareInterface
+final class HypothesisClientAnnotationNormalizer implements NormalizerInterface


The trait needs to go too.

thewilkybarkid · 2018-01-25T08:48:14Z

composer.json

@@ -24,10 +24,12 @@
        "elife/content-negotiator": "^1.0",
        "elife/logging-sdk": "^1.0@dev",
        "elife/ping": "^1.0",
+        "ezyang/htmlpurifier": "dev-master",


Are we relying on commits that haven't been tagged yet?

thewilkybarkid · 2018-01-25T08:56:09Z

src/Annotations/Serializer/HypothesisClientAnnotationNormalizer.php

@@ -80,7 +80,7 @@ public function normalize($object, $format = null, array $context = []) : array

    private function processText(string $text) : array
    {
-        $blocks = $this->docParser->parse($text)->children();
+        $blocks = $this->docParser->parse(escape_math($text))->children();


This means we're escaping Math elements before, then purifying after? This could also compose HtmlRenderer?

(Means more regexes, but we're going to be better off caching the whole thing anyway.)

The Math elements are escaped while the content is in Markdown. We need to determine what CommonMark Block elements that we have before rendering the HTML.

When we need to sanitise MathML we may need a different library then HTMLPurifier: http://htmlpurifier.org/phorum/read.php?5,8091,8091

You mean that CommonMark does something with HTML element? Assumed it wouldn't...

thewilkybarkid · 2018-01-25T09:46:18Z

src/Annotations/Serializer/CommonMark/functions.php

-function clean_paragraph($text)
+use League\CommonMark\Util\Xml;
+
+function clean_paragraph(string $text) : string
 {
    $allowed_tags = '<i><sub><sup><span><del><math><a><br><caption>';


What's the difference between <math> here and escape_math()?

Is it a requirement to drop MathML? Unlike LaTeX we can display it...

Not a requirement. I think it will not be too much trouble do capture and display it particularly when it's in a block rather than inline. But I will take that on as a followup to this. <math> was left in while I was deciding how to treat mathml. I will remove it for now while we are simply escaping MathML.

thewilkybarkid · 2018-01-25T09:51:21Z

tests/Annotations/Serializer/HypothesisClientAnnotationNormalizerWebTest.php


 /**
 * @covers \eLife\Annotations\Serializer\HypothesisClientAnnotationNormalizer
 */
-final class HypothesisClientAnnotationNormalizerTest extends PHPUnit_Framework_TestCase
+final class HypothesisClientAnnotationNormalizerWebTest extends WebTestCase


WebTest? Should just extend ApplicationTestCase if you want AppKernel.

thewilkybarkid · 2018-01-25T13:22:18Z

src/Annotations/Serializer/CommonMark/HtmlPurifierRenderer.php

+    /**
+     * @param Environment $environment
+     */
+    public function __construct(Environment $environment, HTMLPurifier $htmlPurifier)


I meant this should compose the original HtmlRender rather than duplicate everything...

* Exposing problem with greedy regex * Make <math> regex ungreedy

thewilkybarkid · 2018-01-26T13:57:32Z

src/Annotations/AppKernel.php

+            return new HTMLPurifier($app['html_purifier']);
+        };
+
+        $this->app['annotation.serializer.common_mark.math_escape_renderer'] = function () {


Prefer $this->app->extend() to making separate services.

@thewilkybarkid Could you submit a PR for this or provide a code snippet? I see different uses for extend, I'm not sure what the appropriate one is here.

thewilkybarkid · 2018-01-26T13:58:15Z

src/Annotations/Serializer/CommonMark/Block/Renderer/CodeRenderer.php

@@ -17,6 +17,6 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende
            throw new \InvalidArgumentException('Incompatible block type: '.get_class($block));
        }

-        return Xml::escape($block->getStringContent());
+        return Xml::escape(trim($block->getStringContent()));


Whitespace might be meaningful?

thewilkybarkid · 2018-01-26T14:55:41Z

@nlisgo Are you rebasing this branch? Easier to do a merge (keeps the history in the PR intact). Since we're doing squash commits we don't need to worry about the extra commit created.

(And you can use the update branch button here for reasonably trivial updates.)

…anitize-annotations

thewilkybarkid · 2018-01-29T11:41:45Z

src/Annotations/AppKernel.php

            return new HtmlRenderer($this->app['annotation.serializer.common_mark.environment']);
        };

+        $this->app['annotation.serializer.html_purifier'] = function (Application $app) {
+            return new HTMLPurifier($app['html_purifier']);


Using $this->app elsewhere.

…anitize-annotations

thewilkybarkid · 2018-01-29T12:21:31Z

tests/Annotations/Serializer/CommonMark/HtmlPurifierRendererTest.php

@@ -26,7 +26,7 @@ public function setup()
        $this->renderer = $this->getMockBuilder(ElementRendererInterface::class)
            ->getMock();
        $this->abstractBlock = $this->createMock(AbstractBlock::class);
-        $this->htmlPurifierRenderer = new HtmlPurifierRenderer($this->renderer, new HTMLPurifier());
+        $this->htmlPurifierRenderer = new HtmlPurifierRenderer($this->renderer, new HTMLPurifier(['Cache.SerializerPath' => '/tmp']));


sys_get_temp_dir()

thewilkybarkid

Some stylistic stuff (eg final), but .

nlisgo commented Jan 23, 2018

View reviewed changes

nlisgo force-pushed the sanitize-annotations branch from 17cd325 to 813c2d5 Compare January 24, 2018 09:28

giorgiosironi reviewed Jan 24, 2018

View reviewed changes

nlisgo force-pushed the sanitize-annotations branch 3 times, most recently from eedba3e to ec2af83 Compare January 24, 2018 14:24

nlisgo requested a review from thewilkybarkid January 24, 2018 14:37

thewilkybarkid reviewed Jan 24, 2018

View reviewed changes

giorgiosironi reviewed Jan 24, 2018

View reviewed changes

thewilkybarkid reviewed Jan 24, 2018

View reviewed changes

giorgiosironi reviewed Jan 24, 2018

View reviewed changes

thewilkybarkid reviewed Jan 24, 2018

View reviewed changes

nlisgo force-pushed the sanitize-annotations branch from 9bf390b to bd51de7 Compare January 24, 2018 15:27

thewilkybarkid reviewed Jan 25, 2018

View reviewed changes

nlisgo force-pushed the sanitize-annotations branch from fdd675e to c007395 Compare January 25, 2018 13:04

thewilkybarkid reviewed Jan 25, 2018

View reviewed changes

nlisgo and others added 10 commits January 26, 2018 13:35

NormalizerAwareInterface not needed anymore

a78bb5f

html_converter no longer needed

5acdb75

Improve README.md

7893577

Extend HtmlRenderer to integrate HTMLPurifier

660d0e7

Exposing problem with greedy regex (#57)

b7a3585

* Exposing problem with greedy regex * Make <math> regex ungreedy

Simplify regex

8eb26fc

Address various feedback

f7827a1

Remove <math> from the allowed tags

ac4e6aa

Improve HTML purifier and MathML and LaTeX handling

75389de

Improve readability

f226152

nlisgo force-pushed the sanitize-annotations branch from 3f0ffe3 to f226152 Compare January 26, 2018 13:53

thewilkybarkid reviewed Jan 26, 2018

View reviewed changes

nlisgo added 5 commits January 28, 2018 00:32

Move tests to appropriate unit tests

08a12b2

Merge branch 'develop' of github.com:elifesciences/annotations into s…

15f3555

…anitize-annotations

Permit whitespace at the end of code block

c0e7750

Add functional test for sanitisation of annotations

d65d559

Expose configuration option for cache folder

3d9de0b

thewilkybarkid reviewed Jan 29, 2018

View reviewed changes

Merge branch 'develop' of github.com:elifesciences/annotations into s…

b548a3b

…anitize-annotations

nlisgo force-pushed the sanitize-annotations branch from d24e296 to b548a3b Compare January 29, 2018 12:09

Write purifier cache to tmp folder

1bf4471

thewilkybarkid reviewed Jan 29, 2018

View reviewed changes

thewilkybarkid and others added 3 commits January 29, 2018 12:31

Don't create a new service for each element renderer (#65)

eef96bb

Use sys_get_temp_dir()

22ffb73

Address inconsistencies with reference to application in kernel

e6fd322

thewilkybarkid approved these changes Jan 29, 2018

View reviewed changes

nlisgo merged commit b3a3305 into develop Jan 29, 2018

thewilkybarkid deleted the sanitize-annotations branch January 30, 2018 07:35


		final class HypothesisClientAnnotationNormalizer implements NormalizerInterface, NormalizerAwareInterface
		final class HypothesisClientAnnotationNormalizer implements NormalizerInterface

Sanitize annotations #52

Sanitize annotations #52

Conversation

nlisgo commented Jan 23, 2018

nlisgo commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thewilkybarkid Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thewilkybarkid Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thewilkybarkid commented Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thewilkybarkid left a comment

Choose a reason for hiding this comment

thewilkybarkid Jan 24, 2018 •

edited

Loading

thewilkybarkid Jan 25, 2018 •

edited

Loading

thewilkybarkid commented Jan 26, 2018 •

edited

Loading