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

feat: supports setting multiple hosts #2486

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vvanglro
Copy link
Contributor

Summary

On the realization of this discussion
#1529

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

docs/deployment.md Outdated Show resolved Hide resolved
@vvanglro
Copy link
Contributor Author

vvanglro commented Oct 17, 2024

Not sure why that test case is failing, doesn't seem to have anything to do with this PR. The local tests have always passed.

Both PRs failed. #2444 #2431

The last few commits haven't involved changes to failed use cases either, so why is that?🤔

@vvanglro vvanglro changed the title feat: support for setting ipv6 via cli feat: supports setting multiple hosts Oct 17, 2024
@vvanglro
Copy link
Contributor Author

vvanglro commented Oct 17, 2024

Well, the investigation revealed that httptools changed its behavior in 0.6.3.
MagicStack/httptools@560bd9e
#2375

pip install --upgrade httptools==0.6.3
import httptools

REQUEST_AFTER_CONNECTION_CLOSE = b"\r\n".join(
    [
        b"GET / HTTP/1.1",
        b"Host: example.org",
        b"Connection: close",
        b"",
        b"",
        b"GET / HTTP/1.1",
        b"Host: example.org",
        b"",
        b"",
    ]
)

parser = httptools.HttpRequestParser("a")
# Adding this line works fine.
# parser.set_dangerous_leniencies(lenient_data_after_close=True)
print(parser.feed_data(REQUEST_AFTER_CONNECTION_CLOSE))

output:

print(parser.feed_data(REQUEST_AFTER_CONNECTION_CLOSE))
  File "httptools/parser/parser.pyx", line 259, in httptools.parser.parser.HttpParser.feed_data
httptools.parser.errors.HttpParserError: Data after `Connection: close`

@Kludex
Copy link
Member

Kludex commented Oct 17, 2024

Is it a problem on uvicorn or on httptools?

@vvanglro
Copy link
Contributor Author

Is it a problem on uvicorn or on httptools?

REQUEST_AFTER_CONNECTION_CLOSE = b"\r\n".join(
    [
        b"GET / HTTP/1.1",
        b"Host: example.org",
        b"Connection: close",
        b"",
        b"",
        b"GET / HTTP/1.1",
        b"Host: example.org",
        b"",
        b"",
    ]
)

If such a message exists in a real environment, it's a problem with uvicorn.

@Kludex
Copy link
Member

Kludex commented Oct 17, 2024

I don't understand. Is it because of a security issue that was fixed?

@vvanglro
Copy link
Contributor Author

vvanglro commented Oct 17, 2024

MagicStack/httptools@560bd9e

Here are the details of this commit:

  Expose leniency flags via the new `set_dangerous_leniencies` parser
  method if somebody needs to opt into the old vulnerable behavior.

It looks like the error was passed to the caller, maybe it hadn't been before.

I'm not a cython expert.

@vvanglro
Copy link
Contributor Author

Oh, I see, now using a more standardized and strict parsing method. That's why the above message, when processed, reported an error. After the connection is closed, no messages should be sent after it.

@Kludex
Copy link
Member

Kludex commented Oct 17, 2024

Oh, I see, now using a more standardized and strict parsing method. That's why the above message, when processed, reported an error. After the connection is closed, no messages should be sent after it.

So uvicorn should not send anything to the parser after, right?

@vvanglro
Copy link
Contributor Author

So uvicorn should not send anything to the parser after, right?

yes.

PyCharm 2024-10-17 22 16 17
class HttpToolsProtocol(asyncio.Protocol):
    def __init__(
        self,
        config: Config,
        server_state: ServerState,
        app_state: dict[str, Any],
        _loop: asyncio.AbstractEventLoop | None = None,
    ) -> None:
        if not config.loaded:
            config.load()

        self.config = config
        self.app = config.loaded_app
        self.loop = _loop or asyncio.get_event_loop()
        self.logger = logging.getLogger("uvicorn.error")
        self.access_logger = logging.getLogger("uvicorn.access")
        self.access_log = self.access_logger.hasHandlers()
        self.parser = httptools.HttpRequestParser(self)
++      self.parser.set_dangerous_leniencies(lenient_data_after_close=True)

It seems we can only add one line of code to handle this.
After adding this line of code, the response before the connection is closed can be processed normally.
Messages after the connection is closed will not be parsed, That is, the headers value in the figure, only resolved to the pre-close.

@vvanglro
Copy link
Contributor Author

#2238

This is wrong behavior, right?
image

that's right?
image

@Kludex
Copy link
Member

Kludex commented Oct 17, 2024

I don't know, I'd have to read some resources before answering that.

@vvanglro
Copy link
Contributor Author

We'll take the next step when we figure it out.

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.

2 participants