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

Fixes format exception thrown when user agent header value is incorrect #283

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
48 changes: 41 additions & 7 deletions CloudinaryDotNet.Tests/Asset/UrlBuilderTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -475,23 +475,57 @@ public void TestExcludeEmptyTransformation()
Assert.AreEqual(TestConstants.DefaultImageUpPath + "c_fill,x_100,y_100/test", uri);
}

[Test]
public void TestAgentPlatformHeaders()
private HttpRequestMessage CreateRequest(string userPlatform)
{
var request = new HttpRequestMessage { RequestUri = new Uri("http://dummy.com") };
m_api.UserPlatform = "Test/1.0";
m_api.UserPlatform = userPlatform;

m_api.PrepareRequestBody(
request,
HttpMethod.GET,
new SortedDictionary<string, object>(),
new FileDescription(""));
return request;
}

[Test]
public void TestAgentPlatformHeaders()
{
var httpRequestMessage = CreateRequest("UserPlatform/2.3");

//Can't test the result, so we just verify the UserAgent parameter is sent to the server
StringAssert.AreEqualIgnoringCase($"{m_api.UserPlatform} {ApiShared.USER_AGENT}",
request.Headers.UserAgent.ToString());
StringAssert.IsMatch(@"Test\/1\.0 CloudinaryDotNet\/(\d+)\.(\d+)\.(\d+) \(.*\)",
request.Headers.UserAgent.ToString());
StringAssert.IsMatch(@"CloudinaryDotNet\/(\d+)\.(\d+)\.(\d+) \(" + ApiShared.USER_AGENT.Replace("(", "").Replace(")", "") + @"\) UserPlatform/2\.3",
httpRequestMessage.Headers.UserAgent.ToString());
}

[Test]
[TestCase(null)]
[TestCase("")]
[TestCase(" ")]
[TestCase("UserPlatform/")]
[TestCase("UserPlatform/ ")]
[TestCase("UserPlatform / 1.2")]
public void UnexpectedUserPlatformShouldNotThrow(string userPlatorm)
{
Assert.DoesNotThrow(() => CreateRequest(userPlatorm));
}

[Test]
[TestCase("Mono 5.11.0 ((HEAD/768f1b247c6)")]
[TestCase("(")]
[TestCase(")")]
public void MalformedUserAgentShouldNotThrow(string userAgent)
{
var prevAgent = ApiShared.USER_AGENT;
ApiShared.USER_AGENT = userAgent;
try
{
Assert.DoesNotThrow(() => CreateRequest("UserPlatform"));
}
finally
{
ApiShared.USER_AGENT = prevAgent;
}
}

[Test]
Expand Down
59 changes: 55 additions & 4 deletions CloudinaryDotNet/ApiShared.Internal.cs
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,26 @@ protected void HandleUnsignedParameters(IDictionary<string, object> parameters)
}
}

private static void AddCommentToUserAgent(
HttpHeaderValueCollection<ProductInfoHeaderValue> userAgentHeader,
string comment)
{
if (string.IsNullOrEmpty(comment))
{
return;
}

var normalizedComment = RemoveBracketsFrom(comment);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain the problem we are solving here. In a year nobody will remember why we remove brackets from headers.

userAgentHeader.Add(new ProductInfoHeaderValue($"({normalizedComment})"));
}

private static string RemoveBracketsFrom(string comment)
{
return comment
.Replace(")", string.Empty)
.Replace("(", string.Empty);
}

private static SortedDictionary<string, object> GetCallParams(HttpMethod method, BaseParams parameters)
{
parameters?.Check();
Expand Down Expand Up @@ -455,10 +475,11 @@ private void PrePrepareRequestBody(

// Add platform information to the USER_AGENT header
// This is intended for platform information and not individual applications!
var userPlatform = string.IsNullOrEmpty(UserPlatform)
? USER_AGENT
: string.Format(CultureInfo.InvariantCulture, "{0} {1}", UserPlatform, USER_AGENT);
request.Headers.Add("User-Agent", userPlatform);
var userAgentHeader = request.Headers.UserAgent;
userAgentHeader.Add(new ProductInfoHeaderValue("CloudinaryDotNet", CloudinaryVersion.Full));

AddCommentToUserAgent(userAgentHeader, USER_AGENT);
SetUserPlatform(userAgentHeader);

byte[] authBytes = Encoding.ASCII.GetBytes(GetApiCredentials());
request.Headers.Add("Authorization", string.Format(CultureInfo.InvariantCulture, "Basic {0}", Convert.ToBase64String(authBytes)));
Expand All @@ -478,6 +499,36 @@ private void PrePrepareRequestBody(
}
}

private void SetUserPlatform(HttpHeaderValueCollection<ProductInfoHeaderValue> userAgentHeader)
{
Console.WriteLine($"UserPlatform: [{UserPlatform}] ======");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove debug printing.

var up = UserPlatform?.Trim();
if (string.IsNullOrEmpty(up))
{
return;
}

var upp = up.Split('/');
var productName = GetElement(0);
if (string.IsNullOrEmpty(productName))
{
return;
}

var productVersion = GetElement(1);
if (string.IsNullOrEmpty(productVersion))
{
productVersion = "0.1";
Copy link
Contributor

Choose a reason for hiding this comment

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

"0.1" ? is there any special meaning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@const-cloudinary No special meaning here, just a default value.

}

userAgentHeader.Add(new ProductInfoHeaderValue(productName, productVersion));

string GetElement(int index)
{
return upp.ElementAtOrDefault(index)?.Trim();
}
}

private async Task PrepareRequestContentAsync(
HttpRequestMessage request,
SortedDictionary<string, object> parameters,
Expand Down
7 changes: 1 addition & 6 deletions CloudinaryDotNet/ApiShared.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public partial class ApiShared : ISignProvider
/// <summary>
/// User agent for cloudinary API requests.
/// </summary>
public static string USER_AGENT = BuildUserAgent();
public static string USER_AGENT = RuntimeInformation.FrameworkDescription;
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this constant does not correspond the content. Very confusing. Please refactor.


/// <summary>
/// Sends HTTP requests and receives HTTP responses.
Expand Down Expand Up @@ -803,10 +803,5 @@ public string BuildUploadFormShared(string field, string resourceType, SortedDic

return builder.ToString();
}

private static string BuildUserAgent()
{
return $"CloudinaryDotNet/{CloudinaryVersion.Full} ({RuntimeInformation.FrameworkDescription})";
}
}
}