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

fix(BuildUrl): address path encoding lapses #40

Merged
merged 10 commits into from
Apr 12, 2021
3 changes: 3 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"dotnet-test-explorer.testProjectPath":"/tests/Imgix.Tests/Imgix.Tests.csproj"
}
luqven marked this conversation as resolved.
Show resolved Hide resolved
282 changes: 234 additions & 48 deletions src/Imgix/UrlBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ public class UrlBuilder
private static readonly int[] DprRatios = { 1, 2, 3, 4, 5 };
private static readonly int[] DprQualities = { 75, 50, 35, 23, 20 };


const int MaxWidth = 8192;
const int MinWidth = 100;
const double SrcSetWidthTolerance = 0.08;
Expand All @@ -50,46 +49,259 @@ public UrlBuilder(String domain, String signKey, Boolean useHttps)
: this(domain, signKey: signKey, includeLibraryParam: true, useHttps: useHttps)
{
}

/// <summary>
/// Encode the `path` argument to the UTF8 scheme.
/// </summary>
/// <param name="path">path to the image, i.e. "image/file.png"</param>
/// <returns>Formatted URL String that conforms to the
/// "{scheme}://{domain}/{path}" format.</returns>
public String BuildUrl(String path)
{
return BuildUrl(path, new Dictionary<string, string>());
}

/// <summary>
/// Encode the `path` and `paramater` arguments to the UTF8 scheme.
/// </summary>
/// <param name="path">path to the image, i.e. "image/file.png"</param>
/// <param name="parameters">dictionary of query parameters</param>
/// <returns>Formatted URL String that conforms to the
/// "{scheme}://{domain}/{path}{params}" format.</returns>
public String BuildUrl(String path, Dictionary<String, String> parameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a bit of difficulty parsing out the steps of this method despite knowing what it's supposed to do, specifically why qs was used over localParams at times (and vice versa). I think stylistically we can make things clearer for other contributors by either 1) reworking the variable names to be a bit more descriptive (I'll mull this one over) and/or 2) adding a comment describing what the purpose of the if block on L82 is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense. I think the best thing to do here is to break this up similarly to js-core, where sanitizePath, buildParams, and signParams are all extracted out into methods. Makes reading this a lot easier and also allows us remove references to qs or queryString and just say finalParams.

{
String scheme = UseHttps ? "https" : "http";
path = SanitizePath(path);

if (path.StartsWith("http"))
var finalParams = BuildParams(parameters);
if (!String.IsNullOrEmpty(_signKey))
{
path = WebUtility.UrlEncode(path);
finalParams = SignParams(path, finalParams);
}
return String.Format("{0}://{1}/{2}{3}", scheme, Domain, path, finalParams);
}

/// <summary>
/// `UrlEncodes` proxy paths to ensure *all* characters are handled. If
/// path is not being used as a proxy leave legal characters, like '/'
/// and '@', alone while encoding the rest using `EncodeURI` method.
/// </summary>
/// <param name="p">path to the image, i.e. "image/file.png"</param>
/// <returns>UTF8 encoded path string.</returns>
private String SanitizePath(String p)
{
String path = p;
// Strip leading slash first (we'll re-add after encoding).
path = path.TrimEnd('/').TrimStart('/');

// Check if path is a proxy path and store type of proxy.
Dictionary<String, Boolean> proxyStatus = CheckProxyStatus(path);
Object pathIsProxy = proxyStatus["isProxy"];
Object proxyIsEncoded = proxyStatus["isEncoded"];

if (pathIsProxy.Equals(true) && proxyIsEncoded.Equals(false))
{
// Use WebUtility.UrlEncode to ensure *all* characters are handled,
// since it's being used as a path.
return WebUtility.UrlEncode(path);
}
else if (pathIsProxy.Equals(true) && proxyIsEncoded.Equals(true))
{
// Do nothing to URL being used as path since its encoded.
return path;
}
else
{
// The path is not being used as a proxy, so leave legal
// characters like '/' and '@' alone but encode the rest.
return EncodeURI(path);
}
}

/// <summary>
/// Encode string `p` to the UTF8 format. Splits the string on the
/// `/` character to avoid accidentally encoding them. Then, iterate
/// over every character and call the `UrlEncode` method on them.
/// Finally, join the string array on `/`, adding the removed
/// character back.
/// </summary>
/// <param name="p">path to the image, i.e. "image/file.png"</param>
/// <returns>Encoded path string</returns>

private String EncodeURI(String p)
{
string[] splitPath = p.Split('/');
List<String> sanitizedPath = new List<String>();

foreach (String s in splitPath)
{
sanitizedPath.Add(UrlEncode(s));
}

return string.Join("/", sanitizedPath);
}

/// <summary>
/// Check if the path has one of the three possible acceptable proxy
/// prefixes. First we check if the path has the correct ascii prefix.
/// If it does then we know that it is a proxy, but it's not percent
/// encoded. Second, we check if the path is prefixed by a percent-encoded
/// prefix. If it is, we know that it's a proxy and that it's percent-encoded.
/// Finally, if the path isn't prefixed by any of these three prefixes, it is
/// not a valid proxy. This might be "just enough validation," but if we run
/// into issues we can make this check smarter/more-robust.
/// </summary>
/// <param name="p">path to the image, i.e. "image/file.png"</param>
/// <returns>Dictionary status = {isProxy: Boolean, isEncoded: Boolean} </returns>
private static Dictionary<String, Boolean> CheckProxyStatus(String p)
{
String path = p;
Dictionary<String, Boolean> status = new Dictionary<String, Boolean>();

String asciiHTTP = "http://";
String asciiHTTPS = "https://";

String encodedHTTP = "http%3A%2F%2F";
String encodedHTTPS = "https%3A%2F%2F";

String encodedHTTPLower = "http%3a%2f%2f";
String encodedHTTPSLower = "https%3a%ff%2f";

if (path.StartsWith(asciiHTTP) || path.StartsWith(asciiHTTPS))
{
status.Add("isProxy", true);
status.Add("isEncoded", false);

}
else if (path.StartsWith(encodedHTTP) || path.StartsWith(encodedHTTPS))
{
status.Add("isProxy", true);
status.Add("isEncoded", true);

}
else if (path.StartsWith(encodedHTTPLower) || path.StartsWith(encodedHTTPSLower))
{
status.Add("isProxy", true);
status.Add("isEncoded", true);

}
else
{
status.Add("isProxy", false);
status.Add("isEncoded", false);
}

return status;
}

/// <summary>
/// Encode each K,V pair. If key ends with 64, Base64 encode
/// the value. Also encode "+$:? " reserved characters.
/// </summary>
/// <param name="paramters">Dictionary of K,V parameter pairs</param>
/// <returns>Formatted query string which conforms to
/// "{encodedKey}={encodedVal}". If the parameters Dictionary has no k/v
/// pairs, return an empty String.</returns>
private String BuildParams(Dictionary<String, String> parameters)
{
Boolean hasLibParam = parameters.TryGetValue("ixlib", out String hasLib);

if (IncludeLibraryParam && !hasLibParam)
{
parameters.Add("ixlib", String.Format("csharp-{0}", typeof(UrlBuilder).GetTypeInfo().Assembly.GetName().Version));
}

return GenerateUrl(Domain, path, parameters);
if (parameters == null || parameters.Count == 0)
{
return String.Empty;
}
else
{
return "?" + String.Join("&", parameters.Select(p =>
{
String encodedKey = WebUtility.UrlEncode(p.Key);
encodedKey = encodedKey.Replace("+", "%20");
String encodedVal;

if (p.Key.EndsWith("64"))
{
encodedVal = Base64Encode(p.Value);
}
else
{
encodedVal = UrlEncode(p.Value);
}
return String.Format("{0}={1}", encodedKey, encodedVal);
}
));
}
}

private String GenerateUrl(String domain, String path, Dictionary<String, String> parameters)
/// <summary>
/// Add a signature query parameter if there's a signKey present.
/// If there are existing queryParams, concatenate the signature.
/// If there aren't, it will create the query params, `?s={signature}`.
/// SignParams uses the HashString method to create the signature.
/// </summary>
/// <param name="path">path to the image, i.e. "image/file.png"</param>
/// <param name="queryParams">Encoded query String, ie "?paramKey=paramValue"</param>
/// <returns>Encoded String that conforms to "?paramKey=paramValue" </returns>
private String SignParams(String path, String queryParams)
{
String scheme = UseHttps ? "https" : "http";
path = path.TrimEnd('/').TrimStart('/');

var qs = GenerateUrlStringFromDict(parameters);
var localParams = new Dictionary<String, String>(parameters);

if (!String.IsNullOrEmpty(_signKey))
var signatureBase = String.Format("{0}/{1}{2}", _signKey, path, queryParams);
var signature = HashString(signatureBase);
if (queryParams.Length > 0)
{
return queryParams + "&s=" + signature;
}
else
{
var hashString = String.Format("{0}/{1}{2}", _signKey, path, localParams.Any() ? "?" + qs : String.Empty);
localParams.Add("s", HashString(hashString));
return "?s=" + signature;
}

return String.Format("{0}://{1}/{2}{3}", scheme, domain, path, localParams.Any() ? "?" + GenerateUrlStringFromDict(localParams) : String.Empty);
}

/// <summary>
/// Encode a string `value` to the Base64 format. It does
/// this by calling the `System.Convert.ToBase64String` method on `value`
/// and additionally encoding reserved, `=/+` characters.
/// </summary>
/// <param name="value"> A String, ie "belîév∑"</param>
/// <returns>Base64 encoded String</returns>
private String Base64Encode(String value)
{
String encodedVal;
Byte[] valBytes = System.Text.Encoding.UTF8.GetBytes(value);
encodedVal = System.Convert.ToBase64String(valBytes);
encodedVal = encodedVal.Replace("=", "");
encodedVal = encodedVal.Replace("/", "_");
encodedVal = encodedVal.Replace("+", "-");
return encodedVal;
}

/// <summary>
/// Encode a string `value` to the UTF8 format. It does
/// this by calling the `WebUtility.UrlEncode` method on `value` and
/// doing some additional encoding on replacement reserved, `+:?#`,
/// and decoding on delimiter, `~`, characters.
/// </summary>
/// <param name="value"> A String, ie "$hello%world"</param>
/// <returns>UTF8 encoded String</returns>
private String UrlEncode(String value)
{
String encodedVal;

encodedVal = WebUtility.UrlEncode(value);
encodedVal = encodedVal.Replace("+", "%20");
encodedVal = encodedVal.Replace(":", "%3A");
encodedVal = encodedVal.Replace("?", "%3F");
encodedVal = encodedVal.Replace("#", "%23");
encodedVal = encodedVal.Replace("%7E", "~");

return encodedVal;
}


public String BuildSrcSet(String path)
{
return BuildSrcSet(path, new Dictionary<string, string>());
Expand Down Expand Up @@ -248,14 +460,14 @@ private String
// variable quality has been disabled.
Boolean hasQuality = parameters.TryGetValue("q", out String q);

foreach(int ratio in DprRatios)
foreach (int ratio in DprRatios)
{
if (!disableVariableQuality && !hasQuality)
{
srcSetParams["q"] = DprQualities[ratio - 1].ToString();
}
srcSetParams["dpr"] = ratio.ToString();
srcset += BuildUrl(path, srcSetParams) + " " + ratio.ToString()+ "x,\n";
srcset += BuildUrl(path, srcSetParams) + " " + ratio.ToString() + "x,\n";
}

return srcset.Substring(0, srcset.Length - 2);
Expand All @@ -273,7 +485,7 @@ private String GenerateSrcSetPairs(

String srcset = "";

foreach(int width in targets)
foreach (int width in targets)
{
parameters["w"] = width.ToString();
srcset += BuildUrl(path, parameters) + " " + width + "w,\n";
Expand Down Expand Up @@ -333,27 +545,27 @@ public static List<int> GenerateEvenTargetWidths()
*/
private static List<int> ComputeTargetWidths(double begin, double end, double tol)
{
if(NotCustom(begin, end, tol))
if (NotCustom(begin, end, tol))
{
// If not custom, return the default target widths.
return SrcSetTargetWidths.ToList();
}

if (begin == end)
{
return new List<int> {(int) Math.Round(begin) };
return new List<int> { (int)Math.Round(begin) };
}

List<int> resolutions = new List<int>();
while (begin < end && begin < MaxWidth)
{
resolutions.Add((int) Math.Round(begin));
resolutions.Add((int)Math.Round(begin));
begin *= 1 + tol * 2;
}

if (resolutions.Last() < end)
{
resolutions.Add((int) Math.Round(end));
resolutions.Add((int)Math.Round(end));
}

return resolutions;
Expand Down Expand Up @@ -385,31 +597,5 @@ private String HashString(String input)
{
return BitConverter.ToString(MD5.Create().ComputeHash(input.Select(Convert.ToByte).ToArray())).Replace("-", "").ToLower();
}

private String GenerateUrlStringFromDict(Dictionary<String, String> queryDictionary)
{
return queryDictionary == null ?
String.Empty :
String.Join("&", queryDictionary.Select(p =>
{
String encodedKey = WebUtility.UrlEncode(p.Key);
encodedKey = encodedKey.Replace("+", "%20");
String encodedVal;

if (p.Key.EndsWith("64")) {
Byte[] valBytes = System.Text.Encoding.UTF8.GetBytes(p.Value);
encodedVal = System.Convert.ToBase64String(valBytes);
encodedVal = encodedVal.Replace("=", "");
encodedVal = encodedVal.Replace("/", "_");
encodedVal = encodedVal.Replace("+", "-");
} else {
encodedVal = WebUtility.UrlEncode(p.Value);
encodedVal = encodedVal.Replace("+", "%20");
}

return String.Format("{0}={1}", encodedKey, encodedVal);
}
));
}
}
}
1 change: 1 addition & 0 deletions tests/Imgix.Tests/Imgix.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.3.0" />
<PackageReference Include="NUnit" Version="2.6.4" />
<PackageReference Include="NUnitTestAdapter" Version="2.3.0" />
</ItemGroup>

<ItemGroup>
Expand Down
Loading