feat: Add route to fetch server information by IP and port #69
Conversation
|
sounds interesting, have you considered enabling CORS too (not sure if that's already available) |
|
I am inclined to reject this in the spirit of simplicity. list.json is about 200KB compressed, easily cacheable by the server and contains all this data (and more, if you e.g. want to show multiple servers).
The production deployment already has CORS enabled via my nginx config. |
|
I see what you mean, but for a site running on a static site generator, rebuilds don't usually happen until a new commit. On the site I was testing with, it takes several seconds after the page loads for the player count to load. 200kb is trivial for a desktop app, but is several times larger than the total file size I would shoot for on a website homepage. Most API's have lots of ways to query the data as opposed to just dumping it all at once, so it wouldn't be unusual to have even more routes than this. The only thing I would be cautious with about with this implementation is that luanti-org/luanti#7400 (A custom URL protocol) has not decided on a schema for sure yet and it would be nice for the serverlist schema to match. |
server.py
Outdated
| @app.route("/server/<string:ip>/<int:port>") | ||
| def get_server(ip, port): | ||
| try: | ||
| # Validate IP address |
There was a problem hiding this comment.
Do we need this validation? Since it's just a lookup, you will just get a "server not found" if you pass an invalid address. (Furthermore, such validation may itself introduce DoS attack vectors: For example consider the string -> int conversion for arbitrarily large strings below. Arguably Python does limit that however.)
What I'm slightly more concerned about: This seems to not normalize IPv6 addresses; it seems to not allow omitting zeroes from IPv6 addresses (since then not part will be true below)?
This seems especially problematic as in the serverlist, there are IPv6 addresses with omitted zeroes (example: "Free For All Skywars" has 2a02:4780:28:71f7::1).
There was a problem hiding this comment.
These are fair points. I thought about removing the IP validation, but only after I had committed. I can definitely still remove it considering the only advantage is slightly better error messages.
There was a problem hiding this comment.
0e7ba22 IP validation removed. Invalid addresses will now return "Server not found" with a status code of 404.
There was a problem hiding this comment.
Okay, but that still leaves the point about normalization.
I feel like this should be relatively easy to do properly by using Python's built-in ipaddress library rather than manually juggling (generally not normalized) IP address strings everywhere.
(That would also give us validation back, essentially "for free".)
There was a problem hiding this comment.
I'm not sure. How verbose and specific do we need error messages to be. Really it either finds a match or it doesn't. Wouldn't this be better implemented in the serverList.get() function itself?
There was a problem hiding this comment.
I'm not concerned about the error messages. I think the serverlist should probably be using IP objects internally to begin with, and this PR should look up things in the server list by IP object equality.
Really it either finds a match or it doesn't.
The problem is that finding a match by string equality is bogus for IPv6 addresses since the same address can be written in different ways (you can not omit the zeroes, for example).
You could make the formatting part of the API but I don't like that.
Then again of course refactoring the serverlist to use IP address objects may be out of scope.
There was a problem hiding this comment.
The ip field is really just filled with whatever "standard" represenstation Flask/Python or the C stdlib decide on for the client IP address.
| @app.route("/server/<string:ip>/<int:port>") | ||
| def get_server(ip, port): | ||
| try: | ||
| server = serverList.get(ip, port) |
There was a problem hiding this comment.
there's another pitfall here: this matches on the "announce requester IP" and not "IP of the server domain name" (this info is not present) or "specified IP or hostname of server".
so /server/your-land.de/30000 would never return anything
and for dualstack servers it could happen that it randomly announces over v4 and not v6, e.g. you normally do /server/2001:db8::123/30000 except you could get unlucky and it returns nothing but /server/1.2.3.4/30000 would have been right
There was a problem hiding this comment.
so
/server/your-land.de/30000would never return anything
Isn't that a serverList.get problem?
There was a problem hiding this comment.
Sort of. It does what it's supposed to for serverlist-internal usage, but might not do what someone expects of the API you're intending.
This PR adds an API route to fetch information about a particular server as opposed to fetching the whole list.
This will be useful for allowing server owners to add stats such as live player counters to their websites without having to load the whole server list.