Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.I'm creating a job application web form with a File field to upload a resume. I go to create the file field and specify the directory to be sites/default/files/webform/resumes.
I keep getting "The save directory sites/default/files/webform/resumes is not valid."
Permissions check out okay to create the directory, so I went digging into _webform_edit_file_check_directory, specifically this area which verifies if the directory is valid or not.
$base_dir = file_directory_path() . '/webform';
$base_success = file_check_directory($base_dir, FILE_CREATE_DIRECTORY);
$destination_dir = $base_dir . '/' . $element['#value'];
dpm('before');
dpm($destination_dir);
dpm($base_dir);
// Sanity check input to prevent use parent (../) directories.
$real_base = realpath($base_dir);
$real_destination = realpath($destination_dir);
dpm('after');
dpm($real_destination);
dpm($real_base);
if (strpos($real_destination, $real_base) !== 0) {
form_error($element, t('The save directory %directory is not valid.', array('%directory' => $destination_dir)));
}
I get the following output from DPM after trying to add it again:
- before
- sites/default/files/webform/resumes
- sites/default/files/webform
- after
- /home/devact/www/sites/default/files/webform
realpath is correctly returning FALSE for $real_destinaton (as indicated by the blank list item), as that path does not exist (I didn't manually create it). Then realpath checks for the existence of $real_base, which it indeed returns correctly.
So the if statement enters and just prints the error, because it's checking to see if the boolean FALSE is a substring of /home/devact/www/sites/default/files/webform.
Something is off here - the logic should be changed around so that it goes into the else statement to try and create the directory. Am I missing something here?
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | webform_simplify_directory_check.patch | 678 bytes | quicksketch |
| #2 | Screen shot 2011-08-03 at 6.46.37 PM.png | 19.95 KB | bkosborne |
Comments
Comment #1
vernond CreditAttribution: vernond commentedThe only way I've managed to duplicate this error is if I enter sites/default/files/webform/resumes into the 'Upload Directory' textbox on the file component edit page. If I just enter resumes then it works as expected. How are you specifying the upload directory?
Comment #2
bkosborneWrong version, oops.
But yea, I'm specifying the upload directory as you were. See attachment.
But just going purely by logic, isn't it wrong? realpath() returns FALSE if the path does not exist, and that is the case if you specify any upload directory that doesn't already exist. The sanity check if statement seems to be in place to ensure that $real_base makes up the first part of $real_destination. It looks like that would never happen, unless the directory already exists and realpath() returns it.
Comment #3
quicksketchHmm, I think bkosborne is correct. We don't currently try to create the directory before doing the realpath() check. Unfortunately I'm having trouble figuring out how we could check that the paths match correctly without using realpath(). We could try and create the destination directory first and then do the check, but this would mean that a user could use the ../../ path hack to create a directory in some other location... though if it failed I suppose we could delete the newly created folder. That seems like it would work.
So the new approach would be:
- Attempt to create the base directory.
- If successful, attempt to create the sub directory.
- Do the realpath() check to ensure the sub directory is within the base directory.
- If the check fails, delete the (empty) sub directory that was created.
Comment #4
quicksketchHmm, unfortunately I realized that this approach would only work if we wanted to recursively delete the newly created directories. Even with a check to see if we created the directory before deleting it, it seems like this would be a risky bit of code to even have in the project.
Comment #5
flaviovs CreditAttribution: flaviovs commentedWhat do we want to accomplish here? Just check for ".." in path? So why not just:
Comment #6
quicksketch@flaviovs: I like your idea, I think I may have been getting to extravagant with my permissions checks.
Comment #7
quicksketchThis patch corrects the problem by taking @flaviovs's approach, though I switched it to using a regular expression so that it will check both backslashes and forward slashes.
Comment #8
quicksketchI've committed this to both 3.x branches and it will be in the 3.15 version.
Comment #9.0
(not verified) CreditAttribution: commentedclarification