Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Fix login redirects #4580

Merged
merged 1 commit into from
Jan 28, 2019
Merged

Fix login redirects #4580

merged 1 commit into from
Jan 28, 2019

Conversation

brianlovin
Copy link
Contributor

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • api

@mxstbr this closes #4571

The reason we initially did .endsWith('spectrum.chat') was to ensure logins came from our app. Before joining GitHub we changed this to .endsWith('.spectrum.chat') to prevent any malicious auth requests from from something like hackerdomainspectrum.chat

Unfortunately this broke redirects from community views. The problem is that we switched to getting the hostname from new URL(url) which will only ever return a hostname like spectrum.chat or alpha.spectrum.chat, therefore the endsWith was always missing the first period, and isSpectrumUrl returned false every time.

The fix here is to just not use endsWith at all - it's unnecessary since new URL will return the correct hostname from any given url. So we can just match that to prod or alpha and return true.

Can you give this a second eye and make sure there's no security implications here? I've deployed this to alpha and confirmed that it fixes login redirects.

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, looks good to me security-wise! Thanks for digging into this 🙏

@mxstbr mxstbr merged commit f06dc65 into alpha Jan 28, 2019
@mxstbr mxstbr deleted the fix-login-redirects branch January 28, 2019 09:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login redirects may be broken
2 participants