[BUG] symlinks are clobbered by the app

[msfjarvis]

Describe the bug
A password will fail to decrypt if it is a soft link as opposed to a normal file.

To Reproduce
Steps to reproduce the behavior:

1. Create a password that's a soft link to an existing password using ln -s <passwd>.gpg softlink.gpg
2. Pull the repository on device
3. Try to decrypt the password
4. See error

Expected behavior
Password is decrypted and shown in the UI

Device information (please complete the following information):

• Device: Google Pixel 2
• OS: Android R DP1
• App version 1.6.0-SNAPSHOT

Additional context

Relevant log snippet

E  onError getErrorId: 0
E  onError getMessage: No valid OpenPGP encrypted or signed data found!

[Dioxo]

I was looking into the function decryptPassword from PasswordStore.kt and tried to first validate that the file is a symbolik link with Files.isSymbolicLink(item.file.toPath()) as a test to validate its type.
But it appears that file is not even a symbolik link !
To validate this, i created 2 other files (also symbolic links) from 2 different ways, I did it hard coded, it's not the best, but just to quickly validate my hypothesis.

I created a file called "test" that's gonna be normal file and 2 symbolik link files.
Os.symlink("/data/data/dev.msfjarvis.aps.debug/files/store/test", "/data/data/dev.msfjarvis.aps.debug/files/store/test2.gpg")

Runtime.getRuntime().exec("ln -s /data/data/dev.msfjarvis.aps.debug/files/store/test" + " /data/data/dev.msfjarvis.aps.debug/files/store/test3.gpg")

And i opened the Device File Explorer from Android Studio and went where the file is stored. Just to see that the 2 files that i created doesn't have any issue. They are linking my file 'test' and working well with decryption.
I even changed the password from test to see if others files worked well, they did.

So... If what i'm saying is right, the problem is when importing the files and not the file itself.

I'll try to validate this also, but just to know what you think. @msfjarvis

[msfjarvis]

I don't think that's the case, but I only checked with softlinks I created on my PC. I used ln -s test_real.gpg test_softlink.gpg then verified that it indeed was correctly pointing to the source. After I committed both files and pushed to git, the entries failed to deserialize on my phone.

[Dioxo]

Yeah, i did the same to create the file on my PC.
After that i run to validate if it's a symbolik link on my phone Files.isSymbolicLink(item.file.toPath()), and it said no.

So to validate, i created 2 files dinamically, and they went all good.

Try to see the file from the Device File Explorer. You'll see that's a normal file.

[msfjarvis]

Yeah, i did the same to create the file on my PC.
After that i run to validate if it's a symbolik link on my phone Files.isSymbolicLink(item.file.toPath()), and it said no.

So to validate, i created 2 files dinamically, and they went all good.

Try to see the file from the Device File Explorer. You'll see that's a normal file.

That's just the thing, a symlink is not a normal file! See this as an example, chmod does not affect a symlink like it affects a normal file.

[Dioxo]

Yes, i did what you mention, i have my files in my PC like this

I did a push, later pull from my phone and I created 2 more files dinamically on my phone, test2 and test3 that are soft links poining to normal_file.gpg.

In the Device File Explorer, i can see that my files test2 and test3 are linking to normal_file.gpg.

BUT the file soft_link don't say that it's a symbolik link, also the rights aren't set normally to what it should be as a soft link.

[msfjarvis]

Right, so what do you believe is going wrong? Is JGit parsing these wrongly from the index?

[Dioxo]

I think that the problem is when importing the files. Not sure when exactly. I'll try to see this tomorrow.

Also the logic from decrypting a file that (really) is a soft link, works fine, i can decrypt test2 and test3 without problems.
And now it's normal why we are getting "No valid OpenPGP encrypted or signed data found!", because the content from the file soft_link is just plain text with content 'normal_file' as text. That's why i said it's a normal file

[Dioxo]

@msfjarvis After reading more about how git handles symbolic links, I can not see a good solution for this issue.

Git just stores the contents of the link (i.e. the path of the file system object that it links to) in a 'blob' just like it would for a normal file. It then stores the name, mode and type (including the fact that it is a symlink) in the tree object that represents its containing directory.
When you checkout a tree containing the link, it restores the object as a symlink regardless of whether the target file system object exists or not.

Source : https://stackoverflow.com/a/954575

Now i undestand why the soft_link.gpg file stores the file path that is linking to (in our case normal_file.gpg)

In order to try it, i did a test.
I have the file 'test_soft' that links to 'normal_file', i added, commited, pushed after this
With the command git ls-files -s ./test_soft.gpg I can see the mode that git associates to my file

(Note 120000 is the mode listed in ls-files output. It would be something like 100644 for a regular file)

After some modifications to my repository in my phone (commits and push), if i pull from my PC I see that my file is not anymore a soft link.

With git diff is easier to see that my file changed its mode from 120000 to 100644, and also added the path file to its content.
deleted file mode 120000
-normal_file.gpg

new file mode 100644
+normal_file.gpg

So... I don't really see how to resolve this issue, each time that we pull from the phone, it's going to be a normal file, because it's a problem with git and how it stores files.
A possible option that i can think is to read the content from file and go to search the file that reference the content. Because the file's content has the path to the file. But it's going to be harder to try to validate that the path points to a real file (because is going to be a normal file, so we can't assure that is not the user trying to add a normal file with a path instead of a soft link that becames a normal file) , also there's the problem to read the file and know that it's not an encrypted one.

[msfjarvis]

@FabianHenneke what do you think? Would it be too expensive to try reading the first line of a file and seeing if it is a valid relative path before sending the file off to OpenKeychain? We can special-case this scenario but I still think this is a JGit bug rather than Git problem.

[msfjarvis]

We're also pretty behind on JGit releases, is the blocker still Java 8 APIs?

[fmeum]

@FabianHenneke what do you think? Would it be too expensive to try reading the first line of a file and seeing if it is a valid relative path before sending the file off to OpenKeychain? We can special-case this scenario but I still think this is a JGit bug rather than Git problem.

I'm also pretty sure that this is a JGit bug that will only really go away once JGit does... I personally don't like these kind of hacks and would rather focus efforts on replacing JGit with something more easily maintainable. When I tried to use symbolic links in APS, they failed for completely different reasons, so I don't feel like opening this can of worms. But please keep in mind that I'm not using symbolic links in my pass repos myself, so I may be biased.

[Dioxo]

Well, i don't know much about JGit so i can't really say if the probleme is with it. And i couldn't recreate the issue using only my PC (messing around with soft links), so it makes me wonder if it's maybe JGit's fault.

[Dioxo]

In the doc from JGit i see they support Symbolic links, but i'm not sure which version we're using from JGit and if there's a bug in that specific version.

Native symbolic links are supported, provided the file system supports them.

https://github.com/eclipse/jgit#warningscaveats

[fmeum]

I'm not even sure whether symbolic links are supported on all phones. On FAT-formatted SD cards, this is certainly not the case, but this may also be something that could cause issues seemingly out of nowhere.

[Dioxo]

I'd like to know what you think about this issue and possible solutions @FabianHenneke @msfjarvis

[fmeum]

I'd like to know what you think about this issue and possible solutions @FabianHenneke @msfjarvis

Based on my limited understanding of this issue, git is doing the only reasonable thing here to serialize symlinks. When they end up broken on the phone after a push, that has to be JGits fault. I googled for a bit and found jenkinsci/git-client-plugin@11d9be2...d489829, which indicates to me that #719 may fix the symbolic link issue. @msfjarvis @Dioxo Could you give this a try?

[ismail]

Just hit this issue with the latest release on Play Store, fyi.

[msfjarvis]

Just hit this issue with the latest release on Play Store, fyi.

That's why it's still open :) We haven't found a way to resolve it across all Android versions yet.

[Dioxo]

I was looking again at this issue to see if I could find a solution and it magically worked with the recommendation that Fabian had given before, when integrating the java7 version of jgit, it recognizes the symbolic links.
I add a screenshot. See file test.gpg and other.gpg

The only problem is that if the link doesn't have a .gpg extension, APS doesn't show the file, in my case the file soft is not shown in the application. I think we can have a confirmation to show the file if it's soft link and doesn't have extension .gpg

What do you think @FabianHenneke @msfjarvis ? I'll do a pull request to try again this solution?

[532910]

Files that should be symlinks are still plaintext files with symlink target as a content.

[msfjarvis]

Since this is still a bug, might as well.

[fmeum]

I just started debugging the issue and turned up very confused: I expected the issue to be that we don't set our custom FS during the clone operation, given that the CloneCommand setup does not involve repository. However, even with a statically set FS with symlink support, symlinks are not created. In fact, for a symlink, DirCacheEntry.getFileMode() returns the "regular file" type. If I didn't mess up, this means that the corruption would already occur in the Git-internal file mode bits.

[msfjarvis]

I just started debugging the issue and turned up very confused: I expected the issue to be that we don't set our custom FS during the clone operation, given that the CloneCommand setup does not involve repository. However, even with a statically set FS with symlink support, symlinks are not created. In fact, for a symlink, DirCacheEntry.getFileMode() returns the "regular file" type. If I didn't mess up, this means that the corruption would already occur in the Git-internal file mode bits.

Well that does throw a spanner in the works. Upgrading JGit could help, and desugaring is now available on stable AGP so we can give that a shot and see how far we can get without breaking API 23. Or we can just commit, go minSdk 26, and forget about the 115 Play Store users that would alienate. If people are surviving on APS 1.3.3 still (I got another "I recently tried switching from the legacy app" email today, so this is definitely real) they sure can handle APS 1.13.1?

[fmeum]

Well that does throw a spanner in the works. Upgrading JGit could help, and desugaring is now available on stable AGP so we can give that a shot and see how far we can get without breaking API 23. Or we can just commit, go minSdk 26, and forget about the 115 Play Store users that would alienate. If people are surviving on APS 1.3.3 still (I got another "I recently tried switching from the legacy app" email today, so this is definitely real) they sure can handle APS 1.13.1?

Switching to a higher minSdk for 2.0 would probably make sense. Most of our recent feature additions are only applicable to Oreo and up anyway and we are even dropping accessibility support.

[msfjarvis]

Well that does throw a spanner in the works. Upgrading JGit could help, and desugaring is now available on stable AGP so we can give that a shot and see how far we can get without breaking API 23. Or we can just commit, go minSdk 26, and forget about the 115 Play Store users that would alienate. If people are surviving on APS 1.3.3 still (I got another "I recently tried switching from the legacy app" email today, so this is definitely real) they sure can handle APS 1.13.1?

Switching to a higher minSdk for 2.0 would probably make sense. Most of our recent feature additions are only applicable to Oreo and up anyway and we are even dropping accessibility support.

I'll try and upgrade JGit over the weekend then.

[timvisee]

Any progress on this?

Just came across the same issue. After doing a Android Password Store sync, it added a commit with replaced the symlink with a plain text file (having the symlink path in it). Based on the comments above, I'm assuming that JGit fails to clone/pull properly and creates a plaintext file instead of a symlink. This is basically breaking my password store (on other devices) at the moment.

[msfjarvis]

Not at the moment and it doesn't seem like we can do a lot about this either. We've already applied the official recommendations on symbolic link support.

[timvisee]

Could an option to prevent pushing to the remote be added? That would prevent pushing the broken state to the git remote, and to the rest of my machines, at the cost of not adding passwords modified on my phone. I understand that this isn't a solution, but that would at least allow me to sync my passwords to my phone. I hope I'm not going too off-topic with this question.

[msfjarvis]

Could an option to prevent pushing to the remote be added? That would prevent pushing the broken state to the git remote, and to the rest of my machines, at the cost of not adding passwords modified on my phone. I understand that this isn't a solution, but that would at least allow me to sync my passwords to my phone. I hope I'm not going too off-topic with this question.

You can simply use the pull option rather than synchronize, unless I'm not understanding the request?

[timvisee]

@msfjarvis Awesome, just noticed the option now. I've always dragged down which apparently synchronizes. This is perfect, thanks for the quick response.

[equaeghe]

[…] I've always dragged down which apparently synchronizes. […]

This is the problem for me. I have symlinks in my store and apparently cannot avoid to mistakenly drag down when in passwordstore. This then requires a forced push from another clone to undo the desymlinking. Having an option to assign drag-down to pull instead of push+pull(?) or marking the upstream as read-only would really be useful, given the symlink issue.

[msfjarvis]

[…] I've always dragged down which apparently synchronizes. […]

This is the problem for me. I have symlinks in my store and apparently cannot avoid to mistakenly drag down when in passwordstore. This then requires a forced push from another clone to undo the desymlinking. Having an option to assign drag-down to pull instead of push+pull(?) or marking the upstream as read-only would really be useful, given the symlink issue.

I think a read-only option would be a good idea, I'll see what I can do about it.

[pmartycz]

Here's a little script I run to unfuck the symlinks (remove echo before command after you verify it does what you want)

for f in ~/.password-store/**; do if [ 'text/plain' = "$(file -b --mime-type "$f")" ]; then echo ln -sf -- "$(cat "$f")" "$f"; fi; done

[msfjarvis]

It might be worth a shot to expand the FS.Factory we added to use AndroidRetroFile for implementing support on lower Android versions. The library does not provide default implementations of FileSystemProvider and FileTypeDetector but it should be possible to copy that over from MaterialFiles since the license is compatible. I started work on it in this branch, feel free to take a stab at getting the providers implemented.

[msfjarvis]

I believe at this point we've exhausted all avenues and there is no progress to be made, so I'm closing this as part of issue tracker cleanup.

[msfjarvis]

Symlinks work properly in the latest snapshot build, make sure to delete and re-clone your existing repo for JGit's auto-detection of symlink support to kick in.