Remove confirmation overlay from screenshotFullscreen
AbandonedPublic

Authored by davidre on Jun 23 2019, 11:20 AM.

Details

Reviewers
graesslin
Group Reviewers
KWin
Summary

It only appears for the method with the filedescriptor argument and not for the
other argument variations. Also here the user doesn't need to select a Screen
like in screenshotScreen as we always take a screenshot of all screens.

Test Plan

Overlay doesn't appear when you call the Method

Diff Detail

Repository
R108 KWin
Branch
screenshto (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 13208
Build 13226: arc lint + arc unit
davidre created this revision.Jun 23 2019, 11:20 AM
Restricted Application added a project: KWin. · View Herald TranscriptJun 23 2019, 11:20 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidre requested review of this revision.Jun 23 2019, 11:20 AM

+1 for doing it.
Haven't tested the code tough

zzag added a subscriber: zzag.

It seems like that's intentional

This allows to pick a point on the screen which we need to screenshot the screen under the mouse cursor and in future for color picking.

f9f7b84cb4361522c8fef7a32187df1751bc0860

In D22033#488123, @zzag wrote:

It seems like that's intentional

This allows to pick a point on the screen which we need to screenshot the screen under the mouse cursor and in future for color picking.

f9f7b84cb4361522c8fef7a32187df1751bc0860

The point is if it makes sense for the full screen method.
If we just want to screenshot one screen, sure the user can select the screen he wants to screenshot. For fullscreen all screens are captured

graesslin resigned from this revision.Jul 1 2019, 9:32 AM

The interaction is a security feature. It makes it impossible to just screenshot and leak the information. Please keep this in place. Being able to screenshot was one of the big unfixable security issues on X11. Let's please not remove this. Also having a screenshot api which does not enforce user interaction provides a poor man screencast API. The explicit user interaction makes it impossible that someone tries to screencast through this way.

Overall this was very much intended and not a bug or something like that.

As I'm currently not active I won't veto nor ack this change. I strongly recommend to not change it without addressing the security aspects in some other way.

But there are ways to circumvent this. For example if I were a malicious program I could just call the variant that doesn't take a file descriptor and saves to /tmp and then read the file from there.

romangg added a subscriber: romangg.Jul 1 2019, 9:45 AM

Then the correct patch is to fix that security hole.

I agree with Martin that user should be made aware of applications trying to screenshot the screen. If we would have already a nice permission system in place user would be informed which application it was and could set to not be asked again for this particular applications. Also the question is if such ui interfaces belong into KWin or if it should proxy a request to a permission-manager which then does the ui.

But these are issues not being solved short-term.

davidre abandoned this revision.Jul 1 2019, 9:46 AM

I understand and accept your points

But there are ways to circumvent this. For example if I were a malicious program I could just call the variant that doesn't take a file descriptor and saves to /tmp and then read the file from there.

Well it at least shows a notification. This makes stealth mode impossible. And IIRC this method isn't supported on Wayland where the stricter security matters.

Anyway I agree that having to click sometimes sucks and makes some pattern impossible like screenshot with an open menu. That's a problem. A possible solution would be to take the screenshot and afterwards present it from KWin to the user and ask for confirmation to send to another application. This could in future be exchanged with a permission model as Roman mentions (which I am very much supportive of).

And IIRC this method isn't supported on Wayland where the stricter security matters.

I can't see anything stopping it and screenshotting worked fine when I tested it just now.