mirror of
https://github.com/inventree/InvenTree.git
synced 2026-09-09 22:30:17 +00:00
Metadata fix (#12771)
* Fix security hole for metadata API * Regression test for field injection
This commit is contained in:
@@ -900,6 +900,9 @@ class GenericMetadataView(RetrieveUpdateAPI):
|
||||
serializer_class = MetadataSerializer
|
||||
permission_classes = [InvenTree.permissions.ContentTypePermission]
|
||||
|
||||
# Enforce limited range of lookup fields to prevent arbitrary queryset filtering
|
||||
ALLOWED_LOOKUP_FIELDS = {'pk', 'key'}
|
||||
|
||||
def get_permission_model(self):
|
||||
"""Return the 'permission' model associated with this view."""
|
||||
model_name = self.kwargs.get('model', None)
|
||||
@@ -944,6 +947,17 @@ class GenericMetadataView(RetrieveUpdateAPI):
|
||||
)
|
||||
return super().dispatch(request, *args, **kwargs)
|
||||
|
||||
def initial(self, request, *args, **kwargs):
|
||||
"""Validate the lookup field before any queryset is touched.
|
||||
|
||||
This runs inside APIView's own exception handling (unlike dispatch(),
|
||||
which runs before it), and *before* permission checks - so an invalid
|
||||
lookup field is rejected without ever executing a query.
|
||||
"""
|
||||
if self.lookup_field not in self.ALLOWED_LOOKUP_FIELDS:
|
||||
raise ValidationError(f"Invalid lookup field '{self.lookup_field}'")
|
||||
return super().initial(request, *args, **kwargs)
|
||||
|
||||
|
||||
class SimpleGenericMetadataView(GenericMetadataView):
|
||||
"""Simplified version of GenericMetadataView which always uses 'pk' as the lookup field."""
|
||||
|
||||
@@ -4,6 +4,7 @@ from base64 import b64encode
|
||||
from pathlib import Path
|
||||
from tempfile import TemporaryDirectory
|
||||
|
||||
from django.contrib.auth import get_user_model
|
||||
from django.core.exceptions import AppRegistryNotReady
|
||||
from django.test import TestCase
|
||||
from django.urls import reverse
|
||||
@@ -15,6 +16,7 @@ from InvenTree.api_version import INVENTREE_API_VERSION
|
||||
from InvenTree.exceptions import exception_handler
|
||||
from InvenTree.unit_test import InvenTreeAPITestCase, InvenTreeTestCase
|
||||
from InvenTree.version import inventreeApiText, parse_version_text
|
||||
from users.models import ApiToken
|
||||
from users.ruleset import RULESET_NAMES
|
||||
from users.tasks import update_group_roles
|
||||
|
||||
@@ -729,3 +731,56 @@ class GeneralApiTests(InvenTreeAPITestCase):
|
||||
self.assertIn('bom-exporter', keys)
|
||||
self.assertIn('inventree-ui-notification', keys)
|
||||
self.assertIn('inventreelabel', keys)
|
||||
|
||||
def test_generic_metadata_lookup_field_injection(self):
|
||||
"""The generic metadata endpoint must not accept arbitrary lookup expressions.
|
||||
|
||||
Regression test for a vulnerability where 'lookup_field' was passed
|
||||
straight from the URL into the ORM (e.g. 'key__regex'), letting an
|
||||
unprivileged user turn distinguishable 403 / 404 / 500 responses into
|
||||
an oracle to recover another user's raw API token, byte by byte.
|
||||
"""
|
||||
victim = get_user_model().objects.create_user(
|
||||
username='metadata_victim', password='hunter2', email='victim@example.org'
|
||||
)
|
||||
token = ApiToken.objects.create(user=victim)
|
||||
|
||||
def lookup_url(model, lookup_field, lookup_value):
|
||||
return reverse(
|
||||
'api-generic-metadata',
|
||||
kwargs={
|
||||
'model': model,
|
||||
'lookup_field': lookup_field,
|
||||
'lookup_value': lookup_value,
|
||||
},
|
||||
)
|
||||
|
||||
# A pattern which *would* match the victim's token if 'key__regex'
|
||||
# were actually applied as a regex filter against ApiToken.key
|
||||
matching_pattern = f'^{token.key}$'
|
||||
non_matching_pattern = '^this-will-never-match-anything$'
|
||||
|
||||
responses = set()
|
||||
|
||||
for pattern in (matching_pattern, non_matching_pattern):
|
||||
for lookup_field in ('key__regex', 'key__contains', 'key__startswith'):
|
||||
response = self.get(
|
||||
lookup_url('apitoken', lookup_field, pattern), expected_code=400
|
||||
)
|
||||
self.assertIn('Invalid lookup field', str(response.data))
|
||||
responses.add(response.status_code)
|
||||
|
||||
# Matching and non-matching patterns must be indistinguishable
|
||||
self.assertEqual(len(responses), 1)
|
||||
|
||||
# The exact-match 'key' lookup stays permitted (used elsewhere, e.g.
|
||||
# by plugin config lookups) - a non-admin still can't read another
|
||||
# user's token metadata through it, since object permissions apply.
|
||||
self.get(lookup_url('apitoken', 'key', token.key), expected_code=403)
|
||||
|
||||
# Not apitoken-specific: any model/lookup-type combination outside
|
||||
# the allow-list is rejected the same way.
|
||||
response = self.get(
|
||||
lookup_url('user', 'password__startswith', 'x'), expected_code=400
|
||||
)
|
||||
self.assertIn('Invalid lookup field', str(response.data))
|
||||
|
||||
Reference in New Issue
Block a user