Invalid request token #3214

Closed
Ruudt opened this Issue Nov 29, 2011 · 66 comments

Comments

Projects
None yet
6 participants

Ruudt commented Nov 29, 2011

2.10 RC1. Sometimes when I am going back through my browser history (pressing back button) I land on a page with a form I submitted earlier. That form contained an error message to the user, like "field A is mandatory". I get asked if I want to resubmit the data by my browser. When I choose yes I get "Invalid request token! The request token could not be verified. Please go back and try again ..." and so on. I cannot circumvent this behavior, therefor I suspect Contao must be wrong. I don't think visitors should at any point get exposed to error messages like these when they don't need to be (this worked fine with the previous system).

Is this similar to http://dev.contao.org/issues/3207? I do not close any sessions.

--- Originally created on June 28th, 2011, at 07:35pm (ID 3214)

leofeyer was assigned Nov 29, 2011

Ruudt commented Nov 29, 2011

ps, I noticed the be_referer template (it refers to a deprecated extension .tpl) but that can't seriously be a "good solution". I need to manually put in all html code. If I just forward it to a custom Contao page I have no idea where I came from. In any case Contao needs to figure out how to resubmit the data if the user specifically requested it.

--- Originally created on June 28th, 2011, at 07:43pm

Owner

leofeyer commented Nov 29, 2011

This is exactly one of the things the token system is for: prevent the same form from accidentally being sent multiple times.

--- Originally created on June 29th, 2011, at 10:18am

Ruudt commented Nov 29, 2011

I was actually not complaining about the functionality. I was saying that the presentation is wrong. It either costs me much more effort to make the message look acceptable or the user gets confronted with the yellow box. My visitors should never ever have to deal with yellow boxes if a logical problem occurs that is not an error per se. And whatever way I make it look using the be_referer template; I still call it a yellow box.

And also, like I said; people might want to resend the form, the question is asked by the browser in some cases. How does that work with what the token is trying to prevent if it was specifically requested to be sent again?

Finally: I just went back and forth and tried to intentionally send a form that had an error, I fixed the error (so the form was never submitted without errors) and pressed send --> Errormessage about the tokens. My vistors will get aggravated if they went back and filled out an entire form they then cannot send!

Please reconsider...

ps, the message is wrong in any case: templates/be_referer.TPL

--- Originally created on June 29th, 2011, at 10:36am

Member

aschempp commented Nov 29, 2011

I also dislike these "yellow boxes" a lot. What if the form would just output a generic error message (with the same content)? And then allow for resubmitting, preserving the field values but generating a new token?
The only problem I see is that Contao currently does not support "generic" error messages on a form, only on each field. But that problem should be solvable ;-)

--- Originally created on June 29th, 2011, at 01:13pm

Ruudt commented Nov 29, 2011

I'm getting these boxes very often now that I use 2.10 rc1 more frequently. It is actually getting a bit frustrating, but it is still only a release candidate.

  • I was working on testing some login/logout behavior. But when I open the backend I get a login prompt, then I open a frontend tab to login as member and logout as well, switching back to the backend tab I cannot log in with a perfectly fine login form. What would be the reason to say the backend form is invalid?
  • Then, when in the frontend I'm logged in and press logout, and I go back and press logout again I get the yellow box. I checked the behavior with previous versions, and they allow me to submit, redirect me to the page I need to and do not complain. (Although I did have to use two tabs to even get the logout button after logging out)
  • In the ER yesterday evening I was not able to release a new version of an extension I made; request token invalid.

Well, you get the idea; there are many new errors at the moment with the new request tokens. Regardless of what the system was designed for; it ruins user experience every time they see a big yellow "NO CAN'T DO". I hope I do not have to disable this feature, because it is important the user sees or hears no evil (unless it really really really can't be helped like input validation messages: "r#$R#$R is not a valid e-mail address")

--- Originally created on July 16th, 2011, at 10:20am

Member

aschempp commented Nov 29, 2011

The reason for these issues is that the active session is destroyed on logout. Therefore, all existing tokens get invalid. Can't think of a simple solution right now, but I also dont know why the session is being destroyed.

--- Originally created on July 17th, 2011, at 07:30pm

Owner

leofeyer commented Nov 29, 2011

but I also dont know why the session is being destroyed

Mainly for security reasons. It is a bit breaking a fly on the wheel, but I tend to rather having too much security than too little. We can discuss it though …

--- Originally created on July 27th, 2011, at 05:32pm

Member

aschempp commented Nov 29, 2011

Correct, but all user relevant information should be stored in BE_DATA and FE_DATA. Everything outside that scope is not Contao Core data and therefore not your responsibility...
I suggest just to empty these 2 and keep the session.

--- Originally created on July 27th, 2011, at 05:35pm

Owner

leofeyer commented Nov 29, 2011

Nope, not really. The data in BE_DATA and FE_DATA is the session data that is stored in the user/member table. There are quite a few other session variables in the global scope, too.

--- Originally created on July 27th, 2011, at 06:52pm

Member

aschempp commented Nov 29, 2011

Can you give an example?
I'm aware that everyone can store in the global session, but if it is frontend-member data, I wouldn't store it outside FE_DATA.

--- Originally created on July 28th, 2011, at 08:09am

Owner

leofeyer commented Nov 29, 2011

** $_SESSION['TL_INSTALL_**']

  • $_SESSION['TL_ERROR']
  • $_SESSION['TL_INFO']
  • $_SESSION['TL_CONFIRM']
  • $_SESSION['TL_USER_LOGGED_IN']
  • $_SESSION['TL_COMMENT_ADDED']
  • $_SESSION['TL_EMAIL_SENT']
  • $_SESSION['TL_LANGUAGE']

--- Originally created on July 28th, 2011, at 05:12pm

Owner

leofeyer commented Nov 29, 2011

Granted, none of these values has to be preserved if the session ends. But they always say "generate a new session ID if a user logges out". It is a measure of securing an application.

--- Originally created on July 28th, 2011, at 05:13pm

Member

aschempp commented Nov 29, 2011

Then the only thing I can think of is "cache" the tokens and write to the new session?

--- Originally created on July 28th, 2011, at 06:45pm

Ruudt commented Nov 29, 2011

Can you add the tokens to the database? Clear them after they've been used or after say 24 hours.

--- Originally created on July 29th, 2011, at 10:43am

Collaborator

issue-bot commented Nov 29, 2011

I'd like to bump this up as well. Since rolling out a couple sites on 2.10 stable it is an issue especially when doing simple things like submitting a search form, clicking back (as some users tend to do), and submitting again. I know that blocking multiple form submissions is a good thing and that the issue of login/logout session is a separate one, but to me the yellow default screen is a UI flaw as it should not bring down the entire frontend output to implement this feature. A default error messaging system would definitely be preferable to the yellow screen in this case.

--- Originally created by winanscreative on August 25th, 2011, at 03:59pm

Member

Toflar commented Nov 29, 2011

What about a new page type especially for this hint? According to the 404 or 403 pages?

--- Originally created on August 25th, 2011, at 04:11pm

Collaborator

issue-bot commented Nov 29, 2011

I also am experiencing the same issues and definitely, respectfully ask that this be addressed. :) Thanks Leo.

--- Originally created by fbliss on August 25th, 2011, at 05:25pm

Owner

leofeyer commented Nov 29, 2011

If you hit the back button of your browser, you are not sending a new request to the server, hence you are not receiving a new request token. Then when you try to resubmit a form, of course, the error page will be triggered. Request tokens are supposed to do exactly this: Prevent forms from being submitted multiple times and prevent unauthorized POST requests in general. I don't know what could be "fixed" here.

--- Originally created on August 25th, 2011, at 05:53pm

Member

Toflar commented Nov 29, 2011

The behaviour is perfectly correct, but the yellow box should be customizable so we can implement it into the website's template and the users don't feel lost =) That's why I proposed the idea of a new page type (or we could define a jumpTo page in the root page which would be easier to implement I suppose).

--- Originally created on August 25th, 2011, at 06:16pm

Ruudt commented Nov 29, 2011

Leo, you are viewing this from a theoretical and technical point of view. I let my mother try sending a form twice (not a search form, because that is a GET form) and I can tell you that she did not understand, but pressed the back button (something the text seems to suggest!!) +but that doesn't work+ because of the token not being re-issued.

Also, you are not getting the desired result if developers choose to disable tokens because of the error message screen. I've seen several people stating they are doing that for the time being. So that means the implementation fails in its purpose if people feel forced to disable it altogether.

Toflar, the screen is customizable. You can change the template that is displayed. But that is not a solution; pressing back doesn't work and the template is an end page, if it was generated alike you said it would be better already, although I'm not convinced this is clear to users.

--- Originally created on August 25th, 2011, at 06:33pm

Collaborator

issue-bot commented Nov 29, 2011

The problem of "yellow Box" will then still apply as the be_error template is nothing else than a "hardcoded seperate page type".
IMO the Form should rather display itself again with a generic error "invalid token, did you use your browser's back button?" or something like that that actually tells the user what the problem was and how to prevent it in future without cutting the throat of the Frontend.

The drawback on this part is, that every form in the system (form extension, catalog, comments, ... you name it, everything that does some POST) will have to check via some Input class.
This could be prevented eventually by implementing something like $this->Input->validSubmit($strFormId) which will then check "FORM_SUBMIT" and the token.
That way we could provide a token framework rather than a bullet-in-your-face-token system. This of course is not planned to full extent yet and therefore not ready for implementation by long means. It is merely a design proposal put here for discussion.
Every implementation method has it's drawbacks.

--- Originally created by xtra on August 25th, 2011, at 06:43pm

Owner

leofeyer commented Nov 29, 2011

I know about the back button problem, but there is no way to solve this - at least I don't know. You cannot modify the browser history via JavaScript, that is why we have put a "go back" link in the error message. If you click this link, the problem does not occur.

I will see if we can add a custom page type to catch the error, however, you seem to forget that this is a security feature and not a usability feature. Security and usability are conflicting objectives, so you cannot have a maximum of both! If usability is more important to you than security, you can disable the request token system and live with a less secure installation. On the other hand, if security is more important to you than usability, you can leave it the way it is and live with a less usable installation.

--- Originally created on August 26th, 2011, at 12:38pm

Ruudt commented Nov 29, 2011

Would there be major issues against a general error message displayed on the form upon (token) errors? This way the token security feature stays intact and users can submit the form again if they want to with the newly generated token.

--- Originally created on August 26th, 2011, at 06:37pm

Member

aschempp commented Nov 29, 2011

How about fetching a new request token when the page is loaded, for example using the already-triggered cron ajax call? This would issue a new token even when using the back button.

--- Originally created on August 26th, 2011, at 10:48pm

Ruudt commented Nov 29, 2011

Doesn't that defeat the purpose of the tokens? Any form could then be resubmitted without problems (I like that though), the only thing which wouldn't work is hitting submit a second time before the first time completed; that would generate a request token error.

I'm ok with the security feature that does not allow to submit a form twice. I don't understand why this conflicts with the usability feature to show the message on the form itself, like Andreas suggested. If all field values are maintained (and Contao does that upon widget errors already) then the visitor simply submits the newly created form.

That solution is still not ideal from a usability point of view. And no other cms that I've tried blocks users from submitting forms. But then it would be really easy for the user to decide to resend if for any reason they still feel they need to. The message could be customized if tokens gain a state (success, errors, open), then it could read:

  • success: "You've submitted twice, are you sure? The previous attempt was successfully processed"
  • errors: "You've submitted this form a second time now, are you sure you want to do this?"
  • open: *no error, so form can submit

Is there a problem with that approach I'm not aware of?

--- Originally created on August 27th, 2011, at 09:58am

Member

aschempp commented Nov 29, 2011

I don't think generating a new token is an issue. Imho the tokens are not required to prevent a form from being submitted twice, they are a security feature (like the previous) referer check which disallows non-visitors from sending post data to the server.

Very interesting article: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet

--- Originally created on August 27th, 2011, at 11:05am

Member

aschempp commented Nov 29, 2011

Also interesting on this article:

In general, developers need only generate this token once for the current session. After initial generation of this token, the value is stored in the session and is utilized for each subsequent request until the session expires.

--- Originally created on August 27th, 2011, at 11:09am

Collaborator

issue-bot commented Nov 29, 2011

God save Andreas ;-) That's the solution: Keep same Token until session expires.

--- Originally created by Georg on August 27th, 2011, at 01:51pm

Member

aschempp commented Nov 29, 2011

After reading the full article, I can come up with several ideas:

  • Keep the current one-time token for the backend. More secure and you should not use the back button or use multiple tabs here.
  • Implement one-token-per-session for regular forms in the frontend (e.g. search field)
  • Optionally add one-time token for important forms (like frontend login, personal data)

There was also some information on storing the session-token in a cookie in addition to the form submit. The server can validate that the token matches from both sources, that might be even more secure (but does only make sense for session-tokens).

--- Originally created on August 27th, 2011, at 02:08pm

Owner

leofeyer commented Nov 29, 2011

Using the same token throughout the session is not a good idea (see http://securethoughts.com/2009/07/hacking-csrf-tokens-using-css-history-hack/).

--- Originally created on August 29th, 2011, at 12:04pm

Member

aschempp commented Nov 29, 2011

For making this attack unfeasible,

Make your CSRF tokens long enough (8 or more chars) to be unfeasible for a CLIENT SIDE attack. The ever-increasing processing power will make this attack feasible for longer tokens as well.

Store your CSRF token as part of hidden form field, rather than putting in url.

Use a different random token for every form submission and not accept any obsolete token, even for the same session.

We already implement 2 of the 3 solutions, so the mentioned problem is not one for us.

--- Originally created on August 29th, 2011, at 12:14pm

Owner

leofeyer commented Nov 29, 2011

Of course it is! No. 1 and 2 are just prerequisites. We should put our focus on making the existing solution more usable (e.g. by using a special page type instead the yellow box) instead of thinking about how to weaken the CSRF protection.

--- Originally created on August 29th, 2011, at 12:17pm

Member

Toflar commented Nov 29, 2011

I think there really is no problem with the existing solution as long as the yellow box gets more user-friendly and the form data gets temporarily stored somewhere so the user doesn't crack up because the whole message is lost ;)

--- Originally created on August 29th, 2011, at 12:26pm

Collaborator

issue-bot commented Nov 29, 2011

Yes - at the time beeing one-time-token seems to be the only solution in the case of browser history scanning.
But nevertheless You do not need a one-time-token in all cases.

Please remember the scenario of a CSRF attack:

  1. Right at the time of the attack a user must have a valid session with a higher level of permissions in the target system
  2. The attacker must convince this user to open a prepared html package (or a link to a special site) in the browser mentioned in point 1
  3. This package must be especially designed for the target system:
    a browser history search script must know how to find the special credentials (e.g. contao token) of the target system and then produce on the fly in the browser of the abused user a correct post data messages, that will be accepted by the target system (and e.g. introduce the attacker as a super Admin into the target system).

That is a very sophisticated and highly specialized attack. But of course Contao has to cope with this threat.

I think the last proposal of Andreas hit the threat:
- one-time-token only for the front/backend users after login

Please keep an eye on the precondition for a reasonable attack:
a valid session with a higher level of permissions

This will lead to the point:
if Ruuds mother as a public user is surfing my contao portal, she has no permissions that can be missused!
And if she wants to send me a message via the contact formular, than it is a pity when this will end in a yellow box only because she pressed the browser back button for adjusting her message.

For public users I can see no need for a one-time-token - but a lot of disadvantages.

--- Originally created by Georg on August 29th, 2011, at 07:16pm

You cannot modify the browser history via JavaScript, that is why we have put a "go back" link in the error message.

`Leo Feyer: That's not 100% true. Exactly for this problem you have the history API witch allows you to edit the history. https://developer.mozilla.org/en/DOM/Manipulating_the_browser_history and http://it-republik.de/php/news/HTML5-History-API-059986.html.

`Tokens: I think this tokens only work in a lab environment, but not in the real world (internet). You have chashing browsers, error message, diffenrent tabs, ... and a lot of other stuff witch interfere. I have looked to big sites how they deal with this and there are some differences. For example Github.com.

They store the csrf_id in a coockie so you don't have do patch all 3rd party javascripts with use ajax because the coockie is always send to the server.
The other pint is, that github stores the coockie for the session. So you can use the same token for the session and don't have the trouble with resending a formular twice. But sometimes there are change the tokens during the session. They do that ons static sites where you have no need for them. So they have more security without anoy the user.

Maybe we can implement a solution like this to the next release.

--- Originally created on August 30th, 2011, at 04:02am

sorry for the horrable spelling above, but it's veeeeery late and i am very fucking tired :(

--- Originally created on August 30th, 2011, at 04:04am

Member

aschempp commented Nov 29, 2011

As far as I know, Javascript is not to be considered when talking about CSRF attacks. That would be XSS, Javascript can only access the page it currently runs on (keyword "cross domain ajax"). That is a totally different situation and tokens cannot prevent that. Any script can poll the server and request a new token, so there would be no protection.

Afaik, the browser history is only relevant if the token would be in the URL, which is not the case in our situation. No problem here, too.

--- Originally created on August 30th, 2011, at 08:49am

Owner

leofeyer commented Nov 29, 2011

Exactly for this problem you have the history API witch allows you to edit the history

A feature that only works in Firefox, Safari and Opera is not a solution! IE still exists (unfortunately).

I think this tokens only work in a lab environment, but not in the real world (internet).

If this was the case, why does EVERY framework provide the functionality? The tokens exist for a reason and they do work in the real world. The particular problem seems to be the way Contao handles unauthorized requests and not the token mechanism itself.

For example Github.com

Github.com is a bad example, because their implementation is not secure. See symfony/symfony#1880

--- Originally created on August 30th, 2011, at 11:27am

Member

aschempp commented Nov 29, 2011

@leo: did you read the comments on the ticket? Exactly the same as we discuss here, except that in the opposite situation (client asks for more security, developer prefers user experience).

--- Originally created on August 30th, 2011, at 11:50am

Owner

leofeyer commented Nov 29, 2011

Sure I have read the comments. That was exactly the point :)

--- Originally created on August 30th, 2011, at 11:55am

Collaborator

issue-bot commented Nov 29, 2011

@andreas:
1.) Do You still recommend one-time-token for backend?
As far as I understood if the token is not visible in the URL, there is no known attack scenario - correct?
So Your general recommendation for the one-token-per-session as a standard seems to be secure also for the backend use.

2.) Why do You recommend one-time-token for a Login form?
The user has to enter his password in the login form.
So this is the best token available for verification that the user is sending this formular.

--- Originally created by Georg on August 30th, 2011, at 12:05pm

@leo: okay, you don't want to change the regeneration of the tokens, but whats with my idea to store the token in a cookie to send them automaticly on every request? This would make it a lot easier to modify external scripts based on mootools.

--- Originally created on August 30th, 2011, at 03:12pm

Member

aschempp commented Nov 29, 2011

leo.unglaub: I don't think that does make sense. You can't have multiple tokens in a cookie, and writing the cooke for every request (or several times when using ajax) sounds like a major drawback. Imho you should use GET-requests for that situation, and take a look at my ajax.php script how to bypass token generation ;-)

--- Originally created on August 30th, 2011, at 03:17pm

Owner

leofeyer commented Nov 29, 2011

If you look at the routine, you will see that there are up to 25 tokens at one time to support using multiple tabs. We surely don't want to set 25 cookies, do we? Also, it is more difficult to read the token if it is stored in the session only.

--- Originally created on August 30th, 2011, at 03:25pm

Collaborator

issue-bot commented Nov 29, 2011

And the token should protect against a CSRF attack, where the cookies are the week point.
http://en.wikipedia.org/wiki/Cross-site_request_forgery

--- Originally created by Georg on August 30th, 2011, at 04:03pm

if the php session is stored in a cookie as well, it doesn't matter, because with the session you can get your own tokens

--- Originally created on August 30th, 2011, at 05:09pm

Member

aschempp commented Nov 29, 2011

That is not entirely true, as the session (in Contao) is also bound to your IP address. Storing the token in a cookie is definitely not the solution, because cookies are automatically submitted by the browser when performing a request to a site. "Getting your own token" is also not the point because you can't do that without javascript, and javascript can't perform cross-domain requests. Therefore it would not be the browser performing the request, the cookie would not be available and that means a different session.

Here's an example of a CRFS attack: A website you visit contains an (invisible) form that asks your browser to perform a POST request to the attacked server. Your browser will happily perform that, but Contao will not accept the post request because there is no (valid) token. If the form on the malicious website would contain a (valid) token, the protection would not work. However, the malicious website cannot know the token, because it can't read your token from another website (using javascript. again, cross-domain policy). To me, there is no difference in a session or one-time token, because a malicious website can't know it.

--- Originally created on August 30th, 2011, at 05:28pm

Collaborator

issue-bot commented Nov 29, 2011

That is now also my understanding: one-token-per-session will fit the threat.

But some notes before You recommend a token also for public users.
Is that still Your point of view?

Typical public user forms:
1.) Login
2.) contact form
3.) subsriber forms

My Point of view:

  1. the password authenticate already the sender
  2. and 3.: there exists already an optional "token": the answer of the user to the optional security question

So the admin of a portal can make the decision for himself whether he needs protection against automated spam attacks.

--- Originally created by Georg on August 30th, 2011, at 06:40pm

Member

aschempp commented Nov 29, 2011

By examples were all totally wrong, but it is not a protection against spam attacks. If you are logged in to the website and navigate somewhere else without logging out (or using auto login feature), someone can change your personal data and everything you can do as a member.

In theory, there is no need for request tokens as long as no member is logged in...

--- Originally created on August 30th, 2011, at 07:43pm

Collaborator

issue-bot commented Nov 29, 2011

@andreas:
I don't understand Your last note.

Your recommendation is a one-token-per-session.
Agreed.

When a user entered a non public zone (front end after login or back end) of the contao portal, the one-token-per-session has to be stored in the session data of the user.
So Contao will accept only post messages with the valid token of this user session until the user session timed out or the user logged out.
During this period nobody can change his personal data nor do anything you can do as a member without the valid session-token of this user.
Even when the user had left the portal already.

But my focus is on the public user who is only surfing in the public area of the portal.
They don't have permissions on a higher level as a attacker that can be misused and have to be protected with a token.

I am afraid of hassle due to old, not valid tokens in the browser cache when the contao browser cache modus is enabled for the public pages.

--- Originally created by Georg on August 30th, 2011, at 11:45pm

Collaborator

issue-bot commented Nov 29, 2011

After a long discussion - may I ask for the current common understanding of this issue?

My understanding:
a.) we all hate the token: developers, admins and users
b.) furthermore we all hate the token error page with the yellow box
c.) but the token seems to be the only solution against CSRF attacks

Current solutions:
1.) Leo offered to think about improving the way of informing the user when the token error appears
2.) changing the current token-per-request to a token-per-session system will solve most of the problems:

  • still defeat the threat
  • less events with needless token errors
  • no problems with browser back button

Is this the current common sense?

--- Originally created by Georg on August 31st, 2011, at 05:31pm

Member

aschempp commented Nov 29, 2011

Yes, I agree

--- Originally created on August 31st, 2011, at 05:36pm

Ruudt commented Nov 29, 2011

I have two questions about that:

  1. I understood that under some circumstances it was suggested to use one-time-tokens? Doesn't that mean we still have a back button problem afterwards? (which means we need to rely on the solution for the yellow boxes even more to be good) GET forms like search already don't need one anyway afaik.

  2. What do you agree upon is a good solution for the yellow boxes?
    Personally I would still have some problems with a redirect pages though it is already better then the yellow box. And I haven't heard anyone respond to Andreas' original suggestion to just display an error message on the form itself. (which is currently not yet supported, but makes retrying easy for the visitor)

--- Originally created on September 1st, 2011, at 12:58am

Collaborator

issue-bot commented Nov 29, 2011

@ruud:
1a) one-token-per-session inside the post message is secure.
No need for a token-per-request.

1b) at first place a token is not a question of the type of a form:
usually every request of the user is authorised first by the password and then during his member session by resending of the fe_cookie or be_cookie of contao during every request.
So as long as the member session and the browser cookies of this user are valid they can be misused by the CSRF attack - and have to be protected by sending the token of the member session inside of every post message.

Nevertheless You are right: when a form is also visible to public users and the result of the form is public too there is no need for a evaluation of the token.

--- Originally created by Georg on September 1st, 2011, at 09:13am

Collaborator

issue-bot commented Nov 29, 2011

My last postulate does not hit the point exactly:
the CSRF attacker will never receive the answer of the server - the response of the server is only send to the misused browser and therefore is never endangered.

Only the action of the server as the result of the misused browser request could be compromised.
For example:

  • change status of the server
  • change database field
  • give the attacker a super-admin account
  • send emails to all members

And that is the reason why Ruud is right: in every case the result of a search form could not been compromised and therefore the token don't have to be evaluated.

My postulate in addition to that:
Only when the server action as the result of a browser request is not allowed for public users the token has to be evaluated.

Concerning the question 2 of Ruud I have no opinion, because I hope for finding a solution with this error will never occur during standard operations.

--- Originally created by Georg on September 2nd, 2011, at 08:17am

Owner

leofeyer commented Nov 29, 2011

First draft of the "one token per session" approach in 0b40b91. Still need to adjust the JavaScript.

--- Originally created on October 12th, 2011, at 07:52pm

Member

aschempp commented Nov 29, 2011

Wow, thanks for giving in ;-)
Is there are reason we need the BYPASS_TOKEN feature anymore now that there is a session token?

--- Originally created on October 12th, 2011, at 08:47pm

Owner

leofeyer commented Nov 29, 2011

Yes: FancyUpload :)

--- Originally created on October 12th, 2011, at 08:52pm

Owner

leofeyer commented Nov 29, 2011

And maybe instant payment notifications (IPN)? Haven't checked yet …

--- Originally created on October 12th, 2011, at 08:53pm

Member

aschempp commented Nov 29, 2011

Well I'm pretty sure FancyUpload could pass on the token. However, you're right with IPN or any other expected incoming communication.

--- Originally created on October 12th, 2011, at 09:08pm

YOU DID IT! 0b40b91
Thanks for changing the token system and make them useable and usefull.
Greetings
Happy Leo

--- Originally created on October 12th, 2011, at 10:04pm

Collaborator

issue-bot commented Nov 29, 2011

@leo: Can you please change a small part of your validation code:

if ($strToken 

![](= '' && $this->strToken )= '' && $strToken == $this->strToken)
{
       return true;
}

return false;

too

return $strToken 

![](= '' && $this->strToken )= '' && $strToken == $this->strToken;

This looks currently to ugly ;).

--- Originally created by SunBlack on October 13th, 2011, at 01:39am

Why should he change that? It's clean and valid code. It's good that he defines the return type by using boolean values. All other ways are not typesave and crap.

--- Originally created on October 13th, 2011, at 03:14pm

Owner

leofeyer commented Nov 29, 2011

Completed in 09e7c27.

--- Originally created on October 13th, 2011, at 03:18pm

Collaborator

issue-bot commented Nov 29, 2011

@leo.unglaub: Sure it's valid code. But this is similar too:

$myBool = true; //or false
if ($myBool)
{
       return true;
}

return false;

compared too, which don't need an useless/expensive if-statement:

return $myBool;

And in case of the validation code, the change is typesave, because comparison operations return always a boolean value.

--- Originally created by SunBlack on October 13th, 2011, at 03:49pm

Owner

leofeyer commented Nov 29, 2011

--- Originally completed on October 15th, 2011, at 08:03pm

leofeyer closed this Nov 29, 2011

@leofeyer leofeyer added a commit that referenced this issue Nov 30, 2011

@leofeyer leofeyer Changed the request token system from "one token per request" to "one…
… token per session" (see #3214)
0b40b91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment