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

URL matching fails in presence of a path #1940

Open
cbiere opened this issue Nov 25, 2024 · 6 comments
Open

URL matching fails in presence of a path #1940

cbiere opened this issue Nov 25, 2024 · 6 comments

Comments

@cbiere
Copy link

cbiere commented Nov 25, 2024

Describe the bug

Autofill sugggestions are not matched to existing entries anymore, if the URL in the database includes any path after the hostname. This used to work fine until 4.0.8.

To Reproduce

Steps to reproduce the behavior:

  1. Go to https://github.com/login with any browser
  2. Click on Sign in with KeePassDX
  3. Manually search for github to select the entry

Expected behavior

Step 3 should not be necessary, if you have the login URL in your KeePass database entry.

KeePass Database

  • Created with: KeePassDX 4.1.1
  • Version: V2 KDBX3.1
  • Location: internal storage
  • File provider (content:// URI): content://com.android.externalstorage.documents/document/primary
  • Size: 2,72 kB
  • Contains attachment: no

KeePassDX:

Android:

  • Device: Redmi Note 10 Pro with PixelOS (AOSP)
  • Version: Android 14

Additional context

If you change the URL to https://github.com without any path in your KeePass database or add another URL item to it (URL_1), the problem can be worked around.

This issue results from the changes to fix #1820.

@cbiere cbiere added the bug label Nov 25, 2024
@cbiere
Copy link
Author

cbiere commented Nov 25, 2024

I've looked at database/src/main/java/com/kunzisoft/keepass/database/search/SearchHelper.kt
and this check is really bad:

stringToCheck.endsWith("/$word", !searchParameters.caseSensitive)

If I have the following URL in my KeePass database:
https://www.example.com/login.sh

It will match on https://login.sh/. This isn't only wrong but if the path can be registered as a domain, it can be used for phishing. In a LAN this might even work with just a hostname (e.g. http://login/ or http://signin).

@J-Jamet
Copy link
Member

J-Jamet commented Nov 25, 2024

I agree, it's not ideal at all. The problem, as I said, is to use an algorithm that doesn't take time to search. I'd already tried accent searching and it caused ANRs. I'm afraid it'll do the same thing with URL objects. I'll give it a try anyway.

@J-Jamet
Copy link
Member

J-Jamet commented Nov 25, 2024

I'll add unit tests to handle all cases. If anyone sees anything missing, don't hesitate to tell me.

@J-Jamet
Copy link
Member

J-Jamet commented Nov 25, 2024

Unit tests :

    fun testBuildURL() {
        val expected = "domain.org"

        assertTrue(expected.inTheSameDomainAs("domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("http://domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("https://domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("domain.org/login", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("http://domain.org/login", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("https://domain.org/login", sameSubDomain = false))

        assertTrue(expected.inTheSameDomainAs("https://www.domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("www.domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("ww.domain.org", sameSubDomain = false))

        assertTrue(expected.inTheSameDomainAs("domain.org", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("http://domain.org", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("https://domain.org", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("domain.org/login", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("http://domain.org/login", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("https://domain.org/login", sameSubDomain = true))

        assertFalse(expected.inTheSameDomainAs("https://www.domain.org", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("www.domain.org", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("ww.domain.org", sameSubDomain = true))

        assertFalse(expected.inTheSameDomainAs("domain.com", sameSubDomain = false))
        assertFalse(expected.inTheSameDomainAs("omain.org", sameSubDomain = false))
        assertFalse(expected.inTheSameDomainAs("odomain.org", sameSubDomain = false))
        assertFalse(expected.inTheSameDomainAs("tcp://domain.org", sameSubDomain = false))

        assertFalse(expected.inTheSameDomainAs("domain.com", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("omain.org", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("odomain.org", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("tcp://domain.org", sameSubDomain = true))
    }

@cbiere
Copy link
Author

cbiere commented Nov 25, 2024

I don't know how to write code in Kotlin, but maybe I can provide some pseudocode:


// stringtomatch contains the URL from each entry the database
// word is the hostname to search in the database

// extract the hostname from the URL
hostname = stringtomatch.replace("^([a-z]+://)([^/]+)(/.*)?$", $2)
// compare the extracted hostname, case-insensitive
if (hostname.equals(word, true))
return true

// if enabled, check if hostname is a subdomain of word
domain = "." + word
if (searchsubdomain && hostname.endswith(domain)
return true

// no matching URL
return false

If a regular expression is too expensive, the hostname can simply be extracted by removing everything up to the first "://" sequence and then everything after the first slash of the remaining sequence.

@cbiere
Copy link
Author

cbiere commented Nov 26, 2024

As for unit tests I would recommend adding the following checks:

assertFalse(expected.inTheSameDomainAs("https://example.com/domain.org", sameSubDomain = true))
assertFalse(expected.inTheSameDomainAs("https://example.com/www.domain.org", sameSubDomain = false))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants