plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
248 stars 188 forks source link

Error message for invalid credentials appears twice #3983

Closed rohnsha0 closed 3 months ago

rohnsha0 commented 3 months ago

What I did:

Attempted to log in to a Plone site with an incorrect username and password.

What I expect to happen:

System should give error message once.

What actually happened:

A generic error message was displayed: "Error: Login failed. Both login name and password are case sensitive, check that caps lock is not enabled" twice, which seems redundant.

What version of Plone/ Addons I am using:

Plone 6.1.0a4.dev0 (6103) CMF 3.5 Zope 5.10 Python 3.11.9 (main, Apr 19 2024, 16:48:06) [GCC 11.2.0] PIL 10.2.0 (Pillow) WSGI: On Server: waitress 2.1.2

image

stevepiercy commented 3 months ago

@rohnsha0 this is deliberate for security. Never disclose that a username is valid in error messages, because then the attacker may submit a large number of passwords for that username.

rohnsha0 commented 3 months ago

but the error message are also appearing twice?!

stevepiercy commented 3 months ago

I updated the issue title and reopened.

stevepiercy commented 3 months ago

@rohnsha0 please update your issue description to reflect the double error message. It's correct that that error message is vague, but not doubled. Thank you!

rohnsha0 commented 3 months ago

@rohnsha0 please update your issue description to reflect the double error message. It's correct that that error message is vague, but not doubled. Thank you!

done

@rohnsha0 this is deliberate for security. Never disclose that a username is valid in error messages, because then the attacker may submit a large number of passwords for that username.

but cant we provide a clear, specific error message indicating that either the username or password is incorrect, without revealing which one is wrong (for security reasons)?!

rohnsha0 commented 3 months ago

According to my initial lookup, the following lines of code may be causing the issue: https://github.com/plone/Products.CMFPlone/blob/9866a60d56e6eef38dfbecaf57986513bff4bb5d/Products/CMFPlone/browser/login/login.py#L170-L176 however, I am unable to understand the error causing it to behave in this way

stevepiercy commented 3 months ago

There are two different, yet similar, error messages in that Python method. One is for entering an email address, the other a username. Both should be updated to be clear. What language do you suggest?

This conditional:

https://github.com/plone/Products.CMFPlone/blob/9866a60d56e6eef38dfbecaf57986513bff4bb5d/Products/CMFPlone/browser/login/login.py#L159

checks whether the person logging in as Anonymous (or not logged in), and if they have not successfully authenticated, then they are in fact Anonymous, and the rest of the conditional gets executed.

rohnsha0 commented 3 months ago

There are two different, yet similar, error messages in that Python method. One is for entering an email address, the other a username. Both should be updated to be clear. What language do you suggest?

I'll suggest it to just update to

status_msg.addStatusMessage(
                    _(
                        "Login failed. Please check your email address and password. "
                        "Remember that both fields are case-sensitive."
                    ),
                    "error",
                )

or something similar

checks whether the person logging in as Anonymous (or not logged in), and if they have not successfully authenticated, then they are in fact Anonymous, and the rest of the conditional gets executed.

I understood that block of code.. But what may be causing it behave that way or send two same statusMessage?! according to me, IStatusMessage might be having some issues. where are the code for IStatusMessage?

stevepiercy commented 3 months ago

I like it, but I'd shorten it:

"Login failed. Both inputs are case-sensitive."

I don't think we have any lockout procedure (for example, after three failed login attempts, the user's IP address is banned), or throttling mechanism, so we don't have to warn for those.

But what may be causing it behave that way or send two same statusMessage?!

I don't know. Running a debugger with a break point might reveal something, or there might be a test that checks for a bad login.

When looking at the rendered HTML on https://classic.demo.plone.org/en, I noticed that the first message is wrapped with an <aside>, whereas the second is not. Otherwise they are identical. That could be a clue.

petschki commented 3 months ago

This is a duplicate of https://github.com/plone/mockup/issues/1273

petschki commented 3 months ago

Note: theres a fixing PR -> https://github.com/plone/mockup/pull/1387