-
-
Notifications
You must be signed in to change notification settings - Fork 205
Fix passkeys exclusion rules for xml
#2748
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
base: develop
Are you sure you want to change the base?
Conversation
Not all URLs containing `.xml` point to XML files. For example: - https://bafybeifvkyzsg6skzb3tsk7u7sueex574fzzwfoad7sebudldbv6sazuki.ipfs.dweb.link/test.xml.html - https://bafybeifvkyzsg6skzb3tsk7u7sueex574fzzwfoad7sebudldbv6sazuki.ipfs.dweb.link/test.xml/html - https://bafybeifvkyzsg6skzb3tsk7u7sueex574fzzwfoad7sebudldbv6sazuki.ipfs.dweb.link/?test=.xml Type checking is implemented in `keepassxc-browser/content/passkeys-inject.js`
|
|
Before this, I examined git log. The addition of the rule (#2287) was a response to #2286. It contains the following example XML file: https://v1.ispdb.net/office365.com It doesn't fit the pattern. Then this pattern was then removed (#2506). But not for Passkeys. The main code checks Content-Type: keepassxc-browser/keepassxc-browser/content/keepassxc-browser.js Lines 889 to 898 in c2dd4cb
Passkeys code too: keepassxc-browser/keepassxc-browser/content/passkeys-inject.js Lines 89 to 94 in c2dd4cb
As I understand MDN, the pattern isn't a regular expression (it's very limited). It also doesn't track content type, only URLs. |
|
Ah I see thank you |
|
Hmm. Is there some site that actually offers passkeys using those kind of URLs that seem to point to a XML file? |
|
I haven't seen such sites. Therefore, I tested it using a self-written example (links in the PR header).
|
This is why I think this fix is not even needed. |
|
I agree, we added this for a reason and removing it without any specific complaints is not wise. |
|
😕 In that case, will you roll back #2506? Or is there some reason why the search for fields and their auto-completion with traditional password should work on all sites, and WebAuthn only where URL path does not contain And on which "website" did you check #2416 PR? Here manual analysis proved sufficient. |
|
In my opinion we should ignore those URL paths as well. The idea is to restrict content scripts and injected scripts for working with such URLs. I don't see any actual reason why we should remove those protections. If a site is hosting WebAuthn stuff under those kind of strange/faulty URLs where the content doesn't match, it's their problem. I'd rather block content on URL level than let content/injects scripts to fall in their place and only do some kind of verification there. If you are using your own sites with certain generated content as an example that needs to be fixed, it's pretty far away from an actual site where this kind of error even could happen.
Just matching the error codes with: https://github.com/keepassxreboot/keepassxc/blob/develop/src/browser/BrowserMessageBuilder.h |
Not all URLs containing
.xmlpoint to XML files. For example:Type checking is implemented in
keepassxc-browser/keepassxc-browser/content/passkeys-inject.js
Lines 90 to 94 in c2dd4cb
Testing strategy
Through the self-written site above. (You can substitute another URL path)
Type of change
Note
See also: #2506, #2287