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

Handle credentials requests from HTTP Basic Auths separately #2542

Merged

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Dec 8, 2018

Handle credentials requests from HTTP Basic Auths.

Type of change

  • ✅ New feature (non-breaking change which adds functionality)

Description and Context

KeePassXC will detect credential retrieval requests sent from HTTP Basic Auth dialogs. Requests from those dialogs will trigger permission popup every time even if the same URL has been previously denied.

Adds a global setting that can ignore asking HTTP Basic Auth credential permissions.

Related KeePassXC-Browser PR: keepassxreboot/keepassxc-browser#343

Testing strategy

Tested manually in a local server with a page that has both HTTP Basic Auth and a basic username & password inputs.

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]

Copy link
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

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

I don't see a reason to have a separate approval for HTTP auths. My opinion is that HTTP auths should trigger the same approval as before, except it triggers even if approvals are disabled.

src/browser/BrowserAccessControlDialog.cpp Outdated Show resolved Hide resolved
@varjolintu
Copy link
Member Author

@droidmonkey That is actually a nice and simple solution for the problem. I'll modify the PR soon.

@varjolintu varjolintu force-pushed the http_auth_permissions branch 2 times, most recently from f9c244d to 31bd681 Compare December 10, 2018 16:22
@varjolintu
Copy link
Member Author

Necessary changed made.

@varjolintu varjolintu changed the title Handle credentials requests from HTTP Basic Auths with separate permissions Handle credentials requests from HTTP Basic Auths separetely Dec 10, 2018
@varjolintu varjolintu changed the title Handle credentials requests from HTTP Basic Auths separetely Handle credentials requests from HTTP Basic Auths separately Dec 10, 2018
@varjolintu varjolintu force-pushed the http_auth_permissions branch from 31bd681 to 82cc0c6 Compare December 11, 2018 06:36
@varjolintu
Copy link
Member Author

Added a global setting that can ignore asking HTTP Basic Auth credential permissions.

@droidmonkey
Copy link
Member

You have an error in CI:
src/browser/BrowserService.cpp:586:48: error: unused parameter 'httpAuth' [-Werror,-Wunused-parameter] const bool httpAuth)

@varjolintu varjolintu force-pushed the http_auth_permissions branch from 82cc0c6 to 1cbaede Compare December 11, 2018 15:12
@varjolintu
Copy link
Member Author

@droidmonkey Thanks for noticing. That is fixed.

@droidmonkey droidmonkey merged commit a070f1b into keepassxreboot:develop Dec 11, 2018
@varjolintu varjolintu deleted the http_auth_permissions branch December 12, 2018 05:11
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]
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