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 URL port and scheme when requesting credentials #2003

Merged

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented May 27, 2018

Handle URL port and scheme when requesting credentials.

Description

Entries with different port or scheme in URL are ignored. If an entry has a http scheme, credentials are not received when accessing a site with https.

Motivation and context

Fixes keepassxreboot/keepassxc-browser#143.
Fixes #1467.

How has this been tested?

Manually.

Types of changes

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

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

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Looks good, but what about HTTPS vs HTTP?

@varjolintu
Copy link
Member Author

That could be done too. I'll make the fixes to this PR.

@varjolintu varjolintu changed the title Handle URL port when requesting credentials Handle URL port and scheme when requesting credentials May 28, 2018
@varjolintu
Copy link
Member Author

varjolintu commented May 28, 2018

At the same time I was looking an old issue #1340. I'm not sure if it's wise to keep the old compare method where URL is compared to the entry title. I think it should only do the compare to the URL.

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.

This PR conflicts with #2055. I also do not like the way this is implemented, there are a ton of local variables created that don't seem to add to the overall objective.

if ((!entryTitle.isEmpty() && hostname.contains(entryTitle))
|| (!entryUrl.isEmpty() && hostname.contains(entryUrl))
|| (matchUrlScheme(entryTitle) && hostname.endsWith(QUrl(entryTitle).host()))
|| (matchUrlScheme(entryUrl) && hostname.endsWith(QUrl(entryUrl).host())) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use "entryQUrl" that you defined above? This conditional is next to impossible to decipher.

Copy link
Member Author

Choose a reason for hiding this comment

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

Use it for what? I'm not sure what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

You recreate entryQUrl within the conditional when you do QUrl(entryUrl)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes..

@varjolintu varjolintu force-pushed the browser_port_fix branch 4 times, most recently from e8d9be1 to 3d82da4 Compare July 11, 2018 09:03
@varjolintu
Copy link
Member Author

Conflict resolved.

if ((!url.isEmpty() && hostname.contains(url))
|| (matchUrlScheme(url) && hostname.endsWith(QUrl(url).host()))) {
if ((!entryUrl.isEmpty() && hostname.contains(entryUrl))
|| (matchUrlScheme(entryUrl) && hostname.endsWith(QUrl(entryUrl).host()))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're still recreating entryQUrl here

droidmonkey
droidmonkey approved these changes Jul 14, 2018
@droidmonkey droidmonkey merged commit 057cf6a into keepassxreboot:release/2.3.4 Jul 14, 2018
@varjolintu varjolintu deleted the browser_port_fix branch July 15, 2018 04:58
droidmonkey added a commit that referenced this pull request Aug 22, 2018
- Show all URL schemes in entry view [#1768]
- Disable merge when database is locked [#1975]
- Fix intermittent crashes with favorite icon downloads [#1980]
- Provide potential crash warning to Qt 5.5.x users [#2211]
- Disable apply button when creating new entry/group to prevent data loss [#2204]
- Allow for 12 hour timeout to lock idle database [#2173]
- Multiple SSH Agent fixes [#1981, #2117]
- Multiple Browser Integration enhancements [#1993, #2003, #2055, #2116, #2159, #2174, #2185]
- Fix browser proxy application not closing properly [#2142]
- Add real names and Patreon supporters to about dialog [#2214]
- Add settings button to toolbar, Donate button, and Report a Bug button to help menu [#2214]
- Enhancements to release-tool to appsign intermediate build products [#2101]
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.

4 participants