? Pending

User tests: Successful: Unsuccessful:

avatar csthomas
csthomas
21 Jun 2016

Summary of Changes

If $app->login() returns false or throws exception then it should be the same effect, means do not use 303 redirection.
Redirection 303 should be only use on login success.

Backend login fails if:

  • $app->login() return false or
  • $app->login() throw Exception

Or success if:

  • $app->login() return true

Currently Joomla uses redirection if login method return false which is incorrect.

After that patch Apache module security can be use to limit POST requests.
Request POST with http code 200 (login fails) or 303 (login success)

Testing Instructions

Apply patch and

  1. try to login with valid username/password. Joomla should use redirection.
  2. try to login with invalid username/password. Joomla should not use redirection.
avatar csthomas csthomas - open - 21 Jun 2016
avatar brianteeman
brianteeman - comment - 21 Jun 2016

Something isnt correct with this PR as you have a changed memcache file

avatar csthomas
csthomas - comment - 21 Jun 2016

Fixed. Thanks.

avatar joomla-cms-bot joomla-cms-bot - change - 21 Jun 2016
Labels Added: ?
avatar csthomas csthomas - change - 21 Jun 2016
The description was changed
avatar bertmert bertmert - test_item - 21 Jun 2016 - Tested unsuccessfully
avatar bertmert
bertmert - comment - 21 Jun 2016

I have tested this item ? unsuccessfully on 286a6ef

2) try to login with invalid username/password. Joomla should not use redirection.

Afterwards I see a page like this:
21-06-_2016_14-11-08

Instead of:
21-06-_2016_14-23-29

After that patch mod_security can be use to limit POST request to login.

There is no mod_security in Joomla core.

So: ????????????

avatar csthomas
csthomas - comment - 21 Jun 2016

This PR show different template/view after login fails, probably I have to fix it.

About mod security:
This is apache module which can be configured to block IP access for some time
if attacker send POST requests to index.php?task=login too many times.

Ex:
if request is POST and task=login and returned code is 200 5 times per last hour
then block IP for 15 minutes.

avatar joomla-cms-bot
joomla-cms-bot - comment - 21 Jun 2016

This PR has received new commits.

CC: @bertmert


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar csthomas
csthomas - comment - 21 Jun 2016

Incorrect view fixed.
Please try to test once again.

avatar brianteeman brianteeman - change - 21 Jun 2016
Category Administration Authentication
avatar bertmert bertmert - test_item - 21 Jun 2016 - Tested successfully
avatar bertmert
bertmert - comment - 21 Jun 2016

I have tested this item successfully on 94f419e

After patch status 200 when invalid credentials instead of 303.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar csthomas csthomas - change - 21 Jun 2016
Title
After backend login fails (login return false) do not use 303 redirection
After backend login fails return status code 200 instead 303
avatar csthomas csthomas - change - 21 Jun 2016
Title
After backend login fails (login return false) do not use 303 redirection
After backend login fails return status code 200 instead 303
avatar csthomas
csthomas - comment - 23 Jun 2016

@bertmert Thank you for testing.

avatar pe7er
pe7er - comment - 25 Jun 2016

Sorry, I do not know what & how to test exactly.
Furthermore I do not understand when & why the 303 status code should be displayed.
IMHO all screens (login screen, login fail screen and login successful) all exist at the server and the server correctly returns the http status code for succss: 200.

I have tested this PR with Google Chrome + Webdeveloper addon ( http://chrispederick.com/work/web-developer/chrome/ )

Before the patch:
Login attempt /administrator/ with valid credentials: HTTP status code: 200
Login attempt /administrator/ with invalid credentials, login error screen: HTTP status code: 200

After the patch:
Login attempt /administrator/ with valid credentials: HTTP status code: 200
Login attempt /administrator/ with invalid credentials, login error screen: HTTP status code: 200

avatar joomlara joomlara - test_item - 25 Jun 2016 - Tested unsuccessfully
avatar joomlara
joomlara - comment - 25 Jun 2016

I have tested this item ? unsuccessfully on 94f419e

Cannot reproduce this issue.
Before and after the applying the patch, I get code 200. Both for login fail & login success.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar bertmert
bertmert - comment - 25 Jun 2016

The status codes immediately after POST requests:

Without patch and successful login 303

001_without-success

Without patch and wrong credentials 303

002_without-wrong

With patch and successful login 303

003_with-success

With patch and wrong credentials 200

004_with-wrong

avatar csthomas
csthomas - comment - 27 Jun 2016

For Chromium and Google Chrome:

Opening Chrome DevTools

  1. Open from Browser Menu
    You can open Chrome DevTools from Chrome’s menu , click into “More tools” and then click into “Developer Tools.”

  2. Or shortcut Ctrl+Shift+I

  3. Before patch when you use wrong credentials you should get (after POST request):
    backend_fails_303
    means 303 and after that 200 for index.php (which is incorrect, after patch will be only 200)

  4. After patch when you use wrong credentials you should get (after POST request):
    backend_fails_200
    means only one request to index.php with response status code 200.

  5. For correct credentials there is no changes and should looks like:
    backend_success_303

avatar csthomas
csthomas - comment - 28 Jun 2016

@pe7er @joomlara
Is the description of testing is still misunderstood?

avatar conconnl conconnl - test_item - 7 Jul 2016 - Tested successfully
avatar conconnl
conconnl - comment - 7 Jul 2016

I have tested this item successfully on 94f419e

Thank you.
The change is working as expected.
No more redirect status codes when using wrong passwords.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar hardiktailored hardiktailored - test_item - 9 Jul 2016 - Tested successfully
avatar hardiktailored
hardiktailored - comment - 9 Jul 2016

I have tested this item successfully on 94f419e


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar conconnl
conconnl - comment - 9 Jul 2016

@brianteeman RTC please


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar brianteeman
brianteeman - comment - 9 Jul 2016

Just wanted to point out that wp gives a 200 and there is a long standing
issue requesting it to be changed to a 303 which would help mod_security

So just because this pr works I'm personally not convinced yet that it is
the correct thing to do and would like to see more input from sysadmins

avatar conconnl
conconnl - comment - 9 Jul 2016

Then we need to wait on other SysAdmins and DevOps like me. 303 is wrong, 200 is better but 403 is really the correct one.

When you don't have a correct authentication and you don't redirect to a other page/address and therefore don't use any 301,302 or 303. It's a HTTP OK with wrong credentials, because there is nothing wrong with the request.

If you want to communicate it correctly and follow the HTTP status codes you need to use 403: Forbidden
See: https://en.m.wikipedia.org/wiki/List_of_HTTP_status_codes

In simple terms 200 is OK without redirect.
303 is See other (redirect)
403 is forbidden

avatar brianteeman
brianteeman - comment - 9 Jul 2016

For reference this was one of the first posts I found on this https://www.longren.io/wordpress-sending-correct-http-status-code-on-login-failure/ and see the links

avatar csthomas
csthomas - comment - 9 Jul 2016

I thought of it as regular form and validation.
When the form is validated with success then 303,
when the form has invalid field then 200.

avatar conconnl
conconnl - comment - 9 Jul 2016

He has written the same as my other post.

Use 403 which is the correct status code.

Yes, the other arguments would suggest you need to use 401 but this is the error code for 401 Authentication which send you a 401 Challenge which you need to respond with the challenge. This can be credentials, token codes, 2-Way SSL certificate and many more.

Because you already received this challenge when you opened the login page you want to receive the 403 when using wrong credentials.

I hope some other people can contribute on this, so we can make a decision.

@yireo @pe7er @roland-d please comment on this with your developer and Linux experience.

avatar chrisdavenport
chrisdavenport - comment - 9 Jul 2016

403 is the correct response. 401 is only used with the WWW-Authenticate challenge-response protocol (eg. "Basic authentication"), which is not what we're using here. See RFC7235.

avatar andrepereiradasilva
andrepereiradasilva - comment - 9 Jul 2016

+1 for 403

updated:

after readding the PR discussion better i think maybe 200 is ok.
This is a submit action that is submited and the page as returned ok even if the credencials are invalid so for me is a 200 Ok

IMHO 403 is more like you don't have access to that page, when you try a direct URL access to a restricted page, which is not the case.

avatar brianteeman
brianteeman - comment - 10 Jul 2016

This visual chart of all status codes is useful http://racksburg.com/choosing-an-http-status-code/


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar gunjanpatel gunjanpatel - change - 11 Jul 2016
Status New Pending
avatar gunjanpatel
gunjanpatel - comment - 11 Jul 2016

Setting to Pending as we have code.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar gunjanpatel gunjanpatel - change - 11 Jul 2016
Status Pending Ready to Commit
avatar gunjanpatel
gunjanpatel - comment - 11 Jul 2016

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar joomla-cms-bot joomla-cms-bot - change - 11 Jul 2016
Labels Added: ?
avatar gunjanpatel
gunjanpatel - comment - 11 Jul 2016

Setting RTC as there is 3 successful tests.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar brianteeman
brianteeman - comment - 11 Jul 2016

Sorry but I am removing the RTC setting as it is still not agreed that this is the correct thing to do. Setting to needs Review


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar brianteeman brianteeman - change - 11 Jul 2016
Status Ready to Commit Needs Review
Labels
avatar brianteeman brianteeman - change - 11 Jul 2016
Labels Removed: ?
avatar gunjanpatel
gunjanpatel - comment - 11 Jul 2016

Thanks and I apologies if I don't understand and did rtc.

avatar killoltailored killoltailored - test_item - 11 Jul 2016 - Tested successfully
avatar killoltailored
killoltailored - comment - 11 Jul 2016

I have tested this item successfully on 94f419e


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10890.

avatar csthomas
csthomas - comment - 27 Jul 2016

@wilsonge @roland-d can I ask you to take a look?

avatar roland-d
roland-d - comment - 29 Jul 2016

@csthomas I had a look at this the other day and been thinking what would be the right response. So the 200 response is not correct if you ask me because it doesn't say anything about the authorization attempt. It just tells us the page was loaded, well yeah we know that much. The 403 error @chrisdavenport mentioned is the correct one reading the RFC he mentions as the RFC states:

The 403 (Forbidden) status code indicates that the server understood
the request but refuses to authorize it.

So if you ask me, we should give a 403.

avatar csthomas
csthomas - comment - 29 Jul 2016

Discussion from wordpress:
https://core.trac.wordpress.org/ticket/25446

Does anyone send a non-200 for a failed web login? I can't say I've ever noticed this in practice.

Nope. Example Twitter, Facebook, GitHub, Google and BitBucket all returns 200. So I'll +1 for staying with 200

My thoughts about www:

  • If we send POST request (with incorrect credentials) then the request is not forbidden (it is processed) but the result not allow us to go to the next secure page, then we get the login page instead.

  • if we send POST request (with correct credentials) then the request is not forbidden (it is processed) but after authentication success we are redirected to secure page (but also may go to public page too).

  • if we send POST/GET request (example delete data/ get access to secure information) but we do not have rights then we should get 403.

BUT login form do not need to redirect to secure page only.

avatar wilsonge
wilsonge - comment - 29 Jul 2016

I agree. A 403 is supposed to be when you are trying to access secure info. Here we have invalid credentials but importantly the login form is NOT a restricted page. Therefore the correct response is a 200 even tho the login credentials are incorrect

avatar chrisdavenport
chrisdavenport - comment - 29 Jul 2016

How about returning a 403 with a Location header pointing to the login form? The browser will follow the redirect and the subsequent GET will return a 200.

avatar csthomas
csthomas - comment - 25 Aug 2016

@chrisdavenport Can you show us any website (global known) that return 403 code on the login fails?

Stay with current 303 code is the worse solution.

Can we merge that PR and then discuss about replacing 200 to 403 on a new issue?

avatar mbabker
mbabker - comment - 25 Aug 2016

Laracasts issues a 422 response

KNPUniversity (built on Symfony) issues a 302

drupal.org issues a 200

avatar csthomas
csthomas - comment - 25 Aug 2016

Laracasts issues a 422 response

I read http://www.restpatterns.org/HTTP_Status_Codes/422_-_Unprocessable_Entity

The 422 (Unprocessable Entity) status code means the server understands the content type of the request entity [...] but was unable to process the contained instructions.

IMHO 422 is incorrect for the login fails.

How about blocked user.
Login process is processed, login and password is OK but user are blocked.

KNPUniversity (built on Symfony) issues a 302

After POST request we should not use 302 redirection.
http://www.restpatterns.org/HTTP_Status_Codes/302_-_Found

avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Dec 2016

mantainers ... why is this not RTC. this as 4 success tests ...

avatar jeckodevelopment
jeckodevelopment - comment - 13 Dec 2016

As far as I can see, it's not about tests, but about different opinions on the error code that should be issued.

avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Dec 2016

The errors core is not changes in this pr. Look at code ...

avatar jeckodevelopment
jeckodevelopment - comment - 13 Dec 2016

@wilsonge please review this

avatar csthomas csthomas - change - 10 Jan 2017
Title
After backend login fails return status code 200 instead 303
If $app->login() failed and return false or return exception then react in the same way
avatar csthomas csthomas - edited - 10 Jan 2017
avatar joomla-cms-bot joomla-cms-bot - change - 10 Jan 2017
Category Administration Authentication Administration com_login Authentication
avatar wilsonge
wilsonge - comment - 23 Feb 2017

I'm merging this as @csthomas is correct we are already using a 200 - this just makes the behaviour consistent. But I'm also going to open an issue for us to think about what status codes we should be using here #14196

avatar wilsonge wilsonge - change - 23 Feb 2017
Status Needs Review Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-02-23 01:20:21
Closed_By wilsonge
avatar wilsonge wilsonge - close - 23 Feb 2017
avatar wilsonge wilsonge - merge - 23 Feb 2017

Add a Comment

Login with GitHub to post a comment