Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsanitized headers can break body in response #56

Open
D-Nice opened this issue Aug 16, 2021 · 2 comments
Open

Unsanitized headers can break body in response #56

D-Nice opened this issue Aug 16, 2021 · 2 comments

Comments

@D-Nice
Copy link

D-Nice commented Aug 16, 2021

I was creating some custom headers to add to my response, and was using IDE magic to replicate some lines, which resulted in my last header arguement still containing trailing \c\L chars.

My initial presumption was this would be appropriately handled by the library, but it was not, and caused my resulting response body to be missing characters as the \c\L essentially caused overflow into where the body was expected to start.

An easy solution on my end was

 let sanitizedHeaders = HEADERS.strip(leading = false, chars = {'\c', '\L'})

before inserting the headers in send

I am wondering if you're of the opinion that the library should in fact handle this, or if it's up to the user to sanitize. I understand this is supposed to be performance-oriented and such finer details may be left out for performance reasons, however, it also exposes an unsafeSend and send method, and when such a distinction occurs, I would presume it's pretty hard to shoot myself in the foot with send, yet I accomplished it just via adding some basic headers.

Another option which would lessen the impact on send (instead of doing a check/strip on each call) would be a method exposed to the user to create a HttpBeast safe and compatible header string.

@dom96
Copy link
Owner

dom96 commented Aug 16, 2021

hm, can you share exactly what the code looked like? i.e. the send call and what the HEADERS contained?

I think the right approach here could be to just add an assert.

@D-Nice
Copy link
Author

D-Nice commented Aug 17, 2021

Yep, the strip was just a naive solution that worked in my case for my specific issue, under /json1 path below, but it doesn't address the underlying issue, which is showcased by /json2:

import options, asyncdispatch, json
import httpbeast

proc onRequest(req: Request): Future[void] =
  if req.httpMethod == some(HttpGet):
    case req.path.get()
    of "/json1":
      const data = $(%*{"message": "Hello, World!"})
      const headers = "Content-Type: application/json\c\L" &
        "Access-Control-Allow-Methods: GET\c\L" &
        "Access-Control-Allow-Headers: Content-Type\c\L" # trailing \c\L cuts off `"}`
      req.send(Http200, data, headers)
    of "/json2":
      const data = $(%*{"message": "Hello, World!"})
      const headers = "Content-Type: application/json\c\L" &
        "Access-Control-Allow-Methods: GET\c\L" &
        "Access-Control-Allow-Headers: Content-Type" &
        "\c\L\c\L" & # causes overflow, can essentially overwrite response data
        "overflows into body"
      req.send(Http200, data, headers)

run(onRequest)

Responses

curl localhost:8080/json1

got

{"message":"Hello, World!

curl localhost:8080/json2

got

overflows into body

{"me

Expected for both

{"message":"Hello, World!"}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants