Bug 20975 - Improve auto escaping performance
Summary: Improve auto escaping performance
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 13618
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-21 14:01 UTC by Jonathan Druart
Modified: 2019-10-14 19:58 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
AutoEscaping - no filter (1.65 KB, patch)
2018-06-21 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
AutoEscaping - TT html filters (3.19 KB, patch)
2018-06-21 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
AutoEscaping - Template::Stash::AutoEscaping (escape all) (1.38 KB, patch)
2018-06-21 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
AutoEscaping - Do not escape using ignore_escape (1.35 KB, patch)
2018-06-21 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
AutoEscaping - Do not escape using .raw (1.51 KB, patch)
2018-06-21 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
AutoEscaping - Koha::Template::AutoEscaping (5.78 KB, patch)
2018-06-21 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-06-21 14:01:46 UTC
On bug 13618 we are trying to get rid of XSS issues with a global solution.

Using Template::Stash::AutoEscaping we are going to apply a html filter to all of our template variables. The problem is that we can have thousands of variables displayed (and so escaped).
Comment 1 Jonathan Druart 2018-06-21 14:03:41 UTC
Created attachment 76230 [details] [review]
AutoEscaping - no filter

for x in 1 2 3 4 5; do time perl benchmark_AutoEscaping.pl; done
perl benchmark_AutoEscaping.pl  0.20s user 0.00s system 98% cpu 0.208 total
perl benchmark_AutoEscaping.pl  0.19s user 0.02s system 98% cpu 0.214 total
perl benchmark_AutoEscaping.pl  0.18s user 0.02s system 98% cpu 0.204 total
perl benchmark_AutoEscaping.pl  0.18s user 0.02s system 98% cpu 0.202 total
perl benchmark_AutoEscaping.pl  0.18s user 0.02s system 97% cpu 0.205 total
Comment 2 Jonathan Druart 2018-06-21 14:03:46 UTC
Created attachment 76231 [details] [review]
AutoEscaping - TT html filters

for x in 1 2 3 4 5; do time perl benchmark_AutoEscaping.pl; done
perl benchmark_AutoEscaping.pl  0.30s user 0.09s system 95% cpu 0.414 total
perl benchmark_AutoEscaping.pl  0.27s user 0.02s system 97% cpu 0.295 total
perl benchmark_AutoEscaping.pl  0.28s user 0.02s system 99% cpu 0.307 total
perl benchmark_AutoEscaping.pl  0.30s user 0.02s system 98% cpu 0.320 total
perl benchmark_AutoEscaping.pl  0.27s user 0.02s system 99% cpu 0.290 total
Comment 3 Jonathan Druart 2018-06-21 14:03:54 UTC
Created attachment 76232 [details] [review]
AutoEscaping - Template::Stash::AutoEscaping (escape all)

for x in 1 2 3 4 5; do time perl benchmark_AutoEscaping.pl; done
perl benchmark_AutoEscaping.pl  9.67s user 5.08s system 99% cpu 14.837 total
perl benchmark_AutoEscaping.pl  9.00s user 4.10s system 99% cpu 13.167 total
perl benchmark_AutoEscaping.pl  9.24s user 4.97s system 99% cpu 14.263 total
perl benchmark_AutoEscaping.pl  9.91s user 4.19s system 99% cpu 14.188 total
perl benchmark_AutoEscaping.pl  8.86s user 4.44s system 99% cpu 13.347 total
Comment 4 Jonathan Druart 2018-06-21 14:03:59 UTC
Created attachment 76233 [details] [review]
AutoEscaping - Do not escape using ignore_escape

The variable are not escaped (let's suppose it's what we want)

for x in 1 2 3 4 5; do time perl benchmark_AutoEscaping.pl; done
perl benchmark_AutoEscaping.pl  7.37s user 4.12s system 99% cpu 11.557 total
perl benchmark_AutoEscaping.pl  7.15s user 4.24s system 99% cpu 11.438 total
perl benchmark_AutoEscaping.pl  6.99s user 3.70s system 99% cpu 10.751 total
perl benchmark_AutoEscaping.pl  6.32s user 3.76s system 99% cpu 10.120 total
perl benchmark_AutoEscaping.pl  6.32s user 3.71s system 99% cpu 10.057 total
Comment 5 Jonathan Druart 2018-06-21 14:04:05 UTC
Created attachment 76234 [details] [review]
AutoEscaping - Do not escape using .raw

Same as before, vars will not be escaped

for x in 1 2 3 4 5; do time perl benchmark_AutoEscaping.pl; done
perl benchmark_AutoEscaping.pl  7.43s user 3.92s system 99% cpu 11.400 total
perl benchmark_AutoEscaping.pl  6.56s user 3.75s system 99% cpu 10.377 total
perl benchmark_AutoEscaping.pl  6.11s user 4.08s system 99% cpu 10.233 total
perl benchmark_AutoEscaping.pl  7.02s user 3.93s system 99% cpu 10.984 total
perl benchmark_AutoEscaping.pl  6.91s user 3.70s system 99% cpu 10.664 total
Comment 6 Jonathan Druart 2018-06-21 14:04:11 UTC
Created attachment 76235 [details] [review]
AutoEscaping - Koha::Template::AutoEscaping

for x in 1 2 3 4 5; do time perl benchmark_AutoEscaping.pl; done
perl benchmark_AutoEscaping.pl  0.34s user 0.07s system 96% cpu 0.425 total
perl benchmark_AutoEscaping.pl  0.35s user 0.04s system 99% cpu 0.387 total
perl benchmark_AutoEscaping.pl  0.35s user 0.04s system 99% cpu 0.396 total
perl benchmark_AutoEscaping.pl  0.50s user 0.03s system 99% cpu 0.532 total
perl benchmark_AutoEscaping.pl  0.38s user 0.02s system 99% cpu 0.400 total
Comment 7 Jonathan Druart 2018-06-21 14:08:21 UTC
Koha::Template::AutoEscaping is a naive adaptation of Template::Stash::AutoEscaping. I have tried to simplify the code to remove what will not be useful for us.

The performances are good, I guess it helps to avoid 1 object creation (::Escaped::HTML) by variable.

But... it does not work. If you are applying the last patch on top of bug 13618 you will notice that variables are double-escaped.
Comment 8 Julian Maurice 2018-07-13 10:31:56 UTC
(In reply to Jonathan Druart from comment #7)
> The performances are good, I guess it helps to avoid 1 object creation
> (::Escaped::HTML) by variable.

It's actually worse than that. Template::Stash::AutoEscaping creates an object for each string concatenation, and Template::Toolkit concatenates stuff to its internal buffer all the time.
I ran benchmark_AutoEscaping.pl (using Template::Stash::AutoEscaping) with NYTProf and it spends most of the time in Template::Stash::AutoEscaping::Escaped::Base::concat (63% of total time) where it creates 50000 new objects. But apparently most of the time here is spent doing simple string concatenation.
(not so) fun fact: the "HTML-escaping" process represents less than 3% of concat in terms of time spent.

I don't know how (or if) this can be improved, but I think the time loss is not acceptable, and unless we find a better solution we should keep using html filter manually (and probably add a rule in coding guidelines)

Note that we can use TT filters as blocks, like this:

[% FILTER html %]
  [% this_will_be_escaped %] and [% this.will(be).escaped | too %]
[% END %]

Maybe this could reduce the hassle of writing "|html" everywhere
Comment 9 Jonathan Druart 2018-07-19 21:01:29 UTC
No longer valid, new version of bug 13618 has been submitted.

Thanks for the feedback Julian!