Ensure external sources for styles only process CSS responses when inlining stylesheets #417
Conversation
|
@jasekiw are you able to merge this? |
martinnormark
left a comment
martinnormark
left a comment
There was a problem hiding this comment.
First of all, really sorry to have left this sit here for so long. Apologies, and thanks for the PR!
A few comments added inline, and below for your questions.
I see that there's no tests for the WebDownloader at all, so perhaps better to refactor later and add proper tests. If this change is solely setting the Accept header, it doesn't change the logic of the inlining.
Would like the benchmarking changes left out of this.
It can be hard to reason about the course of the exceptions, but I'd rather leave this strict requirement of the response out. It can easily break stuff that's already running. If it should be strictly validated, it should be configurable to require opt-in.
| // We only support this operation for CSS file/content types coming back | ||
| // from the response. If we get something different, throw with the unsupported | ||
| // content type in the message | ||
| if(response.ParseContentType() != CssMimeType) | ||
| throw new NotSupportedException($"The Uri type is giving a response in unsupported content type '{response.ContentType}'."); | ||
|
|
There was a problem hiding this comment.
I'm concerned this could have unintended consequences, since a response of actual CSS could be returned without setting the content type properly.
There was a problem hiding this comment.
Thats a good shout, I'll have a think on how to approach it. The only thing I can currently think of is a manual check with a StartsWith or EndsWith or something like that?
| public static void Main() | ||
| { | ||
| BenchmarkRunner.Run<Realistic>(); | ||
| // Some local environments may run into issues with Windows Defender or |
There was a problem hiding this comment.
Would rather leave this out of the PR here. Perhaps there's another way to configure? Or add an exception to the anti-virus software?
There was a problem hiding this comment.
I like the idea of configuring it another way. I'll have a think about it, maybe some sort of --safeMode flag or something? 🤔
…lining stylesheets
martinnormark
force-pushed
the
fix/233-regex-css-parser-problem
branch
from
e07d661 to
574d92d
Compare
hi mate :) my own apologies are in order here too lol, I'd completely forgotten about this and was only reminded of it this morning! I'll make some tweaks later today, thanks so much for taking the time to provide feedback! |
3 participants
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.
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 oftext/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 theAcceptheader totext/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
Acceptheaders for theWebDownloader.DownloadStringmethod are added to specifically only request CSS content, and we check the responseContent-Typeand 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
WebDownloader.DownloadStringto set theAcceptrequest header for CSS stylesheetsWebDownloader.DownloadStringto check the response and ensure theContent-Typematches the CSS Content-Type.Content-Typeresponse header, to make checking/comparing the response headers a bit easier.Benchmarks.Programclass to use a potentially safer method of benchmarking our code, where we create a config on the fly that usesInProcessNoEmitToolchainto prevent our Benchmarks from spawning too many threads/processes.Questions
IWebDownloaderimplementations tend to be mocked/stubbed out, so given most of my changes live in theWebDownloaderitself, I'd love some guidance on how you'd want me to test this.InProcessNoEmitToolchainconfiguration changes in the Benchmarker project?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 :)