Use HTML5 sanitizer to serve huge payloads through Speedscope remote viewer #102

davidstosik · 2023-10-24T07:56:08Z

Fixes #92.

When we run the app profiler on Shopify core's boot, it is frequent that the generated profile JSON file exceeds 10MB.

Rails::Html::Sanitizer is unable to process such huge payloads (because libxml2 limits text nodes to 10MB by default?).

The rails-html-sanitizer gem introduced HTML5 parsing in its 1.6.0 version, which lifts the text node size limit.

This change intends to use the new HTML5 parser when it's available to sanitize the Speedscope remote viewer's HTML, allowing bigger payloads to be served successfully.

I wasn't sure how to implement such variable superclass (depending on the version of rails-html-sanitizer used), and I was even less sure of how I could implement some tests to avoid regressions. All suggestions will be appreciated! 🙏🏻

…viewer When we run the app profiler on Shopify core's boot, it is frequent that the generated profile JSON file exceeds 10MB. `Rails::Html::Sanitizer` is unable to process such huge payloads (because libxml2 limits text nodes to 10MB by default?). The `rails-html-sanitizer` introduced HTML5 parsing in its 1.6.0 version, which lifts the text node size limit. This change intends to use the new HTML5 parser when it's available to sanitize the Speedscope remote viewer's HTML, allowing bigger payloads to be served successfully.

davidstosik · 2023-10-24T08:00:51Z

Gemfile.lock

    rack (2.2.6.3)
    rack-test (1.1.0)
      rack (>= 1.0, < 3)
    rails-dom-testing (2.0.3)
      activesupport (>= 4.2.0)
      nokogiri (>= 1.6)
-    rails-html-sanitizer (1.5.0)
-      loofah (~> 2.19, >= 2.19.1)
+    rails-html-sanitizer (1.6.0)


It was necessary to upgrade rails-html-sanitizer to 1.6.0 in Gemfile.lock in order to make it available when running tests.

Note that this is not a gem dependency anyway, and the gem is still meant to work with rails-html-sanitizer 1.5.0.

Ideally, I'd want a way to run the same test suite with different versions of rails-html-sanitizer (one before and one after 1.6.0, which is the version that originally defines Rails::HTML – all caps – and implements the HTML5 sanitizer), but I had no idea how I could achieve that. (Also not sure this would be worth the effort.)

Why is it important to support RHS 1.5.x? I'd prefer if you just updated the dependency.

davidstosik · 2023-10-24T08:02:42Z

test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb

@@ -60,6 +60,21 @@ class MiddlewareTest < TestCase
          assert_match(%r(<script type="text/javascript">), html)
        end

+        test "#call show can serve huge payloads" do
+          frames = { "1" => { name: "a" * 1e7 } }


10MB, or about 10 million (1e7) characters, is the size limit the old HTML4 sanitizer wasn't able to exceed.
Reverting rails-html-sanitizer to 1.5.0 in Gemfile.lock, one can run this test and confirm that it fails.

davidstosik · 2023-10-24T08:04:12Z

test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb

+            html[-200..-1],
+            message: "The generated HTML was incomplete"


Note: I'm using a substring because html is huge (10MB+), and this assertion would flood the terminal if it fails.
I also added an error message to clarify what is likely happening.

davidstosik · 2023-10-24T08:05:31Z

test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb

+          html = html.first
+
+          assert_match(
+            %r{'Flamegraph for .*'\);\n</script>},


This is the end of the "rendered" payload, as seen here:

app_profiler/lib/app_profiler/viewer/speedscope_remote_viewer/middleware.rb

Lines 50 to 51 in fbf4c3e

iframe.setAttribute('src', '/app_profiler/viewer/index.html#profileURL=' + objUrl + '&title=' + 'Flamegraph for #{name}');

</script>

If this was rendered, then there likely was no problem rendering the whole text node.

davidstosik · 2023-10-24T08:15:46Z

lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb

@@ -6,7 +6,13 @@ module AppProfiler
  module Viewer
    class SpeedscopeRemoteViewer < BaseViewer
      class BaseMiddleware
-        class Sanitizer < Rails::Html::SafeListSanitizer
+        sanitizer_superclass = if Rails::Html::Sanitizer::VERSION >= "1.6.0" && Rails::HTML::Sanitizer.html5_support?


Rails::Html::Sanitizer::VERSION >= "1.6.0" was necessary because HTML (all caps) and #html5_support? are not defined in older versions of the gem.
This line could also be:

Suggested change

sanitizer_superclass = if Rails::Html::Sanitizer::VERSION >= "1.6.0" && Rails::HTML::Sanitizer.html5_support?

sanitizer_superclass = if Rails::Html.respond_to?(:html5_support?) && Rails::Html::Sanitizer.html5_support?

You can/should use Rails::HTML::Sanitizer.best_supported_vendor for this if you pin to RHS >= 1.6.0

davidstosik · 2023-10-24T08:16:26Z

lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb

+        sanitizer_superclass = if Rails::Html::Sanitizer::VERSION >= "1.6.0" && Rails::HTML::Sanitizer.html5_support?
+          Rails::HTML5::SafeListSanitizer
+        else
+          Rails::Html::SafeListSanitizer


Rails::Html::SafeListSanitizer is the only sanitizer available in old versions of rails-html-sanitizer, and equivalent to Rails::HTML4::SafeListSanitizer in 1.6.0 and later.

See previous comments on my recommendation to use 1.6.0

flavorjones

I want to gently push on the assumption that we need to support Rails::HTML::Sanitizer v1.5.x. v1.6.0 should be backwards-compatible and supports Ruby 2.7 and later.

flavorjones · 2023-10-24T16:21:35Z

lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb

@@ -6,7 +6,13 @@ module AppProfiler
  module Viewer
    class SpeedscopeRemoteViewer < BaseViewer
      class BaseMiddleware
-        class Sanitizer < Rails::Html::SafeListSanitizer
+        sanitizer_superclass = if Rails::Html::Sanitizer::VERSION >= "1.6.0" && Rails::HTML::Sanitizer.html5_support?


You can/should use Rails::HTML::Sanitizer.best_supported_vendor for this if you pin to RHS >= 1.6.0

flavorjones · 2023-10-24T16:22:28Z

Gemfile.lock

    rack (2.2.6.3)
    rack-test (1.1.0)
      rack (>= 1.0, < 3)
    rails-dom-testing (2.0.3)
      activesupport (>= 4.2.0)
      nokogiri (>= 1.6)
-    rails-html-sanitizer (1.5.0)
-      loofah (~> 2.19, >= 2.19.1)
+    rails-html-sanitizer (1.6.0)


Why is it important to support RHS 1.5.x? I'd prefer if you just updated the dependency.

flavorjones · 2023-10-24T16:22:57Z

lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb

+        sanitizer_superclass = if Rails::Html::Sanitizer::VERSION >= "1.6.0" && Rails::HTML::Sanitizer.html5_support?
+          Rails::HTML5::SafeListSanitizer
+        else
+          Rails::Html::SafeListSanitizer


See previous comments on my recommendation to use 1.6.0

davidstosik · 2023-10-25T07:00:29Z

@flavorjones Thank you for your comments!
I'm unsure, but it looks like rails-html-sanitizer was not declared as a direct dependency of the app_profiler Gem, so I made the assumption that the gem was expected to work in any Rails application it's dropped in (including older Rails apps that might still be using rails-html-sanitizer 1.5.x).

I also guess we might not want to declare rails-html-sanitizer as a dependency in the gemspec because this gem could totally profile an app and produce a profile JSON file without it?

If we're happy to stop supporting older versions of rails-html-sanitizer, then I'll be happy to simplify the code, though I'm unsure how we'd gracefully ask users to update rails-html-sanitizer. 🤔

For reference, Rails 7.0.8 has a pretty loose version constraint on rails-html-sanitizer:

  s.add_dependency "rails-html-sanitizer", "~> 1.0", ">= 1.2.0"

flavorjones · 2023-10-25T13:09:21Z

@davidstosik I understand your points, and you're right about the historical expectation. But what I'm suggesting is that we don't care anymore about supporting old versions of RHS (because the new version is backwards compatible) and that the right thing to do is to declare a dependency on RHS >= 1.6.0 and simplify this code.

davidstosik · 2023-10-25T23:22:25Z

lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb

@@ -1,12 +1,13 @@
 # frozen_string_literal: true

+gem "rails-html-sanitizer", ">= 1.6.0"


@flavorjones To enforce the version dependency, I had to add a peer dependency here. (There is no dependency to that gem in the gemspec.)

davidstosik · 2023-10-26T00:00:40Z

@shioyama: I paired with @gmcgibbon this morning and we confirmed that this change also fixes the double escaping issue you mentioned to me yesterday! 🎉

Before	After

davidstosik force-pushed the sto/serve-huge-payloads branch from c636cee to 37e35d9 Compare October 24, 2023 08:10

davidstosik commented Oct 24, 2023

View reviewed changes

davidstosik requested review from gmcgibbon and flavorjones October 24, 2023 08:16

flavorjones reviewed Oct 24, 2023

View reviewed changes

Bump rails-html-sanitizer dependency to 1.6.0

f8f00c9

davidstosik commented Oct 25, 2023

View reviewed changes

gmcgibbon approved these changes Oct 25, 2023

View reviewed changes

Update CHANGELOG

6bafba0

davidstosik merged commit 641f562 into main Nov 7, 2023
5 checks passed

davidstosik deleted the sto/serve-huge-payloads branch November 7, 2023 10:04

shopify-shipit bot had a problem deploying to production December 7, 2023 23:50 Error

shopify-shipit bot temporarily deployed to production December 8, 2023 00:10 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use HTML5 sanitizer to serve huge payloads through Speedscope remote viewer #102

Use HTML5 sanitizer to serve huge payloads through Speedscope remote viewer #102

davidstosik commented Oct 24, 2023 •

edited

Loading

davidstosik Oct 24, 2023 •

edited

Loading

flavorjones Oct 24, 2023

davidstosik Oct 24, 2023

davidstosik Oct 24, 2023 •

edited

Loading

davidstosik Oct 24, 2023

davidstosik Oct 24, 2023

flavorjones Oct 24, 2023

davidstosik Oct 24, 2023

flavorjones Oct 24, 2023

flavorjones left a comment

flavorjones Oct 24, 2023

flavorjones Oct 24, 2023

flavorjones Oct 24, 2023

davidstosik commented Oct 25, 2023 •

edited

Loading

flavorjones commented Oct 25, 2023

davidstosik Oct 25, 2023

davidstosik commented Oct 26, 2023

	iframe.setAttribute('src', '/app_profiler/viewer/index.html#profileURL=' + objUrl + '&title=' + 'Flamegraph for #{name}');
	</script>

	sanitizer_superclass = if Rails::Html::Sanitizer::VERSION >= "1.6.0" && Rails::HTML::Sanitizer.html5_support?
	sanitizer_superclass = if Rails::Html.respond_to?(:html5_support?) && Rails::Html::Sanitizer.html5_support?

		@@ -1,12 +1,13 @@
		# frozen_string_literal: true

		gem "rails-html-sanitizer", ">= 1.6.0"

Use HTML5 sanitizer to serve huge payloads through Speedscope remote viewer #102

Use HTML5 sanitizer to serve huge payloads through Speedscope remote viewer #102

Conversation

davidstosik commented Oct 24, 2023 • edited Loading

davidstosik Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidstosik Oct 24, 2023 • 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

flavorjones left a 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

davidstosik commented Oct 25, 2023 • edited Loading

flavorjones commented Oct 25, 2023

Choose a reason for hiding this comment

davidstosik commented Oct 26, 2023

davidstosik commented Oct 24, 2023 •

edited

Loading

davidstosik Oct 24, 2023 •

edited

Loading

davidstosik Oct 24, 2023 •

edited

Loading

davidstosik commented Oct 25, 2023 •

edited

Loading