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

Fix false positives on Comodo Dragon browser for Dragon-model devices. #7552

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

JBYoshi
Copy link
Contributor

@JBYoshi JBYoshi commented Jan 11, 2024

Description:

While looking at detection of some Chrome-based browsers, I noticed that some Android devices with the word "Dragon" in their model name (Figgers DragonX, Dragon OTT, and HyperX Dragon) were being detected as the Comodo Dragon browser. Comodo Dragon seems to be a desktop-only browser according to their website, so I believe these detections are incorrect.

This bug seems to have been originally introduced in #6019, which loosened the regex from Comodo[ _]Dragon(?:/(\d+[\.\d]+))? to (?:Comodo[ _])?Dragon(?!fruit)(?:/(\d+[\.\d]+))? (making the "Comodo" part optional as long as it isn't in the context of the word "Dragonfruit"). However, that PR didn't add any new test cases for the Comodo Dragon browser to indicate that the browser uses a user agent without the word "Comodo".

Review

@liviuconcioiu
Copy link
Collaborator

liviuconcioiu commented Jan 11, 2024

@JBYoshi can you add a test to Tests/Parser/Client/fixtures/browser.yml for Mozilla/5.0 (Windows NT 10.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/104.0.0.0 Safari/537.36 Dragon/103.0.2674.92? With your fix, this is detect as Chrome.

@JBYoshi
Copy link
Contributor Author

JBYoshi commented Jan 11, 2024

@liviuconcioiu I've updated to handle that test case. The regex now looks for Dragon/ with the slash and version required.

@sanchezzzhak sanchezzzhak merged commit a41bf18 into matomo-org:master Jan 11, 2024
15 checks passed
@JBYoshi JBYoshi deleted the dragon branch January 11, 2024 20:48
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.

3 participants