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

Wait for socket to be created #11

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WhyNotHugo
Copy link
Contributor

@WhyNotHugo WhyNotHugo commented Jun 8, 2022

If the socket does not exist at startup, wait for it to be created, and connect to it as soon as it is created.

This works but does not work. It works in that using it with Firefox and the browser extension work fine. But it does not improve the experience: the extension has a timeout, and if there's no response during before that, it assumes failure. Therefore, when the socket later appears, this isn't handled immediately by the extension.

For this to really make a difference, the browser extension should not assume that the proxy has exited on timeout, and it should keep the response listener alive. By doing so, when the socket is created at any later time, the proxy will connect automatically, and the browser extension would get a valid response, switching to a "ready" state without the requiring any user intervention.

@WhyNotHugo
Copy link
Contributor Author

WhyNotHugo commented Jun 8, 2022

My intent is to work on the changes required on the browser extension side, but that will likely not be soon.

@varjolintu
Copy link
Owner

The best solution for the proxy should be that it's always open as long as the browser instance is open. And the proxy should handle detecting the socket KeePassXC creates, and makes a new connection if KeePassXC is exited and restarted. I've tried to implement that kind of proxy for a while but I always run to some strange problems with it.

This kind of proxy would also solve the problems we have with Automatic Reconnect.

@WhyNotHugo
Copy link
Contributor Author

Yup, that's pretty much my reasoning.

This PR has the minimal changes required on the proxy side, but the browser extension needs some work (basically, it should not time out; the proxy might respond to the message hours later, when KP is started).

@WhyNotHugo
Copy link
Contributor Author

Oh, automatic reconnect is not done here yet either. I've removed most of the unwraps though, which should facilitate making that happen.

@varjolintu
Copy link
Owner

Gotta keep in mind the backwards compatibility also in the extension when we are modifying the proxies. Hah, I wish I had thought things more thoroughly five years ago.

@WhyNotHugo
Copy link
Contributor Author

Gotta keep in mind the backwards compatibility also in the extension when we are modifying the proxies.

I'm currently using the proxy with this patch, but the regular upstream extension. It seems to be backwards compatible so far.

@varjolintu
Copy link
Owner

Sorry for the long delay. This branch should be helpful for testing this: https://github.com/keepassxreboot/keepassxc-browser/tree/expect_permanent_proxy

If the socket does not exist at startup, wait for it to be created, and
connect to it as soon as it is created.

This works in that using it with Firefox and the browser extension works
fine. But it does not improve the experience: the extension has a
timeout, and if there's no response during before that, it assumes
failure and un-registers the response listener. Therefore, when the
socket later appears, its response isn't handled properly by the
extension.

For this to **really** make a difference, the browser extension should
not assume that the proxy has exited on timeout, and it should keep the
response listener connected. By doing so, when the socket is created at
any later time, the proxy will connect automatically, and the browser
extension would get a valid response, switching to a "ready" state
without the requiring any user intervention.
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.

2 participants