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

Fix IndexDefect errors in httpclient on invalid/weird headers #22886

Merged
merged 6 commits into from Nov 1, 2023
Merged

Fix IndexDefect errors in httpclient on invalid/weird headers #22886

merged 6 commits into from Nov 1, 2023

Conversation

ghost
Copy link

@ghost ghost commented Oct 30, 2023

Continuation of #19262

Fixes #19261

The parsing code is still too lenient (e.g. it will happily parse header names with spaces in them, which is outright invalid by the spec), but I didn't want to touch it beyond the simple changes to make sure that std/httpclient won't throw IndexDefects like it does now on those cases:

  • Multiline header values
  • No colon after the header name
  • No value after the header name + colon

One question remains - should I keep toCaseInsensitive exported in httpcore or just copy-paste the implementation?

@ghost
Copy link
Author

ghost commented Oct 30, 2023

A simple test I made specifically for multiline headers:

import std/[asyncdispatch, asyncnet, os, strutils, strformat]

const csp = """default-src 'self';
                                     script-src 'self'
                                                'unsafe-inline'
                                                'unsafe-eval';
                                     img-src 'self'
                                             data:;
                                     style-src 'self'
                                               'unsafe-inline';
                                     font-src 'self';
                                     child-src 'self';
                                     object-src 'none'"""

proc handleClient(client: AsyncSocket) {.async.} =
  let line = await client.recvLine()
  var toAdd = ""
  if "case1" in line:
    # multiline header
    toAdd = &"Content-Security-Policy: {csp}\r\n"
  elif "case2" in line:
    # no colon after the header name!
    toAdd = "X-XSS-Protection 1; mode=block\r\n"
  elif "case3" in line:
    # no value after colon
    toAdd = "X-Xss-Protection:\r\n"
  let
    body = "<h1>Hello World</h1>"
    headers = "HTTP/1.1 200 OK\r\n" &
               "Server: nginx\r\n" &
               # this should work fine
               "Some-Empty-Header: \r\n"&
               "Content-Type: text/html\r\n" &
               "Content-Length: " & $body.len & "\r\n" &
               toAdd &
               "\r\n"
  await client.send(headers & body)
  client.close()

proc main {.async.} =
  let server = newAsyncSocket()
  server.bindAddr(Port(8080))
  server.listen()
  defer: server.close()
  echo "Running on http://localhost:8080/"
  while true:
    let client = await server.accept()
    asyncCheck handleClient(client)

waitFor main()

With client:

import std/[httpclient, strformat, httpcore, strutils, strformat, unittest]

proc testRequest(url: string): Response = 
  echo fmt"Requesting {url}"
  let c = newHttpClient("curl/8.4.0")
  let data = c.get(url)
  return data

const
  server = "http://localhost:8080"

const cspExpected = """default-src 'self';
                                     script-src 'self'
                                                'unsafe-inline'
                                                'unsafe-eval';
                                     img-src 'self'
                                             data:;
                                     style-src 'self'
                                               'unsafe-inline';
                                     font-src 'self';
                                     child-src 'self';
                                     object-src 'none'"""

suite "Test Requests":
  test "Basic empty header - OK":
    let response = testRequest(fmt"{server}/")
    check response.status == "200 OK"
    check response.headers["Some-Empty-Header"] == ""
  
  test "Multiline header - OK":
    let response = testRequest(fmt"{server}/case1")
    check response.status == "200 OK"
    let cspHeader = response.headers.getOrDefault("Content-Security-Policy")
    check cspExpected == cspHeader

  test "No colon after - should fail":
    try:
      let response = testRequest(fmt"{server}/case2")
    except ProtocolError as e:
      check "no colon after" in e.msg 

  test "No value after header + colon - should fail":
    try:
      let response = testRequest(fmt"{server}/case3")
    except ProtocolError as e:
      check "no header value after" in e.msg

lib/pure/httpclient.nim Outdated Show resolved Hide resolved
lib/pure/httpclient.nim Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Oct 31, 2023

I updated the comments so that we just don't crash but not really enforce the spec in any way. Last question - should I keep toCaseInsensitive exported (it's private right now) to use it, or copy the implementation?

@Araq Araq merged commit 40e33de into nim-lang:devel Nov 1, 2023
12 of 18 checks passed
Copy link
Contributor

github-actions bot commented Nov 1, 2023

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 40e33de

Hint: mm: orc; opt: speed; options: -d:release
176356 lines; 11.621s; 767.793MiB peakmem

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

Successfully merging this pull request may close these issues.

IndexDefect in httpclient because of a header spanning multiple lines
2 participants