Fix open redirect in legacy SSO flow (idp)
#18909
Conversation
MadLittleMods
changed the title
idp)
| # Validate the `idp` query parameter | ||
| providers = self._sso_handler.get_identity_providers() | ||
| auth_provider = providers.get(idp) | ||
| if not auth_provider: | ||
| logger.info("Unknown idp %r", idp) | ||
| self._sso_handler.render_error( | ||
| request, "unknown_idp", "Unknown identity provider ID" | ||
| ) | ||
| return |
There was a problem hiding this comment.
Restoring this since it was removed in #17972
With hindsight, I'm not sure why it was removed although perhaps I just thought it was a bit superfluous as we would end up at a 404 anyway.
We could technically still do away with this and rely on the URL encoded value fix above to fix the open redirect problem.
There was a problem hiding this comment.
I think it'd be worth calling out that this code prevents Synapse from reaching out to arbitrary URLs (along with the URL-encoding later). Mostly to prevent it being arbitrarily removed by a future code change (even though the tests help with that as well).
There was a problem hiding this comment.
It was a bit tricky to word but I've added something 👍
| # Validate the `idp` query parameter | ||
| providers = self._sso_handler.get_identity_providers() | ||
| auth_provider = providers.get(idp) | ||
| if not auth_provider: | ||
| logger.info("Unknown idp %r", idp) | ||
| self._sso_handler.render_error( | ||
| request, "unknown_idp", "Unknown identity provider ID" | ||
| ) | ||
| return |
There was a problem hiding this comment.
I think it'd be worth calling out that this code prevents Synapse from reaching out to arbitrary URLs (along with the URL-encoding later). Mostly to prevent it being arbitrarily removed by a future code change (even though the tests help with that as well).
|
Thanks for the review @anoadragon453 🦐 |
2 participants
Fix open redirect in legacy SSO flow:
idpparameter to only accept the ones that are known in the config fileidpparameter for safety's sake (this is the main fix)Fix https://github.com/matrix-org/internal-config/issues/1651 (internal link)
Regressed in #17972
Dev notes
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.