[API Request] Officialise POST support

[Tolriq]

Type of change

API Clarification

Proposal description

It seems quite a few servers already support that but we need to officialise POST support to the API to allow large batch queries like editing playlists, or future extensions.

Backward compatibility impact

No response

Backward compatibility

• No backward compatibility impact.

API details

To be documented by servers that already support that as we should use what's existing to ensure compatibility.

Security impacts

No response

Potential issues

No response

Alternative solutions

No response

[Tolriq]

@opensubsonic/servers We need to move on with this to expand on the new APIs.

The server who already support some cases like form post or something else please post here and let's try to define something simple.

If there's no consensus for the new API it would be best to support json post and not form based list of params.

[deluan]

POSTs are accepted by Navidrome, based on a request by (IIRC) @MichaelBechHansen for play:Sub. It accepts a x-www-form-urlcoded payload, like this:

curl -v -X POST -H 'Content-Type: application/x-www-form-urlencoded' 'http://localhost:4533/rest/ping.view' --data 'c=client&v=1.8.0&f=json&u=user&p=password'

[gravelld]

POSTs are accepted by @Astiga for all requests. The query string can be used for parameters, or x-www-form-urlcoded can be used.

[sentriz]

gonic works the same as Navidrome and Astiga 👍

[Tolriq]

Ok so seems @epoupon LMS does not yet support that, so we can't just say it's in OS and need an extension.

Since most already support x-www-form-urlcoded the extension will just say that this is supported so clients knows they can use it.

We can still add json post later as it won't be incompatible if server check the content type.

[Tolriq]

#74

[crazygolem]

The specification is too ambiguous regarding the handling of duplicated parameters in both the query and body, and specifically the authentication parameters.

The ambiguity could potentially lead to a parameter pollution vulnerability, where a component authenticates one user, and another component handles the request for another user, especially since query parameters and form parameters are often merged in library/framework-specific ways.

Although this could in principle be an issue already when you just have a subsonic server, it can actually become problematic in deployments where a reverse proxy in front is in charge of the authentication. It is especially problematic since a fully secure authentication service becomes potentially vulnerable when the subsonic backend is switched for an opensubsonic one that supports this extension.

I suggest to improve the specification to address the issue specifically for the authentication parameters, as ambiguity with the other parameters should not create a security risk.

My preferred solution would be to specify that authentication parameters must not appear in the body. If an alternative to query parameters is desired, the Authorization header is much more appropriate. This does not actually solve the issue entirely (you still have ambiguity between sources), but we avoid the issue of libraries/frameworks automatically merging parameters from multiple sources in different ways (which is the biggest footgun in this story).

Some other ideas but they all feel just off the mark:

• Servers should check each source independently and raise an error if parameters are found in several sources.
  • This is likely to break current implementations (e.g. clients sending basicauth credentials in addition to query parameters instead of replacing them, servers supporting the Remote-User header but not requiring the authentication service to remove the query parameters).
  • An alternative with lower risk of breaking things would be to specify an order between the sources, and in particular I suggest that any source specified by OS should have lower priority than query, to avoid the scenario where an authenticating reverse-proxy setup becomes vulnerable when the subsonic backend adds support for extra OS-specified sources. Custom sources can still have higher priority than query.
• Authentication parameters should all be fetched from the same source (either all from the query, all from the body, or all from a custom source), i.e. there should not be a fallback mechanism on individual authentication parameters.
  • This does not actually prevent the issue but makes it easier to implement checks.

What do you think?

[Tolriq]

You mix things, what is important is to not change the current API definition that would break existing clients.
Servers already support POST, clients already use POST. If you change how POST works then you break all legacy clients that rely on POST and pass the auth params there.

All that to fix your very specific proxy with 0 security gain for the vast majority of people?

So again this extension does not change anything, it just allow clients to know what server support post without ANY BREAKING CHANGE.
For the record it's actually all the servers ....

Now about the code, I don't care about code, I care about server work, seems for Gonic it's simple, now imagine I use a library that just callback my code with all the params without distinction from where they come? I need to rewrite the server to use another library? I need to write myself a webserver? I need to manually parse everything? You have absolutely no idea about the impact on server code this simple change can make. (And again ignoring that it can break a few legacy clients ...)

And yes there's OS servers that do not support the new auth because the new auth is dumb and requires storing the password in clear and does not allow forwarding of the auth to external more secure systems....

And yes clear text in body or not have 0 impact as I said too, and GET already does not specify what to do if you have multiple u and p parameters.

From Subsonic POV that does not change anything about security to allow or not the auth params in the headers or in the post or in both.

TL;DR; your request brings nothing to Subsonic servers including security, but breaks legacy clients as all servers already support POST it would force all servers to update their code for your proxy.

PS: A lot of clients support adding headers for proper authentification without the need for special proxy that tries to do strange things over Subsonic API. API is clearly not well defined and the reason a new one is created.

So give your requirements in the new auth thread, but we're not breaking legacy clients over your very specific proxy.

[sentriz]

just to add, and could be wrong, the only client that I know of that uses form params is play:Sub. so we could check if it uses that for auth or not. if not then there's no issue presumably @MichaelBechHansen

[Tolriq]

Since POST is the only way to properly manage large playlists I'm pretty sure others have discovered and used that since then.

[crazygolem]

Servers already support POST, clients already use POST. If you change how POST works then you break all legacy clients that rely on POST and pass the auth params there.

Supporting POST and supporting the formPost extension are not exactly the same thing: legacy clients don't support formPost, they just throw whatever at the server hoping it works (it's an image), legacy servers don't support formPost, they just go "meh whatevs" (also not literal).

This being an extension, I was assuming a new thing by opensubsonic for servers that actually support the opensubsonic spec, and the way the documentation has been written in the API Reference suggests to me that it was actually intended to be that way, where actually only the part about servers letting clients know is the extension.

If legacy clients and servers are involved, the basic support for POST form parameters should probably be rebranded as a clarification instead, according to the terminology on OpenSubsonic changes.

With that, I'll drop the idea of not allowing authentication parameters in the body, and instead we can do the following:

1. Clarify that all parameters should be located either in the query or the body, not both.
   • Legacy and actively maintained clients likely already do that, there is not a good reason to mix both sources if you're not trying to break the server.
   • Legacy servers don't care about opensubsonic, so they won't be impacted: if they support post, they already support compliant clients. They will still be vulnerable, but that won't change from the situation before the clarification, also they are called "legacy" for a reason.
   • Actively maintained servers that support this extension can either assume clients will be nice, or add an extra check if they want to be on the secure side.
   • Middlewares can rely on a clear, security-conscious spec to implement their stuff securely.
2. Clarify that each authentication parameters should appear at most once
   • Legacy and actively maintained clients likely already do that, it's just the normal thing to do.
   • Legacy servers don't care: same as above, they don't care when the clients are compliant, they are potentially vulnerable when not but it doesn't change the status quo.
   • Actively maintained servers: same as above.
   • Middleware: same as above.
3. Maybe add a security warning to the documentation, that's just the nice thing to do, and is likely to be propagated to the servers' documentation when they support reverse proxies.

Note that:

• The spec restricts clients, not servers: if the client sends something unexpected, servers can deal with it however they want, as always. Servers don't have to do anything if they don't want to and still remain compliant.
• Genuine clients, and most importantly legacy clients, already work this way, simply because it's the normal thing to do. As such, it doesn't break compatibility.
• It doesn't actually solve the parameter pollution vulnerability entirely, but makes it easier to prevent against it because the remaining issue is highlighted in the spec for implementors.

I still think that the extension misses the mark, as legacy clients that do POST will still send POST to an opensubsonic server that does not support POST, and actively maintained clients that do support opensubsonic could benefit from a better (security-wise) extension because they are able to adapt.

As a clarification of the original subsonic spec, I don't think we need to change much to the extension's specification. But if needed, since the formPost extension is relatively new, I think there is a chance to have all clients and servers that support it agree to the change, and we can just add the clarifications to the extension. Otherwise, we could create a new extension with the updated spec, let's call it formPost2, and deprecate formPost.

What do you think about this?

[Tolriq]

Any change even as suggestion is breaking ....

And yes it's common to pass auth via get and still pass the rest via form specially when code is conditional to the server it talks to .... Many client have if based on server name.

Global auth interceptor adding auth and actual query code that is either get or post depending on the server ....

And for the last time no we will not change current auth spec for no reason as it's breaking. We will build a new one.

The spec is all about servers.... That's their job to ensure it's respected and enforce what it says ....
If you want clients to do something ask directly the client you use, if client have choice they will use what's best for them.