From 860183f178be99e3041ffbdd9fc72bea09bcddfb Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Fri, 17 Jul 2020 14:34:53 -0400 Subject: [PATCH] update the named URL code to properly return 404 vs 403 --- awx/api/generics.py | 24 +++++++++++++++++++++ awx/api/views/__init__.py | 9 ++++++++ awx/main/middleware.py | 19 ++++++++++++++-- awx/main/tests/functional/test_named_url.py | 24 +++++++++++++++++++++ 4 files changed, 74 insertions(+), 2 deletions(-) diff --git a/awx/api/generics.py b/awx/api/generics.py index 83780ad03c..1476de1043 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -51,6 +51,7 @@ from awx.main.utils import ( StubLicense ) from awx.main.utils.db import get_all_field_names +from awx.main.views import ApiErrorView from awx.api.serializers import ResourceAccessListElementSerializer, CopySerializer, UserSerializer from awx.api.versioning import URLPathVersioning from awx.api.metadata import SublistAttachDetatchMetadata, Metadata @@ -188,6 +189,29 @@ class APIView(views.APIView): ''' Log warning for 400 requests. Add header with elapsed time. ''' + + # + # If the URL was rewritten, and we get a 404, we should entirely + # replace the view in the request context with an ApiErrorView() + # Without this change, there will be subtle differences in the BrowseableAPIRenderer + # + # These differences could provide contextual clues which would allow + # anonymous users to determine if usernames were valid or not + # (e.g., if an anonymous user visited `/api/v2/users/valid/`, and got a 404, + # but also saw that the page heading said "User Detail", they might notice + # that's a difference in behavior from a request to `/api/v2/users/not-valid/`, which + # would show a page header of "Not Found"). Changing the view here + # guarantees that the rendered response will look exactly like the response + # when you visit a URL that has no matching URL paths in `awx.api.urls`. + # + if response.status_code == 404 and 'awx.named_url_rewritten' in request.environ: + self.headers.pop('Allow', None) + response = super(APIView, self).finalize_response(request, response, *args, **kwargs) + view = ApiErrorView() + setattr(view, 'request', request) + response.renderer_context['view'] = view + return response + if response.status_code >= 400: status_msg = "status %s received by user %s attempting to access %s from %s" % \ (response.status_code, request.user, request.path, request.META.get('REMOTE_ADDR', None)) diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index a0a39fa1e1..6d864c408b 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -171,6 +171,15 @@ def api_exception_handler(exc, context): exc = ParseError(exc.args[0]) if isinstance(context['view'], UnifiedJobStdout): context['view'].renderer_classes = [renderers.BrowsableAPIRenderer, JSONRenderer] + if isinstance(exc, APIException): + req = context['request']._request + if 'awx.named_url_rewritten' in req.environ and not str(getattr(exc, 'status_code', 0)).startswith('2'): + # if the URL was rewritten, and it's not a 2xx level status code, + # revert the request.path to its original value to avoid leaking + # any context about the existance of resources + req.path = req.environ['awx.named_url_rewritten'] + if exc.status_code == 403: + exc = NotFound(detail=_('Not found.')) return exception_handler(exc, context) diff --git a/awx/main/middleware.py b/awx/main/middleware.py index 112ae17aa5..781266e8dd 100644 --- a/awx/main/middleware.py +++ b/awx/main/middleware.py @@ -14,7 +14,7 @@ from django.conf import settings from django.contrib.auth.models import User from django.db.migrations.executor import MigrationExecutor from django.db import connection -from django.shortcuts import get_object_or_404, redirect +from django.shortcuts import redirect from django.apps import apps from django.utils.deprecation import MiddlewareMixin from django.utils.translation import ugettext_lazy as _ @@ -148,7 +148,21 @@ class URLModificationMiddleware(MiddlewareMixin): def _named_url_to_pk(cls, node, resource, named_url): kwargs = {} if node.populate_named_url_query_kwargs(kwargs, named_url): - return str(get_object_or_404(node.model, **kwargs).pk) + match = node.model.objects.filter(**kwargs).first() + if match: + return str(match.pk) + else: + # if the name does *not* resolve to any actual resource, + # we should still attempt to route it through so that 401s are + # respected + # using "zero" here will cause the URL regex to match e.g., + # /api/v2/users//, but it also means that anonymous + # users will go down the path of having their credentials + # verified; in this way, *anonymous* users will that visit + # /api/v2/users/invalid-username/ *won't* see a 404, they'll + # see a 401 as if they'd gone to /api/v2/users/0/ + # + return '0' if resource == 'job_templates' and '++' not in named_url: # special case for deprecated job template case # will not raise a 404 on its own @@ -178,6 +192,7 @@ class URLModificationMiddleware(MiddlewareMixin): old_path = request.path_info new_path = self._convert_named_url(old_path) if request.path_info != new_path: + request.environ['awx.named_url_rewritten'] = request.path request.path = request.path.replace(request.path_info, new_path) request.path_info = new_path diff --git a/awx/main/tests/functional/test_named_url.py b/awx/main/tests/functional/test_named_url.py index dcf2111992..6482dac3a8 100644 --- a/awx/main/tests/functional/test_named_url.py +++ b/awx/main/tests/functional/test_named_url.py @@ -219,3 +219,27 @@ def test_credential(get, admin_user, credentialtype_ssh): url = reverse('api:credential_detail', kwargs={'pk': test_cred.pk}) response = get(url, user=admin_user, expect=200) assert response.data['related']['named_url'].endswith('/test_cred++Machine+ssh++/') + + +@pytest.mark.django_db +def test_403_vs_404(get): + cindy = User.objects.create( + username='cindy', + password='test_user', + is_superuser=False + ) + bob = User.objects.create( + username='bob', + password='test_user', + is_superuser=False + ) + + # bob cannot see cindy, pk lookup should be a 403 + url = reverse('api:user_detail', kwargs={'pk': cindy.pk}) + get(url, user=bob, expect=403) + + # bob cannot see cindy, username lookup should be a 404 + get('/api/v2/users/cindy/', user=bob, expect=404) + + get(f'/api/v2/users/{cindy.pk}/', expect=401) + get('/api/v2/users/cindy/', expect=404)