search

spl0k / supysonic

Supysonic is a Python implementation of the Subsonic server API.
https://supysonic.readthedocs.io
GNU Affero General Public License v3.0
259 stars 57 forks source link

[Enchancement] Gunicorn request size limit #251

Closed trap000d closed 1 year ago

trap000d commented 1 year ago

Caught it when tried to save long playlist of >100 files (audio book). DSub has refused to create/save it with meaningless error message ("Error. Can't save playlist"). In nginx logs there was only mysterious error code 400 after long request body:

"GET /rest/savePlayQueue.view?u=username&p=enc:[PASSWORD]&v=1.2.0&c=DSub&id=f31c525f............
........&current=09b2e52f-087a-4cf0-b316-1d805b9228fc&position=517730 HTTP/1.1" 404 151 "-" "DSub"

After curling a bit it has made more sense:

curl -v "https://supysonic.local/rest/createPlaylist?u=username&.........
........... a lot of irrelevant characters is skipped ........
<html>
  <head>
    <title>Bad Request</title>
  </head>
  <body>
    <h1><p>Bad Request</p></h1>
    Request Line is too large (5069 &gt; 4094)
  </body>
</html>

WTH the magic number 4094 is? Yep, this is Gunicorn default maximal request body length. Fortunately it's definable if server starts from gunicorn, e.g.:

gunicorn -w 1 -b 127.0.0.1:5722 --limit-request-line 65534 "supysonic.web:create_application()"

Unfortunately, --limit-request-line parameter is not passed to supysonic-server so the following construction does not working:

ExecStart=supysonic-server -h 127.0.0.1 -S gunicorn --processes 4 --limit-request-line 65534

As for body size limit - I've picked 65534 "almost randomly". I guess it should allow to save ~1750 songs in a single playlist, which would be fair enough to me :)

I suppose that limit must be somehow mentioned in documentation. Ideally, it would be nice to able overriding the limit with corresponding command line flag of supysonic-server

Regards,

spl0k commented 1 year ago

This could be extended to "being able to pass any parameter from supysonic-server to the underlying server".

As for the problem in itself, even if you increase this limit you might still run into this issue if you have other proxies between the client and server (might happen especially on mobile connections). This boils down to the HTTP or URL specification. This is somewhat a known limitation of the Subsonic API. It can be worked around using POST requests rather than GET. Supysonic supports POST requests but I think most clients are using GET...

trap000d commented 1 year ago

This could be extended to "being able to pass any parameter from supysonic-server to the underlying server".

Yes, that would be greatly appreciated.

As for the problem in itself, even if you increase this limit you might still run into this issue if you have other proxies between the client and server (might happen especially on mobile connections). This boils down to the HTTP or URL specification. This is somewhat a known limitation of the Subsonic API. It can be worked around using POST requests rather than GET. Supysonic supports POST requests but I think most clients are using GET...

Yep, POST vs GET was an issue from early days of web.

On the other hand, playlist size limit is not a big deal - all has the limit, even Universe. To me the issue is the default body size limit is relatively small, unknown, and error is obscured (documented bug is not a bug, it's a feature).

I'll have a look at DSub repository, may be they will fix this on their side. However, as you've mentioned, most players use GET, so that might have a little point (just checked: Jamstash uses GET too).

spl0k commented 1 year ago

Hello.

After putting it some thought (and work too, actually) I've decided I won't add any parameter or functionality to supysonic-server. I designed this command as a fast and easy way to start the web server, and I intend to keep it simple. Adding more options (that might not be supported by all servers) or forwarding all unknown ones to the underlying server might lead to some confusion and complexity. Since it's basically doing the same thing as running another command to start the underlying server (except for gevent), one might as well invoke this command directly if they want more control over its behavior.

By the way, according to Gunicorn documentation, the maximum value you can use for --limit-request-line is 8190. If you want to disable the limit you can instead pass 0.