Skip to content

files: don't handle symlinks in a special way when home.file.<name>.recursive is set to true #5381

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

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented May 6, 2024

Description

I'm using pass as my password manager.
In order to use it in Firefox, I use the passff extension. The passff extension needs the passff-host native messaging host to access the passwords. Here is what the file structure of the passff-host package looks like:

result
├── etc
│   ├── chromium
│   │   └── native-messaging-hosts
│   │       └── passff.json -> ../../../share/passff-host/passff.json
│   ├── opt
│   │   └── chrome
│   │       └── native-messaging-hosts
│   │           └── passff.json -> ../../../../share/passff-host/passff.json
│   └── vivaldi
│       └── native-messaging-hosts
│           └── passff.json -> ../../../share/passff-host/passff.json
├── lib
│   ├── librewolf
│   │   └── native-messaging-hosts
│   │       └── passff.json -> ../../../share/passff-host/passff.json
│   └── mozilla
│       └── native-messaging-hosts
│           └── passff.json -> ../../../share/passff-host/passff.json
└── share
    └── passff-host
        ├── passff.json
        └── passff.py

As you can see, lib/mozilla/native-messaging-hosts/passff.json is a relative symlink. This is perfectly reasonable.
When adding programs.firefox.nativeMessagingHosts = [ pkgs.passff-host ] to the home-manager configuration, the firefox module first joins all the nativeMessagingHosts using symlinkJoin and stores the result in a variable called nativeMessagingHostsJoined. This creates ff_native-messaging-hosts in the Nix store:

/nix/store/bv62k5yl7jwzkhyci838ir3vgz59gqsa-ff_native-messaging-hosts
├── bin
│   └── firefox -> /nix/store/0zqxaz44w75gjq32xj53i32jl2j91pzy-firefox-125.0.1/bin/firefox
├── etc
│   ├── chromium
│   │   └── native-messaging-hosts
│   │       └── passff.json -> ../../../share/passff-host/passff.json
│   ├── opt
│   │   └── chrome
│   │       └── native-messaging-hosts
│   │           └── passff.json -> ../../../../share/passff-host/passff.json
│   └── vivaldi
│       └── native-messaging-hosts
│           └── passff.json -> ../../../share/passff-host/passff.json
├── lib
│   ├── [...]
│   ├── librewolf
│   │   └── native-messaging-hosts
│   │       └── passff.json -> ../../../share/passff-host/passff.json
│   └── mozilla
│       ├── native-messaging-hosts
│       │   └── passff.json -> ../../../share/passff-host/passff.json
│       └── pkcs11-modules
└── share
    ├── [...]
    └── passff-host
        ├── passff.json -> /nix/store/pag1akgbmls1xa63h6rzmb0h6xxwwzmy-passff-host-1.2.4/share/passff-host/passff.json
        └── passff.py -> /nix/store/pag1akgbmls1xa63h6rzmb0h6xxwwzmy-passff-host-1.2.4/share/passff-host/passff.py

Still perfectly fine.
Then the firefox module sets

home.file.".mozilla/native-messaging-hosts" = {
  source = "${nativeMessagingHostsJoined}/lib/mozilla/native-messaging-hosts";
  recursive = true;
}

The file module then calls lndir -silent "/nix/store/bv62k5yl7jwzkhyci838ir3vgz59gqsa-ff_native-messaging-hosts/lib/mozilla/native-messaging-hosts" ".mozilla/native-messaging-hosts" To see the problem, here is the resulting directory tree:

.mozilla
├── [...]
└── native-messaging-hosts
    └── passff.json -> ../../../share/passff-host/passff.json

Obviously this symlink doesn't go anywhere. lndir created a broken symlink. To fix this, add the -ignorelinks argument to lndir, which causes it to instead just create a symlink to the symlink in ff_native-messaging-hosts:

.mozilla
├── [...]
└── native-messaging-hosts
    └── passff.json -> /nix/store/bv62k5yl7jwzkhyci838ir3vgz59gqsa-ff_native-messaging-hosts/lib/mozilla/native-messaging-hosts/passff.json

Checklist

  • Change is backwards compatible. (I'm honestly not 100% sure on this one)

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@rycee @kira-bruneau @Lillecarl @mlyxshi

@Lillecarl
Copy link
Contributor

This comes back to bite again!

       -ignorelinks
              Causes  the  program to not treat symbolic links in fromdir spe-
              cially.  The link created in todir will point back to the corre-
              sponding  (symbolic  link) file in fromdir.  If the link is to a
              directory, this is almost certainly the wrong thing.

              This option exists mostly to emulate the behavior the C  version
              of lndir had in X11R6.  Its use is not recommended.

@Luflosi For another native messenger i changed the package to copy the files instead of symlinking within the package. While it's not "pretty" and duplicates some kilobytes of data it worked just fine. Considering the "It's use is not recommended" mention and how wide the blast radius of this change is, I'd try that option first even though it's not "covering all bases".

https://gist.github.com/Lillecarl/1e4669bd7546eed5cdfcea900fa1c64e <- nixpkgs patch

final: prev: {
  passff-host = prev.passff-host.overrideAttrs (pattrs: {
    installPhase = ''
      substituteInPlace bin/${version}/passff.json \
        --replace PLACEHOLDER $out/share/passff-host/passff.py

      install -Dt $out/share/passff-host \
        bin/${version}/passff.{py,json}

      nativeMessagingPaths=(
        /lib/mozilla/native-messaging-hosts
        /etc/opt/chrome/native-messaging-hosts
        /etc/chromium/native-messaging-hosts
        /etc/vivaldi/native-messaging-hosts
        /lib/librewolf/native-messaging-hosts
      )

      for manifestDir in "''${nativeMessagingPaths[@]}"; do
        install -d $out$manifestDir
        cp $out/share/passff-host/passff.json $out$manifestDir/
      done
    '';
  });
}

You could also try this (untested) overlay.

Copy link

stale bot commented Aug 8, 2024

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Aug 8, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/hm-24-11-firefox-with-passff-host/57108/2

@stale stale bot removed the status: stale label Dec 7, 2024
Copy link

stale bot commented Mar 7, 2025

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Mar 7, 2025
@khaneliman khaneliman force-pushed the fix-firefox-nativeMessagingHosts-with-relative-symlinks branch from be9177f to 571e313 Compare March 8, 2025 00:13
@stale stale bot removed the status: stale label Mar 8, 2025
@jaredmontoya
Copy link
Contributor

jaredmontoya commented May 8, 2025

@khaneliman is there a reason why this is not being merged?
(I don't know how breaking of a change this is, but it promises to fix https://discourse.nixos.org/t/hm-24-11-firefox-with-passff-host/57108/2)

@khaneliman
Copy link
Collaborator

@khaneliman is there a reason why this is not being merged? (I don't know how breaking of a change this is, but it promises to fix https://discourse.nixos.org/t/hm-24-11-firefox-with-passff-host/57108/2)

"I don't know how breaking of a change this is"
"Change is backwards compatible. (I'm honestly not 100% sure on this one)"

Too much uncertainty and I haven't had any time to verify / test it and its a core piece of HM.

@jaredmontoya
Copy link
Contributor

jaredmontoya commented May 8, 2025

@khaneliman

This branch is outdated and wouldn't work in my nix flake so I created my own based on the latest home manager:
https://github.com/jaredmontoya/home-manager/tree/fix-firefox-nmh-w-relative-symlinks

I ran all tests and they were successful. But I am not sure if there are tests that would catch breakage of this type.

So I rebuilt my system with
nh os switch -- --override-input home-manager github:jaredmontoya/home-manager/fix-firefox-nmh-w-relative-symlinks

After being rebuilt nh shows a diff of what packages changed and only things related to firefox native messaging hosts changed because I modified my configuration in that way, and it did fix the issue it promised to fix. So that's a good sign.

Do you have any ideas of what could break because of this change so I could test it?

…ecursive is set to true

I'm using `pass` as my password manager.
In order to use it in Firefox, I use the passff extension.
The passff extension needs the `passff-host` native messaging host to access the passwords.
Here is what the file structure of the `passff-host` package looks like:
```
result
├── etc
│   ├── chromium
│   │   └── native-messaging-hosts
│   │       └── passff.json -> ../../../share/passff-host/passff.json
│   ├── opt
│   │   └── chrome
│   │       └── native-messaging-hosts
│   │           └── passff.json -> ../../../../share/passff-host/passff.json
│   └── vivaldi
│       └── native-messaging-hosts
│           └── passff.json -> ../../../share/passff-host/passff.json
├── lib
│   ├── librewolf
│   │   └── native-messaging-hosts
│   │       └── passff.json -> ../../../share/passff-host/passff.json
│   └── mozilla
│       └── native-messaging-hosts
│           └── passff.json -> ../../../share/passff-host/passff.json
└── share
    └── passff-host
        ├── passff.json
        └── passff.py
```
As you can see, `lib/mozilla/native-messaging-hosts/passff.json` is a relative symlink. This is perfectly reasonable.
When adding `programs.firefox.nativeMessagingHosts = [ pkgs.passff-host ]` to the home-manager configuration, the firefox module first joins all the `nativeMessagingHosts` using `symlinkJoin` and stores the result in a variable called `nativeMessagingHostsJoined`.
This creates `ff_native-messaging-hosts` in the Nix store:
```
/nix/store/bv62k5yl7jwzkhyci838ir3vgz59gqsa-ff_native-messaging-hosts
├── bin
│   └── firefox -> /nix/store/0zqxaz44w75gjq32xj53i32jl2j91pzy-firefox-125.0.1/bin/firefox
├── etc
│   ├── chromium
│   │   └── native-messaging-hosts
│   │       └── passff.json -> ../../../share/passff-host/passff.json
│   ├── opt
│   │   └── chrome
│   │       └── native-messaging-hosts
│   │           └── passff.json -> ../../../../share/passff-host/passff.json
│   └── vivaldi
│       └── native-messaging-hosts
│           └── passff.json -> ../../../share/passff-host/passff.json
├── lib
│   ├── [...]
│   ├── librewolf
│   │   └── native-messaging-hosts
│   │       └── passff.json -> ../../../share/passff-host/passff.json
│   └── mozilla
│       ├── native-messaging-hosts
│       │   └── passff.json -> ../../../share/passff-host/passff.json
│       └── pkcs11-modules
└── share
    ├── [...]
    └── passff-host
        ├── passff.json -> /nix/store/pag1akgbmls1xa63h6rzmb0h6xxwwzmy-passff-host-1.2.4/share/passff-host/passff.json
        └── passff.py -> /nix/store/pag1akgbmls1xa63h6rzmb0h6xxwwzmy-passff-host-1.2.4/share/passff-host/passff.py
```
Still perfectly fine.
Then the `firefox` module sets
```nix
home.file.".mozilla/native-messaging-hosts" = {
  source = "${nativeMessagingHostsJoined}/lib/mozilla/native-messaging-hosts";
  recursive = true;
}
```
The `file` module then calls `lndir -silent "/nix/store/bv62k5yl7jwzkhyci838ir3vgz59gqsa-ff_native-messaging-hosts/lib/mozilla/native-messaging-hosts" ".mozilla/native-messaging-hosts"`
To see the problem, here is the resulting directory tree:
```
.mozilla
├── [...]
└── native-messaging-hosts
    └── passff.json -> ../../../share/passff-host/passff.json
```
Obviously this symlink doesn't go anywhere. `lndir` created a broken symlink.
To fix this, add the `-ignorelinks` argument to `lndir`, which causes it to instead just create a symlink to the symlink in `ff_native-messaging-hosts`:
```
.mozilla
├── [...]
└── native-messaging-hosts
    └── passff.json -> /nix/store/bv62k5yl7jwzkhyci838ir3vgz59gqsa-ff_native-messaging-hosts/lib/mozilla/native-messaging-hosts/passff.json
```
@khaneliman khaneliman force-pushed the fix-firefox-nativeMessagingHosts-with-relative-symlinks branch from 571e313 to 95a1ec0 Compare May 8, 2025 15:28
@khaneliman
Copy link
Collaborator

khaneliman commented May 8, 2025

@khaneliman

This branch is outdated and wouldn't work in my nix flake so I created my own based on the latest home manager: https://github.com/jaredmontoya/home-manager/tree/fix-firefox-nmh-w-relative-symlinks

Updated real quick, thanks.

I ran all tests and they were successful. But I am not sure if there are tests that would catch breakage of this type.

Yeah, not sure what our test suite looks like for recursive usages.

So I rebuilt my system with nh os switch -- --override-input home-manager github:jaredmontoya/home-manager/fix-firefox-nmh-w-relative-symlinks

After being rebuilt nh shows a diff of what packages changed and only things related to firefox native messaging hosts changed because I modified my configuration in that way, and it did fix the issue it promised to fix. So that's a good sign.

Do you have any ideas of what could break because of this change so I could test it?

Not off the top of my head, I didn't spend much time looking at this though. I mostly forgot about it, tbh. Apparently it wasn't in my tabs for PRs I was tracking. I just worried there would be some unexpected usage in the wild. I don't have much time atm to dedicate to troubleshooting/triage so would probably revert if an issue pops up from merging.. .otherwise just hold until after release is cut.

@Lillecarl
Copy link
Contributor

Lillecarl commented May 8, 2025

@jaredmontoya @khaneliman If you guys want this merged you should probably create an option somewhere that defaults to false that changes that parameter only, as mentioned previously the blast radius is huge and in the wild people depend on all kinds of implementation details. I like using my phone as a hammer! (/s)

I'm not sure where the option would be placed but I'll leave figuring that out to you guys 😄 It'll be easy for anyone to evaluate if things break once it's in master, but getting people to try a PR is 10x harder

EDIT: I'll happily review the default off state for the PR once it's ready, just mention me

@khaneliman
Copy link
Collaborator

#7018

@khaneliman khaneliman closed this May 11, 2025
@Luflosi Luflosi deleted the fix-firefox-nativeMessagingHosts-with-relative-symlinks branch May 12, 2025 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants