Skip to content

Are we testing real-world frameworks? #8116

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

Open
trikko opened this issue Jan 3, 2025 · 23 comments
Open

Are we testing real-world frameworks? #8116

trikko opened this issue Jan 3, 2025 · 23 comments

Comments

@trikko
Copy link
Contributor

trikko commented Jan 3, 2025

I believe that the frameworks included here should at least satisfy a minimum level of adherence to the HTTP standard. I'm not saying they need to be perfect, but they should at least implement the basics of the standard.

Otherwise, this isn't a benchmark of HTTP frameworks, but rather a benchmark of pseudo-HTTP programs designed to win benchmarks, because obviously the fewer checks you perform, the faster you are.

And in that case, they wouldn't even be useful in real-life scenarios, would they?

Give it a try and run this script against any random web framework, and try to check the responses they give. It's just a little subset of what should be tested.

#!/bin/bash

echo "This should be rejected with a 400 (sdsP/4.3 is not a valid protocol)"
echo "----------------------------------"
echo -e "GET /#/pizza/user/100 sdsP/4.3\r\nconnection: close\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo

echo "This should be rejected with a 400 (HTTP/1.3 is not a valid protocol)"
echo "----------------------------------"
echo -e "GET /#/pizza/user/100 HTTP/1.3\r\nconnection: close\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo

echo "This should be rejected with a 400 (HTTP/0.2 is not a valid protocol)"
echo "----------------------------------"
echo -e "GET /#/pizza/user/100 HTTP/0.2\r\nconnection: close\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo

echo "Server should respond with HTTP/1.0 200, not HTTP/1.1 200"
echo "----------------------------------"
echo -e "GET / HTTP/1.0\r\nconnection: close\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo

echo "GETTY is not a valid method, 400 should be returned"
echo "----------------------------------"
echo -e "GETTY / HTTP/1.0\r\nconnection: close\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo

echo "PET is not a valid method, 400 should be returned"
echo "----------------------------------"
echo -e "PET / HTTP/1.0\r\nconnection: close\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo

echo "GIT is not a valid method, 400 should be returned"
echo "----------------------------------"
echo -e "GIT / HTTP/1.0\r\nconnection: close\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo

echo "Invalid headers, 400 should be returned"
echo "----------------------------------"
echo -e "GET / HTTP/1.0\r\nconnection: close\r\ninvalid header\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo

echo "Header with no space between colon and value are still valid. This request should be closed by the server immediately"
echo "----------------------------------"
echo -e "GET / HTTP/1.1\r\nhost:localhost\r\nconnection:close\r\n\r\n" | nc -w 10 localhost 3000 ; echo ; echo


echo "Every http/1.1 response must include date and (content-length or transfer-encoding)"
echo "----------------------------------"
echo -e "GET / HTTP/1.1\r\nhost:localhost\r\nconnection: close\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo
echo -e "GET /user/1009 HTTP/1.1\r\nconnection: close\r\nhost:localhost\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo
echo -e "ààèèììòùù\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo


echo "This is not a valid http/1.0 request, 400 should be returned"
echo "----------------------------------"
echo -e "GET HTTP/1.0\r\n\r\n" | nc -w 3 localhost 3000 ; echo ; echo
@waghanza
Copy link
Collaborator

waghanza commented Jan 4, 2025

You say that we are not testing any real world usage, I cannot disagree.

The scenario are not very useful , we are testing the routing part of a piece of software. There is an open discussion for that (I mean to have a new scenario) #8093

Also we need to change our load tool in order to test at constant rate, there is also an open discussion. The idea is to use a tool like Vegeta to make http calls, instead of wrk to make sure we are more realistic in figures.

Also what you point is that we need to check http headers ... I agree that it could be accurate to test http communication facing some security rules, like OWASP. You script cool be cool and can be used in a one shot manner to see if there is some framework that are falsey, and open an issue on their repository.

@waghanza
Copy link
Collaborator

waghanza commented Jan 4, 2025

I mean that your point @trikko is more related to HTTP part, and that is more the responsibility of HTTP stack, not the whole framework.

For example, node or deno or bun for JavaScript should pass this kind of test, and this will affect express, bunicorn, fastify ... but the root cause is not in their scope

@waghanza
Copy link
Collaborator

waghanza commented Jan 4, 2025

After some verifications, it appears that echo -e "GET /#/pizza/user/100 sdsP/4.3\r\nconnection: close\r\n\r\n" | nc -w 3 $1 3000 returns various kind of things :

@trikko
Copy link
Contributor Author

trikko commented Jan 4, 2025

It was related to http stack just because it was easier to test!

There would be many similar tests, for example, "expect: 100-continue." Surely, if the server does not respond correctly, it has probably never been widely used in real life, because even a simple file upload with curl triggers this. And if not handled, the server likely remains waiting indefinitely.

This should be a strict requirement. 🙂

@trikko
Copy link
Contributor Author

trikko commented Jan 4, 2025

400 means bad (http) request. Is that a http request at all? Even closing connection I think is fine in this case.

There's nothing indicates it's a http request and for sure not a http/1.1! "sdsP" could be a completely different protocol.

For example: https://www.twilio.com/docs/glossary/sip-invites#example-sip-invite

In this case should I reply with HTTP?

@waghanza
Copy link
Collaborator

waghanza commented Jan 4, 2025

Not sure I'm legitimate to answer this 😛

The idea, imho, is more to stick to the standard, but again not in the scope of this project.

@trikko
Copy link
Contributor Author

trikko commented Jan 4, 2025 via email

@waghanza
Copy link
Collaborator

waghanza commented Jan 4, 2025

You have some points 😛 I plan to have OWASP tests here, to test the implementations.

I can some test also to make sure that a server does not return HTTP1.1 if we send HTTP/1.0, but this is more like for investigation, not something we can merge

@trikko
Copy link
Contributor Author

trikko commented Jan 4, 2025

I think I could write a small tool to make some tests, it would be nice to see which % is passed by each server!

IMHO some tests must be mandatory (parsing of http protocol, correct reply to "connection: xxx", etc.), and every server must at least pass x% of non-mandatory tests

It would be nice to have a column with test passed (%)

@waghanza
Copy link
Collaborator

waghanza commented Jan 4, 2025

I think you should start a PR / issue to discuss with communities, but not something we should merge / add to the UI.

imho, this is usefull to make some network handling part more reliable facing http standard

you could even write a list of tests (a plain list of bullet points), I can convert them into ruby for specs here

@dominikzogg
Copy link
Member

dominikzogg commented Jan 4, 2025

@waghanza

@dominikzogg
Copy link
Member

@waghanza it seems to be not possible to read the httpVersion of a ubwesocket.js request.

@waghanza
Copy link
Collaborator

waghanza commented Jan 5, 2025

Maybe an issue with nodejs then for chubbyts @dominikzogg

@trikko
Copy link
Contributor Author

trikko commented Jan 10, 2025

I released a small tool with some tests.
Try it: https://github.com/trikko/http-validator/

Just run dub when a server is running

@trikko
Copy link
Contributor Author

trikko commented Jan 10, 2025

For now, it's just a few tests. But apparently, they're enough to cause several servers to crash catastrophically (and many others fail!).

@cyrusmsk
Copy link
Contributor

Some servers are made only win benchmarks :) not to be used in real solutions :P

@waghanza
Copy link
Collaborator

And some framework used here and widely used in production are not fully respecting http standards

@trikko
Copy link
Contributor Author

trikko commented Jan 11, 2025

Yes but at least I hope they respect the bare minimum and they don't crash just for a request they received.

@trikko
Copy link
Contributor Author

trikko commented Jan 12, 2025

Fun fact. I see a reply to this request (4 bytes) with 200 ok (I wonder which uri...):

"\r\n\r\n"

@waghanza
Copy link
Collaborator

Yeah, I'll open a branch to check that and a PR to ping all buddies concerned by that

@trikko
Copy link
Contributor Author

trikko commented Jan 12, 2025

Another interesting thing: most servers don't seem to check the size of posts.

So I can make a post with a 100TB file by default on a server without it even flinching. There should be a default limit (adjustable, of course). Leaving such an open-ended limit is very dangerous.

@trikko
Copy link
Contributor Author

trikko commented Jan 12, 2025

So GET /user/123 must return 123, what about GET /user/asd? "asd" even if it's not a number? Writing a test for it :)

@trikko
Copy link
Contributor Author

trikko commented Jan 12, 2025

If GET /user/asd is valid, it would be nice to test some other things.

For example:

  • /user/hello%20world must print "hello world"
  • /user/hello/world must fail but
  • /user/hello%2Fworld must print "hello/world"

If you want only /user/:number: all of these example must fail (and we can't test if decoding works properly)

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

No branches or pull requests

4 participants