client_id validation violates spec

[palant]

The remoteStorage spec says:

if no client registration is required, the server MUST ignore the value of the client_id parameter in favor of relying on the origin of the redirect_uri parameter for unique client identification.

This is not how this implementation behaves however. Instead, OAuthModule.validateClient() will enforce redirect URIs starting with the client ID. Two issues here:

1. This will reject client IDs that are valid per spec and work correctly with other servers (e.g. 5apps.com has no client ID validation).
2. Worse yet, this is broken as a security mechanism. Consider client_id being https://google.com yet redirect_uri being https://google.com.malicious.com. This will tell the user that the client is Google, yet send the token to an unrelated domain.

I'd suggest doing what the spec says, just ignore the client ID. Instead, parse redirect_uri and use its origin as your "client identifier."

[fkooman]

Right. That makes sense. Back then it was either violate the OAuth specifcation or the remoteStorage specification. To be fair, the "authorize" page does show both the client_id and redirect_uri. But that's indeed not really helpful for users to make an informed decision.

Proposal:

1. only show redirect_uri in dialog;
2. do not verify client_id at all.

What do you think?

[fkooman]

I just realised that's pretty much what you said :)

[palant]

Sounds good, with client_id being arbitrary and not meaningful.

It might still be a good idea to extract the host from redirect_uri, with users often not being good at reading URLs. Just so they don't get confused by things like username in the URL.

[raucao]

Worse yet, this is broken as a security mechanism. Consider client_id being https://google.com yet redirect_uri being https://google.com.malicious.com. This will tell the user that the client is Google, yet send the token to an unrelated domain.

Just an aside: this is not actually possible if the client ID's full origin is validated properly and compared to the redirect URI's origin. OAuth people also agree that without client registration, the two origins should be the same.

[palant]

That's not how I read the code. But even if origins are being compared here, the origin https://google.com.malicious.com still starts with the origin https://google.com. Or are origins produced here slash-terminated?

[raucao]

I meant that's how it should work in order for it not to be an issue. I haven't checked the code.

[fkooman]

The current check is obviously broken and should be fixed. Comparing Origin seems to be the most sensible yes: scheme :// host (:port)

[raucao]

https://google.com.malicious.com still starts with the origin https://google.com. Or are origins produced here slash-terminated?

Yes, the origin in that case would be https://google.com.malicious.com. You can try it yourself by opening the console on this page and typing:

(new URL('https://google.com.malicious.com/foo')).origin

[fkooman]

Untested as of right now. Any of you in the position to test this out?

[palant]

Not really. The change looks good, but I was merely reading code without having an installation to test things.

[fkooman]

I didn't test it for a long time now. In order to get this going again we really need to run the test suite against it to make sure all is still well...

[raucao]

I'm actually getting "invalid redirect URI" trying to connect the MyFavoriteDrinks example app, both from localhost as well as the deployed version:

[raucao]

I think as soon as this issue is fixed, it would be great to tag a new release for people to use! Especially now that the long-running Apache 304 headers bug has finally been fixed and released, so it's now finally possible to run php-remote-storage with mod_php.

[fkooman]

Sorry, I never got a notification for the redirect URL issue, only seeing it now...

Also, I would not recommend running this with mod_php, but instead with php-fpm...

[raucao]

Also, I would not recommend running this with mod_php, but instead with php-fpm...

Good to know! Looking at my instructions draft again, that's actually what I did. :)

[fkooman]

I think as soon as this issue is fixed, it would be great to tag a new release for people to use! Especially now that the long-running Apache 304 headers bug has finally been fixed and released, so it's now finally possible to run php-remote-storage with mod_php.

Fixed.

But I think the issue should still be open as this hasn't been solved.

Also, the code really needs a good review as it is basically a development branch that hasn't really been tested, nor has the code been cleaned up, nor made 'robust' and secure.

[raucao]

Fixed.

:tada:

Also, the code really needs a good review as it is basically a development branch that hasn't really been tested, nor has the code been cleaned up, nor made 'robust' and secur

If you create issues for necessary tasks to complete before a release, we could use the remoteStorage project accounts to ask people on fedi and birdsite for contributions.