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

System.System.Net.Http.FormUrlEncodedContent(...) can't handle very long parameters/values #1936

Open
totallyrandomtext opened this issue Jun 5, 2015 · 10 comments

Comments

Projects
None yet
10 participants
@totallyrandomtext
Copy link

commented Jun 5, 2015

FormUrlEncodedContent(IEnumerable<KeyValuePair<string, string>>) uses Uri.EscapeDataString which internally uses UriHelper.EscapeString which can't handle strings longer than 65520 characters.

var uc1 = new FormUrlEncodedContent(
	new Dictionary<string, string> {
		{"test", new string('F', 65520)}
	}
);//works fine
var uc = new FormUrlEncodedContent(
	new Dictionary<string, string> {
		{"test", new string('F', 65521)}
	}
); //exception
@davidsh

This comment has been minimized.

Copy link
Member

commented Jun 5, 2015

Thank you for reporting this. We have been aware of this limitation for a while now in the original .NET Desktop APIs. We have considered switching the use of Uri.EscapeDataString to WebUtility.UrlEncode which is present in the System.Runtime.Extensions package. However, there were app-compat concerns making such a change because of possible small variations between the 2 APIs in how they encode strings.

For CoreFx and the use of app-local binaries now, we have an opportunity to make changes of this nature without broad app-compat impacts. We plan to address this bug over the near term as we first verify that we have sufficient test collateral to safely validate a change of this nature.

@davidsh davidsh added the bug label Jun 5, 2015

@davidsh davidsh self-assigned this Jun 5, 2015

@SidharthNabar

This comment has been minimized.

Copy link

commented Jun 23, 2015

@kasthack - As David said, we will look into this issue and try to address it in the near future. However, in the meanwhile, here is a workaround to help unblock you:

  1. Create your own type similar to FormUrlEncodeContent, derived from System.Net.Http.ByteArrayContent (https://msdn.microsoft.com/en-us/library/system.net.http.bytearraycontent(v=vs.110).aspx ) - for example, named FooBarContent. You can look at the source code of FormUrlEncodedContent in this repo as a guide
  2. In the Encode(string data) method, instead of the call to Uri.EscapeDataString , call WebUtility.UrlEncode (https://msdn.microsoft.com/en-us/library/system.net.webutility.urlencode(v=vs.110).aspx) )
  3. Use the new FooBarContent type in your code instead of FormUrlEncodeContent.
@justinvp

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2015

+1, I've been bitten by this size limit a couple times in production apps (not fun).

Regarding any app-compat concerns with Uri.EscapeDataString vs. WebUtility.UrlEncode, why not just extract out the common (internal) code from Uri.EscapeDataString for use in FormUrlEncodedContent and have an option not to do the length check. Then you'd maintain compat and no longer throw. While you're at it, also add an option to encode spaces as "+" instead of "%20" while doing the escaping, to avoid the need to have to call string.Replace and allocate another string after doing the escape.

@JefferyZh

This comment has been minimized.

Copy link

commented Nov 27, 2015

@SidharthNabar Thanks, I fixed my issue by following your steps. PipeCI/BlobStorage#3

@davidsh

This comment has been minimized.

Copy link
Member

commented Nov 28, 2015

Regarding any app-compat concerns with Uri.EscapeDataString vs. WebUtility.UrlEncode , why not just extract out the common (internal) code from Uri.EscapeDataString for use in FormUrlEncodedContent and have an option not to do the length check. Then you'd maintain compat and no longer throw. While you're at it, also add an option to encode spaces as "+" instead of "%20" while doing the escaping, to avoid the need to have to call string.Replace and allocate another string after doing the escape.

@justinvp I agree. Creating an internal common routine (source re-use in src\Common\src\System) plus more tests would be a good way to solve this.

Please free to submit a PR if you wish.

@davidsh davidsh modified the milestones: Future, 1.0.0-rtm Jan 21, 2016

@davidsh davidsh removed their assignment Nov 18, 2016

@karelz

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

Looks like good idea, watch for app compat (at least on the not-so-big strings) - ideally use the old code. We would expect adding test coverage with this.

Medium cost.

@onovotny

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

@karelz should this become a part of the new HttpClient work going on now?

@davidsh

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

This particular issue is not a bug in the HttpClientHandler. So, the work going on to create a new managed handler won't solve this.

This is a bug/issue in the FormUrlEncodeContent class which is part of the larger HttpClient API surface and is not a network I/o issue, per se.

@offbeatful

This comment has been minimized.

Copy link

commented Aug 3, 2017

I am not sure this will be related to this issue but we have a problem with current implementation which replaces %20 with +

            // Escape spaces as '+'.
            return Uri.EscapeDataString(data).Replace("%20", "+");

Can we have some sort of a settings to control whenever we need such replacement?

@tmenier

This comment has been minimized.

Copy link

commented Dec 18, 2017

@davidsh (or anyone else thinking about a solution) I ran into some serious gotchas when switching from Uri.EscapeDataString to WebUtility.UrlEncode in similar scenario where I just wanted to avoid the string length limitation associated with the former. I posted my findings here. Long story short, WebUtility.UrlEncode is a strange animal and probably best to avoid for this purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.