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

Remove KeePassHttp plugin and qhttp #1752

Merged
merged 3 commits into from
Mar 31, 2018
Merged

Conversation

droidmonkey
Copy link
Member

Description

drumroll please.... this is the official removal of the KeePassHttp support and the qhttp backend. With the integration of the KeePassXC-Browser and native messaging we no longer need this plugin and it was deprecated in 2.3.0.

Closes #913

Motivation and context

HTTP servers in a password manager is not a good idea. Also the KeePassHttp protocol is not very secure.

How has this been tested?

It still compiles!

Types of changes

  • ✅ Breaking change (fix or feature that would cause existing functionality to change)

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]
  • ✅ My change requires a change to the documentation and I have updated it accordingly.

@droidmonkey droidmonkey added this to the v2.4.0 milestone Mar 18, 2018
INSTALL.md Outdated
-DWITH_XC_YUBIKEY=[ON|OFF] Enable/Disable YubiKey HMAC-SHA1 authentication support (default: OFF)
-DWITH_XC_BROWSER=[ON|OFF] Enable/Disable KeePassXC-Browser extension support (default: OFF)
-DWITH_XC_NETWORKING=[ON|OFF] Enable/Disable Networking support (favicon download) (default: OFF)

-DWITH_XC_ALL=[ON|OFF] Enable/Disable compiling all plugins above (default: OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to have different spacing from the rest of the stuff here – looks like they use a combination of spaces and tabs, whereas this is just spaces?

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(qhttpengine DEFAULT_MSG QHTTPENGINE_LIBRARY QHTTPENGINE_INCLUDE_DIR)

mark_as_advanced(QHTTPENGINE_LIBRARY QHTTPENGINE_INCLUDE_DIR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is qhttpengine coming back? ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yah great catch haha

@droidmonkey
Copy link
Member Author

Removed remaining references.

Copy link
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

Seems fine to me.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Mar 19, 2018

KeePassHTTP, We will miss you 💔

@wgml
Copy link
Contributor

wgml commented Mar 19, 2018

There is still a Note about KeePassHTTP section in README.

@louib
Copy link
Member

louib commented Mar 20, 2018

me_gusta

@yan12125
Copy link
Contributor

default

(Stolen from mozilla-jetpack/jpm#546 (comment), humor from Mozilla :)

@louib
Copy link
Member

louib commented Mar 20, 2018

@droidmonkey I guess this should be removed or updated.

There's also some references to the http connector in docs/QUICKSTART.md

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Mar 21, 2018

Note: I think this will need a blog post about it or a warning paragraph in the release note, just letting users know that this isn't supported anymore, avoiding thousand of new issues.

@droidmonkey
Copy link
Member Author

The giant yellow banner in 2.3.0 shown for three start-ups wasnt enough?

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Mar 21, 2018

Yes, but now we need to tell users that we actually removed it :D

@droidmonkey
Copy link
Member Author

Hah of course, we could detect that the service is enabled in the existing configuration file and issue them the final warning. I'll add that to this PR.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Mar 22, 2018

I think just a mention in the blog post or the release post is fine, no need to add into KeePassXC itself

@droidmonkey
Copy link
Member Author

Sorry for the delay, I removed the additional HTTP references noted by @louib

@droidmonkey droidmonkey force-pushed the refactor/remove-http branch from 9698549 to ccf272d Compare March 31, 2018 12:40
@droidmonkey droidmonkey merged commit 410d88b into develop Mar 31, 2018
@droidmonkey droidmonkey deleted the refactor/remove-http branch March 31, 2018 20:48
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove qhttp and KeePassHTTP functionality
6 participants