D15721: Make lock on plasmavault icon visible with breeze-dark

[Noah Davis]

ndavis added a comment.


  16px
  F6281723: Screenshot_20180924_005740.png 

  
  22px
  F6281721: Screenshot_20180924_005657.png 


REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D15721

To: ndavis, #vdg
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15721: Make lock on plasmavault icon visible with breeze-dark

[Noah Davis]

ndavis created this revision.
ndavis added a reviewer: VDG.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ndavis requested review of this revision.

REVISION SUMMARY
  Set lock color on Breeze version to #eff0f1 (light2dark script compatibility) 
and the lock on the Breeze Dark version to #31363b.

REPOSITORY
  R266 Breeze Icons

BRANCH
  plasmavault-icon-fix (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15721

AFFECTED FILES
  icons-dark/apps/16/plasmavault.svg
  icons-dark/apps/22/plasmavault.svg
  icons/apps/16/plasmavault.svg
  icons/apps/22/plasmavault.svg

To: ndavis, #vdg
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15718: Do not index the path if the path has no execute permissions.

[James Smith]

smithjd added a comment.


  In D15718#330864 , @ngraham wrote:
  
  > Making files executable that don't need to be executable is a bad security 
habit. What if the contents get replaced with something malicious? Suddenly 
that now-malicious file has execute permissions.
  
  
  Replacing a file's contents requires write permissions on the file. I have 
plenty of executable shell scripts that aren't an immediate security risk, 
though I suppose if someone gained write privileges over my home statistically 
speaking a shell script is (currently) the most likely choice to gut and 
replace with malicious code. If an attacker already has write permission over 
your home you have bigger problems than a forgotten set-executable file in it 
somewhere anyway. The patched state of that machine's software packages 
dictates how devastating that payload was to your administrator, meanwhile your 
home has probably been wiped.
  
  > 
  > 
  >  ---
  > 
  > Conceptually, you are proposing that the rest of the world adapt to our 
software, rather than the other way around. That's simply not practical. Even 
if this were a good idea, the world will never adapt to us. We must adapt to 
the world. Our software does not exist in a perfect state of total control over 
the environment it inhabits; it exists to facilitate busy people with messy 
lives as they work to accomplish their tasks with a minimum of hassle. That 
goal is not enhanced by breaking KDE Plasma's search tool for them unless they 
give all of their files execute permissions.
  > 
  > Sorry, no go. We need to find a better way.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd, ngraham, #baloo
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D11880: Add firewall-config and firewall-applet icons

[Noah Davis]

ndavis updated this revision to Diff 42212.
ndavis added a comment.


  Change wall color of firewall-applet-panic to \#4d4d4d (breeze) and \#f2f2f2 
(breeze-dark)

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11880?vs=42119=42212

BRANCH
  firewalld-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11880

AFFECTED FILES
  icons-dark/apps/16/firewall-applet-error.svg
  icons-dark/apps/16/firewall-applet-panic.svg
  icons-dark/apps/16/firewall-applet.svg
  icons-dark/apps/22/firewall-applet-error.svg
  icons-dark/apps/22/firewall-applet-panic.svg
  icons-dark/apps/22/firewall-applet.svg
  icons-dark/apps/48/firewall-applet.svg
  icons-dark/apps/48/firewall-config.svg
  icons/apps/16/firewall-applet-error.svg
  icons/apps/16/firewall-applet-panic.svg
  icons/apps/16/firewall-applet.svg
  icons/apps/22/firewall-applet-error.svg
  icons/apps/22/firewall-applet-panic.svg
  icons/apps/22/firewall-applet.svg
  icons/apps/48/firewall-applet.svg
  icons/apps/48/firewall-config.svg

To: ndavis, #vdg, #breeze, ngraham
Cc: bruns, abetts, alex-l, svenmauch, kde-frameworks-devel, ngraham, michaelh

D11880: Add firewall-config and firewall-applet icons

[Noah Davis]

ndavis added a comment.


  In D11880#330861 , @ngraham wrote:
  
  > For the panic mode icon, how about leaving the wall itself black, and only 
the lock is orange?
  
  
  Like this? This is #4d4d4d (icon grey), the standard color for small breeze 
icons, not black.
  F6281673: Screenshot_20180924_000854.png 
F6281669: Screenshot_20180924_000627.png 

  
  > I don't want to totally dominate the conversation here, so I'll step aside 
for a bit and let others have their say. But I want to let you know that I 
really appreciate your patience here, @ndavis. I'm sorry this there's been so 
much back-and-forth and that the patch sat ignored for so long. We'll do better 
for your next one!
  
  Thank you! It's been great to see how other people come up with designs here.

REPOSITORY
  R266 Breeze Icons

BRANCH
  firewalld-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11880

To: ndavis, #vdg, #breeze, ngraham
Cc: bruns, abetts, alex-l, svenmauch, kde-frameworks-devel, ngraham, michaelh

D15718: Do not index the path if the path has no execute permissions.

[Nathaniel Graham]

ngraham added a comment.


  Making files executable that don't need to be executable is a bad security 
habit. What if the contents get replaced with something malicious? Suddenly 
that now-malicious file has execute permissions.
  
  ---
  
  Conceptually, you are proposing that the rest of the world adapt to our 
software, rather than the other way around. That's simply not practical. Even 
if this were a good idea, the world will never adapt to us. We must adapt to 
the world. Our software does not exist in a perfect state of total control over 
the environment it inhabits; it exists to facilitate busy people with messy 
lives as they work to accomplish their tasks with a minimum of hassle. That 
goal is not enhanced by breaking KDE Plasma's search tool for them unless they 
give all of their files execute permissions.
  
  Sorry, no go. We need to find a better way.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd, ngraham, #baloo
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D15718: Do not index the path if the path has no execute permissions.

[James Smith]

smithjd added a comment.


  In D15718#330845 , @ngraham wrote:
  
  > In D15718#330844 , @smithjd 
wrote:
  >
  > > In D15718#330836 , @ngraham 
wrote:
  > >
  > > > Wouldn't this have the effect of un-indexing most files? A quick check 
of my documents (text, word processing, excel, etc) reveals that none of them 
have the execute bit set. As-is, I think this would render Baloo mostly useless.
  > >
  > >
  > > A default mask of 0002 or more permissive looks fairly common across 
distros, and is permissive enough to index files by default.
  > >
  > > setfacl -d -m u::rwx ~ or umask 0022 will set default execute permissions 
on created files. You can set something less permissive on your downloads 
directory or plasma vault mount with setfacl -d u::rw or similar.
  > >
  > > chmod -R 755 ~ will recursively give every file in your home directory 
execute permisions.
  >
  >
  > You are proposing fundamentally breaking Baloo and then requiring that 
users or distros clean up our mess for us by making all their files 
executable--in the process reducing their own security. Sorry, but this is 
simply unacceptable.
  >
  > No part of this proposal makes any sense to me. We just can't do this.
  
  
  Folders with the execute bit off already won't allow their contents listed. 
How wouldn't this convention be useful extended to prevent a file from being 
listed in Baloo's database?
  
  How does setting files executable lead to reduced security? Some common 
folders may end up needing more restrictive defaults (downloads for instance) 
and the problem of potentially indexing sensitive information or feeding an 
extractor a specially crafted file designed to exploit a bug in it will have 
been mediated to some extent.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd, ngraham, #baloo
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D11880: Add firewall-config and firewall-applet icons

[Nathaniel Graham]

ngraham added a comment.


  For the panic mode icon, how about leaving the wall itself black, and only 
the lock is orange?
  
  I don't want to totally dominate the conversation here, so I'll step aside 
for a bit and let others have their say. But I want to let you know that I 
really appreciate your patience here, @ndavis. I'm sorry this there's been so 
much back-and-forth and that the patch sat ignored for so long. We'll do better 
for your next one!

REPOSITORY
  R266 Breeze Icons

BRANCH
  firewalld-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11880

To: ndavis, #vdg, #breeze, ngraham
Cc: bruns, abetts, alex-l, svenmauch, kde-frameworks-devel, ngraham, michaelh

D15718: Do not index the path if the path has no execute permissions.

[Nathaniel Graham]

ngraham added a comment.


  In D15718#330844 , @smithjd wrote:
  
  > In D15718#330836 , @ngraham 
wrote:
  >
  > > Wouldn't this have the effect of un-indexing most files? A quick check of 
my documents (text, word processing, excel, etc) reveals that none of them have 
the execute bit set. As-is, I think this would render Baloo mostly useless.
  >
  >
  > A default mask of 0002 or more permissive looks fairly common across 
distros, and is permissive enough to index files by default.
  >
  > setfacl -d -m u::rwx ~ or umask 0022 will set default execute permissions 
on created files. You can set something less permissive on your downloads 
directory or plasma vault mount with setfacl -d u::rw or similar.
  >
  > chmod -R 755 ~ will recursively give every file in your home directory 
execute permisions.
  
  
  You are proposing fundamentally breaking Baloo and then requiring that users 
or distros clean up our mess for us by making all their files executable--in 
the process reducing their own security. Sorry, but this is simply unacceptable.
  
  No part of this proposal makes any sense to me. We just can't do this.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd, ngraham, #baloo
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D15718: Do not index the path if the path has no execute permissions.

[James Smith]

smithjd added a comment.


  In D15718#330836 , @ngraham wrote:
  
  > Wouldn't this have the effect of un-indexing most files? A quick check of 
my documents (text, word processing, excel, etc) reveals that none of them have 
the execute bit set. As-is, I think this would render Baloo mostly useless.
  
  
  A default mask of 0002 or more permissive looks fairly common across distros, 
and is permissive enough to index files by default.
  
  setfacl -d -m u::rwx ~ or umask 0022 will set default execute permissions on 
created files. You can set something less permissive on your downloads 
directory or plasma vault mount with setfacl -d u::rw or similar.
  
  chmod -R 755 ~ will recursively give every file in your home directory 
execute permisions.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd, ngraham, #baloo
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D15583: [Balooctl] remove directory parent check

[Stefan Brüns]

bruns added a comment.


  In D15583#330799 , @smithjd wrote:
  
  > Technically, this check isn't actually needed, though it does prevent the 
user from entering a path more than once. At first glance this looks like it 
should work:
  >
  >   if (folder.startsWith(path))
  >
  >
  > This doesn't prevent the user from also re-specifying valid paths.
  
  
  Exact matches are already rejected in the block above.
  
  `folder.startsWith(path)` is also pointless, a user should be allowed to e.g. 
exclude `/foo/` if `/foo/bar/baz is excluded and `/foo/bar` is

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15583

To: ngraham, #baloo, #dolphin, bruns, #frameworks
Cc: smithjd, anthonyfieroni, kde-frameworks-devel, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D15718: Do not index the path if the path has no execute permissions.

[Nathaniel Graham]

ngraham requested changes to this revision.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd, ngraham, #baloo
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D15718: Do not index the path if the path has no execute permissions.

[Stefan Brüns]

bruns added a comment.


  This is completely wrong ...

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd, ngraham
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D15718: Do not index the path if the path has no execute permissions.

[Stefan Brüns]

bruns added a reviewer: Baloo.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd, ngraham, #baloo
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D15718: Do not index the path if the path has no execute permissions.

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  Wouldn't this have the effect of un-indexing most files? A quick check of my 
documents (text, word processing, excel, etc) reveals that none of them have 
the execute bit set. As-is, I think this would render Baloo mostly useless.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd, ngraham
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15718: Do not index the path if the path has no execute permissions.

[James Smith]

smithjd added a comment.


  Files/folders should not be automatically indexed if the execute bit is 
unset. Downloads from most if not all popular browsers are not executable by 
default.
  
  Setting a file unexecutable would be used to prevent automatic indexing of 
files and most importantly prevent immediate indexing of newly downloaded files 
and indexing of plasma vaults .
  
  Inspiration:
  
  https://phabricator.kde.org/T8066
  https://phabricator.kde.org/D8532

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15718

To: smithjd
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15718: Do not index the path if the path has no execute permissions.

[James Smith]

smithjd created this revision.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
smithjd requested review of this revision.

REVISION SUMMARY
  This will prevent automatic indexing of newly downloaded files and can be 
used to 
  prevent indexing of Plasma vaults.

REPOSITORY
  R293 Baloo

BRANCH
  master-donotindex (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15718

AFFECTED FILES
  src/file/fileindexerconfig.cpp
  src/file/filtereddiriterator.cpp
  src/file/modifiedfileindexer.cpp
  src/file/newfileindexer.cpp
  src/file/unindexedfileiterator.cpp
  src/file/xattrindexer.cpp

To: smithjd
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D11880: Add firewall-config and firewall-applet icons

[Noah Davis]

ndavis added a comment.


  Here's how the checkmark idea looks. I prefer the plain wall since it fits in 
with the other icons better.
  
  16px
  F6281345: firewall-applet(check)16.svg 
  
  22px
  F6281342: firewall-applet(check)22.svg 
  
  Here's another idea with a shield in the middle, similar to the 48px icon. I 
still prefer the plain wall.
  
  16px
  F6281346: firewall-applet(shield)16.svg 
  
  22px
  F6281347: firewall-applet(shield)22.svg 

REPOSITORY
  R266 Breeze Icons

BRANCH
  firewalld-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11880

To: ndavis, #vdg, #breeze, ngraham
Cc: bruns, abetts, alex-l, svenmauch, kde-frameworks-devel, ngraham, michaelh

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Andreas Krutzler]

akrutzler added a comment.


  Thanks for the review. I will address your comments and hopefully have a 
patch ready in the next days. :)

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D15277

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure
Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[James Smith]

smithjd added inline comments.

INLINE COMMENTS

> taglibwritertest.cpp:60
>  
> -QCOMPARE(extractedTitle, QStringLiteral("Title1"));
> -QCOMPARE(extractedArtist, QStringLiteral("Artist1"));
> -QCOMPARE(extractedAlbum, QStringLiteral("Album1"));
> +QCOMPARE(extractedTitle, QString(QStringLiteral("Title1") + 
> stringSuffix));
> +QCOMPARE(extractedArtist, QString(QStringLiteral("Artist1") + 
> stringSuffix));

Is wrapping in a QString necessary?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: smithjd, svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D11880: Add firewall-config and firewall-applet icons

[Noah Davis]

ndavis added a comment.


  In D11880#330788 , @ngraham wrote:
  
  > I like `firewall-config` and `firewall-applet-error` as they are.
  >
  > I think that `firewall-applet` looks maybe a bit too plain at its 22px 
size. The wall seems to need something.
  
  
  Maybe the emblem-checked icon should go in the lower right corner to match 
the style of the other two firewall-applet icons? I choose to make it fairly 
simple so that it would normally fit in with other icons in the system tray 
except for when there is an error or Panic Mode is enabled.
  
  > I like the lock on `firewall-applet-panic`. I'm not sure about the orange 
color though. Orange means "warning". If I'm understanding you correctly, 
`firewall-applet-panic` should evoke feelings of maximum safety.
  
  Yes, but I also agree with @svenmauch about the previous version being too 
positive. If a user enables panic mode by accident, how will they know 
something might be wrong with their settings? As it is, firewall-applet-panic 
is the firewall-applet icon with the emblem-locked icon in the corner and I've 
applied the color of emblem-locked to the wall. While orange may mean 
"warning", it could mean any kind of warning. In this case, it's a warning that 
an extreme setting is being used, but there is a lock in the corner to show 
that it is at least safe.

REPOSITORY
  R266 Breeze Icons

BRANCH
  firewalld-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11880

To: ndavis, #vdg, #breeze, ngraham
Cc: bruns, abetts, alex-l, svenmauch, kde-frameworks-devel, ngraham, michaelh

D15583: [Balooctl] remove directory parent check

[James Smith]

smithjd added a comment.


  Technically, this check isn't actually needed, though it does prevent the 
user from entering a path more than once. At first glance this looks like it 
should work:
  
if (folder.startsWith(path))
  
  This doesn't prevent the user from also re-specifying valid paths.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15583

To: ngraham, #baloo, #dolphin, bruns, #frameworks
Cc: smithjd, anthonyfieroni, kde-frameworks-devel, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D11880: Add firewall-config and firewall-applet icons

[Nathaniel Graham]

ngraham added a comment.


  For the benefit of others, here's what they look like now:
  
  22px:
  F6281196: 22px.png 
  
  48px:
  F6281197: 48px.png 
  
  I like `firewall-config` and `firewall-applet-error` as they are.
  
  I think that `firewall-applet` looks maybe a bit too plain at its 22px size. 
The wall seems to need something.
  
  I like the lock on `firewall-applet-panic`. I'm not sure about the orange 
color though. Orange means "warning". If I'm understanding you correctly, 
`firewall-applet-panic` should evoke feelings of maximum safety.

REPOSITORY
  R266 Breeze Icons

BRANCH
  firewalld-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11880

To: ndavis, #vdg, #breeze, ngraham
Cc: bruns, abetts, alex-l, svenmauch, kde-frameworks-devel, ngraham, michaelh

D8532: [WIP] Restrict file extractor with Seccomp

[Matthieu Gallien]

mgallien added a comment.


  In D8532#289500 , @davidk wrote:
  
  > I was asked in private about the current state of libseccomp integration 
and why there was no progress in a long time.
  >  The current state is, that I have implemented seccomp support in 
kfilemetadata using this API:
  >
  >   bool setProcessReadOnly(uint32_t defaultAction, 
std::vector addionalWhitelist)
  >
  >
  > But there are two blockers, related to external plugins:
  >
  > - External plugins based on interpreters like python/lua/perl etc. need a 
huge whitelist. This is problematic as I want to keep the list of allowed 
syscalls as small as possible (the list would be huge). Additionally, it would 
be difficult to get a list of all needed syscalls. Thus, we would break many 
external plugins.
  > - Baloo is basically unmaintained. Thus, if something breaks, fixing it 
should be as easy as possible. But what if QT requires a new syscall and thus, 
the tests (and deployments) are failing? We need a way to know which syscall 
failed. This works for kfilemetadata plugins, but not for external plugins 
(because they are separate processes). The only way I can image, would be 
running the whole test with strace.
  >
  >   So, if anyone is willing to continue this work, I would be happy to share 
my current state. Otherwise, if everyone agrees that we don't care about 
external plugins (users of external plugins can disable Seccomp support with an 
environment variable), I can finish the patches.
  
  
  Sorry for my late reply
  The external extractors tests of KFileMetaData have always failed and nobody 
ever fixed them. This makes me think that they are not really maintained.
  Is there any use of them apart from the unit tests ?

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D8532

To: davidk, apol, ossi, #frameworks, smithjd, bruns
Cc: mgallien, kde-frameworks-devel, michaelh, #baloo, detlefe, ngraham, 
nicolasfella, ashaposhnikov, astippich, spoorun, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> svuorela wrote in taglibwritertest.cpp:66
> yeah. given you write and read it, if somehow it gets encoded e.g. as 
> iso-8859-15 rather than utf8, the euro sign would be encoded as 0xa4 rather 
> than 0x20ac.
> 
> As you write and read it in the same sequence, there is a possibiliyt for 
> this to pass when it shouldn't.
> 
> Unfortunately roundtripping the files with bad editors can make this happen. 
> Especially on windows.

This should be covered by the read tests. The read tests use binary artifacts.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D8532: [WIP] Restrict file extractor with Seccomp

[Nathaniel Graham]

ngraham added reviewers: Frameworks, smithjd, bruns.
ngraham added a comment.


  +1 for something rather than nothing. No comment on the technical aspect, but 
I'm adding more reviewers who can hopefully help un-wedge this patch.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D8532

To: davidk, apol, ossi, #frameworks, smithjd, bruns
Cc: kde-frameworks-devel, michaelh, #baloo, detlefe, ngraham, nicolasfella, 
ashaposhnikov, astippich, spoorun, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> astippich wrote in taglibwritertest.cpp:66
> To be sure I understand correctly, using stringSuffix.toUTF8() is what you 
> would like to see here?

Something along those lines, yes please, to compare with.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15715: remove own implementation of QString to TString conversion for taglibwriter

[Sune Vuorela]

svuorela added a comment.


  In D15715#330743 , @astippich 
wrote:
  
  > since I'm still unsure about those things: the q2t function was not 
declared static, but was never exported. It is still safe to remove, right?
  
  
  Yes. not exported (and not declared in a header file). Safe.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15715

To: astippich, mgallien, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15220: implement more basic tags for taglibwriter

[Alexander Stippich]

astippich added a dependent revision: D15715: remove own implementation of 
QString to TString conversion for taglibwriter.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15220

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15715: remove own implementation of QString to TString conversion for taglibwriter

[Alexander Stippich]

astippich added a dependency: D15220: implement more basic tags for 
taglibwriter.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15715

To: astippich, mgallien, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15715: remove own implementation of QString to TString conversion for taglibwriter

[Alexander Stippich]

astippich added a comment.


  since I'm still unsure about those things: the q2t function was not declared 
static, but was never exported. It is still safe to remove, right?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15715

To: astippich, mgallien, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15715: remove own implementation of QString to TString conversion for taglibwriter

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns, svuorela.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  similar to D15614 , remove unsafe 
implementaion of own string conversion function

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_write_string

REVISION DETAIL
  https://phabricator.kde.org/D15715

AFFECTED FILES
  src/writers/taglibwriter.cpp

To: astippich, mgallien, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> svuorela wrote in taglibwritertest.cpp:66
> yeah. given you write and read it, if somehow it gets encoded e.g. as 
> iso-8859-15 rather than utf8, the euro sign would be encoded as 0xa4 rather 
> than 0x20ac.
> 
> As you write and read it in the same sequence, there is a possibiliyt for 
> this to pass when it shouldn't.
> 
> Unfortunately roundtripping the files with bad editors can make this happen. 
> Especially on windows.

To be sure I understand correctly, using stringSuffix.toUTF8() is what you 
would like to see here?

> bruns wrote in taglibwritertest.cpp:75
> My idea here was to check each format twice, once with a simple latin1/ascii 
> string (stringsuffix = "") and a second time with some unicode chars (e.g. 
> "€", probably some more from other code blocks).
> 
> This allows to differentiate if only unicode tags are broken.

That makes more sense of course, will update later to a unicode and a none 
unicode test

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15614: remove usage of own TString to QString conversion function

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:4c760d4588f3: remove usage of own TString to QString 
conversion function (authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15614?vs=41964=42200

REVISION DETAIL
  https://phabricator.kde.org/D15614

AFFECTED FILES
  src/extractors/taglibextractor.cpp

To: astippich, bruns, mgallien, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15615: bump required taglib version to 1.11.1

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:26dcd784b45a: bump required taglib version to 1.11.1 
(authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15615?vs=41969=42199

REVISION DETAIL
  https://phabricator.kde.org/D15615

AFFECTED FILES
  CMakeLists.txt

To: astippich, mgallien, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15615: bump required taglib version to 1.11.1

[Alexander Stippich]

astippich retitled this revision from "bump required taglib version" to "bump 
required taglib version to 1.11.1".

REPOSITORY
  R286 KFileMetaData

BRANCH
  bumb_taglib_version

REVISION DETAIL
  https://phabricator.kde.org/D15615

To: astippich, mgallien, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwritertest.cpp:66
> ?
> The file/tags are written inside this test.

yeah. given you write and read it, if somehow it gets encoded e.g. as 
iso-8859-15 rather than utf8, the euro sign would be encoded as 0xa4 rather 
than 0x20ac.

As you write and read it in the same sequence, there is a possibiliyt for this 
to pass when it shouldn't.

Unfortunately roundtripping the files with bad editors can make this happen. 
Especially on windows.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> svuorela wrote in taglibwritertest.cpp:66
> I suggest checking that the last bytes of all these extracted things is the 
> actual actual utf8 bytes, so that if someone compiles or saves this file in a 
> broken encoding that the testing doesn't fail.

?
The file/tags are written inside this test.

> svuorela wrote in taglibwritertest.cpp:75
> Given stringSuffix is the same for all tests, is it needed to have as a test 
> data thing ?

My idea here was to check each format twice, once with a simple latin1/ascii 
string (stringsuffix = "") and a second time with some unicode chars (e.g. "€", 
probably some more from other code blocks).

This allows to differentiate if only unicode tags are broken.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> taglibwritertest.cpp:66
> +QCOMPARE(extractedGenre, QString(QStringLiteral("Genre1") + 
> stringSuffix));
> +QCOMPARE(extractedComment, QString(QStringLiteral("Comment1") + 
> stringSuffix));
>  

I suggest checking that the last bytes of all these extracted things is the 
actual actual utf8 bytes, so that if someone compiles or saves this file in a 
broken encoding that the testing doesn't fail.

> taglibwritertest.cpp:75
>  QTest::addColumn("mimeType");
> +QTest::addColumn("stringSuffix");
>  

Given stringSuffix is the same for all tests, is it needed to have as a test 
data thing ?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15704: increase test coverage of taglibwriter

[Sune Vuorela]

svuorela added a comment.


  Looks good to me. A nice and simple way to drastically increase test coverage.

INLINE COMMENTS

> taglibwriter.cpp:22
>  {
>  QStringList types = {
>  QStringLiteral("audio/mpeg"),

Unrelated. but consider making this static ?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15704

To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  append a unicode character to all test string such that unicode characters 
can be tests

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_write_unicode

REVISION DETAIL
  https://phabricator.kde.org/D15714

AFFECTED FILES
  autotests/taglibwritertest.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Alexander Stippich]

astippich added a dependency: D15704: increase test coverage of taglibwriter.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15714

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15704: increase test coverage of taglibwriter

[Alexander Stippich]

astippich added a dependent revision: D15714: add a string suffix to test data 
and use for unicode testing of taglibwriter.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15704

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

KDE CI: Frameworks » kio » kf5-qt5 WindowsMSVCQt5.11 - Build # 28 - Still unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20WindowsMSVCQt5.11/28/
 Project:
kf5-qt5 WindowsMSVCQt5.11
 Date of build:
Sun, 23 Sep 2018 16:17:31 +
 Build duration:
1 hr 23 min and counting
   JUnit Tests
  Name: (root) Failed: 25 test(s), Passed: 30 test(s), Skipped: 0 test(s), Total: 55 test(s)Failed: TestSuite.kiocore-deletejobtestFailed: TestSuite.kiocore-jobtestFailed: TestSuite.kiocore-kfileitemtestFailed: TestSuite.kiocore-kmountpointtestFailed: TestSuite.kiocore-ktcpsockettestFailed: TestSuite.kiocore-mkpathjobtestFailed: TestSuite.kiofilewidgets-kfilecopytomenutestFailed: TestSuite.kiofilewidgets-kfilecustomdialogtestFailed: TestSuite.kiofilewidgets-kfileplacesmodeltestFailed: TestSuite.kiofilewidgets-kfileplacesviewtestFailed: TestSuite.kiofilewidgets-kfilewidgettestFailed: TestSuite.kiofilewidgets-knewfilemenutestFailed: TestSuite.kiofilewidgets-kurlnavigatortestFailed: TestSuite.kiowidgets-accessmanagertestFailed: TestSuite.kiowidgets-accessmanagertest-qnamFailed: TestSuite.kiowidgets-dropjobtestFailed: TestSuite.kiowidgets-fileundomanagertestFailed: TestSuite.kiowidgets-kdirlistertestFailed: TestSuite.kiowidgets-kdirmodeltestFailed: TestSuite.kiowidgets-kdynamicjobtrackernowidgetstestFailed: TestSuite.kiowidgets-krununittestFailed: TestSuite.kiowidgets-kurifiltertestFailed: TestSuite.kiowidgets-kurlcompletiontestFailed: TestSuite.kiowidgets-kurlcompletiontest-nowaitFailed: TestSuite.kpasswdservertest

D15704: increase test coverage of taglibwriter

[Alexander Stippich]

astippich updated this revision to Diff 42196.
astippich added a comment.


  - don't change test strings for now

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15704?vs=42174=42196

BRANCH
  taglib_write_mimetypes

REVISION DETAIL
  https://phabricator.kde.org/D15704

AFFECTED FILES
  autotests/taglibwritertest.cpp
  autotests/taglibwritertest.h
  src/writers/taglibwriter.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15697: Fix deletion of files from DAV

[Daniel Vrátil]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:7615e3405c30: Fix deletion of files from DAV (authored by 
dvratil).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15697?vs=42175=42192

REVISION DETAIL
  https://phabricator.kde.org/D15697

AFFECTED FILES
  src/ioslaves/http/http.cpp

To: dvratil, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14530: Fix crash when save a QImage to the eps format file

[Friedrich W. H. Kossebau]

kossebau added a comment.


  @zccrs Hi. Does D15405  fix things for 
you, so is this patch here no longer needed? If so, please close this review 
request by selecting and submitting the action "Abandon Revision", so it does 
no longer appear in our "Please review" list :)

REPOSITORY
  R287 KImageFormats

REVISION DETAIL
  https://phabricator.kde.org/D14530

To: zccrs, mlaurent, kossebau, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15704: increase test coverage of taglibwriter

[Stefan Brüns]

bruns added a comment.


  In general this looks good, but I would like two changes:
  
  1. Do the conversion to QTest first, and leave out the change for unicode 
testing (e.g. `Title1` -> `Title €`)
  2. Add a third column like  "stringsuffix", and then add another test (row) 
for each format. `QStringLiteral("Title1")` then becomes 
`QStringLiteral("Title1") + stringsuffix`
  
  (2.) would go in a dependent review.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15704

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15705: Also raise configuration window when reusing it

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, mart.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
broulik requested review of this revision.

REVISION SUMMARY
  It seems `requestActivate` only implies unminimizing but not raising if 
already visible

TEST PLAN
  - Opened applet config, buried it underneath some windows, opened same applet 
config again. Now it would raise itself to the top
  - Same for wallpaper config
  - Verified that when it is minimized it still unminimizes fine

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D15705

AFFECTED FILES
  src/plasmaquick/containmentview.cpp
  src/plasmaquick/view.cpp

To: broulik, #plasma, mart
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15697: Fix deletion of files from DAV

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15697

To: dvratil, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15697: Fix deletion of files from DAV

[Daniel Vrátil]

dvratil edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15697

To: dvratil, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15697: Fix deletion of files from DAV

[Daniel Vrátil]

dvratil updated this revision to Diff 42175.
dvratil added a comment.


  Improve commit message
  
  I thought you were referring to rmdir in the kio slave, but now I
  realize rmdir() is KIO API, not slave API.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15697?vs=42151=42175

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15697

AFFECTED FILES
  src/ioslaves/http/http.cpp

To: dvratil, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15220: implement more basic tags for taglibwriter

[Alexander Stippich]

astippich added a dependent revision: D15704: increase test coverage of 
taglibwriter.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15220

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15704: increase test coverage of taglibwriter

[Alexander Stippich]

astippich added a dependency: D15220: implement more basic tags for 
taglibwriter.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15704

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15704: increase test coverage of taglibwriter

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  increase the test coverage by testing more mimetypes, and use some unicode 
characters

TEST PLAN
  taglibwriter test pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_write_mimetypes

REVISION DETAIL
  https://phabricator.kde.org/D15704

AFFECTED FILES
  autotests/taglibwritertest.cpp
  autotests/taglibwritertest.h
  src/writers/taglibwriter.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15697: Fix deletion of files from DAV

[David Faure]

dfaure added a comment.


  Patch looks ok but I'm surprised by the commit log. Doesn't this method also 
work for directories, even after the patch?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15697

To: dvratil, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15611: [KCollapsibleGroupBox] Respect style's widget animation duration

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D15611

To: cfeck, #frameworks, ngraham, elvisangelaccio
Cc: broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15645: [WIP] Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu updated this revision to Diff 42166.
amhndu marked 4 inline comments as done.
amhndu added a comment.


  Updated with styling suggestions.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15645?vs=42108=42166

BRANCH
  system-default

REVISION DETAIL
  https://phabricator.kde.org/D15645

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h
  src/kcolorschememanager_p.h
  tests/kcolorschemedemo.cpp

To: amhndu, #frameworks
Cc: shubham, ngraham, broulik, kde-frameworks-devel, michaelh, bruns