Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate UUID Format in Public API Requests to Prevent 500 Errors #4794

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions contentcuration/kolibri_public/import_metadata_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from kolibri_content.constants.schema_versions import MIN_CONTENT_SCHEMA_VERSION # Use kolibri_content
from kolibri_public import models # Use kolibri_public models
from kolibri_public.views import metadata_cache
from rest_framework import status
from rest_framework.generics import get_object_or_404
from rest_framework.permissions import AllowAny
from rest_framework.response import Response
Expand Down Expand Up @@ -69,6 +70,14 @@ def retrieve(self, request, pk=None): # noqa: C901
:return: an object with keys for each content metadata table and a schema_version key
"""

try:
UUID(pk)
except ValueError:
return Response(
{"error": "Invalid UUID format."},
status=status.HTTP_400_BAD_REQUEST
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is a retrieve operation, a 404 would be fine here.

Also, you could just put a try catch around:

node = get_object_or_404(models.ContentNode.objects.all(), pk=pk)

Catch the value error and raise a 404 there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like in Kolibri that line was updated to use self.get_object() - not sure if that's catching the ValueError or not, but I suspect it may be: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/content/public_api.py#L75

)

content_schema = request.query_params.get(
"schema_version", self.default_content_schema
)
Expand Down
11 changes: 11 additions & 0 deletions contentcuration/kolibri_public/tests/test_content_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,17 @@ def _recurse_and_assert(self, data, nodes, recursion_depth=0):
)
return recursion_depth if not recursion_depths else max(recursion_depths)

def test_contentnode_tree_invalid_uuid(self):
invalid_uuid = "8f0a5b9d89795"

response = self._get(
reverse("publiccontentnode_tree-detail", kwargs={"pk": invalid_uuid})
)

self.assertEqual(response.status_code, 400)

self.assertEqual(response.data["error"], "Invalid UUID format.")

def test_contentnode_tree(self):
response = self._get(
reverse("publiccontentnode_tree-detail", kwargs={"pk": self.root.id})
Expand Down
11 changes: 11 additions & 0 deletions contentcuration/kolibri_public/tests/test_importmetadata_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,17 @@ def test_import_metadata_through_tags(self):
def test_import_metadata_tags(self):
self._assert_data(public.ContentTag, content.ContentTag, self.tags)

def test_import_metadata_invalid_uuid(self):
invalid_uuid = "8f0a5b9d89795"

response = self.client.get(
reverse("publicimportmetadata-detail", kwargs={"pk": invalid_uuid})
)

self.assertEqual(response.status_code, 400)

self.assertEqual(response.data["error"], "Invalid UUID format.")

def test_schema_version_too_low(self):
response = self.client.get(
reverse("publicimportmetadata-detail", kwargs={"pk": self.node.id})
Expand Down
12 changes: 10 additions & 2 deletions contentcuration/kolibri_public/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import re
from collections import OrderedDict
from functools import reduce
from uuid import UUID

from django.core.exceptions import ValidationError
from django.db.models import Exists
Expand All @@ -35,6 +36,7 @@
from kolibri_public.search import get_available_metadata_labels
from kolibri_public.stopwords import stopwords_set
from le_utils.constants import content_kinds
from rest_framework import status
from rest_framework.permissions import AllowAny
from rest_framework.response import Response

Expand All @@ -45,7 +47,6 @@
from contentcuration.viewsets.base import BaseValuesViewset
from contentcuration.viewsets.base import ReadOnlyValuesViewset


logger = logging.getLogger(__name__)


Expand Down Expand Up @@ -697,8 +698,15 @@ def retrieve(self, request, pk=None):
:return: an object representing the parent with a pagination object as "children"
"""

queryset = self.get_tree_queryset(request, pk)
try:
UUID(pk)
except ValueError:
return Response(
{"error": "Invalid UUID format."},
status=status.HTTP_400_BAD_REQUEST
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, can 404.

As this is mostly just copy pasted from Kolibri, we could just update the get_tree_queryset method to match the Kolibri implementation: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/content/api.py#L1028

(this is useful because it means we continue to keep the two implementations as parallel as possible).

)

queryset = self.get_tree_queryset(request, pk)
# We explicitly order by lft here, so that the nodes are in tree traversal order, so we can iterate over them and build
# out our nested representation, being sure that any ancestors have already been processed.
nodes = self.serialize(queryset.order_by("lft"))
Expand Down