Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix security issue when running on Glitch #913

Merged
merged 5 commits into from
Jul 2, 2020

Conversation

edwardhorsford
Copy link
Contributor

The Prototype kit mostly works fine when running on glitch, but has an issue.

When you run the prototype, no username and password is required.

The kit is meant to require a username and password when run online, so that people don't accidentally come across the prototypes.

Glitch doesn't set NODE_ENV, so the kit incorrectly thinks it's running in a local environment. This pr checks an env var glitch uses, and if so, sets env to production.

This exposes a second bug - that the kit tries to apply https, but gets caught in a redirection loop because glitch sends a comma separated list for X-Forwarded-Proto. The accepted fix seems to be to split on , and use the first part (which would have no effect for most values of X-Forwarded-Proto).


I've tested this locally and on heroku and on glitch. It should cause no change locally or on heroku, but now correctly require username and password on glitch (and not crash if you set env to production).

If GDS doesn't want to 'officially' support glitch, I'd still suggest applying this pr - as it fixes the current security issue that Glitch prototypes incorrectly don't require a username and password.

I've avoided updating the docs about setting username and password on the assumption 'full' glitch support isn't yet wanted. But if wanted, I could add a single line - which would basically say to set username and password in .env.

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks Ed.

Would you be able to add an entry to the changelog, please?

@edwardhorsford
Copy link
Contributor Author

Looks good to me, thanks Ed.

Would you be able to add an entry to the changelog, please?

Sorry missed this. Done now.

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Thanks for doing that, @edwardhorsford 👍

This just needs a second review from @hannalaakso and then we can get this merged.

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

@edwardhorsford Looks great, thanks for your work on this 🙌

@hannalaakso hannalaakso merged commit 42e3b09 into alphagov:master Jul 2, 2020
@vanitabarrett vanitabarrett mentioned this pull request Jul 29, 2020
@vanitabarrett vanitabarrett added this to the v9.9.0 milestone Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants