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

Let's report issues #32

Closed
15 tasks done
ImMorpheus opened this issue Mar 15, 2017 · 1 comment
Closed
15 tasks done

Let's report issues #32

ImMorpheus opened this issue Mar 15, 2017 · 1 comment

Comments

@ImMorpheus
Copy link

ImMorpheus commented Mar 15, 2017

According to SpongePowered/Ore#241 . Here is what I found so far:

  • Currently, trying to sign up with a uppercase, already-taken username return a 500, internal server error (Example: try to sign up with Lukegb)

  • Cookies without secure flag (As I said in the other issue, it's not like it's a major problem, but it won't hurt)

  • Password's minimum length should be changed from 8 to 10 (Password shorter than 10 character are considered weak )

  • The password input field should have an autocomplete attribute set to off

  • You said the headers were already reported so I suppose they were also suggested for SpongeAuth

  • Charset is not specified in the 404 page

  • <img tag without an alt

  • I'm pretty sure id should be unique in the input tag, but there are two input with id="submit-id-save" in /accounts/settings/

  • © 2016 :(

  • The about link should point to stable not to master

  • In the registeration page there's a <div element with two class attribute (<div class="pull-right" class="form-group">)

  • <a is not allowed as a child of <ul in this context

PS: I've been having problem with the confirmation email (nothing arrived) so I can't look at it, but do you require the current credentials for an account before updating sensitive account information such as the user's password, user's email, etc. ?

If any of those look too "extreme" (the password length for example, don't implement them). Sometimes I exaggerate when it comes to security
I will add more issues as soon as I have some time

@lukegb
Copy link
Collaborator

lukegb commented Mar 15, 2017

The first one: yeah, I know. I realised quite late that you could sign up with a username that was already taken but in a different case, so I just added a database UNIQUE index to prevent that, without any special code handling that right now.

Although I do want to change the minimum password length, I'm not comfortable doing so. Users are encouraged, but not required to set longer passwords - I'll be making this more explicit in future with a password strength meter of some sort.

I'm pretty sure setting autocomplete="off" on password fields negatively impacts security (since all modern browsers have decent built-in password managers), and I think browsers ignore it anyway. OWASP advice since 2014 is to not bother trying to override saving passwords anyway.

Changing your password requires your old password. You cannot currently change your email address (#12, #13). Soon all actions on auth.spongepowered.org itself will require a "recent" authentication (#17).

I'll set the charset on the 404 page.

lukegb added a commit that referenced this issue Jul 29, 2017
Per WAI, image is purely decorative and is sufficiently described by
adjacent text.

Part of #32.
lukegb added a commit that referenced this issue Jul 29, 2017
lukegb added a commit that referenced this issue Jul 29, 2017
lukegb added a commit that referenced this issue Jul 29, 2017
lukegb added a commit that referenced this issue Jul 29, 2017
lukegb added a commit that referenced this issue Jul 29, 2017
lukegb added a commit that referenced this issue Jul 29, 2017
lukegb added a commit that referenced this issue Jul 29, 2017
lukegb added a commit that referenced this issue Jul 29, 2017
* Empty alt for navbar spongie SVG.

Per WAI, image is purely decorative and is sufficiently described by
adjacent text.

Part of #32.

* Add img alt text to avatar on profile page.

Part of #32.

* Add alt text for TOTP setup QR code

Part of #32.

* Add current year to footer

Part of #32.

* FormActions should not output two class attributes.

Part of #32.

* Avoid duplication of ids on settings page

Part of #32.

* Set session/CSRF cookies with secure flag.

Part of #32.

* Don't use a ul in an inappropriate context.

Part of #32.

* Fix lint

* Fix coverage decrease
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

No branches or pull requests

2 participants