Prevent protected files from being modified by unprivileged users. #2430
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Alex-Jordan
approved these changes
Jun 12, 2024
drgrice1
force-pushed
the
file-manager-protected-files
branch
2 times, most recently
from
1a405b3 to
b8a7fb8
Compare
|
I added a second commit to this that addresses #2437. I also slightly tweaked how protected file uploads are handled. Instead of just refusing to do it, it now gives the user a chance to rename it. |
drgrice1
force-pushed
the
file-manager-protected-files
branch
3 times, most recently
from
2595912 to
9a571ad
Compare
drgrice1
force-pushed
the
file-manager-protected-files
branch
from
9a571ad to
82b855a
Compare
drgrice1
force-pushed
the
file-manager-protected-files
branch
from
82b855a to
1b2b9bf
Compare
By "protected files" I mean the files listed in the `$uneditableCourseFiles` list in the course environemt that is originally defined in `defaults.config`. This usually contains the files `course.conf` and `simple.conf`. As before, only users with the "edit_restricted_files" permission can edit these files. The only change here is that `Mojo::File::realpath` is used instead of the `File::Spec::canonpath`. This is a more reliable method since it properly resolves to a path without `..` (canonpath does not do this). It is possible to use `..` to create a filename that wouild still resolve to one of the protected files. For instance, `templates/../course.conf` could be used to overwrite `course.conf`. The canonpath method would not correctly realize that those are the same file paths. If a user that does not have the permission to "edit_restricted_files", and the user attempts to upload a file in the list, then the user will be forced to choose a new name for the upload, or cancel. The overwrite option will not be given. Users with the "edit_restricted_files" permission can still overwrite the file. If a user that does not have the permission to "edit_restricted_files" extracts an archive that would overwrite a file in the list, then that file will be skipped and will not be extracted. A message will notify the user that this file from the archive is not extracted. Deletion or renaming of one of the files in the list is not allowed. Not even for users that have the "edit_restricted_files" permission. These files are required, and there is no reason that this should ever be done. Finally, required course directories (all directories in the `courseDirs` hash in the course environment) are not allowed to be deleted or renamed by any user. These directories are required, and so there is not reason that even a user with the "edit_restricted_files" permission should be allowed to do this.
invalid file names, rather than attempting to fix and return a workable file name. Also, instead of just giving a message if a user attempts to upload a protected file, give the user a chance to rename it.
drgrice1
force-pushed
the
file-manager-protected-files
branch
from
1b2b9bf to
165fa8e
Compare
somiaj
approved these changes
Jul 30, 2024
|
The changes to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By "protected files" I mean the files listed in the
$uneditableCourseFileslist in the course environemt that is originally defined indefaults.config. This usually contains the filescourse.confandsimple.conf.As before, only users with the "edit_restricted_files" permission can edit these files. The only change here is that
Mojo::File::realpathis used instead of theFile::Spec::canonpath. This is a more reliable method since it properly resolves to a path without..(canonpath does not do this). It is possible to use..to create a filename that wouild still resolve to one of the protected files. For instance,templates/../course.confcould be used to overwritecourse.conf. The canonpath method would not correctly realize that those are the same file paths.If a user that does not have the permission to "edit_restricted_files", and the user attempts to upload a file in the list, then the user will be forced to choose a new name for the upload, or cancel. The overwrite option will not be given. Users with the "edit_restricted_files" permission can still overwrite the file.
If a user that does not have the permission to "edit_restricted_files" extracts an archive that would overwrite a file in the list, then that file will be skipped and will not be extracted. A message will notify the user that this file from the archive is not extracted.
Deletion or renaming of one of the files in the list is not allowed. Not even for users that have the "edit_restricted_files" permission. These files are required, and there is no reason that this should ever be done.
Finally, required course directories (all directories in the
courseDirshash in the course environment) are not allowed to be deleted or renamed by any user. These directories are required, and so there is not reason that even a user with the "edit_restricted_files" permission should be allowed to do this.This is intended to resolve discussion #2427.