Improve file name check for scoring download exploit bug. #774
Conversation
…me from the FileManager. This insures that the file name corresponds to the same rules used by the FileManager.
…heckName from FileManager. This actually cures another exploit. By entering an illegal path one might have been able to clobber files that were not in the scoring directory. As it is one can replace files already in the scoring directory. This is probably a feature not a bug.
|
No problem. I'll wait until you give the word to test, but is to be tested the same way as the previous PR? |
| unless ( $file eq WeBWorK::ContentGenerator::Instructor::FileManager::checkName($file) ) { # | ||
| $self->addbadmessage("Your file name is not valid. ". | ||
| "A file name cannot begin with a dot, it cannot be empty, it cannot contain a " . | ||
| "directory path component and only the characters [^-_.a-zA-Z0-9 ] are allowed." |
There was a problem hiding this comment.
I think you want to remove the ^ from [^-_.a-zA-Z0-9 ] (that negates the character set to set for NOT those characters in the checkName() function).
There was a problem hiding this comment.
Good catch. I fixed the typo in the message.
There was a problem hiding this comment.
It looks like you removed the ^ from a similar line in Scoring.pm, but this instance that Davide commented on in ScoringDownload.pm is still here. Also it is not clear to me if escape tildes are needed here as they are used in the similar message in Scoring.pm.
There was a problem hiding this comment.
@Alex-Jordan that's true. This is just a message to the user however. Perhaps it should say:
...only the characters a-z A-Z 0-9 - . and _ are allowed.
to be clearly for someone who doesn't speak regular expressions.
There was a problem hiding this comment.
Yes. I guess I missed that empty space in the RE.
There was a problem hiding this comment.
I've updated the error message so that it is the same in both places. I also mention space as a legal character explicitly instead of being cute and using the regular expression notation.
|
I think this is ready now. To test: 1: Enter the following url as per bug #3793: You should get an error complaining about the badly formed file name.
then press the action button. You should get an error message and that fileName is incorrect. (previously There is also an error message if you forget to pick the sets to be graded. These two error messages should work independently of each other. |
|
I improved the error messages when the file name was bad and made the error messages the same in both the Scoring.pm and ScoringDownload.pm files. |
|
OK, and just to be clear, I can merge this even though I haven't tested it myself? Not a good habit in general, but just getting me to run through a merge? |
A more robust check of the fileName using the FileManager checkName function. This insures
that the fileName for the scoring file conforms precisely to the same rules used by the FileManager for files.
This pull request is not quite ready yet. I have to implement a similar check on the scoring tools page so that you cannot enter an incorrect file name there. I'll do that shortly.
Sorry to put you through the extra work -- I caught this problem this morning around 8 and I thought I had closed the pull request, but I guess I didn't push "commit and close" on the github web page.