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

Remediate errors in various favicon fetch scenarios (#2779) #2795

Conversation

kneitinger
Copy link
Contributor

Fixes stuck "Download favicon" button on icon download attempts for IP
address hosts by skipping attempts to get 2nd level domain resources
(which resulted in calls to 0.0.0.).

Fixes some cases when DuckDuckGo fallback fails to find icon of >2-level
domains, by adding a request to a DDG URL based on entry's 2nd level
domain.

Repurposes EditWidgetIcons' private fetchCanceled slot (which as of #2439,
is unused by any code) into public abortRequests slot, which is
connected to the entry edit widget's accepted and rejected signals (in
other words, Ok or Cancel was pressed).

Type of change

  • ✅ Bug fix (non-breaking change which fixes an issue)

Description and Context

Fixes issues discussed in comments of #2779

Testing strategy

Test cases:

  1. When an entry's URL is an IP address, requests should not hang, and "Download favicon" button should not be stuck in pressed state for this entry and later edited entries:
    • Created entry with an IP address as URL
    • Clicked "Download favicon button"
    • Verified that either red failure banner or green success banner appeared, and "Download favicon" button is not still depressed in this entry's edit widget.
    • Navigated to new entry, verified that "Download favicon" button was clickable.
  2. When an entry needs to rely on DDG fallback for favicon download, if there are >2 levels of domains and FQDN fails in DDG, 2nd level domain should be attempted in DDG
    • Created entry with previously problematic URL https://www.secretescapes.de/
    • Clicked "Download favicon button"
    • Verified icon successfully downloaded
  3. Upon clicking "Ok" or "Cancel" buttons in the entry edit widget, any remaining request should be aborted.
    • Started debug session and set breakpoint in EditWidgetIcons::abortRequests()
    • Opened an entry for editing, and click "Download favicon" button
    • Clicked "Ok" or "Cancel"
    • Verified that breakpoint was triggered,

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@droidmonkey droidmonkey changed the base branch from develop to release/2.4.0 March 17, 2019 21:44
@droidmonkey droidmonkey self-requested a review March 17, 2019 21:44
@droidmonkey droidmonkey added this to the v2.4.0 milestone Mar 17, 2019
)

Fixes stuck "Download favicon" button on icon download attempts for IP
address hosts by skipping attempts to get 2nd level domain resources
(which resulted in calls to 0.0.0.<rightmost octet of original IP>).

Fixes some cases when DuckDuckGo fallback fails to find icon of >2-level
domains, by adding a request to a DDG URL based on entry's 2nd level
domain.

Repurposes EditWidgetIcons' private fetchCanceled slot (which as of keepassxreboot#2439,
is unused by any code) into public abortRequests slot, which is
connected to the entry edit widget's accepted and rejected signals (in
other words, Ok or Cancel was pressed).
@droidmonkey droidmonkey force-pushed the feature/fix_favicon_fetch_connection_errors branch from ca0c197 to ae4eafc Compare March 17, 2019 21:52
@droidmonkey droidmonkey merged commit 84f5adb into keepassxreboot:release/2.4.0 Mar 17, 2019
droidmonkey added a commit that referenced this pull request Mar 19, 2019
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
@phoerious phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: bugfix Pull request that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants