X Tutup
Skip to content

check public link already authenticated before checking password of link#38016

Merged
phil-davis merged 1 commit intomasterfrom
fix-unwanted-hooks
Oct 20, 2020
Merged

check public link already authenticated before checking password of link#38016
phil-davis merged 1 commit intomasterfrom
fix-unwanted-hooks

Conversation

@karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented Oct 17, 2020

Description

ShareManager was checking password of already authenticated public links.
This situation has been led to wrong "share.failedpasswordcheck" events emitting in already authenticated links and led to the related issue on brute force protection app. This problem has been resolved by first checking link already authenticated.

@micbar I would like to add this PR to 10.6 if it is possible.

Related Issue

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@karakayasemi karakayasemi added this to the development milestone Oct 17, 2020
@karakayasemi karakayasemi self-assigned this Oct 17, 2020
@karakayasemi karakayasemi mentioned this pull request Oct 17, 2020
Closed
@phil-davis
Copy link
Contributor

phil-davis commented Oct 17, 2020

I restarted drone - in the previous run there were lots of pipelines that had 10-minute timeouts waiting for stuff.

mrow4a
mrow4a requested changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@mrow4a mrow4a left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Before this PR we always checked for password first (even if public_link_authenticated is set), or was that just a bug? . Do you think this can have some security implications? Could you comment on this in the code (if relevant)?

@jvillafanez
Copy link
Member

jvillafanez commented Oct 19, 2020

It might be a better idea to let the shareManager->checkPassword() function handle the "public_link_authenticated" session variable.
Basically, you don't do anything with the "public_link_authenticated" variable outside the function, but call always to the checkPassword function. It will be the checkPassword function the one that checks (and sets) the session variable. It could also provide a better control of when the "share.failedpasswordcheck" event is triggered.

@phil-davis
Copy link
Contributor

phil-davis commented Oct 19, 2020

I restarted ddrone CI - there were some silly startup fails of PHP unit test pipelines.

@micbar
Copy link
Contributor

micbar commented Oct 19, 2020

@karakayasemi Can we merge this soon to get into 10.6?

@phil-davis
Copy link
Contributor

phil-davis commented Oct 19, 2020

@micbar - @mrow4a gave a "change requested" review comment. That needs to be cleared (assuming the comment has been addressed...)

@karakayasemi
Copy link
Contributor Author

karakayasemi commented Oct 19, 2020 •
edited
Loading

Before this PR we always checked for password first (even if public_link_authenticated is set), or was that just a bug? . Do you think this can have some security implications? Could you comment on this in the code (if relevant)?

@mrow4a if checkPassword fails, the code looks public_link_authenticated anyway. So, in my opinion there is no risk in terms of security.

It might be a better idea to let the shareManager->checkPassword() function handle the "public_link_authenticated" session variable.

@jvillafanez I thought same first but there are other usages of checkPassword method. Since it will change the inside logic of checkPassword, I did not want to touch it to keep the change minimal.

@karakayasemi Can we merge this soon to get into 10.6?

@micbar IMHO, PR is ready to merge after CI problems resolved. There were some unrelated CI fails, I hope this time we will see green CI.

@phil-davis
Copy link
Contributor

phil-davis commented Oct 19, 2020

fsweb.test.owncloud.com seems to be down (or not working nicely). All core CI pipelines that test against it are failing, in all PRs currently running.

mrow4a
mrow4a approved these changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@mrow4a mrow4a left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@karakayasemi @jvillafanez I did some research, and indeed this change is bugfix. Also, the public link code is spaghetti a little that begs for refactoring https://github.com/owncloud/core/blob/v10.5.0/apps/files_sharing/lib/Controllers/ShareController.php . Also used in public apps https://github.com/owncloud/richdocuments/blob/740bd280caf35c7e8b6e8e96f05320acd9e689b1/lib/Storage.php#L136-L141 . Considering how it is currently implemented, legit solution.

@phil-davis
Copy link
Contributor

phil-davis commented Oct 20, 2020

Rebased to get fresh CI.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 20, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phil-davis phil-davis merged commit e28fc36 into master Oct 20, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-unwanted-hooks branch October 20, 2020 03:33
@karakayasemi karakayasemi mentioned this pull request Nov 12, 2020
Closed
@karakayasemi karakayasemi mentioned this pull request Dec 11, 2020
Closed
@karakayasemi karakayasemi mentioned this pull request Feb 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

@mrow4a mrow4a mrow4a approved these changes

@micbar micbar Awaiting requested review from micbar

Assignees

@karakayasemi karakayasemi

Labels

3 - To Review

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

(1.1.0) Password Protected Public Link Subfolder issue

5 participants

@karakayasemi @phil-davis @jvillafanez @micbar @mrow4a
X Tutup