improve request validation #67

tomerd · 2019-07-10T18:33:34Z

motivation: safer handling of request validation and mutation

changes:

fix schema logic to be non-case sensitive
made request version, method and url immutable
made request scheme and host internal
adjusted redirect ahndler implementation to stricter request immutabllity
adjust and add tests

artemredkin · 2019-07-10T18:44:34Z

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

@@ -12,7 +12,7 @@
 //
 //===----------------------------------------------------------------------===//

-import AsyncHTTPClient
+@testable import AsyncHTTPClient


i think it's better to move this test to HTTPClientInternalTests, this test class is intended to test public API methods

tomerd · 2019-07-18T01:00:41Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+            try self.setUrl(url)
+        }
+
+        public mutating func setUrl(_ url: URL) throws {


not sure if the preferred casing is setUrl or setURL i can go either way, or just url() can work for me too

I think setURL is the preferred casing.

Does this need to be public? I understand that redirect() needs to use it but that doesn't mean it needs to be public, does it?

I think it should be public to make it easier for users to mutate a Request they may be holding.

OK yes I guess it makes sense given a lot of the rest of the things on Request are mutable. We should have doc comments on public var url/scheme/host saying that if you want to change the property you should use setURL().

if i understand you correctly, the difference in opinion boils down to the last statement:

Sending a signal that perhaps mutating the field would lead to some logical error. .... In this case, neither is true...

the current code has some fragile logic tied to url, scheme and host and mutating the url does cause logical issues which is what this PR as well as #56 try to address. the fact we are talking about this so much should be a fairly strong signal that there is an issue here. my original suggestion was to circumvent the current logical issue and potential future ones by not allowing mutation of the url until we have a good reason to expose it

we don't gain anything by making it hard to do this, and we do push costs onto users by doing it

as far as i can judge there is not a good use case to justify exposing this to users beyond redirect which we already handle and discussed above, so imo there is no pragmatic cost here to push onto the users

this is why i still think its a better trade-off to delay exposing a mutable URL until there s a good reason to do so, hope this makes more sense now. as mentioned above i am willing to move forward with the current state of the PR for the sake of making progress

my original suggestion was to circumvent the current logical issue and potential future ones by not allowing mutation of the url until we have a good reason to expose it

And my counter argument is that there is no such thing as "not allowing mutation of the URL": the example code above does so. The question is only whether you can spell it "I'd like to change the URL", or whether you have to spell it "I'd like to create a new Request whose bits all happen to be the same as the one I already have".

I think it's really important to stress that making the url property let literally does not prevent the users from doing anything that this patch allows them to do. If you can construct a single example of what the current revision of the patch allows that cannot be achieved while the property is let, please let me know, because my understanding of Swift is that there is absolutely nothing.

@Lukasa of course! the point is not that user can't create a new request with a different url. the point is that the internal logical state of the request object is easier to manage by making url immutable, since changing the url triggers logic & validation. and since there is not a clear, pragmatic use case to make it mutable, we can get away by making it immutable and save ourselves maintenance headache, at least until a good reason shows up. hope this clarifies where i am coming from

worth mentioning again i am okay moving forward with the mutable URL option too, for the sake of making progress. so i suggest at this point we ask @artemredkin @weissi @ianpartridge and @tanner0101 to express their opinions too and just move with whatever the majority feels is the right thing, as both options are technically viable and this is more opinions

I think this just represents a fundamental disagreement here, because I don’t think there is any more difficulty in validating the URL via a setter function versus via the constructor. Regardless, though, I think we’re arguing about technicalities: anyone can achieve anything regardless of what construction is used here, so we’ve fundamentally just exerted far more energy here than we need to.

Given that it seems that I’m the only one arguing for a mutator function I think it’s easier for me to simply withdraw the objection than to poll for consensus. Let’s just move on.

Sources/AsyncHTTPClient/HTTPHandler.swift

Lukasa

Cool, this is a lot closer to what I'd like to see. I have left a few specific notes in the diff on one or two points but they're pretty minor.

Lukasa · 2019-07-18T08:31:42Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+            try self.setUrl(url)
+        }
+
+        public mutating func setUrl(_ url: URL) throws {


I think setURL is the preferred casing.

Lukasa · 2019-07-18T08:32:29Z

Sources/AsyncHTTPClient/HTTPHandler.swift

-            self.body = body
+            self._url = url
+            self._schema = scheme
+            self._host = host


Do we need to recalculate these? Unless we have a strong performance argument for doing so, it seems like it'd be better for scheme and host to be computed properties.

@Lukasa two reasons:

=> if we use computed properties they will need to force unwrap since host and scheme are optional on URL. this means we rely on the fact we validated they are not empty in the url setter, which we do now. so could work. having said that, in my experience this would be a fragile design as someone in the future may decide to change the validation and forget there are computed properties that make this assumption leading to runtime errors. the current implementation makes this very explicit and hard to get wrong in the future

=> we are lowercasing the scheme which is used for business logic and validation downstream code. using the url's schema will not include this manipulation and can cause business logic issues. we could instead create a new url object with the lowercased schema but the current implementation seems less intrusive and expensive

I think the first reason is more compelling than the second. Maintaining the case of the URL scheme is not worthwhile, and so creating and storing the new URL object with the lowercased URL scheme seems reasonable enough to me.

The bigger concern for me is your worry about the business logic changing and leading to runtime errors. It seems to me that the correct defense against that risk is a unit test that validates that it is impossible to create a request with a missing host or scheme. No matter what the implementation of the host/scheme getter, as long as that test continues to pass then by definition they cannot fail to obtain a value.

The argument about fragile design can be made in the opposite direction, incidentally. If a user changes the validation logic they may forget to update scheme or host, which will then lead to separate business logic issues. My general assumption is that it is better to have a single source of truth than to split it.

That said, if you feel strongly I'm ok to let this slide. In that case, the only note I have is that self._schema is incorrectly named, and it should be self._scheme.

i could go either way on this one, no strong opinion. @artemredkin @ianpartridge @tanner0101 ?

Tbh I don't think it's worth spending the cycles discussing it. Let's leave it as is, fixing the typo of schema to scheme.

Sources/AsyncHTTPClient/HTTPHandler.swift

ianpartridge · 2019-07-18T16:06:13Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+            try self.setUrl(url)
+        }
+
+        public mutating func setUrl(_ url: URL) throws {


Does this need to be public? I understand that redirect() needs to use it but that doesn't mean it needs to be public, does it?

ianpartridge · 2019-07-18T16:07:24Z

Sources/AsyncHTTPClient/HTTPHandler.swift

        public var headers: HTTPHeaders
        public var body: Body?
+        private var _url: URL!
+        private var _schema: String!


Shouldn't this be _scheme not _schema?

tomerd · 2019-07-23T16:23:37Z

@artemredkin i rebased and made this [more] immutable again. i think its ready to go

artemredkin · 2019-07-23T18:18:37Z

Sources/AsyncHTTPClient/HTTPHandler.swift

@@ -89,15 +89,15 @@ extension HTTPClient {
    /// Represent HTTP request.
    public struct Request {
        /// Request HTTP version, defaults to `HTTP/1.1`.
-        public var version: HTTPVersion


Johannes suggest to drop version from request completely (see discussion in #56)

even better! done in 73c400f

artemredkin · 2019-07-29T14:40:22Z

Sources/AsyncHTTPClient/HTTPHandler.swift

        /// Request HTTP method, defaults to `GET`.
-        public var method: HTTPMethod
+        public let method: HTTPMethod


does method have to be let? It's not part of the url change verification, so we can keep it as var (maybe move closer to all other vars in the file?

artemredkin · 2019-07-29T14:43:06Z

Sources/AsyncHTTPClient/HTTPHandler.swift

-        } else {
-            preconditionFailure("redirectURL doesn't contain a scheme")
-        }
+        let originalRequest = self.request


I think having something like:

var request = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body) ... if convertToGet { request.method = .GET request.body = nil request.headers.remove(name: "Content-Length") request.headers.remove(name: "Content-Type") } if !originalURL.hasTheSameOrigin(as: redirectURL) { request.headers.remove(name: "Origin") request.headers.remove(name: "Cookie") request.headers.remove(name: "Authorization") request.headers.remove(name: "Proxy-Authorization") } return self.execute(newRequest).futureResult.cascade(to: promise)

could look event simpler, wdyt?

artemredkin

Two small comments, but otherwise lgtm

tomerd · 2019-07-29T19:59:50Z

@artemredkin personally i prefer this as-is. if you want/prefer method to be mutable and "nicer" redirect handler we can go back to 1e3e29f and make setURL private. happy to do that too. wdyt?

artemredkin · 2019-07-30T13:33:13Z

@tomerd no, current version works for me, let's fix formatting issue and merge

motivation: safer handling of request validation and mutation changes: * fix scheme logic to be non-case sensitive * made request version, method and url immutable * made request scheme and host internal * adjusted redirect handler implementation to stricter request immutabllity * adjust and add tests

This was referenced Jul 10, 2019

refactor to make request non-throwing #56

Closed

make API non-throwing #66

Closed

artemredkin reviewed Jul 10, 2019

View reviewed changes

weissi requested a review from artemredkin July 17, 2019 09:52

tomerd force-pushed the request-fixes branch 2 times, most recently from e660552 to 4be0b6f Compare July 18, 2019 00:57

tomerd commented Jul 18, 2019

View reviewed changes

Sources/AsyncHTTPClient/HTTPHandler.swift Outdated Show resolved Hide resolved

tomerd requested review from Lukasa and weissi July 18, 2019 01:02

Lukasa requested changes Jul 18, 2019

View reviewed changes

tomerd requested a review from ianpartridge July 18, 2019 15:53

ianpartridge reviewed Jul 18, 2019

View reviewed changes

tomerd force-pushed the request-fixes branch from b3d9394 to 50fd77d Compare July 23, 2019 16:20

artemredkin reviewed Jul 23, 2019

View reviewed changes

artemredkin mentioned this pull request Jul 25, 2019

replace preconditions with errors #64

Closed

artemredkin reviewed Jul 29, 2019

View reviewed changes

artemredkin approved these changes Jul 29, 2019

View reviewed changes

tomerd added 4 commits July 30, 2019 09:13

make [more] immutable again

9c7e8d4

drop version

a6d9e84

format

1ff199a

tomerd force-pushed the request-fixes branch from 1d7c9af to 1ff199a Compare July 30, 2019 16:14

tomerd merged commit e64448e into swift-server:master Jul 30, 2019

artemredkin mentioned this pull request Jul 30, 2019

Changing HTTPClient.Request doesn't change other url components #43

Closed

Lukasa mentioned this pull request Jul 7, 2021

HTTPClient.Request.url is a let constant #395

Open

improve request validation #67

improve request validation #67

Uh oh!

Conversation

tomerd commented Jul 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemredkin left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artemredkin commented Jul 30, 2019

Uh oh!

Uh oh!

tomerd Jul 18, 2019 •

edited

Loading

tomerd Jul 18, 2019 •

edited

Loading

tomerd Jul 18, 2019 •

edited

Loading

tomerd commented Jul 23, 2019 •

edited

Loading

tomerd commented Jul 29, 2019 •

edited

Loading