Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add client symbolic link dereferencing (readlink support for linux) #3335

Closed
basos9 opened this issue May 18, 2021 · 5 comments
Closed

Add client symbolic link dereferencing (readlink support for linux) #3335

basos9 opened this issue May 18, 2021 · 5 comments
Labels
enhancement enhancement of a already implemented feature/code

Comments

@basos9
Copy link

basos9 commented May 18, 2021

Add support for client symbolic link de referencing.

REFERENCES


Here follows a use case description of an upload and download scenario.

UPLOAD

When the client (uploader) encounters a symlink (in a platform that supports this and also the platform client support this), the client would sync the contents.

  • If it is a file symlink sync the file
  • If it is directory symlink sync the contents of the directory.
  • If we do not support it, we print a nice exclamation mark and ignore this (present behavior)
  • If the link is broken, we print a nice exclamation mark (error level) and temporarily ignore this resource

For the sake of example lest say that I create the directory dir-LNK (witch points to /data/dir). The client will sync a directory named dir-LNK (without anything on the server to denote this is/was a symlink)

DOWNLOAD

This was the first client (uploader lets say) scenario. Now, when a new client (downloader) registers to sync with this folder, a normal sync should occur, agnostic of symlinks. More specifically, there are two possibilities:

  • an empty root, do the syncing, without any symlink knowledge,
    create directory named dir-LNK inside the nextcloud root.
  • an existing root with symlink setup manually.
    I create a symlink in the nextcloud root dir-LNK which points to /externalstorage/dir
    When sync begins, just read the conents for dir-LNK and do the sync (i.e. do not treat symlink specifically
    • If the link is broken, we print a nice exclamation mark (error level) and temporarily ignore this resource

So in general, we do not treat symlinks specifically when they exist, except for the case when the link is broken, when we print an error and ignore the resource. This means that there is no need to ever write a symlink. In other words all the symlink hassle is a client issue. The server is agnostic of symlink.

@basos9 basos9 added the enhancement enhancement of a already implemented feature/code label May 18, 2021
@RokeJulianLockhart
Copy link

Wouldn't this mean an awful lot of server-side duplication and lost organization for those of us who use symlinks instead of .desktop or .lnk files? This needs to be optional.

@basos9
Copy link
Author

basos9 commented Oct 23, 2023

Yes, absolutely. Copying refs from the discussion.

Summing up.
Client dereferencing should handle

  • Possible security issues for linking sensitive data outside of the sync root.
  • Cyclic dependency detection
  • Performance of links to external volumes
  • Various corner cases, which increase if both sym link as a ref (type A below) and symlink deref (type B below) are implemented.

As it turns, Syncing as a reference might be more useful

  • since multiple sync folders parially solves the dereferencing use schenarios in a more clean way (and it is implemented)
  • sync as a ref is something that specially linux users might expect for (since the may be more familiar with symlinks and activelly use them for organizing tasks.
  • we will avoid many complexities and security issues.

So I am closing this to focus on #250

https://help.nextcloud.com/t/symbolic-link-support/220/25

The point is that there is a fundamental difference between two scenarios: one where links are never dereferenced (aka followed) by either the client or server of nextcloud, which is inherently secure, and one where they need to be dereferenced (followed, which I understand is your use case), which can be troublesome in terms of security, resource usage and timelessness of sync and needs thus to be handled with more care.

To mention a few potential issues:

How if, deep in your data, you end up having a symbolic link to a place with private sensitive stuff of yours? You may get that because your data is music and someone, maliciously, gives you a directory of mp3 files and you do not notice that one of the file is not a file, but a link to ~/Private. If links are dereferenced, immediately you start sharing sensitive data with many users. If users can upload symbolic links, the implications can be even more serious.

How if, deep in your data, you end up having a cycle of symbolic links: A -> B -> C -> D -> E -> A. Unless some (possibly costy) algorithm is added to the sync code, the latter may start looping on dereferencing this cycle rather than doing what it should do.

How if your sync dir contains a link to a volume, as you say, and a file in the volume is changed. In many situations the sync software cannot be reliably notified of this change, and the syncing of the modified file may start with a significant delay.

This is not to say that your usage scenario is not interesting (even if I think that in some cases the possibility to sync multiple folders can substitute for your usage of symbolic links). It is to say that only users who know what they are doing should IMHO activate it. Dropbox itself (that only supports the scenario you are talking about) actively discourages relaying on it because it can end up in bad surprises for users who do not fully understand what they are doing.

This is why, IMHO, the two cases should be considered separately and, if both implemented, have independent and mutually exclusive ways to enable them.

.
https://help.nextcloud.com/t/symbolic-link-support/220/32

I just restate the symbolic link processing strategies:

TYPE A: No dereferencing
TYPE B. Client dereferencing

Some opinions:
I came because I needed the type B (deref). I implemented a poc 16. It is a poc since it does not address the issues you mention (loop check, file system notifications).
I agree that the multiple sync folders functionality, partially solves this issue, but again it has the problem of copying the data in the synced folder under the root folder. With symbolic links you can preserve your documents organization and just symlink the stuff you want to publish under root, without wasting space.
Regarding security, indeed, this should be added as an advanced opt in option, targeted to power users, who want this conveniency.
Also I can see that it should be more practical to have this option per synced root (per client).
Note, that this is a client implementation. The server does not know anything. Also clients that are on platforms no supporting symlink, they too, not do any special processing.

...

And like you said, if they are to be implemented both, they should be mutually exclusive. Also the client should handle various corner cases

client a type A, client b type B =>
    handle name clashes, on both ends, ignore if type mismatch,
    b skips remote links,
client a type A, client b no link processing
    b skip remote links
client a type A, then change setting to no processing
    what do we do on remote? Do we delete the links ? Just leave them ? (maybe this suggests that link setting should be a remote setting shared by all clients ?
Another though, links are first classified as external (linking outside the sync root) and internal. Then the type A setting applies only to internal and type B only to external. Again, what if I change one link from external to internal ?

@basos9 basos9 closed this as completed Oct 23, 2023
@RokeJulianLockhart
Copy link

#3335 (comment)

@basos9, shouldn't this have been closed as not planned rather than https://github.com/nextcloud/desktop/issues?q=is%3Aissue+is%3Aclosed+reason%3Acompleted?

@basos9 basos9 closed this as not planned Won't fix, can't repro, duplicate, stale Oct 28, 2023
@RokeJulianLockhart

This comment was marked as resolved.

@f1d094
Copy link

f1d094 commented Nov 8, 2023

@RokeJulianLockhart: I moved the post to #250 where I felt it was more appropriate. You can find it along with the relevant comments/conversation there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement enhancement of a already implemented feature/code
Projects
None yet
Development

No branches or pull requests

3 participants