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

System setting to disable logging in with a password #2504

Merged
merged 4 commits into from
Dec 5, 2017

Conversation

zachmullen
Copy link
Member

@manthey PTAL

@manthey
Copy link
Member

manthey commented Dec 5, 2017

If you have no oauth providers enabled and you disable password login, there are no login methods. Should we guard against that in any way?

@jeffbaumes
Copy link
Member

@manthey It would seem difficult to know whether another login mechanism (could be oauth or something else in the future) was enabled through a plugin, so I'd suggest leaving it to the admin to do the right thing.

@manthey
Copy link
Member

manthey commented Dec 5, 2017

Then, I'll assume that if an admin turns off all login methods for all admins, that someone has access to the database so they can turn them back on the hard way.

@zachmullen zachmullen merged commit d01087d into master Dec 5, 2017
@zachmullen zachmullen deleted the password-login-disable branch December 5, 2017 17:55
@brianhelba
Copy link
Member

@jeffbaumes @manthey Apropos to administrators managing to lock themselves out of their own Girder instance, note that we currently have a block of code which verifies and enables a user if they're the only user in the instance. Since the default setting is to not require verification / enablement, this is another case of not trusting / expecting admins to "to the right thing".

I'm generally uncomfortable with putting in settings that allow admins to break their own instance. Can we add some additional guards to prevent this case, or warn users?

Also, I'd like to look further into how we're passing public settings. Please hold on merging until I have a chance to review further.

@danlamanna
Copy link
Member

Can we add a changelog entry under added features?

brianhelba added a commit that referenced this pull request Jan 17, 2018
While the browser is parsing an inline Javascript string literal, a
"</script>" tag will terminate Javascript segment and return to HTML
parsing. The "json" filter alone does nothing to escape HTML content within
strings.

If an attacker managed to bypass validation of the ENABLE_PASSWORD_LOGIN
setting (which could occur via another vulnerability,), it previously would
have been possible to inject a malicious string into this setting value.
brianhelba added a commit that referenced this pull request Jan 17, 2018
While the browser is parsing an inline Javascript string literal, a
"</script>" tag will terminate Javascript segment and return to HTML
parsing. The "json" filter alone does nothing to escape HTML content within
strings.

If an attacker managed to bypass validation of the ENABLE_PASSWORD_LOGIN
setting (which could occur via another vulnerability), it previously would
have been possible to inject a malicious string into this setting value.
brianhelba added a commit that referenced this pull request Jan 18, 2018
Fix an XSS vulnerability from #2504
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants