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

Use safer HeaderDictionaryExtensions #14064

Merged
merged 1 commit into from Aug 4, 2023
Merged

Use safer HeaderDictionaryExtensions #14064

merged 1 commit into from Aug 4, 2023

Conversation

jtkech
Copy link
Member

@jtkech jtkech commented Aug 3, 2023

While trying donet 8 new analyzers suggest to not use .Headers.Add() but .Headers.Append(), the first is a collection method that may throw if the key already exists, the second is a IHeaderDictionary extension that checks if the key already exists and if so merge the provided value to the existing StringValues.

@@ -80,7 +80,7 @@ public override async Task<ActivityExecutionResult> ExecuteAsync(WorkflowExecuti

foreach (var header in headers)
{
response.Headers.Add(header);
response.Headers.Append(header.Key, header.Value);
Copy link
Member

Choose a reason for hiding this comment

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

Why it's safer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw it while testing dotnet 8 preview where new analyzers warn on it, and then I was not able to compile. In fact the warning was in ModularTenantContainerMiddleware on.

httpContext.Response.Headers.Add(HeaderNames.RetryAfter, "10");

So we add at a given key a string that is implicitly converted to a StringValues (may hold multiple values), this by using the IDictionary.Add() method that may throw if the key already exists.

By using

httpContext.Response.Headers.Append(HeaderNames.RetryAfter, "10");

We use an HeaderDictionaryExtensions helper that checks if the key already exists and if so retrieves the existing StringValues and just add to it the new provided string.

In this file there was no warning because Headers.Add(header) uses another interface method, the ICollection.Add() method to add a KeyValuePair<string, StringValues>, but this method also fails if the key already exists, so I applied the same change.

@jtkech jtkech merged commit 16d9fb3 into main Aug 4, 2023
3 checks passed
@jtkech jtkech deleted the jtkech/header-dico branch August 4, 2023 03:06
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.

None yet

2 participants