feature: Form Tokens #4509
feature: Form Tokens #4509
Conversation
|
This is Round 1. Please review/test and let me know what you find. Cheers. |
include/class.lock.php
Outdated
| @@ -90,6 +99,7 @@ function renew($lockTime=0) { | |||
| SqlFunction::NOW(), | |||
| SqlInterval::MINUTE($lockTime) | |||
| ); | |||
| $this->_signature = md5($this->lock_id.$this->code.strtotime($this->expire)); | |||
There was a problem hiding this comment.
Don't set hash in multiple places - getSignature should be the only spot.
$this->_signature = null;
There was a problem hiding this comment.
@protich , do i have to implement only this $this->_signature = null; or this to $this->_signature = md5($this->lock_id.$this->code.strtotime($this->expire));.
There was a problem hiding this comment.
The changes are already implemented...just apply the changes from the Changes tab.
Cheers.
There was a problem hiding this comment.
Thanks, i will use the changes tab!
include/class.lock.php
Outdated
| @@ -124,8 +134,10 @@ static function acquire($staffId, $lockTime) { | |||
| ), | |||
| 'code' => Misc::randCode(10) | |||
| )); | |||
| if ($lock->save(true)) | |||
| if ($lock->save(true)) { | |||
| $lock->_signature = md5($lock->lock_id.$lock->code.strtotime($lock->expire)); | |||
include/class.lock.php
Outdated
| if (!$code) | ||
| return false; | ||
|
|
||
| $lock = Lock::objects() |
There was a problem hiding this comment.
if (!($lock = Lock::lookup(array('code' => $code)))
return false;
include/class.lock.php
Outdated
| if (!$lock) | ||
| return false; | ||
|
|
||
| if ($token == md5($lock->getId().$lock->getCode().$lock->getExpireTime())) |
There was a problem hiding this comment.
Never do this... it spreads hash all over the place (see my other comments). Also I think we should change signature to token.
if (strcmp($token, $token->getToken()) !== 0)
return false;
return $lock->delete();
include/class.ticket.php
Outdated
| // Validate Form Token to prevent form resubmissions | ||
| if (!$vars['form_token'] || !$vars['lockCode']) | ||
| $errors['err'] = __('Invalid Form Token'); | ||
|
|
There was a problem hiding this comment.
Validation shouldn't happen inside any model... it's a form validation, not part of data validation.
include/class.ticket.php
Outdated
| if (!$vars['form_token'] || !$vars['lockCode']) | ||
| $errors['err'] = __('Invalid Form Token'); | ||
|
|
||
| if (!$vars['token_validated'] && !Lock::validateToken($vars['form_token'], $vars['lockCode'])) { |
There was a problem hiding this comment.
As mentioned above - resubmission prevention should be at form level. In this case it should be part of Response Form.
tickets.php
Outdated
| @@ -85,6 +85,8 @@ | |||
| 'message' => $_POST['message'] | |||
| ); | |||
| $vars['cannedattachments'] = $attachments->getClean(); | |||
| $vars['lockCode'] = $_POST['lock_code'] ?: null; | |||
| $vars['form_token'] = $_POST['form_token'] ?: null; | |||
There was a problem hiding this comment.
This is a result of doing validation in the wrong place... results is mudding $vars
JediKev
force-pushed
the
feature/form-tokens
branch
2 times, most recently
from
28c07e4
to
13079e3
Compare
scp/js/ticket.js
Outdated
| @@ -178,6 +178,10 @@ | |||
| // to the lock.code retrieved (if any) | |||
| if (lock.code) | |||
| $(this.options.lockInput, this.$element).val(lock.code); | |||
| // If there is an input with the name 'form_token', then set the value | |||
| // to the lock.signature retrieved (if any) | |||
| if (lock.signature) | |||
include/ajax.tickets.php
Outdated
| @@ -118,7 +118,7 @@ function acquireLock($tid) { | |||
|
|
|||
| return $this->json_encode(array( | |||
| 'id'=>$lock->getId(), 'time'=>$lock->getTime(), | |||
| 'code' => $lock->getCode() | |||
| 'code' => $lock->getCode(), 'signature'=>$lock->getSignature() | |||
There was a problem hiding this comment.
This will crash... should be 'token' => $lock->getToken()
JediKev
force-pushed
the
feature/form-tokens
branch
from
13079e3
to
a2d0561
Compare
This adds a hidden input called "form_token" to ticket creation forms, response forms, and message forms that should prevent form resubmissions. This will address issues like duplicate tickets created on page refresh, etc.
JediKev
force-pushed
the
feature/form-tokens
branch
from
a2d0561
to
4081007
Compare
| if (strcmp($token, $lock->getToken()) !== 0) | ||
| return false; | ||
|
|
||
| return $lock->delete(); |
There was a problem hiding this comment.
Are you sure you want to delete the lock here - What about if the action (e.g posReply) errors? Re-acquiring the lock will change the token (due to new expire date - please confirm) - so I don't the need to delete here. Lock cleaning mechanism should handle deleting expired sessions.
|
So basically i should follow all the stept from above to every single step? |
|
What..? Just apply the changes from the Changes tab. Cheers. |
Thank you! IT worked! |
This adds a hidden input called "form_token" to ticket creation forms, response forms, and message forms that should prevent form resubmissions. This will address issues like duplicate tickets created on page refresh, etc.