Httpclient improvements (#15919)

* Allow passing Uri instead of strings

* Teach httpclient about 308

* Deprecate request proc where httpMethod is string

* More use of HttpMethod enum

Also fix handling of 308, I forgot to add the hunk to the previous
commit.

* Well behaved redirect handler

* Also remove Transfer-Encoding

* Removed unused proc

* Secure redirection rules

Strip sensitive headers for cross-domain redirects.

* Allow httpMethod to be a string again

This way unknown http verbs can be used without any problem.

* Respect user-specified Host header

* Missed multipart argument.

* Try another method

* add changelog

* Fix hidden deprecation warning, parseEnum failing

* This is wrong

* Have to do it manually, parseEnum is not suitable

* Review comments

* update

Co-authored-by: LemonBoy <thatlemon@gmail.com>
Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com>

4 files changed, 149 insertions, 112 deletions

diff --git a/changelog.md b/changelog.md
index afe49f1f3..0705d0090 100644
--- a/changelog.md
+++ b/changelog.md
@@ -34,6 +34,9 @@
 - Removed deprecated `iup` module from stdlib, it has already moved to
   [nimble](https://github.com/nim-lang/iup).
 
+- various functions in `httpclient` now accept `url` of type `Uri`. Moreover `request` function's
+  `httpMethod` argument of type `string` was deprecated in favor of `HttpMethod` enum type.
+
 - `nodejs` backend now supports osenv: `getEnv`, `putEnv`, `envPairs`, `delEnv`, `existsEnv`.
 
 - Added `cmpMem` to `system`.
diff --git a/lib/pure/httpclient.nim b/lib/pure/httpclient.nim
index 70f3327b1..ea847ef8d 100644
--- a/lib/pure/httpclient.nim
+++ b/lib/pure/httpclient.nim
@@ -325,10 +325,14 @@ proc getDefaultSSL(): SslContext =
       result = defaultSslContext
       doAssert result != nil, "failure to initialize the SSL context"
 
-proc newProxy*(url: string, auth = ""): Proxy =
+proc newProxy*(url: string; auth = ""): Proxy =
   ## Constructs a new ``TProxy`` object.
   result = Proxy(url: parseUri(url), auth: auth)
 
+proc newProxy*(url: Uri; auth = ""): Proxy =
+  ## Constructs a new ``TProxy`` object.
+  result = Proxy(url: url, auth: auth)
+
 proc newMultipartData*: MultipartData {.inline.} =
   ## Constructs a new ``MultipartData`` object.
   MultipartData()
@@ -457,29 +461,23 @@ proc sendFile(socket: Socket | AsyncSocket,
     await socket.send(buffer)
   file.close()
 
-proc redirection(status: string): bool =
-  const redirectionNRs = ["301", "302", "303", "307", "308"]
-  for i in items(redirectionNRs):
-    if status.startsWith(i):
-      return true
-
-proc getNewLocation(lastURL: string, headers: HttpHeaders): string =
-  result = headers.getOrDefault"Location"
-  if result == "": httpError("location header expected")
+proc getNewLocation(lastURL: Uri, headers: HttpHeaders): Uri =
+  let newLocation = headers.getOrDefault"Location"
+  if newLocation == "": httpError("location header expected")
   # Relative URLs. (Not part of the spec, but soon will be.)
-  let r = parseUri(result)
-  if r.hostname == "" and r.path != "":
-    var parsed = parseUri(lastURL)
-    parsed.path = r.path
-    parsed.query = r.query
-    parsed.anchor = r.anchor
-    result = $parsed
-
-proc generateHeaders(requestUrl: Uri, httpMethod: string, headers: HttpHeaders,
+  let parsedLocation = parseUri(newLocation)
+  if parsedLocation.hostname == "" and parsedLocation.path != "":
+    result = lastURL
+    result.path = parsedLocation.path
+    result.query = parsedLocation.query
+    result.anchor = parsedLocation.anchor
+  else:
+    result = parsedLocation
+
+proc generateHeaders(requestUrl: Uri, httpMethod: HttpMethod, headers: HttpHeaders,
                      proxy: Proxy): string =
   # GET
-  let upperMethod = httpMethod.toUpperAscii()
-  result = upperMethod
+  result = $httpMethod
   result.add ' '
 
   if proxy.isNil or requestUrl.scheme == "https":
@@ -898,7 +896,7 @@ proc newConnection(client: HttpClient | AsyncHttpClient,
         connectUrl.hostname = url.hostname
         connectUrl.port = if url.port != "": url.port else: "443"
 
-        let proxyHeaderString = generateHeaders(connectUrl, $HttpConnect,
+        let proxyHeaderString = generateHeaders(connectUrl, HttpConnect,
             newHttpHeaders(), client.proxy)
         await client.socket.send(proxyHeaderString)
         let proxyResp = await parseResponse(client, false)
@@ -967,14 +965,12 @@ proc override(fallback, override: HttpHeaders): HttpHeaders =
   for k, vs in override.table:
     result[k] = vs
 
-proc requestAux(client: HttpClient | AsyncHttpClient, url, httpMethod: string,
-                body = "", headers: HttpHeaders = nil,
+proc requestAux(client: HttpClient | AsyncHttpClient, url: Uri,
+                httpMethod: HttpMethod, body = "", headers: HttpHeaders = nil,
                 multipart: MultipartData = nil): Future[Response | AsyncResponse]
                 {.multisync.} =
   # Helper that actually makes the request. Does not handle redirects.
-  let requestUrl = parseUri(url)
-
-  if requestUrl.scheme == "":
+  if url.scheme == "":
     raise newException(ValueError, "No uri scheme supplied.")
 
   var data: seq[string]
@@ -992,13 +988,13 @@ proc requestAux(client: HttpClient | AsyncHttpClient, url, httpMethod: string,
       await client.parseBodyFut
       client.parseBodyFut = nil
 
-  await newConnection(client, requestUrl)
+  await newConnection(client, url)
 
   let newHeaders = client.headers.override(headers)
   if not newHeaders.hasKey("user-agent") and client.userAgent.len > 0:
     newHeaders["User-Agent"] = client.userAgent
 
-  let headerString = generateHeaders(requestUrl, httpMethod, newHeaders,
+  let headerString = generateHeaders(url, httpMethod, newHeaders,
                                      client.proxy)
   await client.socket.send(headerString)
 
@@ -1020,12 +1016,13 @@ proc requestAux(client: HttpClient | AsyncHttpClient, url, httpMethod: string,
   elif body.len > 0:
     await client.socket.send(body)
 
-  let getBody = httpMethod.toLowerAscii() notin ["head", "connect"] and
+  let getBody = httpMethod notin {HttpHead, HttpConnect} and
                 client.getBody
   result = await parseResponse(client, getBody)
 
-proc request*(client: HttpClient | AsyncHttpClient, url: string,
-              httpMethod: string, body = "", headers: HttpHeaders = nil,
+proc request*(client: HttpClient | AsyncHttpClient, url: Uri | string,
+              httpMethod: HttpMethod | string = HttpGet, body = "",
+              headers: HttpHeaders = nil,
               multipart: MultipartData = nil): Future[Response | AsyncResponse]
               {.multisync.} =
   ## Connects to the hostname specified by the URL and performs a request
@@ -1040,33 +1037,87 @@ proc request*(client: HttpClient | AsyncHttpClient, url: string,
   ##
   ## You need to make sure that the ``url`` doesn't contain any newline
   ## characters. Failing to do so will raise ``AssertionDefect``.
-  doAssert(not url.contains({'\c', '\L'}), "url shouldn't contain any newline characters")
+  ##
+  ## **Deprecated since v1.5**: use HttpMethod enum instead; string parameter httpMethod is deprecated
+  when url is string:
+    doAssert(not url.contains({'\c', '\L'}), "url shouldn't contain any newline characters")
+    let url = parseUri(url)
+
+  when httpMethod is string:
+    {.warning:
+       "Deprecated since v1.5; use HttpMethod enum instead; string parameter httpMethod is deprecated".}
+    let httpMethod = case httpMethod
+      of "HEAD":
+        HttpHead
+      of "GET":
+        HttpGet
+      of "POST":
+        HttpPost
+      of "PUT":
+        HttpPut
+      of "DELETE":
+        HttpDelete
+      of "TRACE":
+        HttpTrace
+      of "OPTIONS":
+        HttpOptions
+      of "CONNECT":
+        HttpConnect
+      of "PATCH":
+        HttpPatch
+      else:
+        raise newException(ValueError, "Invalid HTTP method name: " & httpMethod)
 
   result = await client.requestAux(url, httpMethod, body, headers, multipart)
 
   var lastURL = url
   for i in 1..client.maxRedirects:
-    if result.status.redirection():
-      let redirectTo = getNewLocation(lastURL, result.headers)
-      # Guarantee method for HTTP 307: see https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307
-      var meth = if result.status == "307": httpMethod else: "GET"
-      result = await client.requestAux(redirectTo, meth, body, headers, multipart)
-      lastURL = redirectTo
-
-proc request*(client: HttpClient | AsyncHttpClient, url: string,
-              httpMethod = HttpGet, body = "", headers: HttpHeaders = nil,
-              multipart: MultipartData = nil): Future[Response | AsyncResponse]
-              {.multisync.} =
-  ## Connects to the hostname specified by the URL and performs a request
-  ## using the method specified.
-  ##
-  ## Connection will be kept alive. Further requests on the same ``client`` to
-  ## the same hostname will not require a new connection to be made. The
-  ## connection can be closed by using the ``close`` procedure.
-  ##
-  ## When a request is made to a different hostname, the current connection will
-  ## be closed.
-  result = await request(client, url, $httpMethod, body, headers, multipart)
+    let statusCode = result.code
+
+    if statusCode notin {Http301, Http302, Http303, Http307, Http308}:
+      break
+
+    let redirectTo = getNewLocation(lastURL, result.headers)
+    var redirectMethod: HttpMethod
+    var redirectBody: string
+    # For more informations about the redirect methods see:
+    # https://developer.mozilla.org/en-US/docs/Web/HTTP/Redirections
+    case statusCode
+    of Http301, Http302, Http303:
+      # The method is changed to GET unless it is GET or HEAD (RFC2616)
+      if httpMethod notin {HttpGet, HttpHead}:
+        redirectMethod = HttpGet
+      else:
+        redirectMethod = httpMethod
+      # The body is stripped away
+      redirectBody = ""
+      # Delete any header value associated with the body
+      if not headers.isNil():
+        headers.del("Content-Length")
+        headers.del("Content-Type")
+        headers.del("Transfer-Encoding")
+    of Http307, Http308:
+      # The method and the body are unchanged
+      redirectMethod = httpMethod
+      redirectBody = body
+    else:
+      # Unreachable
+      doAssert(false)
+
+    # Check if the redirection is to the same domain or a sub-domain (foo.com
+    # -> sub.foo.com)
+    if redirectTo.hostname != lastURL.hostname and
+      not redirectTo.hostname.endsWith("." & lastURL.hostname):
+      # Perform some cleanup of the header values
+      if headers != nil:
+        # Delete the Host header
+        headers.del("Host")
+        # Do not send any sensitive info to a unknown host
+        headers.del("Authorization")
+
+    result = await client.requestAux(redirectTo, redirectMethod, redirectBody,
+                                     headers, multipart)
+    lastURL = redirectTo
 
 proc responseContent(resp: Response | AsyncResponse): Future[string] {.multisync.} =
   ## Returns the content of a response as a string.
@@ -1079,79 +1130,79 @@ proc responseContent(resp: Response | AsyncResponse): Future[string] {.multisync
     return await resp.bodyStream.readAll()
 
 proc head*(client: HttpClient | AsyncHttpClient,
-          url: string): Future[Response | AsyncResponse] {.multisync.} =
+          url: Uri | string): Future[Response | AsyncResponse] {.multisync.} =
   ## Connects to the hostname specified by the URL and performs a HEAD request.
   ##
   ## This procedure uses httpClient values such as ``client.maxRedirects``.
   result = await client.request(url, HttpHead)
 
 proc get*(client: HttpClient | AsyncHttpClient,
-          url: string): Future[Response | AsyncResponse] {.multisync.} =
+          url: Uri | string): Future[Response | AsyncResponse] {.multisync.} =
   ## Connects to the hostname specified by the URL and performs a GET request.
   ##
   ## This procedure uses httpClient values such as ``client.maxRedirects``.
   result = await client.request(url, HttpGet)
 
 proc getContent*(client: HttpClient | AsyncHttpClient,
-                 url: string): Future[string] {.multisync.} =
+                 url: Uri | string): Future[string] {.multisync.} =
   ## Connects to the hostname specified by the URL and returns the content of a GET request.
   let resp = await get(client, url)
   return await responseContent(resp)
 
 proc delete*(client: HttpClient | AsyncHttpClient,
-             url: string): Future[Response | AsyncResponse] {.multisync.} =
+             url: Uri | string): Future[Response | AsyncResponse] {.multisync.} =
   ## Connects to the hostname specified by the URL and performs a DELETE request.
   ## This procedure uses httpClient values such as ``client.maxRedirects``.
   result = await client.request(url, HttpDelete)
 
 proc deleteContent*(client: HttpClient | AsyncHttpClient,
-                    url: string): Future[string] {.multisync.} =
+                    url: Uri | string): Future[string] {.multisync.} =
   ## Connects to the hostname specified by the URL and returns the content of a DELETE request.
   let resp = await delete(client, url)
   return await responseContent(resp)
 
-proc post*(client: HttpClient | AsyncHttpClient, url: string, body = "",
+proc post*(client: HttpClient | AsyncHttpClient, url: Uri | string, body = "",
            multipart: MultipartData = nil): Future[Response | AsyncResponse]
            {.multisync.} =
   ## Connects to the hostname specified by the URL and performs a POST request.
   ## This procedure uses httpClient values such as ``client.maxRedirects``.
-  result = await client.request(url, $HttpPost, body, multipart=multipart)
+  result = await client.request(url, HttpPost, body, multipart=multipart)
 
-proc postContent*(client: HttpClient | AsyncHttpClient, url: string, body = "",
+proc postContent*(client: HttpClient | AsyncHttpClient, url: Uri | string, body = "",
                   multipart: MultipartData = nil): Future[string]
                   {.multisync.} =
   ## Connects to the hostname specified by the URL and returns the content of a POST request.
   let resp = await post(client, url, body, multipart)
   return await responseContent(resp)
 
-proc put*(client: HttpClient | AsyncHttpClient, url: string, body = "",
+proc put*(client: HttpClient | AsyncHttpClient, url: Uri | string, body = "",
           multipart: MultipartData = nil): Future[Response | AsyncResponse]
           {.multisync.} =
   ## Connects to the hostname specified by the URL and performs a PUT request.
   ## This procedure uses httpClient values such as ``client.maxRedirects``.
-  result = await client.request(url, $HttpPut, body, multipart=multipart)
+  result = await client.request(url, HttpPut, body, multipart=multipart)
 
-proc putContent*(client: HttpClient | AsyncHttpClient, url: string, body = "",
+proc putContent*(client: HttpClient | AsyncHttpClient, url: Uri | string, body = "",
                  multipart: MultipartData = nil): Future[string] {.multisync.} =
   ## Connects to the hostname specified by the URL andreturns the content of a PUT request.
   let resp = await put(client, url, body, multipart)
   return await responseContent(resp)
 
-proc patch*(client: HttpClient | AsyncHttpClient, url: string, body = "",
+proc patch*(client: HttpClient | AsyncHttpClient, url: Uri | string, body = "",
             multipart: MultipartData = nil): Future[Response | AsyncResponse]
             {.multisync.} =
   ## Connects to the hostname specified by the URL and performs a PATCH request.
   ## This procedure uses httpClient values such as ``client.maxRedirects``.
-  result = await client.request(url, $HttpPatch, body, multipart=multipart)
+  result = await client.request(url, HttpPatch, body, multipart=multipart)
 
-proc patchContent*(client: HttpClient | AsyncHttpClient, url: string, body = "",
+proc patchContent*(client: HttpClient | AsyncHttpClient, url: Uri | string, body = "",
                    multipart: MultipartData = nil): Future[string]
                   {.multisync.} =
   ## Connects to the hostname specified by the URL and returns the content of a PATCH request.
   let resp = await patch(client, url, body, multipart)
   return await responseContent(resp)
 
-proc downloadFile*(client: HttpClient, url: string, filename: string) =
+proc downloadFile*(client: HttpClient, url: Uri | string, filename: string) =
   ## Downloads ``url`` and saves it to ``filename``.
   client.getBody = false
   defer:
@@ -1167,10 +1218,10 @@ proc downloadFile*(client: HttpClient, url: string, filename: string) =
   if resp.code.is4xx or resp.code.is5xx:
     raise newException(HttpRequestError, resp.status)
 
-proc downloadFile*(client: AsyncHttpClient, url: string,
+proc downloadFile*(client: AsyncHttpClient, url: Uri | string,
                    filename: string): Future[void] =
   proc downloadFileEx(client: AsyncHttpClient,
-                      url, filename: string): Future[void] {.async.} =
+                      url: Uri | string, filename: string): Future[void] {.async.} =
     ## Downloads ``url`` and saves it to ``filename``.
     client.getBody = false
     let resp = await client.get(url)
diff --git a/lib/pure/httpcore.nim b/lib/pure/httpcore.nim
index 9287e9864..774de1260 100644
--- a/lib/pure/httpcore.nim
+++ b/lib/pure/httpcore.nim
@@ -29,24 +29,26 @@ type
     HttpVer11,
     HttpVer10
 
-  HttpMethod* = enum ## the requested HttpMethod
-    HttpHead,        ## Asks for the response identical to the one that would
-                     ## correspond to a GET request, but without the response
-                     ## body.
-    HttpGet,         ## Retrieves the specified resource.
-    HttpPost,        ## Submits data to be processed to the identified
-                     ## resource. The data is included in the body of the
-                     ## request.
-    HttpPut,         ## Uploads a representation of the specified resource.
-    HttpDelete,      ## Deletes the specified resource.
-    HttpTrace,       ## Echoes back the received request, so that a client
-                     ## can see what intermediate servers are adding or
-                     ## changing in the request.
-    HttpOptions,     ## Returns the HTTP methods that the server supports
-                     ## for specified address.
-    HttpConnect,     ## Converts the request connection to a transparent
-                     ## TCP/IP tunnel, usually used for proxies.
-    HttpPatch        ## Applies partial modifications to a resource.
+  HttpMethod* = enum         ## the requested HttpMethod
+    HttpHead = "HEAD"        ## Asks for the response identical to the one that
+                             ## would correspond to a GET request, but without
+                             ## the response body.
+    HttpGet = "GET"          ## Retrieves the specified resource.
+    HttpPost = "POST"        ## Submits data to be processed to the identified
+                             ## resource. The data is included in the body of
+                             ## the request.
+    HttpPut = "PUT"          ## Uploads a representation of the specified
+                             ## resource.
+    HttpDelete = "DELETE"    ## Deletes the specified resource.
+    HttpTrace = "TRACE"      ## Echoes back the received request, so that a
+                             ## client
+                             ## can see what intermediate servers are adding or
+                             ## changing in the request.
+    HttpOptions = "OPTIONS"  ## Returns the HTTP methods that the server
+                             ## supports for specified address.
+    HttpConnect = "CONNECT"  ## Converts the request connection to a transparent
+                             ## TCP/IP tunnel, usually used for proxies.
+    HttpPatch = "PATCH"      ## Applies partial modifications to a resource.
 
 
 const
@@ -150,7 +152,6 @@ func newHttpHeaders*(keyValuePairs:
       else:
         result.table[key] = @[pair.val]
 
-
 func `$`*(headers: HttpHeaders): string {.inline.} =
   $headers.table
 
@@ -378,21 +379,3 @@ func is4xx*(code: HttpCode): bool {.inline.} =
 func is5xx*(code: HttpCode): bool {.inline.} =
   ## Determines whether ``code`` is a 5xx HTTP status code.
   code.int in {500 .. 599}
-
-func `$`*(httpMethod: HttpMethod): string {.inline.} =
-  runnableExamples:
-    doAssert $HttpHead == "HEAD"
-    doAssert $HttpPatch == "PATCH"
-    doAssert $HttpGet == "GET"
-    doAssert $HttpPost == "POST"
-
-  result = case httpMethod
-    of HttpHead: "HEAD"
-    of HttpGet: "GET"
-    of HttpPost: "POST"
-    of HttpPut: "PUT"
-    of HttpDelete: "DELETE"
-    of HttpTrace: "TRACE"
-    of HttpOptions: "OPTIONS"
-    of HttpConnect: "CONNECT"
-    of HttpPatch: "PATCH"
diff --git a/tests/stdlib/thttpclient.nim b/tests/stdlib/thttpclient.nim
index 6a634d90f..4881370ee 100644
--- a/tests/stdlib/thttpclient.nim
+++ b/tests/stdlib/thttpclient.nim
@@ -39,7 +39,7 @@ proc makeIPv6HttpServer(hostname: string, port: Port,
 
 proc asyncTest() {.async.} =
   var client = newAsyncHttpClient()
-  var resp = await client.request("http://example.com/")
+  var resp = await client.request("http://example.com/", HttpGet)
   doAssert(resp.code.is2xx)
   var body = await resp.body
   body = await resp.body # Test caching
@@ -48,7 +48,7 @@ proc asyncTest() {.async.} =
   resp = await client.request("http://example.com/404")
   doAssert(resp.code.is4xx)
   doAssert(resp.code == Http404)
-  doAssert(resp.status == Http404)
+  doAssert(resp.status == $Http404)
 
   resp = await client.request("https://google.com/")
   doAssert(resp.code.is2xx or resp.code.is3xx)
@@ -102,14 +102,14 @@ proc asyncTest() {.async.} =
 
 proc syncTest() =
   var client = newHttpClient()
-  var resp = client.request("http://example.com/")
+  var resp = client.request("http://example.com/", HttpGet)
   doAssert(resp.code.is2xx)
   doAssert("<title>Example Domain</title>" in resp.body)
 
   resp = client.request("http://example.com/404")
   doAssert(resp.code.is4xx)
   doAssert(resp.code == Http404)
-  doAssert(resp.status == Http404)
+  doAssert(resp.status == $Http404)
 
   resp = client.request("https://google.com/")
   doAssert(resp.code.is2xx or resp.code.is3xx)