Skip to content

fix: detect files app in guest whitelist checking #618

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Jan 23, 2024

Description

In AppWhitelist.php function preSetup:

			$path = \OC::$server->getRequest()->getPathInfo();

Path can be something like /dav/files/[email protected]/

In that case, getRequestedApp($path) was returning false

Actually a path like that is to do with the files app. So return 'files'.

Then we can do if (!\in_array($app, $whitelist, true)) (use a strict check that the app has to be in the whitelist array). This will help ensure that only requests to "app URLs" that are whitelisted will work for guests.

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@phil-davis phil-davis self-assigned this Jan 23, 2024
@phil-davis
Copy link
Contributor Author

@DeepDiver1975 I found that dav/files was not being detected as a URL belonging to the "files" app. That was what prevented adding the strict check to in_array

@DeepDiver1975
Copy link
Member

nice catch - what strange logic .....

@phil-davis
Copy link
Contributor Author

And maybe it should be more generic to match any:
/dav/ is "dav" app.
/webdav/ is also "dav" app.
?

Does it matter? Is there any use-case where the admin wants to whitelist "dav" but not "files", or whitelist "files" but not "dav"?
I doubt it - there is little point having guest users if they are not given permission to use both the "dav" and "files" apps.

@pako81 pako81 mentioned this pull request Jan 23, 2024
43 tasks
@phil-davis phil-davis requested a review from jnweiger February 20, 2024 06:31
@phil-davis
Copy link
Contributor Author

@DeepDiver1975 @jnweiger I rebased.
IMO this is "a good thing" and can be merged.

Copy link

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.

2 participants