From 9574f84b6fd14e9879b6d27f23ad049def6f6803 Mon Sep 17 00:00:00 2001 From: DeepikaGhodki Date: Tue, 19 Jul 2022 15:05:54 -0400 Subject: [PATCH 1/5] Handle forbidden characters in asset path --- dandiapi/api/tests/test_asset.py | 45 ++++++++++++++++++++++++++++++++ dandiapi/api/views/asset.py | 28 +++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index 680f9de53..1730c2c0c 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -538,6 +538,51 @@ def test_asset_create(api_client, user, draft_version, asset_blob): assert draft_version.status == Version.Status.PENDING +@pytest.mark.parametrize( + 'path,valid', + [ + ('foo.txt', True), + ('/foo', False), + ('', False), + ("/", False), + ("./foo", False), + ('../foo', False), + ("foo/.", False), + ("foo/..", False), + ("foo/./bar", False), + ("foo/../bar", False), + ("foo//bar", False), + ("foo\0bar", False), + ("foo/.bar", True), + ], +) +@pytest.mark.django_db +def test_asset_create_path_validation(api_client, user, draft_version, asset_blob, path, valid): + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + + metadata = { + 'schemaVersion': settings.DANDI_SCHEMA_VERSION, + 'encodingFormat': 'application/x-nwb', + 'path': path, + 'meta': 'data', + 'foo': ['bar', 'baz'], + '1': 2, + } + + resp = api_client.post( + f'/api/dandisets/{draft_version.dandiset.identifier}' + f'/versions/{draft_version.version}/assets/', + {'metadata': metadata, 'blob_id': asset_blob.blob_id}, + format='json', + ) + + if valid: + assert resp.status_code == 200, resp.data + else: + assert resp.status_code != 200, resp.data + + @pytest.mark.django_db def test_asset_create_embargo(api_client, user, draft_version, embargoed_asset_blob): assign_perm('owner', user, draft_version.dandiset) diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 763b4ec03..eff9fadc8 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -15,6 +15,7 @@ import os.path from urllib.parse import urlencode +from pathlib import Path, PurePath from django.core.paginator import EmptyPage, Page, Paginator from django.db import transaction @@ -251,9 +252,33 @@ def validate(self, data): {'blob_id': 'Exactly one of blob_id or zarr_id must be specified.'} ) - if 'path' not in data['metadata']: + if 'path' not in data['metadata'] or not data['metadata']['path']: raise serializers.ValidationError({'metadata': 'No path specified in metadata.'}) + path = data['metadata']['path'] + # paths starting with / + if PurePath.is_absolute(Path(path)): + raise serializers.ValidationError( + {'metadata': 'Absolute path not allowed for an asset'} + ) + + sub_paths = path.split('/') + # checking for . in path + for sub_path in sub_paths: + if len(set(sub_path)) == 1 and sub_path[0] == '.': + raise serializers.ValidationError( + {'metadata': 'Invalid characters (.) in file path'} + ) + + # Invalid characters ['\0'] + filepath_regex = '^[^\0]+$' + # match characters repeating more than once + multiple_occurrence_regex = '[/]{2,}' + if not re.search(filepath_regex, path): + raise serializers.ValidationError({'metadata': 'Invalid characters (\0) in file name'}) + if re.search(multiple_occurrence_regex, path): + raise serializers.ValidationError({'metadata': 'Invalid characters (/)) in file name'}) + return data @@ -409,6 +434,7 @@ def create(self, request, versions__dandiset__pk, versions__version): _maybe_validate_asset_metadata(asset) serializer = AssetDetailSerializer(instance=asset) + print(serializer.data) return Response(serializer.data, status=status.HTTP_200_OK) @swagger_auto_schema( From b4f012a00700ed8d934eb6beec3896003f7a0011 Mon Sep 17 00:00:00 2001 From: DeepikaGhodki Date: Tue, 19 Jul 2022 17:56:31 -0400 Subject: [PATCH 2/5] Trimming leading slashes in asset path --- dandiapi/api/tests/factories.py | 7 +++++++ dandiapi/api/tests/test_asset.py | 18 +++++++++--------- dandiapi/api/views/asset.py | 2 +- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/dandiapi/api/tests/factories.py b/dandiapi/api/tests/factories.py index 31090804d..20ba187a8 100644 --- a/dandiapi/api/tests/factories.py +++ b/dandiapi/api/tests/factories.py @@ -191,6 +191,13 @@ def metadata(self): del metadata[key] return metadata + @classmethod + def _adjust_kwargs(cls, **kwargs): + path = kwargs['path'] + if path[0] == '/': + kwargs['path'] = path.replace(path[0], '') + return kwargs + class PublishedAssetFactory(DraftAssetFactory): @classmethod diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index 1730c2c0c..220582b33 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -544,16 +544,16 @@ def test_asset_create(api_client, user, draft_version, asset_blob): ('foo.txt', True), ('/foo', False), ('', False), - ("/", False), - ("./foo", False), + ('/', False), + ('./foo', False), ('../foo', False), - ("foo/.", False), - ("foo/..", False), - ("foo/./bar", False), - ("foo/../bar", False), - ("foo//bar", False), - ("foo\0bar", False), - ("foo/.bar", True), + ('foo/.', False), + ('foo/..', False), + ('foo/./bar', False), + ('foo/../bar', False), + ('foo//bar', False), + ('foo\0bar', False), + ('foo/.bar', True), ], ) @pytest.mark.django_db diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index eff9fadc8..c176554d7 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -14,8 +14,8 @@ MinioStorage = type('FakeMinioStorage', (), {}) import os.path -from urllib.parse import urlencode from pathlib import Path, PurePath +from urllib.parse import urlencode from django.core.paginator import EmptyPage, Page, Paginator from django.db import transaction From eaef457c79e609329171956f2dd167108b639ca1 Mon Sep 17 00:00:00 2001 From: DeepikaGhodki Date: Tue, 19 Jul 2022 18:18:57 -0400 Subject: [PATCH 3/5] Setting relative path for assets --- dandiapi/api/tests/factories.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/dandiapi/api/tests/factories.py b/dandiapi/api/tests/factories.py index 20ba187a8..eace897e9 100644 --- a/dandiapi/api/tests/factories.py +++ b/dandiapi/api/tests/factories.py @@ -174,7 +174,7 @@ class DraftAssetFactory(factory.django.DjangoModelFactory): class Meta: model = Asset - path = factory.Faker('file_path', extension='nwb') + path = factory.Faker('file_path', absolute=False, extension='nwb') blob = factory.SubFactory(AssetBlobFactory) @factory.lazy_attribute @@ -191,13 +191,6 @@ def metadata(self): del metadata[key] return metadata - @classmethod - def _adjust_kwargs(cls, **kwargs): - path = kwargs['path'] - if path[0] == '/': - kwargs['path'] = path.replace(path[0], '') - return kwargs - class PublishedAssetFactory(DraftAssetFactory): @classmethod From be969467f82e8c58bca3867ffc3460b95637b67b Mon Sep 17 00:00:00 2001 From: DeepikaGhodki Date: Mon, 25 Jul 2022 03:36:03 -0400 Subject: [PATCH 4/5] Updated test case with status code assertion --- dandiapi/api/tests/test_asset.py | 37 ++++++++++++++++---------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index 220582b33..244dee2b8 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -539,25 +539,27 @@ def test_asset_create(api_client, user, draft_version, asset_blob): @pytest.mark.parametrize( - 'path,valid', + 'path,expected_status_code', [ - ('foo.txt', True), - ('/foo', False), - ('', False), - ('/', False), - ('./foo', False), - ('../foo', False), - ('foo/.', False), - ('foo/..', False), - ('foo/./bar', False), - ('foo/../bar', False), - ('foo//bar', False), - ('foo\0bar', False), - ('foo/.bar', True), + ('foo.txt', 200), + ('/foo', 400), + ('', 400), + ('/', 400), + ('./foo', 400), + ('../foo', 400), + ('foo/.', 400), + ('foo/..', 400), + ('foo/./bar', 400), + ('foo/../bar', 400), + ('foo//bar', 400), + ('foo\0bar', 400), + ('foo/.bar', 200), ], ) @pytest.mark.django_db -def test_asset_create_path_validation(api_client, user, draft_version, asset_blob, path, valid): +def test_asset_create_path_validation( + api_client, user, draft_version, asset_blob, path, expected_status_code +): assign_perm('owner', user, draft_version.dandiset) api_client.force_authenticate(user=user) @@ -577,10 +579,7 @@ def test_asset_create_path_validation(api_client, user, draft_version, asset_blo format='json', ) - if valid: - assert resp.status_code == 200, resp.data - else: - assert resp.status_code != 200, resp.data + assert resp.status_code == expected_status_code, resp.data @pytest.mark.django_db From 14b94a73957937a1329331ecbdf3c31e986fbf1d Mon Sep 17 00:00:00 2001 From: DeepikaGhodki Date: Mon, 25 Jul 2022 16:11:51 -0400 Subject: [PATCH 5/5] Resolved PR comments --- dandiapi/api/tests/test_asset.py | 5 +---- dandiapi/api/views/asset.py | 10 +++------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index 244dee2b8..c59cf0f74 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -567,9 +567,6 @@ def test_asset_create_path_validation( 'schemaVersion': settings.DANDI_SCHEMA_VERSION, 'encodingFormat': 'application/x-nwb', 'path': path, - 'meta': 'data', - 'foo': ['bar', 'baz'], - '1': 2, } resp = api_client.post( @@ -784,7 +781,7 @@ def test_asset_create_no_path(api_client, user, draft_version, asset_blob): format='json', ) assert resp.status_code == 400 - assert resp.data == {'metadata': ['No path specified in metadata.']} + assert resp.data == {'metadata': ['No path specified in metadata.']}, resp.data @pytest.mark.django_db diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index c176554d7..0f64254d7 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -14,7 +14,7 @@ MinioStorage = type('FakeMinioStorage', (), {}) import os.path -from pathlib import Path, PurePath +from pathlib import PurePosixPath from urllib.parse import urlencode from django.core.paginator import EmptyPage, Page, Paginator @@ -251,13 +251,12 @@ def validate(self, data): raise serializers.ValidationError( {'blob_id': 'Exactly one of blob_id or zarr_id must be specified.'} ) - if 'path' not in data['metadata'] or not data['metadata']['path']: raise serializers.ValidationError({'metadata': 'No path specified in metadata.'}) path = data['metadata']['path'] # paths starting with / - if PurePath.is_absolute(Path(path)): + if PurePosixPath(path).is_absolute(): raise serializers.ValidationError( {'metadata': 'Absolute path not allowed for an asset'} ) @@ -270,11 +269,9 @@ def validate(self, data): {'metadata': 'Invalid characters (.) in file path'} ) - # Invalid characters ['\0'] - filepath_regex = '^[^\0]+$' # match characters repeating more than once multiple_occurrence_regex = '[/]{2,}' - if not re.search(filepath_regex, path): + if '\0' in path: raise serializers.ValidationError({'metadata': 'Invalid characters (\0) in file name'}) if re.search(multiple_occurrence_regex, path): raise serializers.ValidationError({'metadata': 'Invalid characters (/)) in file name'}) @@ -434,7 +431,6 @@ def create(self, request, versions__dandiset__pk, versions__version): _maybe_validate_asset_metadata(asset) serializer = AssetDetailSerializer(instance=asset) - print(serializer.data) return Response(serializer.data, status=status.HTTP_200_OK) @swagger_auto_schema(