Skip to content

Commit

Permalink
Ensure browse endpoints use relative paths
Browse files Browse the repository at this point in the history
This prevents the browse endpoints for Spaces and Locations to
traverse the file system up their full path.
  • Loading branch information
replaceafill authored and sevein committed Jul 8, 2021
1 parent f2c8abe commit e30f9db
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 0 deletions.
24 changes: 24 additions & 0 deletions storage_service/locations/api/resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@

# Third party dependencies, alphabetical
import bagit

try:
from pathlib import Path
except ImportError:
from pathlib2 import Path
import six
from tastypie.authentication import (
BasicAuthentication,
Expand Down Expand Up @@ -68,6 +73,15 @@
LOGGER = logging.getLogger(__name__)


def _is_relative_path(path1, path2):
"""Ensure path2 is relative to path1"""
try:
Path(path2).resolve().relative_to(path1)
return True
except ValueError:
return False


# FIXME ModelResources with ForeignKeys to another model don't work with
# validation = CleanedDataFormValidation On creation, it errors with:
# "Select a valid choice. That choice is not one of the available choices."
Expand Down Expand Up @@ -318,6 +332,11 @@ def browse(self, request, bundle, **kwargs):
if not path.startswith(space.path):
path = os.path.join(space.path, path)

if not _is_relative_path(space.path, path):
return http.HttpBadRequest(
_("The path parameter must be relative to the space path")
)

objects = self.get_objects(space, path)

return self.create_response(request, objects)
Expand Down Expand Up @@ -478,6 +497,11 @@ def browse(self, request, bundle, **kwargs):
if not path.startswith(location_path):
path = os.path.join(location_path, path)

if not _is_relative_path(location_path, path):
return http.HttpBadRequest(
_("The path parameter must be relative to the location path")
)

objects = self.get_objects(location.space, path)

return self.create_response(request, objects)
Expand Down
49 changes: 49 additions & 0 deletions storage_service/locations/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@
import json
import os
import shutil
import uuid
import vcr

from django.contrib.auth.models import User
from django.test import TestCase
from django.urls import reverse
from django.utils.six.moves.urllib.parse import urlparse

from locations import models
Expand Down Expand Up @@ -80,6 +82,25 @@ def test_create_space(self):
protocol_model = models.S3.objects.get(space_id=response_data["uuid"])
assert protocol_model.endpoint_url == data["endpoint_url"]

def test_browse_doesnt_traverse_up(self):
space_uuid = str(uuid.uuid4())
models.Space.objects.create(
uuid=space_uuid,
path="/home/foo",
)
response = self.client.get(
reverse(
"browse",
kwargs={"api_name": "v2", "resource_name": "space", "uuid": space_uuid},
),
{"path": "/home/foo/../../etc"},
)
assert response.status_code == 400
assert (
"The path parameter must be relative to the space path"
in response.content.decode("utf8")
)


class TestLocationAPI(TestCase):

Expand Down Expand Up @@ -257,6 +278,34 @@ def test_cant_move_to_disabled_locations(self):
# Verify error
assert response.status_code == 404

def test_browse_doesnt_traverse_up(self):
location_uuid = str(uuid.uuid4())
space = models.Space.objects.create(
uuid=str(uuid.uuid4()),
path="/home",
)
models.Location.objects.create(
uuid=location_uuid,
space=space,
relative_path="foo",
)
response = self.client.get(
reverse(
"browse",
kwargs={
"api_name": "v2",
"resource_name": "location",
"uuid": location_uuid,
},
),
{"path": base64.b64encode(b"/home")},
)
assert response.status_code == 400
assert (
"The path parameter must be relative to the location path"
in response.content.decode("utf8")
)


class TestPackageAPI(TempDirMixin, TestCase):

Expand Down

0 comments on commit e30f9db

Please sign in to comment.