demianturner / sgl-docs-tickets-migration-test

0 stars 0 forks source link

Reliance on $input->submitted in various managers allows invalid submission #1660

Open demianturner opened 11 years ago

demianturner commented 11 years ago

= Problem Description = In at least a few managers, user input is validated based on the assumption $req->get('submitted') is not empty, rather than checking what action was passed. This method of validating user input can allow a malicious user to deliberately send a form with a valid action name, but without sending 'submitted' and cause the form to pass as validated. This can also occur without malicious intent (). [[BR]] [[BR]]

= Problem Scenarios = 1) With a form using , Firefox sends the value of 'submitted', while IE doesn't. IE users would submit the form without validating it.[[BR]] 2) A malicious user could rename the submitted button, or set its value to nothing before submitting a form (or otherwise send without 'submitted' using any number of techniques, but this one is easy to illustrate). {{{ javascript:void(document.form[0].submitted.value = ''); }}} [[BR]]

= Proposed Solution = == Overview == Validation functions specific to actions, automatically executed by the controller in a similar fashion to actions. Maintain backward-compatibility for easy integration with existing modules.

== Implementation == New variable added to SGL_Manager, $_aValidationsMapping, contains an array of action names mapped to any number of validation functions. This allows for more code reuse if for example 'insert' and 'update' required the same validations, _valid_submit may be used for both. If 'update' required specialized validation in addition to _valid_submit, and an additional _valid_update could be created.[[BR]] [[BR]] {{{ $_aValidationsMapping = array('actionName' => array() /* str function names */ ); }}} [[BR]] do_validate function created in SGL_Manager which calls validate first, then checks if $_aValidationsMapping has action-specific validation functions defined for the current action, calling _valid_functionName() on $this. [[BR]] [[BR]] = PATCH = This solution is included in the attached libValidation.patch, and example usage is also included in ContactUsMgr.php

demianturner commented 11 years ago

[demian] zbateson - thanks for your input, I think this degree of change should happen in trunk (0.9) rather than bugfix. i need to resolve some plugin work first then will look at validation after.