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

Modifications to Redetect Fields #2422

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

Conversation

varjolintu
Copy link
Member

Noticed that the get_status request after Redetect Fields was not an internal call, but it requested status from KeePassXC that is not even needed here. It is enough to update the page variables and refresh the popup with new data directly.

Other improvements:

  • Replace direct calls to page with proper functions.
  • Only set username field detected when the content script URL matches the page URL. Otherwise any iframes (from Google etc.) would set the username field detected variable back to false.

Testing strategy

Manually. For example Patreon's login page can be used when Predefined Sites is disabled in the extension settings.

Type of change

  • ✅ Refactor (significant modification to existing code)

@varjolintu varjolintu added this to the 1.9.6 milestone Dec 24, 2024
@@ -125,7 +125,8 @@ const sendMessageToTab = async function(message) {
}

statusResponse(await browser.runtime.sendMessage({
action: 'get_status'
action: 'get_status',
args: [ true ]
Copy link
Member Author

Choose a reason for hiding this comment

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

This boolean changes the request to an internal call without sending messages to KeePassXC.

@droidmonkey
Copy link
Member

What if the login dialog is in an iframe with a different url and I need to set the username only detection?

@varjolintu
Copy link
Member Author

varjolintu commented Dec 25, 2024

What if the login dialog is in an iframe with a different url and I need to set the username only detection?

It's valid concern that I also thought about. I need to find a site where I can test this.

EDIT: Found one. Gonna postpone this fix to a later release. Needs some adjustments. Even the version before this PR does not work good enough.

@varjolintu varjolintu modified the milestones: 1.9.6, 1.9.7 Dec 25, 2024
@varjolintu varjolintu marked this pull request as draft December 26, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants