fix: support non-ASCII usernames in Windows pipe path

[Lurito]

Description

This PR fixes a critical issue on Windows where the agent fails to connect to the KeePassXC named pipe when the current USERNAME contains non-ASCII characters (e.g., Chinese, Japanese).

The root cause is that KeePassXC generates its pipe name by replacing non-ASCII characters based on the system's ANSI Code Page (ACP). This PR ensures our pipe path generation logic matches KeePassXC's internal behavior.

This resolves the "Failed to locate socket, Caused by: N/A" error on non-ASCII Windows.

Changes

• Added windows-sys dependency with Win32_Globalization feature.
• Implemented MaxCharSize detection via GetACP and GetCPInfo.
• Updated WindowsSocketPath262::get_path to use U+FFFD replacement characters for non-ASCII/control characters in usernames.
• Added a OnceLock cache for the replacement string to maintain performance.

Checklist

• I have rebased my branch so that it has no conflicts
• I have added tests where appropriate

No tests were added. Because the primary goal of testing would be to check if it matches the pipe name generated by KeePassXC. I am not able to do that due to a lack of C++ skills. However, I successfully connected to KeePassXC on the CP936 (Chinese) system.

• Commit messages are compliant with Conventional Commits

Is this a breaking change?

No. This is a bug fix. It's unlikely any system will depend on this bug.

[Frederick888]

Thanks for the patch! I was not aware of this. Could you share a link to the code where this is done in KPXC? Also any assisting references regarding code pages you think I may need since I have virtually zero knowledge about it :P

[Lurito]

I realized that the logic submitted earlier does not apply to many scenarios:

• Many character sets, such as GBK (corresponding to CP936), can be variable-length encoded, so the character length may be shorter than MaxCharSize.
• Some character mapping encodings might result in "regular" characters instead of the U+FFFD replacement character that indicates an error.

That solution earlier just happened to work well on my system, but it lacked general applicability.

Therefore, I looked through the source code of KeePassXC and Qt. I noticed that KeePassXC directly calls qgetenv(...) to read environment variables (see here), and the source code for Qt's qgetenv(...) is very straightforward:

QByteArray qgetenv(const char *varName)
{
    const auto locker = qt_scoped_lock(environmentMutex);
    return QByteArray(::getenv(varName));
}

This means we can simply call the getenv(...) in C stdlib to reproduce that logic bug-to-bug.

Following this approach, I have completed the latest commit. This should perfectly match the named pipe path used by KeePassXC.

Additionally, I have added a test for this change.

[Frederick888]

Therefore, I looked through the source code of KeePassXC and Qt. I noticed that KeePassXC directly calls qgetenv(...) to read environment variables (see here), and the source code for Qt's qgetenv(...) is very straightforward:

QByteArray qgetenv(const char *varName)
{
    const auto locker = qt_scoped_lock(environmentMutex);
    return QByteArray(::getenv(varName));
}

This means we can simply call the getenv(...) in C stdlib to reproduce that logic bug-to-bug.

Thank you for the details. I now have a much clearer understanding towards this issue.

It appears to me that this is more of a limitation than a feature on KPXC's side. It looks unintentional too. After reading https://doc.qt.io/qt-6/qtenvironmentvariables.html#qgetenv, I wonder if you replace qgetenv() in KPXC with qEnvironmentVariable(), will it retrieve the original username and create the named pipe with it just fine?

[Lurito]

I wonder if you replace qgetenv() in KPXC with qEnvironmentVariable(), will it retrieve the original username and create the named pipe with it just fine?

I think yes. But I'm not sure if other downstreams of KPXC have any dependencies on this, e.g. would other plugins depend on the wrong logic... I also considered whether to PR KPRC to fix this, or to reproduce it, and then ultimately chose the latter.

And honestly, I think this is an issue that deserves KPXC's attention. I see that it also creates a named pipe to ensure that a user only opens one instance of the program. So this bug would mean that if two users with non-ASCII names (in GBK, this collision are very likely to occur if usernames have the same number of characters) were converted to the same wrong character, they would not be able to open KPXC at the same time.

[Frederick888]

I wonder if you replace qgetenv() in KPXC with qEnvironmentVariable(), will it retrieve the original username and create the named pipe with it just fine?

I think yes. But I'm not sure if other downstreams of KPXC have any dependencies on this, e.g. would other plugins depend on the wrong logic...

I don't think third-party callers actually concern the KPXC maintainers, otherwise we wouldn't have a whole array of socket path candidates here in the first place 😅 But in all fairness, the socket/pipe was probably never designed for third-party usage either.

I also considered whether to PR KPRC to fix this, or to reproduce it, and then ultimately chose the latter.

Personally I encourage you to do the KPXC PR, especially if you've already got a dev environment according to https://github.com/keepassxreboot/keepassxc/wiki/Set-up-Build-Environment-on-Windows.

If that's too troublesome, you can probably draft up the PR and ping their team for artefacts. I didn't find it but it's gotta be somewhere in their TeamCity?

And honestly, I think this is an issue that deserves KPXC's attention. I see that it also creates a named pipe to ensure that a user only opens one instance of the program. So this bug would mean that if two users with non-ASCII names (in GBK, this collision are very likely to occur if usernames have the same number of characters) were converted to the same wrong character, they would not be able to open KPXC at the same time.

Actually the reason why they added USERNAME was due to a similar issue: keepassxreboot/keepassxc#5393. So if you haven't got the time for a PR, I can't see why they wouldn't welcome an issue report.

---

Back to this PR, FWIW I'd love to merge it anyway. If there's any progress in KPXC down the line, the code is still useful for backwards-compatibility.

Could you please

1. Rebase your PR. I've just suppressed the lint errors chore: Suppress some clippy::unnecessary_unwrap #113
2. Fix the one failing test case

?

[Lurito]

This is a very strange filature. I can pass the tests on my platform both with MSVC and GNU targets, in fact this test set is also generated by C on my platform, but it doesn't work in CI...

[Frederick888]

This is a very strange filature. I can pass the tests on my platform both with MSVC and GNU targets, in fact this test set is also generated by C on my platform, but it doesn't work in CI...

In that case, I'm happy if you just remove that test case. Reason being, if someone has the same environment setup as GHA, I imagine they'll get the same value from getenv() here and in KPXC anyway, although that value is different from what you get on your machine.

But of course, be my guest if you have time to get to the bottom of it later :)

PS: About to be AFK. My next reply will be tomorrow or later.

[Lurito]

I understand what the problem is. In to_local_8bit_win32 we used the current program's ACP to call WideCharToMultiByte, while my computer's ACP is 936, Github's maybe 437. This results in different bytes will be generated with different system ACP.

I'll remove this test. The real reason for removing it is that we're actually creating the corrupted string in the String::from_utf8_lossy(&value_bytes) of c_putenv. So for the tests that follow, we're really just testing ourselves. So even if we fix an ACP to call to_local_8bit_win32, the test is still meaningless.

As long as we can confirm that KPXC contains a logic equivalent to String::from_utf8_lossy(&value_bytes), we can be certain the change is correct.

[Lurito]

Thank you for your patience these past few days, 谢谢. It's been a pleasure communicating with you.

Regarding report this to KPXC, to be honest, that is a language challenge for me - one in C++ and the other in English. But I think I'll give it a try after I'm done my job now.

[Lurito]

Yeah, you're right. I didn't realize that.

[Lurito]

There is also a "redundant" change in Cargo-lock.toml. But I noticed that it mentions It is not intended for manual editing., so I think maybe it would be better to keep it?

[Frederick888]

There is also a "redundant" change in Cargo-lock.toml. But I noticed that it mentions It is not intended for manual editing., so I think maybe it would be better to keep it?

Personally I prefer cleaning it up. cargo does not bump version if there are no other changes. Anyway, don't worry about it, I'll reset it on my end.

Thanks again for your contribution! I also learnt something :)

Edit: I should have some time to do a little chore and publish a new version this weekend.