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>
2026-06-05 03:14:08 +00:00 · 2021-01-10 15:40:53 +02:00
parent 65df5762a1
commit 7bde6aa37f
4 changed files with 147 additions and 110 deletions
--- 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`.
--- 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
+  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: string, headers: HttpHeaders,
+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
+    let statusCode = result.code

-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)
+    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)
--- 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"
--- 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)