Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ticket/13568] Validate imagick path as readable absolute path #3359

Closed
wants to merge 8 commits into from

Conversation

@marc1706
Copy link
Member

commented Feb 2, 2015

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2015

Result is invalid for windows

@marc1706 marc1706 force-pushed the marc1706:ticket/13568 branch from e12ab04 to 19421fc Feb 2, 2015

@marc1706

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2015

Added new validation option for absolute paths and also added functional tests for the imagick path setting.

// Absolute file path
case 'wapath':
case 'apath':

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 2, 2015

Contributor

better useful names then short ones? 🙊

This comment has been minimized.

Copy link
@marc1706

marc1706 Feb 2, 2015

Author Member

<nickvergessen> better use apath <nickvergessen> or absolute path

But sure, I'll use more descriptive ones

$cfg_array[$config_name] = '';
break;
}

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 2, 2015

Contributor

This is all duplicated from above.

I'd say try something like:

$tmp =  ($config_definition['validate'] === 'wpath' || $config_definition['validate'] === 'path') ? $phpbb_root_path . $cfg_array[$config_name] : $cfg_array[$config_name];

and then use $tmp for validation

$crawler = self::request('GET', 'adm/index.php?i=attachments&mode=attach&sid=' . $this->sid);
$form = $crawler->selectButton('Submit')->form(array('config[img_imagick]' => $imagick_path));

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 2, 2015

Contributor

While adding a test, please set all values to any extreme values :)

$form = $crawler->selectButton('Submit')->form(array('config[img_imagick]' => $imagick_path));
$crawler = self::submit($form);
$this->assertContains($expected, $crawler->text());

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 2, 2015

Contributor

please use a filter

return array(
array('/usr/bin', 'Configuration updated successfully'),
array('/usr/bin/', 'Configuration updated successfully'),
array('C:\Windows\system32', 'The entered path “C:\Windows\system32” does not exist.'),

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 2, 2015

Contributor

Test fails on windows :/

public function data_imagick_path()
{
return array(
array('/usr/bin', 'Configuration updated successfully'),

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 2, 2015

Contributor

test fails on windows

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2015

you need to create 2 tests and skip them if on windows/not on windows
(or 3 because of mac?)

@marc1706

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2015

I'd say skip these tests if not on linux.

array('C:\Windows\system32', 'The entered path “C:\Windows\system32” does not exist.'),
array('/usr/nope', 'The entered path “/usr/nope” does not exist.'),
array('mkdir /usr/test', 'The entered path “mkdir /usr/test” does not exist.'),
array('/usr/bin/which', 'The entered path “/usr/bin/which” is not a directory.'),

This comment has been minimized.

Copy link
@cs278

cs278 Feb 2, 2015

Contributor

What about a test case for . which should fail, but doesn't look like it will.

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2015

I'd say skip these tests if not on linux.

yes and then add another test suite that works on windows.
We want to make sure this works on IIS right?!

@marc1706

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2015

Test case and unit tests that cover more paths will be added for a develop-ascraeus patch

@nickvergessen nickvergessen added this to the 3.0.14 milestone Feb 2, 2015

@marc1706

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2015

Tests are passing on windows for me now.

@marc1706

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2015

Test failure is due to travis having dropped the PHP 5.2 environment.

@marc1706

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2015

@nickvergessen @bantu if this PR looks fine to you I'll create one based on this for ascraeus that also adds unit tests

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

yes please

@marc1706 marc1706 removed the 3.1 (Ascraeus) label Feb 6, 2015

@marc1706

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2015

@bantu PHP 5.2 builds seem to not work again

@bantu

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

@marc1706 I have been told 5.2 is permanently gone and 5.3.3 will follow.

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2015

http://docs.travis-ci.com/user/build-environment-updates/2015-02-03/#PHP-VM

PHP VM

  • PHP updates: 5.4.35 → 5.4.37, 5.5.19 → 5.5.21, 5.6.3 → 5.6.5
  • HHVM 3.4.0 → 3.5.0
  • xdebug 2.2.5 → 2.2.7
  • PHP 5.2.17 is removed
@Nicofuma

This comment has been minimized.

@marc1706 marc1706 closed this Feb 13, 2015

@marc1706 marc1706 reopened this Feb 13, 2015

@Nicofuma Nicofuma closed this in b9db47e Mar 29, 2015

@Nicofuma

This comment has been minimized.

Copy link
Member

commented Mar 29, 2015

merged in b9db47e

@marc1706 marc1706 deleted the marc1706:ticket/13568 branch Mar 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.