Review Request 128477: Do not delete system relevant files in tests (if we might succeed)

[Martin Gräßlin]

> On July 18, 2016, 11:30 p.m., David Faure wrote:
> > Wow, it never occured to me that someone might run this test as root. I thought it was well known that development should not be done as root ;)
> > But I can see how it might happen when creating packages with unittests enabled or something.
> > 
> > The patch looks fine to me, not sure why you say "it probably still needs some more work" ?
> > 
> > Unfortunately I see no other way to test files owned by other users. But of course as long as this is tested once, the other tests could change permissions on a temp file to get into "missing permissions" error cases.
> 
> Tobias Berner wrote:
>     It needs more work, because the test in itself is still basically a russian roulette. My patch merely tries to break the fingers of the players before their turn's up.
>     
>     
>     I find it highly scary/nightmare inducing that there is a testcase that has as 'failure' a destroyed operating system. This cannot meet any quality standards I can think of.
>     And I really find it quite scary that you use the "don't run as root" excuse. Yes I grant you, that running tests with privileges may be wrong, but 
>     I enabled them in our package builder to give them a go -- and there you go...
>     However, if running tests as root is wrong, trying to rm /etc or /boot probably contradicts some Geneva convention.
>     
>     
>     I think the proper way would be to make the test to only touch files it itself creates & chowns. 
>     Or the test could only try to remember a hardcoded `/tmp/kio_test/file` and `/tmp/kio_test/dir` that have to be created before running the test 
>     by the developer for these tests. 
>     But it should _never_ opt to _well look at that nice system config file/dir, that is certainly owned by root, let's try to remove that_ .
>     
>     
>     What do you think would it be possible to reimplement the tests in that way? Or would that break the tests?
>     
>     
>     
>     [Please note, this is not an attack on you personally, but on the really scary stuff the tests do].
> 
> Martin Gräßlin wrote:
>     what about adding a check to our cmake configs to disallow running ctest as root? I doubt that kio is the only KDE software which has dangerous tests when running as root. I wouldn't trust my kwin tests to get executed as root (as they interact with hardware).
> 
> Tobias Berner wrote:
>     That would probably be a good first step. Can this be added somewhere top-level (ecm), or would it have to be added to every KDE project? 
>     I will gladly provide a patch for this.

I was thinking of ecm, though my cmake knowledge is not sufficient to properly know it. A candidate could be ecm-mark-as-test.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128477/#review97577
-----------------------------------------------------------


On July 18, 2016, 7:10 p.m., Tobias Berner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128477/
> -----------------------------------------------------------
> 
> (Updated July 18, 2016, 7:10 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> Some tests for kio try to move system relevant files&paths with the blind assumption that 
> the permissions to touch these files is not present. 
> The files are 
> - /etc/passwd
> - /etc/cups
> - /etc
> - /boot
> [sic!].
> 
> 
> Check that the process does not actually have the rights to touch system
> relevant files when running the
> - TestTrash::trashDirectoryOwnedByRoot
> - TestTrash::trashFileOwnedByRoot
> - JobTest::moveFileNoPermissions
> - JobTest::moveDirectoryNoPermissions
> tests -- and bail out of them if so.
> 
> 
> This patch probably still needs some more work [maybe I also missed another naughty test?],
> and I welcome every kind of input on it (apart from the straw man *don't run tests as root* ;) ).
> 
> 
> Diffs
> -----
> 
>   autotests/jobtest.cpp 579c507 
>   src/ioslaves/trash/tests/testtrash.cpp c71df13 
> 
> Diff: https://git.reviewboard.kde.org/r/128477/diff/
> 
> 
> Testing
> -------
> 
> Without patch:
> - enjoying two hours of restoring a system without /etc & /boot
> 
> With patch:
> - grep 'must not' Testing/Temporary/LastTest.log.tmp
>      SKIP   : TestTrash::trashFileOwnedByRoot() Test must not be run by root.
>      SKIP   : TestTrash::trashDirectoryOwnedByRoot() Test must not be run by root.
>      SKIP   : JobTest::moveFileNoPermissions() Test must not be run by root.
>      SKIP   : JobTest::moveDirectoryNoPermissions() Test must not be run by root.
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160720/60e5e4ef/attachment-0001.html>