Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Ensure that only the path and query are used to make the hash.
While including the entire protocol, host, port, path and query would be
even better in theory, it gets complicated when proxies and load
balancers are involved.

Fixes #3442
  • Loading branch information
markstory committed May 7, 2014
1 parent 559d9d3 commit 1103ca7
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 1 deletion.
28 changes: 28 additions & 0 deletions lib/Cake/Test/Case/View/Helper/FormHelperTest.php
Expand Up @@ -1370,6 +1370,34 @@ public function testSecuredAndDisabled() {
$this->assertEquals($expected, $this->Form->fields);
}

/**
* Test that only the path + query elements of a form's URL show up in their hash.
*
* @return void
*/
public function testSecuredFormUrlIgnoresHost() {
$this->Form->request['_Token'] = array('key' => 'testKey');

$expected = '5181b484c13caea4776618ed26a3aebbb026ecd8%3A';
$this->Form->create('Address', array(
'url' => array('controller' => 'articles', 'action' => 'view', 1, '?' => array('page' => 1))
));
$result = $this->Form->secure();
$this->assertContains($expected, $result);

$this->Form->create('Address', array('url' => 'http://localhost/articles/view/1?page=1'));
$result = $this->Form->secure();
$this->assertContains($expected, $result, 'Full URL should only use path and query.');

$this->Form->create('Address', array('url' => '/articles/view/1?page=1'));
$result = $this->Form->secure();
$this->assertContains($expected, $result, 'URL path + query should work.');

$this->Form->create('Address', array('url' => '/articles/view/1'));
$result = $this->Form->secure();
$this->assertNotContains($expected, $result, 'URL is different');
}

/**
* testDisableSecurityUsingForm method
*
Expand Down
7 changes: 6 additions & 1 deletion lib/Cake/View/Helper/FormHelper.php
Expand Up @@ -466,7 +466,12 @@ public function create($model = null, $options = array()) {
$this->setEntity($model, true);
$this->_introspectModel($model, 'fields');
}
$this->_lastAction = $action;
$query = parse_url($action, PHP_URL_QUERY);
if ($query) {
$query .= '?';
}
$this->_lastAction = parse_url($action, PHP_URL_PATH) . $query;

return $this->Html->useTag('form', $action, $htmlAttributes) . $append;
}

Expand Down

2 comments on commit 1103ca7

@casdevs
Copy link

@casdevs casdevs commented on 1103ca7 May 7, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markstory: thanks for handling this! And indeed I haven't had reverse proxies etc. in mind when suggesting that hashing the full url including host would be preferable - good point!

One little question though: shouldn't the question mark rather be prepended to $query instead of being appended?

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should, I will get that fixed.

Please sign in to comment.