Skip to content

Added option for connection timeout used when browser extension is trying to connect to KeePassXC #2752

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.

Sign up for GitHub

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

Open
Talkabout wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Added option for connection timeout used when browser extension is trying to connect to KeePassXC #2752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5923319
added option for connection timeout used when browser extension is tr…
Talkabout Nov 9, 2025
1c82121
Improved based on review feedback
Talkabout Nov 9, 2025
5a06415
Removed german translations
Talkabout Nov 9, 2025
0b63edb
Removed en_GB translations
Talkabout Nov 9, 2025
a7ef451
Changed approach of setting connection timeout by introducing a slide…
Talkabout Nov 9, 2025
511b9ed
- added constant for default connection timeout value
Talkabout Nov 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions keepassxc-browser/_locales/en/messages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,14 @@
"message": "Credentials are cleared from background tabs after the timeout and permissions for those pages will be asked again.",
"description": "Clear credentials timeout help text."
},
"optionsConnectionTimeout": {
"message": "KeepassXC connection timeout: $1 seconds",
"description": "Connection timeout label text."
},
"optionsConnectionTimeoutHelpText": {
"message": "When KeePassXC browser extension tries to connect to to keepass, this timeout value is used. Default value is 1.5. Maximum is 60.0.",
"description": "Connection timeout label text."
},
"optionsVersionInfoText": {
"message": "KeePassXC-Browser needs KeePassXC to retrieve credentials.",
"description": "Settings page version info text."
Expand Down
7 changes: 6 additions & 1 deletion keepassxc-browser/background/keepass.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,7 +804,12 @@ keepass.disableAutomaticReconnect = function() {
keepass.reconnectLoop = null;
};

keepass.reconnect = async function(tab = null, connectionTimeout = 1500) {
keepass.reconnect = async function(tab = null, connectionTimeout = CONNECTION_TIMEOUT) {
if (arguments.length < 2) {
Copy link
Copy Markdown
Member

@varjolintu varjolintu Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code block is not needed.

Copy link
Copy Markdown
Author

@Talkabout Talkabout Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view it is the most important part of the change as it makes sure to use the user configured timeout if no argument is provided. Am I missing something?

Copy link
Copy Markdown
Member

@varjolintu varjolintu Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value CONNECTION_TIMEOUT in the function is already setting that to the value if no argument is provided.

Copy link
Copy Markdown
Author

@Talkabout Talkabout Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONNECTION_TIMEOUT is set to a hard value of 1500 in global.js. The user defined value in the settings is not the same value. We need to make sure that if a developer has not provided any value to the second argument of "reconnect" we are considering the settings value.

Copy link
Copy Markdown
Member

@droidmonkey droidmonkey Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By definition, by allowing a connectionTimeout parameter at all we can ignore the setting value. You can set it to whatever you want with a direct call. If we don't want that behavior then you need to remove the parameter altogether and always use the settings value (defaulting to CONNECTION_TIMEOUT if invalid or unset).

Copy link
Copy Markdown
Author

@Talkabout Talkabout Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea behind the implementation was to keep the backward compatibility of the "reconnect" method calls to not require the second parameter and still use the user setting value. I am aware that passing the settings value with every call would be possible too, but that means that in the future a developer needs to remember to pass the settings value when a new call to "reconnect" is made. To avoid this dependency I decided to handle it in "reconnect" directly. Do you think passing the settings value with every "reconnect" call is a better solution?

Copy link
Copy Markdown
Member

@droidmonkey droidmonkey Nov 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is to drop the parameter altogether and use the settings value directly in this function, falling back to the default value if unset. This is an internal function call and there is no reason to retain any compatibility.

Copy link
Copy Markdown
Author

@Talkabout Talkabout Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think dropping this parameter is an option. Looking at background/init.js there we are using this exact parameter to pass a different timeout. It is the only place I found, but I assume this was changed for a reason :)

Copy link
Copy Markdown
Member

@varjolintu varjolintu Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The easiest solution for this is to:

  • Keep the default parameter value connectionTimeout = CONNECTION_TIMEOUT in the function. It doesn't hurt to keep the default here.
  • From init.js the parameter is overridden with 5000 (we should make this a const too).
  • The function is called from kpxcEvent.onReconnect() where the page.settings.connectionTimeout must be used as a parameter.
  • The function is called also from keepass.enableAutomaticReconnect(). This function is not used at the point, but it should also include page.settings.connectionTimeout as the parameter.

connectionTimeout = page.settings.connectionTimeout;
}

connectionTimeout = Math.min(60000, Math.max(CONNECTION_TIMEOUT, connectionTimeout))
keepassClient.connectToNative();
keepass.generateNewKeyPair();
const keyChangeResult = await keepass.changePublicKeys(tab, !!connectionTimeout, connectionTimeout).catch(() => false);
Comment thread
Talkabout marked this conversation as resolved.
Expand Down
2 changes: 2 additions & 0 deletions keepassxc-browser/background/page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const defaultSettings = {
bannerPosition: BannerPosition.TOP,
checkUpdateKeePassXC: CHECK_UPDATE_NEVER,
clearCredentialsTimeout: 10,
connectionTimeout: CONNECTION_TIMEOUT,
colorTheme: 'system',
credentialSorting: SORT_BY_GROUP_AND_TITLE,
debugLogging: false,
Expand Down Expand Up @@ -47,6 +48,7 @@ page.autoSubmitPerformed = false;
page.attributeMenuItems = [];
page.blockedTabs = [];
page.clearCredentialsTimeout = null;
page.connectionTimeout = null;
Copy link
Copy Markdown
Member

@varjolintu varjolintu Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is not used, and not needed.

page.currentRequest = {};
page.currentTabId = -1;
page.isFirefox = false;
Expand Down
3 changes: 3 additions & 0 deletions keepassxc-browser/common/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ const CHECK_UPDATE_THREE_DAYS = 3;
const CHECK_UPDATE_ONE_WEEK = 7;
const CHECK_UPDATE_ONE_MONTH = 30;

// Default value for connection timeout
const CONNECTION_TIMEOUT = 1500;

const URL_WILDCARD = '1kpxcwc1';
const schemeSegment = '(\\*|http|https|ws|wss|ftp)';
const hostSegment = '(\\*|(?:\\*\\.)?(?:[^/*]+))?';
Expand Down
4 changes: 4 additions & 0 deletions keepassxc-browser/managed_storage.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@
"title": "Clear credential info from tabs after timeout. Default (seconds): 10",
"type": "integer"
},
"connectionTimeout": {
"title": "Connection timeout to KeePassXC. Default (seconds): 1.5",
"type": "integer"
},
"colorTheme": {
"title": "Extension color scheme. Default: system",
"type": "string"
Expand Down
8 changes: 8 additions & 0 deletions keepassxc-browser/options/options.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,14 @@ <h2 class="pb-3 mt-0" data-i18n="optionsGeneralSettingsTab"></h2>
<div class="form-text" data-i18n="optionsClearCredentialsTimeoutHelpText"></div>
</div>


<!-- Keepass connectiontimeout -->
<div class="form-group mt-2 pb-1">
<label id="connectionTimeoutLabel" class="font-weight-normal" for="connectionTimeout" data-i18n="optionsConnectionTimeout" data-i18n-placeholder="1.5"></label>
<input type="range" class="form-range" id="connectionTimeout" name="connectionTimeout" min="1.5" max="60.0" step="0.5" value="1.5">
<div class="form-text" data-i18n="optionsConnectionTimeoutHelpText"></div>
</div>

<!-- Debug logging -->
<div class="form-group mt-2 pb-1">
<div class="form-check form-switch">
Expand Down
23 changes: 18 additions & 5 deletions keepassxc-browser/options/options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ options.initGeneralSettings = async function() {
$('#tab-general-settings input[type=radio]#checkUpdateOneMonth').value = CHECK_UPDATE_ONE_MONTH;
$('#tab-general-settings input[type=radio]#checkUpdateNever').value = CHECK_UPDATE_NEVER;

$('#tab-general-settings input[type=range]').value = options.settings['redirectAllowance'];
$('#tab-general-settings #redirectAllowance').value = options.settings['redirectAllowance'];
$('#redirectAllowanceLabel').textContent = tr('optionsRedirectAllowance',
options.settings['redirectAllowance'] === 11 ? 'Infinite' : String(options.settings['redirectAllowance']));

Expand All @@ -124,6 +124,9 @@ options.initGeneralSettings = async function() {
$('#tab-general-settings input#defaultGroup').value = options.settings['defaultGroup'];
$('#tab-general-settings input#defaultPasskeyGroup').value = options.settings['defaultPasskeyGroup'];
$('#tab-general-settings input#clearCredentialTimeout').value = options.settings['clearCredentialsTimeout'];
const connectionTimeout = (options.settings['connectionTimeout']/1000);
Copy link
Copy Markdown
Member

@varjolintu varjolintu Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do this. Just retrieve the value from settings and do any modifications to it when inserting it as element value.

$('#tab-general-settings input#connectionTimeout').value = connectionTimeout;
$('#connectionTimeoutLabel').textContent = tr('optionsConnectionTimeout', String(connectionTimeout));

const generalSettingsRadioInputs = document.querySelectorAll('#tab-general-settings input[type=radio]');
for (const radio of generalSettingsRadioInputs) {
Expand Down Expand Up @@ -173,7 +176,17 @@ options.initGeneralSettings = async function() {
});

// Change label text dynamically with the range input
$('#tab-general-settings input[type=range]').addEventListener('input', function(e) {
$('#tab-general-settings input#connectionTimeout').addEventListener('input', function(e) {
$('#connectionTimeoutLabel').textContent = tr('optionsConnectionTimeout', e.target.value);
});

$('#tab-general-settings input#connectionTimeout').addEventListener('change', async function(e) {
options.settings['connectionTimeout'] = Math.min(60000, Math.max(CONNECTION_TIMEOUT, e.target.valueAsNumber * 1000))
await options.saveSettings();
});

// Change label text dynamically with the range input
$('#tab-general-settings input#redirectAllowance').addEventListener('input', function(e) {
const currentValue = e.target.valueAsNumber === 11 ? 'Infinite' : e.target.value;
$('#redirectAllowanceLabel').textContent = tr('optionsRedirectAllowance', currentValue);
});
Expand Down Expand Up @@ -726,7 +739,7 @@ options.initSitePreferences = function() {

// Page URL
row.children[0].children[0].children[0].value = url;
row.children[0].children[0]?.addEventListener('dblclick', (e) =>
row.children[0].children[0]?.addEventListener('dblclick', (e) =>
enterEditMode(e, row, inputField, editButton, cancelButton, saveButton)
);

Expand Down Expand Up @@ -891,7 +904,7 @@ const getBrowserId = function(userAgent) {
return `${query.name} ${getVersion(userAgent, query.findStr)}`;
}
}

return 'Other/Unknown';
};

Expand Down Expand Up @@ -919,7 +932,7 @@ const updateDropdownPosition = function(e, dropdown) {
if (!rect) {
return;
}

const zoom = getComputedStyle(document.body).zoom || 1;
const scrollTop = document.defaultView.scrollY / zoom;
const scrollLeft = document.defaultView?.scrollX / zoom;
Expand Down