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

Made incorrect username and password show the same error, for security purposes #2791

Merged
merged 1 commit into from
Feb 8, 2014

Conversation

lorian
Copy link
Contributor

@lorian lorian commented Feb 8, 2014

Should solve issue #2423 (#2423).

Note: I'm totally fine if this doesn't count as a "critical" bug fix for purposes of the nifty new hammer; it just seemed like a thing I could fix, and there are precious few of those.

@lefnire
Copy link
Contributor

lefnire commented Feb 8, 2014

Thanks for the fix @lorian! merging

Err... we may need to add a tag "hammer" and reword the bailey. Basically the hammer is for these excruciating last-boss bugs where many have tried and failed. Not to diminish your commit, it's very valuable! And definitely deserves a contrib.lvl++, [edit: nabbed UUID from other commit, upped your level]

actually, maybe that's not the best way to approach this after all. a crit is a crit, no need for a new tag. looking through the queue, this is the only crit which doesn't require very heavy involvement. @lorian I hope you don't take it personally, would you be ok if we gave you a hammer on your next crit?

lefnire added a commit that referenced this pull request Feb 8, 2014
Made incorrect username and password show the same error, for security purposes
@lefnire lefnire merged commit 982f194 into HabitRPG:develop Feb 8, 2014
@lorian
Copy link
Contributor Author

lorian commented Feb 8, 2014

No, that's totally cool, like I said. It definitely stood out to me as the odd bug in the crit list! I wish I could contribute to the really obnoxious bugs, but my dirty secret is I don't actually know any javascript, coffeescript or jade (heck, I'm not even sure if jade is a language), so I'm afraid I'm not much help with the complex parts.

@lorian lorian deleted the security-error branch February 8, 2014 06:33
@lefnire
Copy link
Contributor

lefnire commented Feb 8, 2014

Wha?? It's amazing to me how far people get without knowing these languages. Esp. that stealth reworking, complete with tests & all. Thanks again!

@colegleason
Copy link
Contributor

I tagged that as a critical because I thought it was important to the security of the site. We should probably define what critical really means, as I used it to mean "really important". Critical should probably imply something that makes the site unusable for a large portion of users.

Not that Habit is a huge target or anything, but I get the feeling the codebase probably has several large holes. We may want to do a security audit someday.

Off the top of my head:

  • Are logins susceptible to timing attacks?
  • The API Token design needs some rework in terms of permissions, etc.
  • Do we leak data via some API calls that shouldn't be public?

Anyways, thanks a ton @lorian. Great work, and we are happy to have your contributions. I wasn't a Javascript developer 12 months ago, so I will warn you that you get sucked in quickly once you start!

@deilann
Copy link
Contributor

deilann commented Feb 8, 2014

I just want to say, as someone who's never touched js before Habit, that
your code, @lefnire is very easy to read. It's well documented and your
variables and functions are named so that I can quickly figure out what
you're doing, even if I'm not familiar with the language. This means that
pretty much anyone who has basic programming knowledge can jump in and
tweak things!

Unfortunately, this is not all to common. Many people write code that's
hard for someone to understand, even if they know the language. So props
@lefnire!

On Fri, Feb 7, 2014 at 10:37 PM, Cole Gleason [email protected]:

I tagged that as a critical because I thought it was important to the
security of the site. We should probably define what critical really means,
as I used it to mean "really important". Critical should probably imply
something that makes the site unusable for a large portion of users.

Not that Habit is a huge target or anything, but I get the feeling the
codebase probably has several large holes. We may want to do a security
audit someday.

Off the top of my head:

  • Are logins susceptible to timing attacks?
  • The API Token design needs some rework in terms of permissions, etc.
  • Do we leak data via some API calls that shouldn't be public?

Anyways, thanks a ton @lorian https://github.com/lorian. Great work,
and we are happy to have your contributions. I wasn't a Javascript
developer 12 months ago, so I will warn you that you get sucked in quickly
once you start!

Reply to this email directly or view it on GitHubhttps://github.com//pull/2791#issuecomment-34536836
.

@deilann
Copy link
Contributor

deilann commented Feb 8, 2014

Also, I agree with that definition of critical. So I would argue that
perhaps a "hammer" tag would make sense. A not-so-difficult bug that
compromises security is a critical bug because it needs to be resolved ASAP.

On Fri, Feb 7, 2014 at 11:23 PM, Ryan [email protected] wrote:

I just want to say, as someone who's never touched js before Habit, that
your code, @lefnire is very easy to read. It's well documented and your
variables and functions are named so that I can quickly figure out what
you're doing, even if I'm not familiar with the language. This means that
pretty much anyone who has basic programming knowledge can jump in and
tweak things!

Unfortunately, this is not all to common. Many people write code that's
hard for someone to understand, even if they know the language. So props
@lefnire!

On Fri, Feb 7, 2014 at 10:37 PM, Cole Gleason [email protected]:

I tagged that as a critical because I thought it was important to the
security of the site. We should probably define what critical really means,
as I used it to mean "really important". Critical should probably imply
something that makes the site unusable for a large portion of users.

Not that Habit is a huge target or anything, but I get the feeling the
codebase probably has several large holes. We may want to do a security
audit someday.

Off the top of my head:

  • Are logins susceptible to timing attacks?
  • The API Token design needs some rework in terms of permissions, etc.
  • Do we leak data via some API calls that shouldn't be public?

Anyways, thanks a ton @lorian https://github.com/lorian. Great work,
and we are happy to have your contributions. I wasn't a Javascript
developer 12 months ago, so I will warn you that you get sucked in quickly
once you start!

Reply to this email directly or view it on GitHubhttps://github.com//pull/2791#issuecomment-34536836
.

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.

4 participants