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

Client-sent Host header can include port as "None" #4039

Closed
brasie opened this issue Sep 5, 2019 · 2 comments · Fixed by #4040
Closed

Client-sent Host header can include port as "None" #4039

brasie opened this issue Sep 5, 2019 · 2 comments · Fixed by #4040

Comments

@brasie
Copy link

brasie commented Sep 5, 2019

Long story short

When the client forms the Host header, it is possible for it to include the port as "None".

This came up for me when using aiodocker to try to connect to the Docker API container attach websocket endpoint, which used a URL of the form "unix://localhost/..." and let to a "Host" header of "localhost:None", triggering a 400 error from docker with a message like:

parse ws://localhost:None/v1.35/containers/CONTAINER_ID/attach/ws?stdin=1&stdout=0&stderr=0&stream=1: invalid port ":None" after host

Expected behaviour

At least, not to send "None" as a port number for the Host header.

According to RFC 7230 Section 5.4:

If the authority component is missing or undefined for the target URI, then a client MUST send a Host header field with an empty field-value.

So perhaps it should be possible for the aiohttp client to get and recognize such a URI and send a blank Host header field.

At the moment though, I think, it doesn't seem possible to send such an "authority"-less URL to ws_connect nor does there currently exist a conditional path for the Host header construction to make a blank Host header field: client_reqrep.py lines 314-320

Actual behaviour

The Host header includes the string "None" as the port when making requests whose URL registers as not is_default_port() but has no port defined, e.g. unix://localhost/path/to/endpoint.

Steps to reproduce

This occurred for me while using the aiodocker package to attach to stdin of a running container.

A sort of silly example server/client that displays the behavior is as follows:

from aiohttp import web
from asyncio import sleep, create_task
import aiohttp

SOCK_PATH = '/tmp/example.sock'

async def hello(request):
  print('Host: '+request.headers['Host'])
  return web.Response()

async def make_request():
  await sleep(1) # Let the server become available.
  conn = aiohttp.UnixConnector(path=SOCK_PATH)
  async with aiohttp.ClientSession(connector=conn) as session:
    async with session.get('unix://localhost/'):
      pass # Produces a Host of "localhost:None"
    async with session.get('http://localhost/'):
      pass # Produces a Host of "localhost"

async def schedule_request(_):
  create_task(make_request())

app = web.Application()
app.add_routes([web.get('/', hello)])
app.on_startup.append(schedule_request)

web.run_app(app, path=SOCK_PATH)

Output:

======== Running on http://unix:/tmp/example.sock: ========
(Press CTRL+C to quit)
Host: localhost:None
Host: localhost

Your environment

  • Debian 9
  • Python 3.7.4
  • aiohttp 3.5.4
  • aiodocker 0.14.0
  • Docker 19.03.2-ce

BTW the specific thing that I think make this appear where it didn't before was a security update to Go that make URL parsing more strict: https://github.com/golang/go/issues?q=milestone%3AGo1.12.8

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2019

Thanks for the report! The fix is ready

@helpr helpr bot added pr-merged and removed pr-available labels Sep 6, 2019
asvetlov added a commit that referenced this issue Sep 6, 2019
…ovided (#4040)

(cherry picked from commit db760f7)

Co-authored-by: Andrew Svetlov <[email protected]>
asvetlov added a commit that referenced this issue Sep 6, 2019
…ovided (#4040) (#4041)

(cherry picked from commit db760f7)

Co-authored-by: Andrew Svetlov <[email protected]>
@brasie
Copy link
Author

brasie commented Sep 6, 2019

And thanks for the fix!

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

Successfully merging a pull request may close this issue.

2 participants