diff --git a/src/apps/api/tests/test_submissions.py b/src/apps/api/tests/test_submissions.py index 67ed65be4..8601f0aa2 100644 --- a/src/apps/api/tests/test_submissions.py +++ b/src/apps/api/tests/test_submissions.py @@ -45,11 +45,11 @@ def setUp(self): leaderboard=None ) - # add submission with that is on the leaderboard + # add submission with that is on the leaderboard (leaderboard submissions should always be finished) self.leaderboard_submission = SubmissionFactory( phase=self.phase, owner=self.participant, - status=Submission.SUBMITTED, + status=Submission.FINISHED, leaderboard=self.leaderboard ) @@ -260,6 +260,24 @@ def test_who_can_see_detailed_result_when_visualization_is_true_and_competition_ resp = self.client.get(url) assert resp.status_code == 200 + def test_anonymous_cannot_list_or_retrieve_submissions(self): + """ + SubmissionViewSet's general list/retrieve endpoints must not leak submission + data to anonymous users, even for a finished submission on a leaderboard. + Public leaderboard data is meant to be served only through + PhaseViewSet.get_leaderboard, which uses a restricted serializer. + """ + # List: the leaderboard submission must not appear + resp = self.client.get(reverse('submission-list')) + assert resp.status_code == 200 + results = resp.data.get('results', resp.data) + assert all(item['id'] != self.leaderboard_submission.pk for item in results) + + # Retrieve: must 404, not leak the record + url = reverse('submission-detail', args=(self.leaderboard_submission.pk,)) + resp = self.client.get(url) + assert resp.status_code == 404 + class SubmissionGetDetailsAPITests(APITestCase): def setUp(self): @@ -373,23 +391,19 @@ def test_hidden_details_actually_stops_submission_creator_from_seeing_output(sel def test_anonymous_cannot_get_details_of_finished_leaderboard_submission(self): """ - Unlike the two tests above, uses a finished submission that IS on a leaderboard, - so it's reachable by anonymous users; expect it to still be denied to them. - - SubmissionViewSet.get_queryset() in src/apps/api/views/submissions.py has a branch - for unauthenticated GET requests that filters on leaderboard__isnull=False (plus - status=FINISHED and is_soft_deleted=False). That's what lets an anonymous request - find this submission via get_details' super().get_object() call in the first place -- - so this test's leaderboard_submission (leaderboard set, status=FINISHED in setUp) - is what actually exercises that branch, unlike existing_submission (leaderboard=None) - used above, which fails the leaderboard__isnull=False filter and 404s before - get_details' owner/admin check is ever reached. + Unlike the two tests above, uses a finished submission that IS on a leaderboard. + Being on a leaderboard must not make submission details reachable by anonymous users. + + SubmissionViewSet.get_queryset() in src/apps/api/views/submissions.py returns an + empty queryset for unauthenticated GET requests, so get_details' super().get_object() + never finds the submission and we get a 404 (rather than a 403 confirming it exists). + Public leaderboard data is served separately by PhaseViewSet.get_leaderboard. """ url = reverse('submission-get-details', args=(self.leaderboard_submission.pk,)) - # Anonymous: object is found via the leaderboard queryset, but must still be denied + # Anonymous: filtered out at the queryset level, existence is not leaked resp = self.client.get(url) - assert resp.status_code == 403 + assert resp.status_code == 404 # Non-owner, non-admin authenticated user: filtered out at the queryset level self.client.force_login(self.other_user) diff --git a/src/apps/api/views/submissions.py b/src/apps/api/views/submissions.py index b4a2a7aed..c3fab49de 100644 --- a/src/apps/api/views/submissions.py +++ b/src/apps/api/views/submissions.py @@ -114,28 +114,12 @@ def get_queryset(self): qs = super().get_queryset() if self.request.method == 'GET': if not self.request.user.is_authenticated: - # Show leaderboard submissions to unauthenticated users - return ( - qs.filter( - leaderboard__isnull=False, - is_soft_deleted=False, - status=Submission.FINISHED, - ) - .select_related( - 'phase', - 'phase__competition', - 'participant', - 'participant__user', - 'owner', - 'data', - ) - .prefetch_related( - 'children', - 'scores', - 'scores__column', - 'task', - ) - ) + # Anonymous users get nothing here. This endpoint returns full + # submission records (filenames, status details, fact sheet + # answers, internal ids, ...); the public leaderboard view is + # served separately by PhaseViewSet.get_leaderboard, which uses + # a restricted serializer. + return qs.none() # Check if admin is requesting to see soft-deleted submissions show_is_soft_deleted = self.request.query_params.get('show_is_soft_deleted', 'false').lower() == 'true' @@ -177,12 +161,9 @@ def get_queryset(self): Q(phase__competition__created_by=self.request.user) | Q(phase__competition__collaborators__in=[self.request.user.pk]) ) is not qs: - ValidationError("Request Contained Submissions you don't have authorization for") + raise ValidationError("Request Contained Submissions you don't have authorization for") if self.action in ['re_run_many_submissions']: - print(f'debug {qs}') - print(f'debug {qs.first().status}') qs = qs.filter(status__in=[Submission.FINISHED, Submission.FAILED, Submission.CANCELLED]) - print(f'debug {qs}') return qs def create(self, request, *args, **kwargs):