Add RawTarget property to IHttpRequestFeature (#596) #639

cesarblum · 2016-05-25T04:11:43Z

Let me know if I'm missing anything on this repo.

Once this is merged I need to change:

Kestrel
WebListener
What else? 😁

Do you think RawTarget is a good name for the property? I didn't want to use Uri, Url, or anything like that since this is called "target" in the RFCs.

cc @Tratcher @halter73 @JunTaoLuo @blowdart @davidfowl

davidfowl · 2016-05-25T04:15:36Z

src/Microsoft.AspNetCore.Http.Features/IHttpRequestFeature.cs

@@ -49,6 +49,11 @@ public interface IHttpRequestFeature
        string QueryString { get; set; }

        /// <summary>
+        /// The request target as it was sent in the HTTP request.
+        /// </summary>
+        string RawTarget { get; set; }


RawTarget? Thats super cryptic

That's ok in this case, it's not like we want people to use it. It's only the emergency backup.

That said, I'm sure @blowdart has some very strong language to put in the comments to the effect of Don't even think about using this, you won't do it right and then you'll get hacked.

It's the technically correct term 😛

What about RawRequest? Or RawPath?

Inaccurate.

I like RawPath, it's "hidden" already on the feature and people aren't stupid, let's not try to protect them.

The problem with RawPath is that it's incorrect. We'll include the query with it, not just the path. Plus the feature distinguishes between Path and PathBase, which makes it even more confusing.

Path is wrong, it's Path and Query at a minimum. And for things like Options * it's not Path or Query, it's just *. Target is the right term.

Call it RawTarget then, we'll be forever explaining this to people 😄 because the spec says so!

That's what doc comments are for :-). Link to the RFC section.

Besides, most people don't even know what feature interfaces are.

Uh huh 😁

Tratcher · 2016-05-25T04:16:16Z

Check Hosting's TestHost.

cesarblum · 2016-05-25T04:37:01Z

So do I get a squirrel?

cesarblum · 2016-05-25T04:43:06Z

I mean, I guess we have to wait for @blowdart's feedback on stating the new property should not be used unless someone really really really knows what they're doing.

davidfowl · 2016-05-25T04:44:52Z

Tratcher · 2016-05-25T05:43:47Z

You still need more doc comments. An RFC section link and that the filed contains the escaped full path and/or query, or * for Options *.

Tratcher · 2016-05-25T05:44:36Z

src/Microsoft.AspNetCore.Owin/OwinFeatureCollection.cs

@@ -102,6 +102,12 @@ string IHttpRequestFeature.QueryString
            set { Prop(OwinConstants.RequestQueryString, Utilities.RemoveQuestionMark(value)); }
        }

+        string IHttpRequestFeature.RawTarget
+        {
+            get { throw new NotSupportedException(); }


Property Getters shouldn't throw, return string.Empty. Setters can throw.

davidfowl · 2016-05-25T09:15:54Z

I mean, I guess we have to wait for @blowdart's feedback on stating the new property should not be used unless someone really really really knows what they're doing.

Leave that for the docs. Let's get the feature in after the changes @Tratcher suggests and worry about that later 😄

blowdart · 2016-05-25T13:05:29Z

Let's worry about it now. It's not just a docs issue, it's an xmldocs issue too.

So how about

/// This property is not used internally for routing or authorization decisions. It has not been UrlDecoded and care should be taken in its use.

cesarblum · 2016-05-25T18:49:18Z

Updated.

blowdart · 2016-05-25T21:39:41Z

🚢 (I hate that squirrel)

halter73 · 2016-05-25T22:36:47Z

src/Microsoft.AspNetCore.Http.Features/IHttpRequestFeature.cs

@@ -49,6 +49,15 @@ public interface IHttpRequestFeature
        string QueryString { get; set; }

        /// <summary>
+        /// The request target as it was sent in the HTTP request. This property contains the
+        /// escaped path and full query, as well as other request targets such as * for OPTIONS


"escaped path"

Does this mean that the precent encoded characters in path are unescaped or raw? I think the word escaped implies that the server did some sort of escaping.

I was equally confused 😄 But "escaped" here means they are URL-encoded i.e. the %s will be there.

Just use "raw" then

cesarblum · 2016-05-25T22:39:08Z

Turns out there was a bit more that I needed to change.

@blowdart I've moved your warning on the property's usage to <remarks>. Is that alright?

blowdart · 2016-05-25T22:40:11Z

Yup :)

Tratcher · 2016-05-26T16:47:14Z

halter73 · 2016-05-26T22:26:16Z

dnfclas added the cla-already-signed label May 25, 2016

cesarblum force-pushed the cesarbs/raw-target-request-feature branch from a585486 to be421c9 Compare May 25, 2016 04:12

cesarblum changed the title ~~Add RawTarget property to IHttpRequestFeature.~~ Add RawTarget property to IHttpRequestFeature (#596) May 25, 2016

davidfowl reviewed May 25, 2016
View reviewed changes

Tratcher reviewed May 25, 2016
View reviewed changes

halter73 reviewed May 25, 2016
View reviewed changes

This was referenced May 25, 2016

Implement IHttpRequestFeature.RawTarget (aspnet/HttpAbstractions#596) aspnet/KestrelHttpServer#880

Merged

Implement IHttpRequestFeature.RawTarget. aspnet/HttpSysServer#199

Closed

cesarblum force-pushed the cesarbs/raw-target-request-feature branch from a91444e to d778521 Compare May 26, 2016 22:20

cesarblum force-pushed the cesarbs/raw-target-request-feature branch from d778521 to 8a804a9 Compare May 27, 2016 21:07

Add RawTarget property to IHttpRequestFeature (#596).

8212694

cesarblum force-pushed the cesarbs/raw-target-request-feature branch from 8a804a9 to 8212694 Compare May 27, 2016 23:54

cesarblum merged commit 8212694 into dev May 31, 2016

cesarblum deleted the cesarbs/raw-target-request-feature branch May 31, 2016 23:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RawTarget property to IHttpRequestFeature (#596) #639

Add RawTarget property to IHttpRequestFeature (#596) #639

cesarblum commented May 25, 2016

davidfowl May 25, 2016

Tratcher May 25, 2016

cesarblum May 25, 2016

Tratcher May 25, 2016

davidfowl May 25, 2016 •

edited

Loading

cesarblum May 25, 2016 •

edited

Loading

Tratcher May 25, 2016

davidfowl May 25, 2016 •

edited

Loading

Tratcher May 25, 2016 •

edited

Loading

davidfowl May 25, 2016

Tratcher commented May 25, 2016

cesarblum commented May 25, 2016

cesarblum commented May 25, 2016

davidfowl commented May 25, 2016

Tratcher commented May 25, 2016

Tratcher May 25, 2016

davidfowl commented May 25, 2016

blowdart commented May 25, 2016

cesarblum commented May 25, 2016

blowdart commented May 25, 2016

halter73 May 25, 2016

cesarblum May 25, 2016 •

edited

Loading

halter73 May 26, 2016

cesarblum commented May 25, 2016

blowdart commented May 25, 2016

Tratcher commented May 26, 2016

halter73 commented May 26, 2016

Add RawTarget property to IHttpRequestFeature (#596) #639

Add RawTarget property to IHttpRequestFeature (#596) #639

Conversation

cesarblum commented May 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl May 25, 2016 • edited Loading

Choose a reason for hiding this comment

cesarblum May 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl May 25, 2016 • edited Loading

Choose a reason for hiding this comment

Tratcher May 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented May 25, 2016

cesarblum commented May 25, 2016

cesarblum commented May 25, 2016

davidfowl commented May 25, 2016

Tratcher commented May 25, 2016

Choose a reason for hiding this comment

davidfowl commented May 25, 2016

blowdart commented May 25, 2016

cesarblum commented May 25, 2016

blowdart commented May 25, 2016

Choose a reason for hiding this comment

cesarblum May 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum commented May 25, 2016

blowdart commented May 25, 2016

Tratcher commented May 26, 2016

halter73 commented May 26, 2016

davidfowl May 25, 2016 •

edited

Loading

cesarblum May 25, 2016 •

edited

Loading

davidfowl May 25, 2016 •

edited

Loading

Tratcher May 25, 2016 •

edited

Loading

cesarblum May 25, 2016 •

edited

Loading