Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure external sources for styles only process CSS responses when inlining stylesheets #417

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

agrande-ea
Copy link

Background

This fix relates to issues when a HTML template contains a <link> node that points at a resource which returns something that isn't a CSS stylesheet. We had a user which created a HTML template (that we render and serve, hence the need to inline stylesheets), which managed to spike CPU usage on our Azure App Service Plan to 100%, with no end in sight. It simply just peaked there, and kept spinning, bringing several other apps on the same App Service Plan down and/or non-responsive.

We ran a .NET trace on the Azure App Service to try and figure out where the issue was coming from, and we came across this issue which seemed to match up with what we were seeing, at least on the surface.

image

Once we dug into it a bit more, we realized the template had a <link> node defined that once evaluated by the PreMailer.Net library, would result in a Content-Type return value of text/html. It was returning plain HTML which seemed to be throwing the Regex parsers for a loop!

The dodgy <link> in question was pointing at a resource that would only return actual CSS (what PreMailer.Net would normally expect), if we set the Accept header to text/css, to force the server to return the CSS string. Otherwise it would render the "normal" HTML page, as if you'd navigated to it via a browser.

So the fix for this is fairly straightforward, where we ensure the Accept headers for the WebDownloader.DownloadString method are added to specifically only request CSS content, and we check the response Content-Type and ensure that matches CSS content too, and throw if we detect something different.

Open to feedback on any other/cleaner ways of fixing this :)

Changes

  • Updated WebDownloader.DownloadString to set the Accept request header for CSS stylesheets
  • Updated WebDownloader.DownloadString to check the response and ensure the Content-Type matches the CSS Content-Type.
  • Introduced an extension method to parse the Content-Type response header, to make checking/comparing the response headers a bit easier.
  • Updated the Benchmarks.Program class to use a potentially safer method of benchmarking our code, where we create a config on the fly that uses InProcessNoEmitToolchain to prevent our Benchmarks from spawning too many threads/processes.
    • This was a really strange one. See "Questions" below.

Questions

  • What would be a good way of testing this? I noticed that most areas that potentially use IWebDownloader implementations tend to be mocked/stubbed out, so given most of my changes live in the WebDownloader itself, I'd love some guidance on how you'd want me to test this.
  • Did you want me to roll back the InProcessNoEmitToolchain configuration changes in the Benchmarker project?
    • I had my work anti-virus application flag the Benchmark app as suspicious, and it would prevent the benchmark from running at all. I'm happy to revert this bit if you don't find it useful.
  • Are you happy with the types of Exceptions I'm throwing? I wasn't entirely sure if throwing was the correct way forward for how we normally handle issues like this within PreMailer.Net, so I'm also open to feedback on changing this to something other than Exceptions being thrown everywhere :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant