Make Invalid Token warnings more user friendly #8445

izharaazmi · 2015-11-16T10:25:21Z

The session token validation implementation in various controller methods currently gives an empty white screen with "Invalid token" text. This is total uninformative to a normal user.

This update implicitly redirects to the previous page if the token is not valid. To prevent implicit redirect pass false as the second argument to ~~JSession::validateToken()~~ $this->checkToken() from the JControllerLegacy controller instance.

To make use of this method we need to replace any occurrence of JSession::checkToken('post') or jexit(JText::_('JINVALID_TOKEN')); with ~~JSession::validateToken('post');~~ $this->checkToken();

Summary of Changes

Based on the discussion below I've removed the method from JSession class and put it in the closer context of its application viz. JControllerLegacy.
All the controllers should now be able to simply call $this->checkToken() and the redirection shall be handled implicitly. Implicit redirection is done to the referrer (calling) page always.
If a custom handling is required such as alternate redirection url or custom response format like JSON one may call $this->checkToken like:

if (!$this->checkToken('post', false))
{
     // ...
}

or even directly call the current JSession::checkToken (without the die part) like:

if (!JSession::checkToken('post'))
{
     // ...
}

This will skip the implicit redirection and return false if the token does not match. Then the necessary if - else may be implemented as needed.

Test Instruction

To see the effect the respective forms should be submitted after manipulating the token input in the form so as to trigger invalid token situation. This can usually be done by selecting in the browser "inspect element" (or similar) and modify the DOM.

If the token is valid (not manipulated or expired), everything should be working as usual without any difference.

The necessary changes are made in the front-end which should be tested:

In com_users component try to register a new user, login, logout, remind username, reset password, save user profile.
In com_contact submit the contact form to send an email.
In com_content submit a vote.

EDIT June 26, 2016:
Added Summary of changes and Test Instructions based on the new changes.

izharaazmi · 2015-11-16T10:29:12Z

I was making changes in the checkToken method itself. But made this a separate method for clarity.

The other similar discussion on this topic is ongoing in the PR #8416.

izharaazmi · 2015-11-16T10:46:17Z

Another idea is to allow passing the second argument to JSession::validateToken($method, $redirect) as:

Value of `$redirect`	Behaviour
`true`	Redirect to the referrer page (default value)
`false`	No redirect, just return false if token is invalid
Custom URL String	redirect to this specific url if token is invalid

izharaazmi · 2015-12-04T16:06:51Z

I was recently thinking about this method getting influenced by

... it is the wrong way and against design principles: JSession is a specialised class for sessionhandling and not about redirection handling ... #8416 (comment)

I see most of these checks are done in controllers and all (almost) are inherited from JControllerLegacy. Would that be an appropriate location to move this method validateToken, however I like the name checkToken more.
So that we can do the check like:

$this->checkToken('post', true); // checkToken($method, $redirect)

Then gradually update the core components to use this test by test.

peterpeter · 2015-12-19T13:55:54Z

Better.

But you don't have to move your new method, just use the allready existing ones from JControllerLegacy instead:
You just have to change

JSession::checkToken('post') or jexit(JText::_('JINVALID_TOKEN'));

to

JSession::checkToken('post') or $this->setRedirect($url, JText::_('JINVALID_TOKEN'), 'error')->redirect();

with the right url and your improved message.

Your solution is still a code- (and thus test-) duplication.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8445.}

izharaazmi · 2015-12-19T14:15:24Z

@peterpeter I almost agree. But to me the code does not appear to be any duplication. It internally uses the already existing method. The extra method introduced is just to provide the ability to auto calculate the REFERER page url for redirection. If that is not needed then of course we can follow what you suggested. But I think it could be useful.

henkrijneveld · 2016-04-15T12:47:46Z

I agree this is a real problem, but in current state untestable (we do not have a solution). Please elaborate, because it is a good idea to solve this.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8445.}

roland-d · 2016-06-24T21:10:04Z

Hello @izharaazmi

Thank you for your contribution.

The last comment here was on April 15th. Can you update this PR so it is testable?
If no reply is received within 4 weeks we will close this issue.

Thanks for understanding!

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8445.}

…white screen with "Invalid token" text. This is total uninformative to a normal user. This update implicitly redirects to the previous page if the token is not valid. To prevent implicit redirect pass `false` as the second argument to `JSession::validateToken()` To make use of this method we need to replace any occurrence of `JSession::checkToken('post') or jexit(JText::_('JINVALID_TOKEN'));` with `JSession::validateToken('post');` Example is set up in com_users > login method.

…oser context to the usage. This allows implicit redirect to the REFERER page in case of token mismatch. In all other case it is same as calling JSession::checkToken() directly. Changes made in the frontend components to make this change test-able. Note: We intend to discourage the `die` usage everywhere which leads the the said uninformative white page.

izharaazmi · 2016-06-26T09:41:03Z

I have made the necessary changes in the front-end components. Please see #8445 (comment) for updated notes.

I hope it is now testable for the front-end. I'll preferably do backend implementation as a separate PR if this is approved.

hardiktailored · 2016-07-12T12:19:41Z

I have tested this item 🔴 unsuccessfully on 4b630b0

I found issue, after applying patch and I modified token of login form presented on sidebar. It redirects to blank screen, Network tab shows "500 Internal Server Error". Before applying patch "The most recent request was denied because it contained an invalid security token. Please refresh the page and try again." message was appearing on screen.

For the other inner pages warning message "The security token did not match. The request was aborted to prevent any security breach. Please try again." appears after applying the patch.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8445.}

hardiktailored · 2016-07-12T12:24:29Z

Check the warning message after patch applied(All pages except login form problem mentioned above):

Before applying patch (for all screens):

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8445.}

izharaazmi · 2016-08-23T06:47:04Z

@hardiktailored I tested it again and it worked fine. I may be missing something... can you please explain the issue a little more?

AnishaTailored · 2016-09-05T05:41:03Z

I have tested this item 🔴 unsuccessfully on 4b630b0

I have also found the issue after applying this patch (using the patchtester component) same as @hardiktailored, when I login (after modifying the token from DOM), I get the following error "
Fatal error: Call to undefined method JSession::validateToken() in components/com_users/controllers/user.php on line 30".

Before applying the patch login function has JSession::checkToken('post') or jexit(JText::_('JINVALID_TOKEN')); instead of JSession::validateToken('post');
I have checked for other forms (registration, reset password, remind username, edit profile etc.), they are working fine.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8445.}

izharaazmi · 2016-09-05T09:01:07Z

@AnishaTailored Thanks for explaining the issue in detail. I found that in my previous commit I had missed changing one occurrence of validateToken that was causing failures.

I've fixed it now, please test this again.

AnishaTailored · 2016-09-05T09:16:02Z

I have tested this item ✅ successfully on 39a5a50

Now it works fine.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8445.}

izharaazmi · 2016-09-05T10:07:06Z

@hardiktailored Can you test this again please.

andrepereiradasilva · 2016-09-05T10:09:28Z

libraries/legacy/controller/legacy.php

+	 *
+	 * @return  boolean  True if found and valid, otherwise return false or redirect to referrer page.
+	 *
+	 * @since   3.6.0


please use __DEPLOY_VERSION__

Thanks. Done.

@andrepereiradasilva

Thanks @andrepereiradasilva

Devportobello · 2016-09-22T15:41:27Z

components/com_users/controllers/user.php

@@ -440,6 +440,6 @@ public function remind()
 	public function resend()
 	{
 		// Check for request forgeries
-		JSession::checkToken('post') or jexit(JText::_('JINVALID_TOKEN'));
+		// $this->checkToken('post');


I did that because I don't see any code in that function. I don't understand why that method is kept there.

hvdmeer · 2016-11-04T13:38:39Z

I have tested this item ✅ successfully on b05765d

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/8445.}
I was missing some information about what exactly DOM is but my neighbor at the PBFNL was so kind to explain it to me. Thus resulting in a succesful test.

renekreijveld · 2016-11-04T14:06:08Z

I have tested this item ✅ successfully on b05765d

Tested with modification of the token through Firebug. After patch installation better message in a alert.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/8445.}

zero-24 · 2016-11-04T21:18:34Z

Please resolve the merge conflicts.

@rdeutz or @wilsonge please review and take a final decision here.

izharaazmi · 2016-11-05T07:17:19Z

I was resolving the conflict, then I saw the missing register method the controller. Little investigation led me to bae1d43#diff-f820ef502f338b5be5a00aac838e756fL287. I could not find the exact PR which actually made this change. On 3.7.x branch this method still exists. Somebody guide me at this please!

zero-24 · 2016-11-05T07:20:24Z

This is removed in 3.6.4 to avoid registations over the wrong way. 3.7.0-dev branch is not updated with this change as staging is now 3.7.-dev

izharaazmi · 2016-11-05T07:22:33Z

Won't it break b/c for components redirecting/submitting to that task for registration?

zero-24 · 2016-11-05T07:27:59Z

Yes. But it was the decision of the JSST to remove that method as it is not used anymore and was never correctly implemented.

…/staging into checktoken

izharaazmi · 2016-11-05T07:42:02Z

@zero-24 Thanks for clarification. I have updated my branch to latest staging hence resolved the conflicts.

izharaazmi · 2016-11-05T10:15:44Z

Thanks everybody for making this to this point 👍

I think this should followed up by some documentation update. Please advise where can I do that.

andrepereiradasilva · 2016-11-05T11:05:58Z

@izharaazmi can you do the same for all other token dies (where applies) so we have a consistent core?

There a lot of JSession::checkToken('xxxxx') or die(JText::_('JINVALID_TOKEN'));

egrep -R "JSession::checkToken" /path/to/joomla-staging/ | awk '{print $1}'

izharaazmi · 2016-11-05T11:17:51Z

@andrepereiradasilva Sure, hope to see it this Monday! Btw, I'm having 152 occurrences. Please do let me know if you find more.

izharaazmi · 2016-11-07T06:21:26Z

@andrepereiradasilva I have made the required changes. Please test the other PR at your convenience.

@andrepereiradasilva

* The session token validation implementation currently gives an empty white screen with "Invalid token" text. This is total uninformative to a normal user. This update implicitly redirects to the previous page if the token is not valid. To prevent implicit redirect pass `false` as the second argument to `JSession::validateToken()` To make use of this method we need to replace any occurrence of `JSession::checkToken('post') or jexit(JText::_('JINVALID_TOKEN'));` with `JSession::validateToken('post');` Example is set up in com_users > login method. * Moved the checkToken method to JControllerLegacy as it is the more closer context to the usage. This allows implicit redirect to the REFERER page in case of token mismatch. In all other case it is same as calling JSession::checkToken() directly. Changes made in the frontend components to make this change test-able. Note: We intend to discourage the `die` usage everywhere which leads the the said uninformative white page. * Fix wrong function call * use automatic since tag macro Thanks @andrepereiradasilva

joomla-cms-bot added Language Change This is for Translators PR-staging labels Nov 16, 2015

izharaazmi mentioned this pull request Nov 16, 2015

Improving invalid token error #8416

Closed

izharaazmi added 2 commits June 26, 2016 14:47

izharaazmi force-pushed the checktoken branch from 5b736d4 to 4b630b0 Compare June 26, 2016 09:28

Fix wrong function call

39a5a50

andrepereiradasilva reviewed Sep 5, 2016
View reviewed changes

use automatic since tag macro

b05765d

Thanks @andrepereiradasilva

Devportobello suggested changes Sep 22, 2016

View reviewed changes

Merge commit '49684115e94144613fb742043b29f714e3af0d84' of joomla-cms…

37a95b2

…/staging into checktoken

joomla-cms-bot removed the Language Change This is for Translators label Nov 5, 2016

rdeutz added this to the Joomla 3.7.0 milestone Nov 5, 2016

rdeutz merged commit a17c0d7 into joomla:staging Nov 5, 2016

joomla-cms-bot added the Language Change This is for Translators label Nov 5, 2016

izharaazmi deleted the checktoken branch November 5, 2016 10:15

This comment was marked as abuse.

Sign in to view

izharaazmi mentioned this pull request Nov 7, 2016

Updating checkToken usage to utilise the new JControllerLegacy::checkToken #12801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Invalid Token warnings more user friendly #8445

Make Invalid Token warnings more user friendly #8445

izharaazmi commented Nov 16, 2015 •

edited by brianteeman

Loading

izharaazmi commented Nov 16, 2015

izharaazmi commented Nov 16, 2015

izharaazmi commented Dec 4, 2015

peterpeter commented Dec 19, 2015

izharaazmi commented Dec 19, 2015

henkrijneveld commented Apr 15, 2016

roland-d commented Jun 24, 2016

izharaazmi commented Jun 26, 2016

hardiktailored commented Jul 12, 2016

hardiktailored commented Jul 12, 2016

izharaazmi commented Aug 23, 2016

AnishaTailored commented Sep 5, 2016

izharaazmi commented Sep 5, 2016

AnishaTailored commented Sep 5, 2016

izharaazmi commented Sep 5, 2016

andrepereiradasilva Sep 5, 2016

izharaazmi Sep 5, 2016

Devportobello Sep 22, 2016

izharaazmi Sep 29, 2016

hvdmeer commented Nov 4, 2016 •

edited

Loading

renekreijveld commented Nov 4, 2016

zero-24 commented Nov 4, 2016

izharaazmi commented Nov 5, 2016

zero-24 commented Nov 5, 2016

izharaazmi commented Nov 5, 2016

zero-24 commented Nov 5, 2016

izharaazmi commented Nov 5, 2016

izharaazmi commented Nov 5, 2016

andrepereiradasilva commented Nov 5, 2016 •

edited

Loading

izharaazmi commented Nov 5, 2016

This comment was marked as abuse.

izharaazmi commented Nov 7, 2016

Make Invalid Token warnings more user friendly #8445

Make Invalid Token warnings more user friendly #8445

Conversation

izharaazmi commented Nov 16, 2015 • edited by brianteeman Loading

Summary of Changes

Test Instruction

izharaazmi commented Nov 16, 2015

izharaazmi commented Nov 16, 2015

izharaazmi commented Dec 4, 2015

peterpeter commented Dec 19, 2015

izharaazmi commented Dec 19, 2015

henkrijneveld commented Apr 15, 2016

roland-d commented Jun 24, 2016

izharaazmi commented Jun 26, 2016

hardiktailored commented Jul 12, 2016

hardiktailored commented Jul 12, 2016

izharaazmi commented Aug 23, 2016

AnishaTailored commented Sep 5, 2016

izharaazmi commented Sep 5, 2016

AnishaTailored commented Sep 5, 2016

izharaazmi commented Sep 5, 2016

andrepereiradasilva Sep 5, 2016

Choose a reason for hiding this comment

izharaazmi Sep 5, 2016

Choose a reason for hiding this comment

Devportobello Sep 22, 2016

Choose a reason for hiding this comment

izharaazmi Sep 29, 2016

Choose a reason for hiding this comment

hvdmeer commented Nov 4, 2016 • edited Loading

renekreijveld commented Nov 4, 2016

zero-24 commented Nov 4, 2016

izharaazmi commented Nov 5, 2016

zero-24 commented Nov 5, 2016

izharaazmi commented Nov 5, 2016

zero-24 commented Nov 5, 2016

izharaazmi commented Nov 5, 2016

izharaazmi commented Nov 5, 2016

andrepereiradasilva commented Nov 5, 2016 • edited Loading

izharaazmi commented Nov 5, 2016

This comment was marked as abuse.

izharaazmi commented Nov 7, 2016

izharaazmi commented Nov 16, 2015 •

edited by brianteeman

Loading

hvdmeer commented Nov 4, 2016 •

edited

Loading

andrepereiradasilva commented Nov 5, 2016 •

edited

Loading