> On July 18, 2016, 9: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. > > Martin Gräßlin wrote: > I was thinking of ecm, though my cmake knowledge is not sufficient to > properly know it. A candidate could be ecm-mark-as-test.
I modified jobtest to create its own "unreadable" file or dir (asking for people to create something before running automated tests is certainly not possible, we need a fully automated solution, for continuous integration and ease of use). Similar contributions to the kio_trash unittest are welcome. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128477/#review97577 ----------------------------------------------------------- On July 23, 2016, 7:23 a.m., Tobias Berner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128477/ > ----------------------------------------------------------- > > (Updated July 23, 2016, 7:23 a.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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
