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

fix: respect upload and directory listing permissions #1352

Merged
merged 7 commits into from Jun 10, 2023

Conversation

fsbraun
Copy link
Sponsor Member

@fsbraun fsbraun commented Jun 8, 2023

Description

This PR fixes a security issue: A staff user without any permissions

  • Can browse the filer folder structure
  • List files in a folder
  • Add files
  • Move files and folders

Thanks to Akshar Tank for reporting this issue.

Fix

This fix enforces the following permissions

  • can_use_directory_listing
  • change_folder
  • add_folder
  • add_file(also for drag&drop upload)

Desired side effects

  • Users need to have the permission add_file to upload files
  • Users need to have the permission can_use_directory_listing to browse the filer folders

Related resources

  • #...
  • #...

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

@fsbraun fsbraun requested a review from a team June 8, 2023 23:39
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #1352 (9f439a9) into master (34803bc) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1352      +/-   ##
==========================================
- Coverage   72.39%   72.35%   -0.05%     
==========================================
  Files          72       72              
  Lines        3268     3277       +9     
  Branches      532      534       +2     
==========================================
+ Hits         2366     2371       +5     
- Misses        735      739       +4     
  Partials      167      167              
Impacted Files Coverage Δ
filer/admin/clipboardadmin.py 94.11% <100.00%> (+0.67%) ⬆️
filer/admin/folderadmin.py 71.94% <100.00%> (+0.08%) ⬆️
filer/models/filemodels.py 85.20% <100.00%> (-1.35%) ⬇️
filer/models/foldermodels.py 85.99% <100.00%> (-0.49%) ⬇️

@fsbraun fsbraun requested review from vinitkumar and jrief June 9, 2023 00:17
tests/test_admin.py Fixed Show fixed Hide fixed
tests/test_admin.py Outdated Show resolved Hide resolved
filer/models/filemodels.py Show resolved Hide resolved
'Filedata': file_obj,
'jsessionid': self.client.session.session_key
}
response = self.client.post(url, post_data, **extra_headers) # noqa

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable response is not used.
tests/test_admin.py Fixed Show fixed Hide fixed
tests/test_admin.py Fixed Show fixed Hide fixed
tests/test_admin.py Fixed Show fixed Hide fixed
'admin:filer-ajax_upload',
kwargs={'folder_id': folder.pk}
) + '?filename=%s' % self.image_name
response = self.client.post( # noqa

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable response is not used.
tests/test_admin.py Fixed Show fixed Hide fixed
tests/test_admin.py Fixed Show fixed Hide fixed
@fsbraun fsbraun merged commit f90715e into django-cms:master Jun 10, 2023
28 of 29 checks passed
fsbraun added a commit that referenced this pull request Jun 10, 2023
* fix: respect `can_use_directory_listing`, `change_folder`, `add_folder`, `add_file` permissions

* Update tests

* fix flake8 error

* Close files in tests

* Add test for has_... permissions of File and Folder class

* Remove unused variables from tests

* Remove unnecessary noqa
@fsbraun fsbraun deleted the fix/respect_permissions branch June 28, 2023 16:59
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.

None yet

3 participants