Remove paste into folder functionality, Check for the existence of a file or directory when pasting #457

jelly · 2024-05-16T12:31:38Z

When copy pasting pass the --archive flag to cp so permissions are retained. This also allows us to enable superuser try as permissions are now retained.

Related: #435

jelly · 2024-05-21T10:19:26Z

Let's add --interactive as well and .input() and fail if there is a conflict as we have no conflict handling.

jelly · 2024-05-23T08:04:53Z

conclusion from Matrix discussion with @allisonkarlitskaya , drop paste into folder and detect with fsinfo if we try to paste into an existing file or dir and then refuse.

martinpitt

Sorry, but I put a very hard "no" on this. This has "exploit!" written all over it in big bold letters. The status quo is much preferable IMHO.

We keep running into these permission problems -- perhaps in version 1 we should restrict ourselves to files/dirs owned by the user (home dir) and run everything without superuser, or files/dirs owned by root and then do all copies as root. The rest would be a "sorry, not currently supported" thing.

martinpitt · 2024-05-28T11:42:16Z

src/fileActions.jsx

+                    if (err.message.match(/cp: overwrite .*?/)) {
+                        addAlert(_("Paste failed, not overwriting files"), "danger", new Date().getTime());


Hmm, that's quite hackish, and the error message doesn't contain the conflicting file, so for large directories it may be hard to see what's wrong? If you parse error messages anyway, perhaps capture the .* in group and show it as "not overwriting $0"?

Not a blocker, but this feels like "we really should fix this" material.

This no longer exists? I am not sure where you found it?

Sorry, my first review round only applied to the first commit, and I forgot to review the other commits. But let's sort out the central superuser question first.

martinpitt · 2024-05-28T11:43:18Z

test/check-application

-            runuser -u admin echo "test_text" > /home/admin/newfile
+            runuser -u admin touch /home/admin/newfile
+            echo "test_text" > /home/admin/newfile


What does that change? If the file already exists, then echoing as root should not change the permissions. And if you do expect that, why touch it first?

Good point, I should just echo as runuser...

martinpitt · 2024-05-28T11:45:34Z

test/check-application

+        b.wait_visible("[data-item='copyDir']")
+        b.click("[data-item='copyDir']")
+        b.wait_text("#description-list-owner dd", "admin")
+        b.wait_text("#description-list-group dd", "users")


I don't understand this. The hunk above changes the copy source from 'newfile' to 'copyDir' -- but here you again loko at the copy source, not the pasted version in newdir? (This is a bit hard to read and could use some comment)

martinpitt · 2024-05-28T11:46:16Z

test/check-application

+        b.mouse("[data-item='newfile']", "contextmenu")
+        b.click(".contextMenu button:contains('Copy')")
+
+        b.click(".breadcrumb-button:nth-of-type(4)")


Magic numbers -- could use comments into which directory that changes.

martinpitt · 2024-05-28T11:48:02Z

test/check-application

+        b.wait_visible("[data-item='newfile']")
+        b.wait_visible("[data-item='newfile2']")


The point of this PR is to retain permissions, so this could check that the testdir/newfile* copies are still owned by admin:admin?

martinpitt · 2024-05-28T11:56:30Z

src/fileActions.jsx

            ...sourcePath,
            targetPath
-        ]).catch(err => addAlert(err.message, "danger", new Date().getTime()));
+        ], { superuser: "try", environ: ["LC_ALL=C"] }).input()


Somehow GH ate my most important comment at all: This is a major security hole. If you copy a dir from e.g. /home/joe to /home/jane, or /tmp/download/ into /var/lib/postgresql, you give the source user a foot into the door of the target user. The CLI default is quite deliberately to copy files as root, to avoid this.

And how about mv /home/joe/x /home/jane/x?

What I mean to say is:

The CLI default is quite deliberately to copy files as root, to avoid this.

"citation needed".

I know you're very much allergic to this "different owner than parent directory" thing but other very common coreutils commands do exactly what you're suggesting is so evil, by default.

That creates the same problem of course, but indeed unfortunately there's no warning. Of course on the CLI you, shouldn't do that, but the CLI gives you all sorts of ways to shoot yourself into the foot.

Also, pasting something feels more like a clipboard, and it's not at all obvious during that operation what the source/target owner will be. mv /home/joe /home/jane is at least very explicit.

martinpitt · 2024-05-28T12:01:00Z

we should restrict ourselves to files/dirs owned by the user (home dir) and run everything without superuser, or files/dirs owned by root and then do all copies as root.

I think we can generalize this: We only support copying files (or copy/paste etc.) if the source and target directories have the same owner. Then superuser and --archive should be safe.

jelly · 2024-05-28T12:09:40Z

we should restrict ourselves to files/dirs owned by the user (home dir) and run everything without superuser, or files/dirs owned by root and then do all copies as root.

I think we can generalize this: We only support copying files (or copy/paste etc.) if the source and target directories have the same owner. Then superuser and --archive should be safe.

The goal of this PR is to get something releasable but we keep getting stuck on discussions around permissions. I am not saying they are important but I feel that we are missing the big picture here.

So this whole PR aims to:

Disallow paste into folder, so we don't have to deal with conflict files when pasting
Error out when pasting and the directory name already exists (This is really not a great UI but we can improve on it)
When copying retain permissions, because you for sure want to keep mode,ownership,timestamps properties.

So I am fine with dropping superuser: try and putting that on the $things to be figured out with permissions. As we did the same for upload.

martinpitt · 2024-05-28T12:27:33Z

So this whole PR aims to:

Disallow paste into folder, so we don't have to deal with conflict files when pasting

Where else would you paste stuff if not a folder (directory)? Do you mean copying folders and pasting them? I.e. you only want to support copy&paste for individual files?

Error out when pasting and the directory name already exists (This is really not a great UI but we can improve on it)

Big 👍

When copying retain permissions, because you for sure want to keep mode,ownership,timestamps properties.

I for sure don't want that 😁 Keeping timestamps is also rather dubious -- I didn't think of that in my review, and I don't want it either, but I'd let that slide.

jelly · 2024-05-28T12:31:19Z

So this whole PR aims to:

Disallow paste into folder, so we don't have to deal with conflict files when pasting

Where else would you paste stuff if not a folder (directory)? Do you mean copying folders and pasting them? I.e. you only want to support copy&paste for individual files?

It does support that.

Error out when pasting and the directory name already exists (This is really not a great UI but we can improve on it)

Big 👍

When copying retain permissions, because you for sure want to keep mode,ownership,timestamps properties.

I for sure don't want that 😁 Keeping timestamps is also rather dubious -- I didn't think of that in my review, and I don't want it either, but I'd let that slide.

Uhhh, the more I read the more I want to nuke this feature.

martinpitt · 2024-05-28T12:37:51Z

src/fileActions.jsx

        cockpit.spawn([
            "cp",
-            "-R",
-            ...sourcePath,
+            "--archive",


This includes file permissions, which in particular means that it will happily copy suid/sgid binaries or world-writable directories. This also is not desirable.

That no longer happens as superuser so is it ok then? Otherwise hmm we need to scrap the whole functionality.

Yes, without superuser it seems fine to me.

I have created #467 for most of the mentioned / issues with copy / pasting.

This is brittle when we add new menu entries or change the order.

We want to disallow pasting into a folder until we have a good way to detect if the folder contains something we are pasting into it. The plan is to in the future handle conflicts when pasting, then we will revise the paste into folder situation.

jelly · 2024-05-29T10:20:43Z

@martinpitt I've reworked the whole PR, so now the code and test changes are together and hopefully this addresses all your comments.

martinpitt

Thanks! This is by and large ok now, and if you can't see it any more I'll change to +1.

src/fileActions.jsx

test/check-application

martinpitt · 2024-05-29T10:46:25Z

src/fileActions.jsx

    const currentPath = path.join("/") + "/";
    const menuItems = [];

    const spawnPaste = (sourcePaths, targetPath) => {
+        if (sourcePaths.some(sourcePath => cwdInfo?.entries[basename(sourcePath)])) {
+            addAlert(_("Paste not overwriting existing files"), "danger", "paste-error");


Not the nicest English, and for many files it'd be good to give a hint -- perhaps "$0 exists, not overwriting with paste". Garrett may have a better idea.

Reworked too:

For now we don't have conflict resolution implemented yet for copy and paste so disallow pasting when any of the targets exist.

cockpituous · 2024-05-29T14:06:35Z

src/common.ts

+    if (elements.length === 0) {
+        return '/';


These 2 added lines are not executed by any test.

martinpitt

Dankjewel!

jelly requested review from mvollmer and allisonkarlitskaya May 16, 2024 12:31

jelly force-pushed the cp-as-admin branch from f8327ff to 03d273b Compare May 21, 2024 16:35

jelly mentioned this pull request May 22, 2024

Copy, cut & paste issues #467

Open

jelly force-pushed the cp-as-admin branch from fbfcefc to 5aa8049 Compare May 23, 2024 14:36

jelly requested a review from martinpitt May 28, 2024 11:19

martinpitt requested changes May 28, 2024

View reviewed changes

martinpitt reviewed May 28, 2024

View reviewed changes

jelly force-pushed the cp-as-admin branch from 5aa8049 to 22d8b37 Compare May 28, 2024 12:18

spawnPaste takes multiple sources so plural

4ca3b5a

martinpitt reviewed May 28, 2024

View reviewed changes

jelly added 2 commits May 29, 2024 11:41

test: don't hardcode contextmenu menu offsets

01dee86

This is brittle when we add new menu entries or change the order.

Remove paste into folder functionality

76ec022

We want to disallow pasting into a folder until we have a good way to detect if the folder contains something we are pasting into it. The plan is to in the future handle conflicts when pasting, then we will revise the paste into folder situation.

jelly force-pushed the cp-as-admin branch from 22d8b37 to d617825 Compare May 29, 2024 10:06

jelly requested a review from martinpitt May 29, 2024 10:20

jelly force-pushed the cp-as-admin branch from d717557 to d617825 Compare May 29, 2024 10:29

martinpitt changed the title ~~Retain permissions when copy/pasting~~ Remove paste into folder functionality, Check for the existence of a file or directory when pasting May 29, 2024

martinpitt requested changes May 29, 2024

View reviewed changes

Check for the existence of a file or directory when pasting

903e0e8

For now we don't have conflict resolution implemented yet for copy and paste so disallow pasting when any of the targets exist.

jelly force-pushed the cp-as-admin branch from d617825 to 903e0e8 Compare May 29, 2024 13:57

cockpituous reviewed May 29, 2024

View reviewed changes

jelly requested a review from martinpitt June 3, 2024 08:02

martinpitt approved these changes Jun 3, 2024

View reviewed changes

martinpitt merged commit 95b76dd into cockpit-project:main Jun 3, 2024
12 checks passed

jelly deleted the cp-as-admin branch June 3, 2024 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove paste into folder functionality, Check for the existence of a file or directory when pasting #457

Remove paste into folder functionality, Check for the existence of a file or directory when pasting #457

jelly commented May 16, 2024

jelly commented May 21, 2024

jelly commented May 23, 2024

martinpitt left a comment

martinpitt May 28, 2024

jelly May 28, 2024

martinpitt May 28, 2024

martinpitt May 28, 2024

jelly May 28, 2024

martinpitt May 28, 2024

martinpitt May 28, 2024

martinpitt May 28, 2024

martinpitt May 28, 2024

allisonkarlitskaya May 28, 2024

allisonkarlitskaya May 28, 2024

martinpitt May 28, 2024

martinpitt May 28, 2024

martinpitt commented May 28, 2024

jelly commented May 28, 2024 •

edited

martinpitt commented May 28, 2024

jelly commented May 28, 2024

martinpitt May 28, 2024

jelly May 28, 2024

martinpitt May 28, 2024

jelly May 28, 2024

jelly May 28, 2024

jelly commented May 29, 2024

martinpitt left a comment

martinpitt May 29, 2024

jelly May 29, 2024

cockpituous May 29, 2024

martinpitt left a comment

		if (err.message.match(/cp: overwrite .*?/)) {
		addAlert(_("Paste failed, not overwriting files"), "danger", new Date().getTime());

		b.wait_visible("[data-item='newfile']")
		b.wait_visible("[data-item='newfile2']")

Remove paste into folder functionality, Check for the existence of a file or directory when pasting #457

Remove paste into folder functionality, Check for the existence of a file or directory when pasting #457

Conversation

jelly commented May 16, 2024

jelly commented May 21, 2024

jelly commented May 23, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented May 28, 2024

jelly commented May 28, 2024 • edited

martinpitt commented May 28, 2024

jelly commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented May 29, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

jelly commented May 28, 2024 •

edited