From 3e0f14ae2b0b5a44bd038a472f17eac75f538524 Mon Sep 17 00:00:00 2001 From: Frederic Guillot Date: Sat, 23 Sep 2017 20:56:54 -0700 Subject: [PATCH] Do not expose IDs in forms --- app/Controller/BaseController.php | 62 ++++++++++++++++--- app/Controller/CommentController.php | 43 +++---------- app/Controller/SubtaskController.php | 12 ++-- app/Controller/SubtaskConverterController.php | 5 +- .../SubtaskRestrictionController.php | 4 +- app/Controller/SubtaskStatusController.php | 10 +-- app/Controller/TaskExternalLinkController.php | 37 +++++------ app/Controller/TaskInternalLinkController.php | 34 +++------- app/Controller/TaskModificationController.php | 2 + app/Controller/TaskRecurrenceController.php | 1 + app/Template/comment/create.php | 2 - app/Template/comment/edit.php | 3 - app/Template/subtask/create.php | 3 +- app/Template/subtask/edit.php | 2 - app/Template/task_external_link/edit.php | 2 +- app/Template/task_external_link/find.php | 1 - app/Template/task_external_link/form.php | 2 - app/Template/task_internal_link/create.php | 1 - app/Template/task_internal_link/edit.php | 4 +- app/Template/task_modification/show.php | 2 - 20 files changed, 112 insertions(+), 120 deletions(-) diff --git a/app/Controller/BaseController.php b/app/Controller/BaseController.php index 1ac7ed2038..41fcef1c37 100644 --- a/app/Controller/BaseController.php +++ b/app/Controller/BaseController.php @@ -138,14 +138,7 @@ protected function getUser() return $user; } - /** - * Get the current subtask - * - * @access protected - * @return array - * @throws PageNotFoundException - */ - protected function getSubtask() + protected function getSubtask(array $task) { $subtask = $this->subtaskModel->getById($this->request->getIntegerParam('subtask_id')); @@ -153,9 +146,62 @@ protected function getSubtask() throw new PageNotFoundException(); } + if ($subtask['task_id'] != $task['id']) { + throw new AccessForbiddenException(); + } + return $subtask; } + protected function getComment(array $task) + { + $comment = $this->commentModel->getById($this->request->getIntegerParam('comment_id')); + + if (empty($comment)) { + throw new PageNotFoundException(); + } + + if (! $this->userSession->isAdmin() && $comment['user_id'] != $this->userSession->getId()) { + throw new AccessForbiddenException(); + } + + if ($comment['task_id'] != $task['id']) { + throw new AccessForbiddenException(); + } + + return $comment; + } + + protected function getExternalTaskLink(array $task) + { + $link = $this->taskExternalLinkModel->getById($this->request->getIntegerParam('link_id')); + + if (empty($link)) { + throw new PageNotFoundException(); + } + + if ($link['task_id'] != $task['id']) { + throw new AccessForbiddenException(); + } + + return $link; + } + + protected function getInternalTaskLink(array $task) + { + $link = $this->taskLinkModel->getById($this->request->getIntegerParam('link_id')); + + if (empty($link)) { + throw new PageNotFoundException(); + } + + if ($link['task_id'] != $task['id']) { + throw new AccessForbiddenException(); + } + + return $link; + } + protected function getColumn(array $project) { $column = $this->columnModel->getById($this->request->getIntegerParam('column_id')); diff --git a/app/Controller/CommentController.php b/app/Controller/CommentController.php index 9a89103eeb..a29491a31d 100644 --- a/app/Controller/CommentController.php +++ b/app/Controller/CommentController.php @@ -13,29 +13,6 @@ */ class CommentController extends BaseController { - /** - * Get the current comment - * - * @access protected - * @return array - * @throws PageNotFoundException - * @throws AccessForbiddenException - */ - protected function getComment() - { - $comment = $this->commentModel->getById($this->request->getIntegerParam('comment_id')); - - if (empty($comment)) { - throw new PageNotFoundException(); - } - - if (! $this->userSession->isAdmin() && $comment['user_id'] != $this->userSession->getId()) { - throw new AccessForbiddenException(); - } - - return $comment; - } - /** * Add comment form * @@ -49,14 +26,6 @@ public function create(array $values = array(), array $errors = array()) { $project = $this->getProject(); $task = $this->getTask(); - - if (empty($values)) { - $values = array( - 'user_id' => $this->userSession->getId(), - 'task_id' => $task['id'], - ); - } - $values['project_id'] = $task['project_id']; $this->response->html($this->helper->layout->task('comment/create', array( @@ -106,7 +75,7 @@ public function save() public function edit(array $values = array(), array $errors = array()) { $task = $this->getTask(); - $comment = $this->getComment(); + $comment = $this->getComment($task); if (empty($values)) { $values = $comment; @@ -130,9 +99,13 @@ public function edit(array $values = array(), array $errors = array()) public function update() { $task = $this->getTask(); - $this->getComment(); + $comment = $this->getComment($task); $values = $this->request->getValues(); + $values['id'] = $comment['id']; + $values['task_id'] = $task['id']; + $values['user_id'] = $comment['user_id']; + list($valid, $errors) = $this->commentValidator->validateModification($values); if ($valid) { @@ -157,7 +130,7 @@ public function update() public function confirm() { $task = $this->getTask(); - $comment = $this->getComment(); + $comment = $this->getComment($task); $this->response->html($this->template->render('comment/remove', array( 'comment' => $comment, @@ -175,7 +148,7 @@ public function remove() { $this->checkCSRFParam(); $task = $this->getTask(); - $comment = $this->getComment(); + $comment = $this->getComment($task); if ($this->commentModel->remove($comment['id'])) { $this->flash->success(t('Comment removed successfully.')); diff --git a/app/Controller/SubtaskController.php b/app/Controller/SubtaskController.php index 5fa55f6bcf..b9bb09342d 100644 --- a/app/Controller/SubtaskController.php +++ b/app/Controller/SubtaskController.php @@ -66,6 +66,7 @@ public function save() { $task = $this->getTask(); $values = $this->request->getValues(); + $values['task_id'] = $task['id']; list($valid, $errors) = $this->subtaskValidator->validateCreation($values); @@ -103,7 +104,7 @@ public function save() public function edit(array $values = array(), array $errors = array()) { $task = $this->getTask(); - $subtask = $this->getSubtask(); + $subtask = $this->getSubtask($task); $this->response->html($this->template->render('subtask/edit', array( 'values' => empty($values) ? $subtask : $values, @@ -123,9 +124,12 @@ public function edit(array $values = array(), array $errors = array()) public function update() { $task = $this->getTask(); - $this->getSubtask(); + $subtask = $this->getSubtask($task); $values = $this->request->getValues(); + $values['id'] = $subtask['id']; + $values['task_id'] = $task['id']; + list($valid, $errors) = $this->subtaskValidator->validateModification($values); if ($valid) { @@ -149,7 +153,7 @@ public function update() public function confirm() { $task = $this->getTask(); - $subtask = $this->getSubtask(); + $subtask = $this->getSubtask($task); $this->response->html($this->template->render('subtask/remove', array( 'subtask' => $subtask, @@ -166,7 +170,7 @@ public function remove() { $this->checkCSRFParam(); $task = $this->getTask(); - $subtask = $this->getSubtask(); + $subtask = $this->getSubtask($task); if ($this->subtaskModel->remove($subtask['id'])) { $this->flash->success(t('Sub-task removed successfully.')); diff --git a/app/Controller/SubtaskConverterController.php b/app/Controller/SubtaskConverterController.php index 404c50d0e8..dafbb7e002 100644 --- a/app/Controller/SubtaskConverterController.php +++ b/app/Controller/SubtaskConverterController.php @@ -13,7 +13,7 @@ class SubtaskConverterController extends BaseController public function show() { $task = $this->getTask(); - $subtask = $this->getSubtask(); + $subtask = $this->getSubtask($task); $this->response->html($this->template->render('subtask_converter/show', array( 'subtask' => $subtask, @@ -24,7 +24,8 @@ public function show() public function save() { $project = $this->getProject(); - $subtask = $this->getSubtask(); + $task = $this->getTask(); + $subtask = $this->getSubtask($task); $task_id = $this->subtaskTaskConversionModel->convertToTask($project['id'], $subtask['id']); diff --git a/app/Controller/SubtaskRestrictionController.php b/app/Controller/SubtaskRestrictionController.php index 99315931f2..315b023d1d 100644 --- a/app/Controller/SubtaskRestrictionController.php +++ b/app/Controller/SubtaskRestrictionController.php @@ -20,7 +20,7 @@ class SubtaskRestrictionController extends BaseController public function show() { $task = $this->getTask(); - $subtask = $this->getSubtask(); + $subtask = $this->getSubtask($task); $this->response->html($this->template->render('subtask_restriction/show', array( 'status_list' => array( @@ -41,7 +41,7 @@ public function show() public function save() { $task = $this->getTask(); - $subtask = $this->getSubtask(); + $subtask = $this->getSubtask($task); $values = $this->request->getValues(); // Change status of the previous "in progress" subtask diff --git a/app/Controller/SubtaskStatusController.php b/app/Controller/SubtaskStatusController.php index ef16fce0ff..c912848e71 100644 --- a/app/Controller/SubtaskStatusController.php +++ b/app/Controller/SubtaskStatusController.php @@ -18,7 +18,7 @@ class SubtaskStatusController extends BaseController public function change() { $task = $this->getTask(); - $subtask = $this->getSubtask(); + $subtask = $this->getSubtask($task); $fragment = $this->request->getStringParam('fragment'); $status = $this->subtaskStatusModel->toggleStatus($subtask['id']); @@ -43,19 +43,19 @@ public function change() public function timer() { $task = $this->getTask(); - $subtaskId = $this->request->getIntegerParam('subtask_id'); + $subtask = $this->getSubtask($task); $timer = $this->request->getStringParam('timer'); if ($timer === 'start') { - $this->subtaskTimeTrackingModel->logStartTime($subtaskId, $this->userSession->getId()); + $this->subtaskTimeTrackingModel->logStartTime($subtask['id'], $this->userSession->getId()); } elseif ($timer === 'stop') { - $this->subtaskTimeTrackingModel->logEndTime($subtaskId, $this->userSession->getId()); + $this->subtaskTimeTrackingModel->logEndTime($subtask['id'], $this->userSession->getId()); $this->subtaskTimeTrackingModel->updateTaskTimeTracking($task['id']); } $this->response->html($this->template->render('subtask/timer', array( 'task' => $task, - 'subtask' => $this->subtaskModel->getByIdWithDetails($subtaskId), + 'subtask' => $this->subtaskModel->getByIdWithDetails($subtask['id']), ))); } diff --git a/app/Controller/TaskExternalLinkController.php b/app/Controller/TaskExternalLinkController.php index df23f87bb7..946451fcb7 100644 --- a/app/Controller/TaskExternalLinkController.php +++ b/app/Controller/TaskExternalLinkController.php @@ -74,6 +74,8 @@ public function save() { $task = $this->getTask(); $values = $this->request->getValues(); + $values['task_id'] = $task['id']; + list($valid, $errors) = $this->externalLinkValidator->validateCreation($values); if ($valid) { @@ -108,22 +110,14 @@ public function save() public function edit(array $values = array(), array $errors = array()) { $task = $this->getTask(); - $link_id = $this->request->getIntegerParam('link_id'); - - if ($link_id > 0) { - $values = $this->taskExternalLinkModel->getById($link_id); - } - - if (empty($values)) { - throw new PageNotFoundException(); - } - - $provider = $this->externalLinkManager->getProvider($values['link_type']); + $link = $this->getExternalTaskLink($task); + $provider = $this->externalLinkManager->getProvider($link['link_type']); $this->response->html($this->template->render('task_external_link/edit', array( - 'values' => $values, - 'errors' => $errors, - 'task' => $task, + 'values' => empty($values) ? $link : $values, + 'errors' => $errors, + 'task' => $task, + 'link' => $link, 'dependencies' => $provider->getDependencies(), ))); } @@ -136,7 +130,12 @@ public function edit(array $values = array(), array $errors = array()) public function update() { $task = $this->getTask(); + $link = $this->getExternalTaskLink($task); + $values = $this->request->getValues(); + $values['id'] = $link['id']; + $values['task_id'] = $link['task_id']; + list($valid, $errors) = $this->externalLinkValidator->validateModification($values); if ($valid && $this->taskExternalLinkModel->update($values)) { @@ -155,12 +154,7 @@ public function update() public function confirm() { $task = $this->getTask(); - $link_id = $this->request->getIntegerParam('link_id'); - $link = $this->taskExternalLinkModel->getById($link_id); - - if (empty($link)) { - throw new PageNotFoundException(); - } + $link = $this->getExternalTaskLink($task); $this->response->html($this->template->render('task_external_link/remove', array( 'link' => $link, @@ -177,8 +171,9 @@ public function remove() { $this->checkCSRFParam(); $task = $this->getTask(); + $link = $this->getExternalTaskLink($task); - if ($this->taskExternalLinkModel->remove($this->request->getIntegerParam('link_id'))) { + if ($this->taskExternalLinkModel->remove($link['id'])) { $this->flash->success(t('Link removed successfully.')); } else { $this->flash->failure(t('Unable to remove this link.')); diff --git a/app/Controller/TaskInternalLinkController.php b/app/Controller/TaskInternalLinkController.php index 7c80016541..02cc15c4dd 100644 --- a/app/Controller/TaskInternalLinkController.php +++ b/app/Controller/TaskInternalLinkController.php @@ -13,24 +13,6 @@ */ class TaskInternalLinkController extends BaseController { - /** - * Get the current link - * - * @access private - * @return array - * @throws PageNotFoundException - */ - private function getTaskLink() - { - $link = $this->taskLinkModel->getById($this->request->getIntegerParam('link_id')); - - if (empty($link)) { - throw new PageNotFoundException(); - } - - return $link; - } - /** * Creation form * @@ -45,9 +27,7 @@ public function create(array $values = array(), array $errors = array()) $task = $this->getTask(); if (empty($values)) { - $values = array( - 'another_tasklink' => $this->request->getIntegerParam('another_tasklink', 0) - ); + $values['another_tasklink'] = $this->request->getIntegerParam('another_tasklink', 0); $values = $this->hook->merge('controller:tasklink:form:default', $values, array('default_values' => $values)); } @@ -68,6 +48,7 @@ public function save() { $task = $this->getTask(); $values = $this->request->getValues(); + $values['task_id'] = $task['id']; list($valid, $errors) = $this->taskLinkValidator->validateCreation($values); @@ -106,7 +87,7 @@ public function save() public function edit(array $values = array(), array $errors = array()) { $task = $this->getTask(); - $task_link = $this->getTaskLink(); + $task_link = $this->getInternalTaskLink($task); if (empty($values)) { $opposite_task = $this->taskFinderModel->getById($task_link['opposite_task_id']); @@ -131,7 +112,11 @@ public function edit(array $values = array(), array $errors = array()) public function update() { $task = $this->getTask(); + $task_link = $this->getInternalTaskLink($task); + $values = $this->request->getValues(); + $values['task_id'] = $task['id']; + $values['id'] = $task_link['id']; list($valid, $errors) = $this->taskLinkValidator->validateModification($values); @@ -155,7 +140,7 @@ public function update() public function confirm() { $task = $this->getTask(); - $link = $this->getTaskLink(); + $link = $this->getInternalTaskLink($task); $this->response->html($this->template->render('task_internal_link/remove', array( 'link' => $link, @@ -172,8 +157,9 @@ public function remove() { $this->checkCSRFParam(); $task = $this->getTask(); + $link = $this->getInternalTaskLink($task); - if ($this->taskLinkModel->remove($this->request->getIntegerParam('link_id'))) { + if ($this->taskLinkModel->remove($link['id'])) { $this->flash->success(t('Link removed successfully.')); } else { $this->flash->failure(t('Unable to remove this link.')); diff --git a/app/Controller/TaskModificationController.php b/app/Controller/TaskModificationController.php index 1892a20982..338ed54014 100644 --- a/app/Controller/TaskModificationController.php +++ b/app/Controller/TaskModificationController.php @@ -98,6 +98,8 @@ public function update() { $task = $this->getTask(); $values = $this->request->getValues(); + $values['id'] = $task['id']; + $values['project_id'] = $task['project_id']; list($valid, $errors) = $this->taskValidator->validateModification($values); diff --git a/app/Controller/TaskRecurrenceController.php b/app/Controller/TaskRecurrenceController.php index c6fdfa3781..7f14cb5344 100644 --- a/app/Controller/TaskRecurrenceController.php +++ b/app/Controller/TaskRecurrenceController.php @@ -47,6 +47,7 @@ public function update() { $task = $this->getTask(); $values = $this->request->getValues(); + $values['id'] = $task['id']; list($valid, $errors) = $this->taskValidator->validateEditRecurrence($values); diff --git a/app/Template/comment/create.php b/app/Template/comment/create.php index 0e19ac1964..55e972dc07 100644 --- a/app/Template/comment/create.php +++ b/app/Template/comment/create.php @@ -8,8 +8,6 @@
form->csrf() ?> - form->hidden('task_id', $values) ?> - form->hidden('user_id', $values) ?> form->textEditor('comment', $values, $errors, array('autofocus' => true, 'required' => true)) ?> diff --git a/app/Template/comment/edit.php b/app/Template/comment/edit.php index 04f6ffd477..db8d2921fc 100644 --- a/app/Template/comment/edit.php +++ b/app/Template/comment/edit.php @@ -4,9 +4,6 @@ form->csrf() ?> - form->hidden('id', $values) ?> - form->hidden('task_id', $values) ?> - form->hidden('user_id', $values) ?> form->textEditor('comment', $values, $errors, array('autofocus' => true, 'required' => true)) ?> diff --git a/app/Template/subtask/create.php b/app/Template/subtask/create.php index 96ad7a468d..bbb6400538 100644 --- a/app/Template/subtask/create.php +++ b/app/Template/subtask/create.php @@ -3,9 +3,8 @@ - form->csrf() ?> - form->hidden('task_id', $values) ?> + subtask->renderTitleField($values, $errors, array('autofocus')) ?> subtask->renderAssigneeField($users_list, $values, $errors) ?> subtask->renderTimeEstimatedField($values, $errors) ?> diff --git a/app/Template/subtask/edit.php b/app/Template/subtask/edit.php index 7c0266a8e7..aed57e957f 100644 --- a/app/Template/subtask/edit.php +++ b/app/Template/subtask/edit.php @@ -4,8 +4,6 @@ form->csrf() ?> - form->hidden('id', $values) ?> - form->hidden('task_id', $values) ?> subtask->renderTitleField($values, $errors, array('autofocus')) ?> subtask->renderAssigneeField($users_list, $values, $errors) ?> diff --git a/app/Template/task_external_link/edit.php b/app/Template/task_external_link/edit.php index df10d444fa..e448b10f3f 100644 --- a/app/Template/task_external_link/edit.php +++ b/app/Template/task_external_link/edit.php @@ -2,7 +2,7 @@

- + render('task_external_link/form', array('task' => $task, 'dependencies' => $dependencies, 'values' => $values, 'errors' => $errors)) ?> modal->submitButtons() ?>
diff --git a/app/Template/task_external_link/find.php b/app/Template/task_external_link/find.php index a3665c0d08..29d85101ef 100644 --- a/app/Template/task_external_link/find.php +++ b/app/Template/task_external_link/find.php @@ -4,7 +4,6 @@
form->csrf() ?> - form->hidden('task_id', array('task_id' => $task['id'])) ?> form->label(t('External link'), 'text') ?> form->text( diff --git a/app/Template/task_external_link/form.php b/app/Template/task_external_link/form.php index 932ca52178..4ad2b2e0f3 100644 --- a/app/Template/task_external_link/form.php +++ b/app/Template/task_external_link/form.php @@ -1,6 +1,4 @@ form->csrf() ?> -form->hidden('task_id', array('task_id' => $task['id'])) ?> -form->hidden('id', $values) ?> form->hidden('link_type', $values) ?> form->label(t('URL'), 'url') ?> diff --git a/app/Template/task_internal_link/create.php b/app/Template/task_internal_link/create.php index c5e80f414b..bab4125378 100644 --- a/app/Template/task_internal_link/create.php +++ b/app/Template/task_internal_link/create.php @@ -5,7 +5,6 @@ form->csrf() ?> - form->hidden('task_id', array('task_id' => $task['id'])) ?> form->hidden('opposite_task_id', $values) ?> form->label(t('Label'), 'link_id') ?> diff --git a/app/Template/task_internal_link/edit.php b/app/Template/task_internal_link/edit.php index 5abf7b65e3..fab84d0b67 100644 --- a/app/Template/task_internal_link/edit.php +++ b/app/Template/task_internal_link/edit.php @@ -3,10 +3,8 @@ - form->csrf() ?> - form->hidden('id', $values) ?> - form->hidden('task_id', $values) ?> + form->hidden('opposite_task_id', $values) ?> form->label(t('Label'), 'link_id') ?> diff --git a/app/Template/task_modification/show.php b/app/Template/task_modification/show.php index 710abedff2..ebe9f6fd6c 100644 --- a/app/Template/task_modification/show.php +++ b/app/Template/task_modification/show.php @@ -3,8 +3,6 @@ form->csrf() ?> - form->hidden('id', $values) ?> - form->hidden('project_id', $values) ?>