From dd547ae60c3acb434fd23f454bf4827a1859d1cd Mon Sep 17 00:00:00 2001 From: M0nd0R Date: Tue, 14 Apr 2026 23:42:16 +0800 Subject: [PATCH 1/4] Testcase detail: protect task logs --- .../handlers/testcase_detail/show.py | 11 ++++- .../testcase_detail/testcase_status_events.py | 14 +++++-- .../handlers/testcase_detail/show_test.py | 42 +++++++++++++++++++ .../testcase_status_events_test.py | 21 ++++++++-- 4 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/appengine/handlers/testcase_detail/show.py b/src/appengine/handlers/testcase_detail/show.py index 712f0647957..beaadb73f73 100644 --- a/src/appengine/handlers/testcase_detail/show.py +++ b/src/appengine/handlers/testcase_detail/show.py @@ -640,9 +640,18 @@ class TaskLogHandler(base_handler.Handler): @handler.get(handler.TEXT) def get(self): """Serve the task log.""" + testcase_id = helpers.cast(flask.request.args.get('testcase_id'), int, + "The param 'testcase_id' is not a number.") + access.check_access_and_get_testcase(testcase_id) + task_id = flask.request.args.get('task_id') + if not task_id: + raise helpers.EarlyExitError('No task ID provided.', 400) + task_name = flask.request.args.get('task_name') - testcase_id = flask.request.args.get('testcase_id') + if not task_name: + raise helpers.EarlyExitError('No task name provided.', 400) + log_content = testcase_status_events.get_task_log(testcase_id, task_id, task_name) diff --git a/src/appengine/handlers/testcase_detail/testcase_status_events.py b/src/appengine/handlers/testcase_detail/testcase_status_events.py index 0a364cd92f2..ac140715039 100644 --- a/src/appengine/handlers/testcase_detail/testcase_status_events.py +++ b/src/appengine/handlers/testcase_detail/testcase_status_events.py @@ -31,6 +31,11 @@ _BASE_LOGS_URL = 'https://console.cloud.google.com/logs/viewer' +def _quote_logging_filter_value(value: str) -> str: + """Formats a string literal for a Cloud Logging query filter.""" + return json.dumps(value) + + def _format_timestamp(timestamp: datetime.datetime) -> str: """Formats a timestamp.""" return timestamp.strftime('%Y-%m-%d %H:%M:%S.%f UTC') @@ -221,9 +226,12 @@ def _get_time_range_filter(self, days: int) -> str: def _get_task_log_query_filter(self, task_id: str, task_name: str) -> str: """Returns the filter string for querying task logs.""" - query = (f'jsonPayload.extras.task_id="{task_id}" AND ' - f'jsonPayload.extras.testcase_id="{self._testcase_id}" AND ' - f'jsonPayload.extras.task_name="{task_name}"') + query = ('jsonPayload.extras.task_id=' + f'{_quote_logging_filter_value(task_id)} AND ' + 'jsonPayload.extras.testcase_id=' + f'{_quote_logging_filter_value(str(self._testcase_id))} AND ' + 'jsonPayload.extras.task_name=' + f'{_quote_logging_filter_value(task_name)}') query += f' AND {self._get_time_range_filter(days=31)}' return query diff --git a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py index e72db6a48b5..d03b2e77581 100644 --- a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py +++ b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py @@ -18,6 +18,9 @@ import os import unittest +import flask +import webtest + from clusterfuzz._internal.datastore import data_types from clusterfuzz._internal.tests.test_libs import helpers as test_helpers from clusterfuzz._internal.tests.test_libs import test_utils @@ -498,6 +501,45 @@ def test_reproducible_get(self): 'lines': [show.Line(1, 'crash_stacktrace', False)] }, result['last_tested_crash_stacktrace']) + +@test_utils.with_cloud_emulators('datastore') +class TaskLogHandlerTest(unittest.TestCase): + """Tests for TaskLogHandler.""" + + def setUp(self): + test_helpers.patch(self, [ + 'handlers.testcase_detail.show.access.check_access_and_get_testcase', + 'handlers.testcase_detail.show.testcase_status_events.get_task_log', + ]) + flaskapp = flask.Flask('testflask') + flaskapp.add_url_rule('/', view_func=show.TaskLogHandler.as_view('/')) + self.app = webtest.TestApp(flaskapp) + self.mock.get_task_log.return_value = 'task log content' + + def test_get(self): + """Ensure the handler checks testcase access before returning logs.""" + response = self.app.get( + '/?testcase_id=123&task_id=task-1&task_name=minimize') + + self.assertEqual(200, response.status_int) + self.assertEqual('task log content', response.text) + self.assertEqual('text/plain; charset=utf-8', + response.headers['Content-Type']) + self.assertEqual('attachment; filename="task_task-1_log.txt"', + response.headers['Content-Disposition']) + self.mock.check_access_and_get_testcase.assert_called_once_with(123) + self.mock.get_task_log.assert_called_once_with(123, 'task-1', 'minimize') + + def test_invalid_testcase_id(self): + """Ensure invalid testcase IDs are rejected before querying logs.""" + response = self.app.get( + '/?testcase_id=abc&task_id=task-1&task_name=minimize', + expect_errors=True) + + self.assertEqual(400, response.status_int) + self.mock.check_access_and_get_testcase.assert_not_called() + self.mock.get_task_log.assert_not_called() + def test_unreproducible_get(self): """Test valid unreproducible testcase.""" self.mock.get_last_crash_time.return_value = datetime.datetime(2000, 1, 1) diff --git a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/testcase_status_events_test.py b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/testcase_status_events_test.py index 68cc30ff108..c647749318a 100644 --- a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/testcase_status_events_test.py +++ b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/testcase_status_events_test.py @@ -15,6 +15,7 @@ # pylint: disable=protected-access import datetime +import json import unittest from unittest import mock @@ -746,9 +747,23 @@ def test_get_task_log_query_filter(self): """Verify that the task log query filter is generated correctly.""" result = self.event_history._get_task_log_query_filter( 'task123', 'minimize') - expected = (f'jsonPayload.extras.task_id="task123" AND ' - f'jsonPayload.extras.testcase_id="{self.testcase_id}" AND ' - 'jsonPayload.extras.task_name="minimize" AND ' + quoted_testcase_id = json.dumps(str(self.testcase_id)) + expected = (f'jsonPayload.extras.task_id={json.dumps("task123")} AND ' + f'jsonPayload.extras.testcase_id={quoted_testcase_id} AND ' + f'jsonPayload.extras.task_name={json.dumps("minimize")} AND ' + 'timestamp >= "2025-01-01T00:00:00Z"') + self.assertEqual(result, expected) + + def test_get_task_log_query_filter_escapes_values(self): + """Verify that task values are escaped before building the log filter.""" + task_id = 'task123" OR severity>="WARNING' + task_name = 'minimize\\latest' + + result = self.event_history._get_task_log_query_filter(task_id, task_name) + quoted_testcase_id = json.dumps(str(self.testcase_id)) + expected = (f'jsonPayload.extras.task_id={json.dumps(task_id)} AND ' + f'jsonPayload.extras.testcase_id={quoted_testcase_id} AND ' + f'jsonPayload.extras.task_name={json.dumps(task_name)} AND ' 'timestamp >= "2025-01-01T00:00:00Z"') self.assertEqual(result, expected) From 3c7d4dc58657aeeca11c93280ed696a6da43a19b Mon Sep 17 00:00:00 2001 From: M0nd0R Date: Wed, 15 Apr 2026 00:03:16 +0800 Subject: [PATCH 2/4] Fix task log test coverage --- .../handlers/testcase_detail/show.py | 5 +- .../handlers/testcase_detail/show_test.py | 115 ++++++++++-------- 2 files changed, 65 insertions(+), 55 deletions(-) diff --git a/src/appengine/handlers/testcase_detail/show.py b/src/appengine/handlers/testcase_detail/show.py index beaadb73f73..78352057680 100644 --- a/src/appengine/handlers/testcase_detail/show.py +++ b/src/appengine/handlers/testcase_detail/show.py @@ -640,8 +640,9 @@ class TaskLogHandler(base_handler.Handler): @handler.get(handler.TEXT) def get(self): """Serve the task log.""" - testcase_id = helpers.cast(flask.request.args.get('testcase_id'), int, - "The param 'testcase_id' is not a number.") + testcase_id = helpers.cast( + flask.request.args.get('testcase_id'), int, + "The param 'testcase_id' is not a number.") access.check_access_and_get_testcase(testcase_id) task_id = flask.request.args.get('task_id') diff --git a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py index d03b2e77581..f31e0bcc306 100644 --- a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py +++ b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py @@ -28,6 +28,13 @@ from libs import helpers +def assert_dict_contains_subset(test_case, expected_subset, actual): + """Compatibility helper for the removed assertDictContainsSubset.""" + for key, expected_value in expected_subset.items(): + test_case.assertIn(key, actual) + test_case.assertEqual(expected_value, actual[key]) + + class ParseSuspectedClsTest(unittest.TestCase): """Test _parse_suspected_cls.""" @@ -491,54 +498,15 @@ def test_reproducible_get(self): } self.maxDiff = None - self.assertDictContainsSubset(expected_subset, result) + assert_dict_contains_subset(self, expected_subset, result) self.assertEqual(result['testcase'].key.id(), testcase.key.id()) - self.assertDictContainsSubset({ - 'lines': [show.Line(1, 'crash_stacktrace', False)] - }, result['crash_stacktrace']) - self.assertDictContainsSubset({ - 'lines': [show.Line(1, 'crash_stacktrace', False)] - }, result['last_tested_crash_stacktrace']) - - -@test_utils.with_cloud_emulators('datastore') -class TaskLogHandlerTest(unittest.TestCase): - """Tests for TaskLogHandler.""" - - def setUp(self): - test_helpers.patch(self, [ - 'handlers.testcase_detail.show.access.check_access_and_get_testcase', - 'handlers.testcase_detail.show.testcase_status_events.get_task_log', - ]) - flaskapp = flask.Flask('testflask') - flaskapp.add_url_rule('/', view_func=show.TaskLogHandler.as_view('/')) - self.app = webtest.TestApp(flaskapp) - self.mock.get_task_log.return_value = 'task log content' - - def test_get(self): - """Ensure the handler checks testcase access before returning logs.""" - response = self.app.get( - '/?testcase_id=123&task_id=task-1&task_name=minimize') - - self.assertEqual(200, response.status_int) - self.assertEqual('task log content', response.text) - self.assertEqual('text/plain; charset=utf-8', - response.headers['Content-Type']) - self.assertEqual('attachment; filename="task_task-1_log.txt"', - response.headers['Content-Disposition']) - self.mock.check_access_and_get_testcase.assert_called_once_with(123) - self.mock.get_task_log.assert_called_once_with(123, 'task-1', 'minimize') - - def test_invalid_testcase_id(self): - """Ensure invalid testcase IDs are rejected before querying logs.""" - response = self.app.get( - '/?testcase_id=abc&task_id=task-1&task_name=minimize', - expect_errors=True) - - self.assertEqual(400, response.status_int) - self.mock.check_access_and_get_testcase.assert_not_called() - self.mock.get_task_log.assert_not_called() + assert_dict_contains_subset( + self, {'lines': [show.Line(1, 'crash_stacktrace', False)]}, + result['crash_stacktrace']) + assert_dict_contains_subset( + self, {'lines': [show.Line(1, 'crash_stacktrace', False)]}, + result['last_tested_crash_stacktrace']) def test_unreproducible_get(self): """Test valid unreproducible testcase.""" @@ -640,12 +608,53 @@ def test_unreproducible_get(self): } self.maxDiff = None - self.assertDictContainsSubset(expected_subset, result) + assert_dict_contains_subset(self, expected_subset, result) self.assertEqual(result['testcase'].key.id(), testcase.key.id()) - self.assertDictContainsSubset({ - 'lines': [show.Line(1, 'crash_stacktrace', False)] - }, result['crash_stacktrace']) - self.assertDictContainsSubset({ - 'lines': [show.Line(1, 'crash_stacktrace', False)] - }, result['last_tested_crash_stacktrace']) + assert_dict_contains_subset( + self, {'lines': [show.Line(1, 'crash_stacktrace', False)]}, + result['crash_stacktrace']) + assert_dict_contains_subset( + self, {'lines': [show.Line(1, 'crash_stacktrace', False)]}, + result['last_tested_crash_stacktrace']) + + +@test_utils.with_cloud_emulators('datastore') +class TaskLogHandlerTest(unittest.TestCase): + """Tests for TaskLogHandler.""" + + def setUp(self): + test_helpers.patch(self, [ + 'handlers.testcase_detail.show.access.check_access_and_get_testcase', + 'handlers.testcase_detail.show.testcase_status_events.get_task_log', + ]) + self.flaskapp = flask.Flask('testflask') + self.flaskapp.add_url_rule('/', view_func=show.TaskLogHandler.as_view('/')) + self.app = webtest.TestApp(self.flaskapp) + self.mock.get_task_log.return_value = 'task log content' + + def test_get(self): + """Ensure the handler checks testcase access before returning logs.""" + response = self.app.get( + '/?testcase_id=123&task_id=task-1&task_name=minimize') + + self.assertEqual(200, response.status_int) + self.assertEqual('task log content', response.text) + self.assertEqual('text/plain', response.headers['Content-Type']) + self.assertEqual('attachment; filename="task_task-1_log.txt"', + response.headers['Content-Disposition']) + self.mock.check_access_and_get_testcase.assert_called_once_with(123) + self.mock.get_task_log.assert_called_once_with(123, 'task-1', 'minimize') + + def test_invalid_testcase_id(self): + """Ensure invalid testcase IDs are rejected before querying logs.""" + with self.flaskapp.test_request_context( + '/?testcase_id=abc&task_id=task-1&task_name=minimize'): + with self.assertRaises(helpers.EarlyExitError) as cm: + show.TaskLogHandler().get() + + self.assertEqual(400, cm.exception.status) + self.assertEqual("The param 'testcase_id' is not a number.", + str(cm.exception)) + self.mock.check_access_and_get_testcase.assert_not_called() + self.mock.get_task_log.assert_not_called() From eca0371d6eb7e6c5593ec28232e311a1e85c8f53 Mon Sep 17 00:00:00 2001 From: M0nd0R Date: Wed, 15 Apr 2026 00:17:56 +0800 Subject: [PATCH 3/4] Remove redundant task log testcase cast --- src/appengine/handlers/testcase_detail/show.py | 10 ++++------ .../handlers/testcase_detail/show_test.py | 15 +++++++++------ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/appengine/handlers/testcase_detail/show.py b/src/appengine/handlers/testcase_detail/show.py index 78352057680..4e49c52f29a 100644 --- a/src/appengine/handlers/testcase_detail/show.py +++ b/src/appengine/handlers/testcase_detail/show.py @@ -640,10 +640,8 @@ class TaskLogHandler(base_handler.Handler): @handler.get(handler.TEXT) def get(self): """Serve the task log.""" - testcase_id = helpers.cast( - flask.request.args.get('testcase_id'), int, - "The param 'testcase_id' is not a number.") - access.check_access_and_get_testcase(testcase_id) + testcase = access.check_access_and_get_testcase( + flask.request.args.get('testcase_id')) task_id = flask.request.args.get('task_id') if not task_id: @@ -653,8 +651,8 @@ def get(self): if not task_name: raise helpers.EarlyExitError('No task name provided.', 400) - log_content = testcase_status_events.get_task_log(testcase_id, task_id, - task_name) + log_content = testcase_status_events.get_task_log(testcase.key.id(), + task_id, task_name) response = flask.make_response(log_content) response.headers['Content-Type'] = 'text/plain; charset=utf-8' diff --git a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py index f31e0bcc306..aa29a2c81e3 100644 --- a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py +++ b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py @@ -631,6 +631,7 @@ def setUp(self): self.flaskapp = flask.Flask('testflask') self.flaskapp.add_url_rule('/', view_func=show.TaskLogHandler.as_view('/')) self.app = webtest.TestApp(self.flaskapp) + self.mock.check_access_and_get_testcase.return_value.key.id.return_value = 123 self.mock.get_task_log.return_value = 'task log content' def test_get(self): @@ -643,18 +644,20 @@ def test_get(self): self.assertEqual('text/plain', response.headers['Content-Type']) self.assertEqual('attachment; filename="task_task-1_log.txt"', response.headers['Content-Disposition']) - self.mock.check_access_and_get_testcase.assert_called_once_with(123) + self.mock.check_access_and_get_testcase.assert_called_once_with('123') self.mock.get_task_log.assert_called_once_with(123, 'task-1', 'minimize') def test_invalid_testcase_id(self): - """Ensure invalid testcase IDs are rejected before querying logs.""" + """Ensure invalid testcase IDs are rejected by the access helper.""" + self.mock.check_access_and_get_testcase.side_effect = ( + helpers.EarlyExitError('Invalid test case!', 404)) + with self.flaskapp.test_request_context( '/?testcase_id=abc&task_id=task-1&task_name=minimize'): with self.assertRaises(helpers.EarlyExitError) as cm: show.TaskLogHandler().get() - self.assertEqual(400, cm.exception.status) - self.assertEqual("The param 'testcase_id' is not a number.", - str(cm.exception)) - self.mock.check_access_and_get_testcase.assert_not_called() + self.assertEqual(404, cm.exception.status) + self.assertEqual('Invalid test case!', str(cm.exception)) + self.mock.check_access_and_get_testcase.assert_called_once_with('abc') self.mock.get_task_log.assert_not_called() From 4eb8c8318a2007d9e5edcad02d0d8a371fa33b93 Mon Sep 17 00:00:00 2001 From: M0nd0R Date: Wed, 15 Apr 2026 00:23:55 +0800 Subject: [PATCH 4/4] Keep task log tests scoped --- .../handlers/testcase_detail/show_test.py | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py index aa29a2c81e3..7ecec3a48f4 100644 --- a/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py +++ b/src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py @@ -28,13 +28,6 @@ from libs import helpers -def assert_dict_contains_subset(test_case, expected_subset, actual): - """Compatibility helper for the removed assertDictContainsSubset.""" - for key, expected_value in expected_subset.items(): - test_case.assertIn(key, actual) - test_case.assertEqual(expected_value, actual[key]) - - class ParseSuspectedClsTest(unittest.TestCase): """Test _parse_suspected_cls.""" @@ -498,15 +491,15 @@ def test_reproducible_get(self): } self.maxDiff = None - assert_dict_contains_subset(self, expected_subset, result) + self.assertDictContainsSubset(expected_subset, result) self.assertEqual(result['testcase'].key.id(), testcase.key.id()) - assert_dict_contains_subset( - self, {'lines': [show.Line(1, 'crash_stacktrace', False)]}, - result['crash_stacktrace']) - assert_dict_contains_subset( - self, {'lines': [show.Line(1, 'crash_stacktrace', False)]}, - result['last_tested_crash_stacktrace']) + self.assertDictContainsSubset({ + 'lines': [show.Line(1, 'crash_stacktrace', False)] + }, result['crash_stacktrace']) + self.assertDictContainsSubset({ + 'lines': [show.Line(1, 'crash_stacktrace', False)] + }, result['last_tested_crash_stacktrace']) def test_unreproducible_get(self): """Test valid unreproducible testcase.""" @@ -608,15 +601,15 @@ def test_unreproducible_get(self): } self.maxDiff = None - assert_dict_contains_subset(self, expected_subset, result) + self.assertDictContainsSubset(expected_subset, result) self.assertEqual(result['testcase'].key.id(), testcase.key.id()) - assert_dict_contains_subset( - self, {'lines': [show.Line(1, 'crash_stacktrace', False)]}, - result['crash_stacktrace']) - assert_dict_contains_subset( - self, {'lines': [show.Line(1, 'crash_stacktrace', False)]}, - result['last_tested_crash_stacktrace']) + self.assertDictContainsSubset({ + 'lines': [show.Line(1, 'crash_stacktrace', False)] + }, result['crash_stacktrace']) + self.assertDictContainsSubset({ + 'lines': [show.Line(1, 'crash_stacktrace', False)] + }, result['last_tested_crash_stacktrace']) @test_utils.with_cloud_emulators('datastore')