Possible wrong check in django.utils._os

[Moorthy RS]

I have a model that accepts an Image. And I have specified the upload_dir to /home/myproject/media/upload. And whenever I try to save an image, it gives an error "SuspiciousOperation: Attempted access to /home/myproject/media/upload/image.png denied". Problem appears only when used with Apache, but with the development server, there were no errors. I searched and found that the only suggestion was a possible missing or extra "/" separator. I tried specifying without and with the leading slash, but with no luck.

I debugged around and found this code, responsible for the error:

    if not final_path.startswith(base_path) \
       or final_path[base_path_len:base_path_len+1] not in ('', sep):
        raise ValueError('the joined path is located outside of the base path'
                         ' component')

I found base_path was a "/" and final_path was "/home/myproject/media/upload". And hence "final_path.startswith(base_path)" returns true and "final_path[base_path_len:base_path_len+1] returns 'h' which IS not in empty string or sep. So the if condition should fail, but it was succeeding. I changed the code to this snippet (by adding appropriate paranthesis like "if not (condition)") and it started working correctly!

    if not (final_path.startswith(base_path) \
       or final_path[base_path_len:base_path_len+1] not in ('', sep)):
        raise ValueError('the joined path is located outside of the base path'
                         ' component')

I am a newbie in python (and as well in django), but I see the lack of paranthesis has got the precedence going wrong, and an unexpected result.

This needs to be corrected, as indicated above.

[Karen Tracey]

No, the problem is not caused by missing parentheses. Consider the comment above that code block, which explains what the check is doing:

    # Ensure final_path starts with base_path and that the next character after
    # the final path is os.sep (or nothing, in which case final_path must be
    # equal to base_path).
    if not final_path.startswith(base_path) \
       or final_path[base_path_len:base_path_len+1] not in ('', sep):
        raise ValueError('the joined path is located outside of the base path'
                         ' component')

So, the code wishes to proceed with the all-clear if: [A and B]

Negating that to check for the error condition is: [not (A and B)], or equivalently [(not A) or (not B)], which is how it is coded.

Your proposed change is to make that [not (A or B)], which is not the same thing at all. Consider how it would work with some paths other than base_path="/", as compared to the original:

>>> # check1 -- as originally implemented
...
>>> def check1(base_path, final_path):
...    base_path_len = len(base_path)
...    if not final_path.startswith(base_path) \
...       or final_path[base_path_len:base_path_len+1] not in ('', '/'):
...       raise Exception('joined path is located outside of base path')
...    return 'OK'
...
>>> # final path being somewhere under base path is OK
...
>>> check1("/home/myproject", "/home/myproject/file1.gif")
'OK'
>>> # final path being entirely outside of base path is not OK
...
>>> check1("/home/myproject", "/home/yourproject/file1.gif")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 5, in check1
Exception: joined path is located outside of base path
>>> # not OK even if the names start with the same prefix -- the final path must
... # have a path seperator after the end of the common part to be "under" base path
...
>>> check1("/home/myproject", "/home/myproject_alt/file1.gif")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 5, in check1
Exception: joined path is located outside of base path
>>>
>>> # check2 -- with proposed change
...
>>> def check2(base_path, final_path):
...    base_path_len = len(base_path)
...    if not (final_path.startswith(base_path) \
...       or final_path[base_path_len:base_path_len+1] not in ('', '/')):
...       raise Exception('joined path is located outside of base path!')
...    return 'OK'
...
>>> # final path being somewhere under base path is OK
...
>>> check2("/home/myproject", "/home/myproject/file1.gif")
'OK'
>>> # OK, that one worked
...
>>> # final path being entirely outside of base path is not OK
...
>>> check2("/home/myproject", "/home/yourproject/file1.gif")
'OK'
>>> # Wrong answer!
...
>>> # not OK even if the names start with the same prefix -- the final path must
... # have a path seperator after the end of the common part to be "under" base path
...
>>> check2("/home/myproject", "/home/myproject_alt/file1.gif")
'OK'
>>> # Wrong answer!
...
>>>

The problem you have found has to do with how the code behaves specifically when base_path="/", which is not really a normal case, so I'm not entirely sure what should be done about it. Apparently you have MEDIA_ROOT set to "/"? That means you are saying it is OK for web users to upload files anywhere in the file system. Are you sure you want to be doing that?

[Karen Tracey]

Replying to kmtracey:

Your proposed change is to make that [not (A or B)], which is not the same thing at all. Consider how it would work with some paths other than base_path="/", as compared to the original:

Typo here, the proposed change is to [not (A or not B)], still not the same thing.

[Jacob]

I think we need more info from the original poster to know if it is indeed MEDIA_ROOT = / causing this, or something else.

rsmoorthy, if you can provide more information (responding to kmtracy's questions) then please feel free to re-open.

Thanks!