-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Added parameter check when setting Host header #505
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
Added parameter check when setting Host header #505
Conversation
RestSharp/RestRequest.cs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this use a case insensitive regex? Also, do header values not allow international characters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
You're right, I overlooked the case insensitive part and will remedy that
ASAP. Regarding international hostnames, they are as far as I can tell
actually ecoded in plain ASCII using punycode and a special prefix. It's up
to each application to convert such names back into unicode for display
purposes.
On Feb 7, 2014 5:23 PM, "Phil Haack" notifications@github.com wrote:
In RestSharp/RestRequest.cs:
@@ -356,6 +357,10 @@ public IRestRequest AddParameter (string name, object value, ParameterType type)
///
public IRestRequest AddHeader (string name, string value)
{
if (name == "Host" && (value.Length > 255 || !Regex.IsMatch(value, @"^\w[a-z0-9-]{0,62}(.\w[a-z0-9-]{0,62})*(:\d+)?$")))
Shouldn't this use a case insensitive regex? Also, do header values not
allow international characters?Reply to this email directly or view it on GitHubhttps://github.com//pull/505/files#r9544514
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I wonder if there's an existing method somewhere for validating this. You'd think System.Net.Http or something would have it.
If not, I'd also probably look at creating a static regex for this and calling IsMatch
on that rather than Regex.IsMatch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I found System.Uri.CheckHostName()
which seems to perform the validation that is needed. However it doesn't cater for port
if given as part of the header value, and it doesn't fail too long host names either (exceeding 255 chars).
I guess we could switch to that one and just optionally split host values on :
before testing.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@haacked what do you think about this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the empty string case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, multiple colon case? "foo:bar:baz"
Added parameter check when setting Host header
Tested with the additional cases and it passed. Will merge this in and then make a commit to add them. |
…from the change in RestRequest.
Fixes #504