Skip to content

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

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.

Sign up for GitHub

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

Open
agrande-ea wants to merge 1 commit into
base: main
Choose a base branch
from
+50 −4
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions PreMailer.Net/Benchmarks/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,30 @@
using AngleSharp;
using AngleSharp.Html.Parser;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;
using BenchmarkDotNet.Toolchains.InProcess.NoEmit;

public static class Program
{
public static void Main()
{
BenchmarkRunner.Run<Realistic>();
// Some local environments may run into issues with Windows Defender or
Copy link
Contributor

@martinnormark martinnormark Jan 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@agrande-ea agrande-ea Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of configuring it another way. I'll have a think about it, maybe some sort of --safeMode flag or something? 🤔

// SentinelOne (and others) when running a benchmark. This ensures we
// keep our toolchain within our process and stops the above apps from blocking
// our benchmark process, but can slow the execution time.
var avSafeConfig = DefaultConfig.Instance
.AddJob(
Job.ShortRun
.WithToolchain(InProcessNoEmitToolchain.Instance)
.WithIterationCount(100)
);

BenchmarkRunner.Run<Realistic>(avSafeConfig);
}
}

[SimpleJob(invocationCount: 100, iterationCount: 100)]
[MemoryDiagnoser]
public class Realistic
{
Expand Down Expand Up @@ -47,7 +60,7 @@ public void MoveCssInline_AllFlags()

<meta http-equiv=""Content-Type"" content=""text/html; charset=UTF-8"" />
<title>PreMailer Benchmark</title>

</head>

<body bgcolor=""#123"">
Expand Down
11 changes: 10 additions & 1 deletion PreMailer.Net/PreMailer.Net/Downloaders/WebDownloader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using PreMailer.Net.Extensions;
using System;
using System.IO;
using System.Net;
Expand All @@ -8,7 +9,7 @@ namespace PreMailer.Net.Downloaders
public class WebDownloader : IWebDownloader
{
private static IWebDownloader _sharedDownloader;

private const string CssMimeType = "text/css";
public static IWebDownloader SharedDownloader
{
get
Expand All @@ -29,8 +30,16 @@ public static IWebDownloader SharedDownloader
public string DownloadString(Uri uri)
{
var request = WebRequest.Create(uri);
request.Headers.Add(HttpRequestHeader.Accept, CssMimeType);

using (var response = request.GetResponse())
{
// 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}'.");

Comment on lines +37 to +42
Copy link
Contributor

@martinnormark martinnormark Jan 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned this could have unintended consequences, since a response of actual CSS could be returned without setting the content type properly.

Copy link
Author

@agrande-ea agrande-ea Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

switch (response)
{
case HttpWebResponse httpWebResponse:
Expand Down
24 changes: 24 additions & 0 deletions PreMailer.Net/PreMailer.Net/Extensions/WebResponseExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
using System;
using System.Net;

namespace PreMailer.Net.Extensions
{
public static class WebResponseExtensions
{
public static string ParseContentType(this WebResponse response)
{
if(response == null)
throw new NullReferenceException("Malformed response detected when parsing WebResponse Content-Type");

if(string.IsNullOrEmpty(response.ContentType))
throw new NullReferenceException("Malformed Content-Type response detected when parsing WebResponse");

var results = response.ContentType.Split(';');

if(results.Length == 0)
throw new FormatException("Malformed Content-Type response detected when parsing WebResponse");

return results[0];
}
}
}
Loading