httpclient.nim Fixes #14794 and an issue where content-header is not set on postContent (#18208)

* Fixed missing newline after bound marker in mulipart post (#14794) and a problem where calling postContent with multipart data does not set content-length header.

* Update lib/pure/httpclient.nim

* Added comment outlining the reason for changes to httpclient.nim and added tests to ensure that multipart post has a newline at the end of the body, and that the content-length header is present.

* Fixed typo in comments.

* Removed redundant blank lines in thttpclient_standalone.nim.

Co-authored-by: Mark Pointing <mark@futurepoint.com.au>
Co-authored-by: Andreas Rumpf <rumpf_a@web.de>

2 files changed, 38 insertions, 3 deletions

diff --git a/lib/pure/httpclient.nim b/lib/pure/httpclient.nim
index 2f9f1913f..33da6ef6c 100644
--- a/lib/pure/httpclient.nim
+++ b/lib/pure/httpclient.nim
@@ -975,7 +975,7 @@ proc format(client: HttpClient | AsyncHttpClient,
     if entry.isFile:
       length += entry.fileSize + httpNewLine.len
 
-  result.add "--" & bound & "--"
+  result.add "--" & bound & "--" & httpNewLine
 
   for s in result: length += s.len
   client.headers["Content-Length"] = $length
@@ -1010,12 +1010,16 @@ proc requestAux(client: HttpClient | AsyncHttpClient, url: Uri,
 
   await newConnection(client, url)
 
-  let newHeaders = client.headers.override(headers)
+  var newHeaders: HttpHeaders
 
   var data: seq[string]
   if multipart != nil and multipart.content.len > 0:
+    # `format` modifies `client.headers`, see 
+    # https://github.com/nim-lang/Nim/pull/18208#discussion_r647036979
     data = await client.format(multipart)
+    newHeaders = client.headers.override(headers)
   else:
+    newHeaders = client.headers.override(headers)
     # Only change headers if they have not been specified already
     if not newHeaders.hasKey("Content-Length"):
       if body.len != 0:
diff --git a/tests/stdlib/thttpclient_standalone.nim b/tests/stdlib/thttpclient_standalone.nim
index 44a88e91e..5fa1ea41a 100644
--- a/tests/stdlib/thttpclient_standalone.nim
+++ b/tests/stdlib/thttpclient_standalone.nim
@@ -2,7 +2,7 @@ discard """
   cmd: "nim c --threads:on $file"
 """
 
-import asynchttpserver, httpclient, asyncdispatch
+import asynchttpserver, httpclient, asyncdispatch, strutils
 
 block: # bug #16436
   proc startServer() {.async.} =
@@ -21,3 +21,34 @@ block: # bug #16436
   asyncCheck startServer()
   doAssertRaises(ProtocolError):
     waitFor runClient()
+
+block: # bug #14794 (And test for presence of content-length header when using postContent)
+  proc startServer() {.async.} =
+    var killServer = false
+    proc cb(req: Request) {.async.} =
+      doAssert(req.body.endsWith(httpNewLine), "Multipart body does not end with a newline.")
+      # this next line is probably not required because asynchttpserver does not call
+      # the callback when there is no content-length header.  It instead errors with 
+      # Error: unhandled exception: 411 Length Required
+      # Added for good measure in case the server becomes more permissive.
+      doAssert(req.headers.hasKey("content-length"), "Content-Length header is not present.")
+      killServer = true
+      asyncCheck req.respond(Http200, "OK")
+
+    var server = newAsyncHttpServer()
+    server.listen(Port(5556))
+    while not killServer:
+      if server.shouldAcceptRequest():
+        await server.acceptRequest(cb)
+      else:
+        poll()
+
+  proc runClient() {.async.} =
+    let c = newAsyncHttpClient()
+    var data = newMultipartData()
+    data.add("file.txt", "This is intended to be an example text file.\r\nThis would be the second line.\r\n")
+    let r = await c.postContent("http://127.0.0.1:5556", multipart = data)
+    c.close()
+
+  asyncCheck startServer()
+  waitFor runClient()