From 8c37c2cc7be97309443ea2057f93460f8daed9d4 Mon Sep 17 00:00:00 2001 From: Petr Marinec Date: Sat, 4 Apr 2026 21:04:27 +0200 Subject: [PATCH] Add authentication and input validation to TaskLogHandler The TaskLogHandler endpoint at /testcase-detail/task-log was missing authentication and authorization checks, unlike every other handler in the same file that accesses testcase data. This change: 1. Adds access.check_access_and_get_testcase() to verify the caller is authenticated and authorized to view the testcase before querying Cloud Logging. 2. Validates task_name against the known set of valid task names to prevent Cloud Logging filter injection via crafted query parameters. 3. Adds _sanitize_filter_value() to escape double quotes and backslashes in user-supplied values before interpolating them into the Cloud Logging filter string, as defense-in-depth against filter injection. 4. Adds unit tests for the sanitization function and task name validation. --- .../handlers/testcase_detail/show.py | 14 ++++++++- .../testcase_detail/testcase_status_events.py | 16 ++++++++-- .../handlers/testcase_detail/show_test.py | 30 ++++++++++++++++++ .../testcase_status_events_test.py | 31 +++++++++++++++++++ 4 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/appengine/handlers/testcase_detail/show.py b/src/appengine/handlers/testcase_detail/show.py index 712f0647957..2eb9f787090 100644 --- a/src/appengine/handlers/testcase_detail/show.py +++ b/src/appengine/handlers/testcase_detail/show.py @@ -640,9 +640,21 @@ class TaskLogHandler(base_handler.Handler): @handler.get(handler.TEXT) def get(self): """Serve the task log.""" + testcase_id = flask.request.args.get('testcase_id') + + # Verify the user is authenticated and has access to this testcase. + testcase = access.check_access_and_get_testcase(testcase_id) + task_id = flask.request.args.get('task_id') task_name = flask.request.args.get('task_name') - testcase_id = flask.request.args.get('testcase_id') + + # Validate task_name against the known set to prevent filter injection. + valid_task_names = ( + testcase_status_events.TestcaseStatusInfo.TASK_EVENTS_NAMES + + testcase_status_events.TestcaseStatusInfo.CHROME_TASK_EVENTS_NAMES) + if task_name and task_name not in valid_task_names: + raise helpers.EarlyExitError('Invalid task name.', 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..881b76a0485 100644 --- a/src/appengine/handlers/testcase_detail/testcase_status_events.py +++ b/src/appengine/handlers/testcase_detail/testcase_status_events.py @@ -219,11 +219,21 @@ def _get_time_range_filter(self, days: int) -> str: start_time = utils.utcnow() - datetime.timedelta(days=days) return f'timestamp >= "{start_time.isoformat()}Z"' + @staticmethod + def _sanitize_filter_value(value: str) -> str: + """Sanitize a value for use in a Cloud Logging filter string. + + Escapes double quotes and backslashes to prevent filter injection.""" + return value.replace('\\', '\\\\').replace('"', '\\"') + 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}"') + safe_task_id = self._sanitize_filter_value(task_id) + safe_task_name = self._sanitize_filter_value(task_name) + safe_testcase_id = self._sanitize_filter_value(str(self._testcase_id)) + query = (f'jsonPayload.extras.task_id="{safe_task_id}" AND ' + f'jsonPayload.extras.testcase_id="{safe_testcase_id}" AND ' + f'jsonPayload.extras.task_name="{safe_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..84eda2fdb95 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 @@ -607,3 +607,33 @@ def test_unreproducible_get(self): self.assertDictContainsSubset({ 'lines': [show.Line(1, 'crash_stacktrace', False)] }, result['last_tested_crash_stacktrace']) + + +class TaskLogHandlerValidationTest(unittest.TestCase): + """Test that TaskLogHandler validates task_name against the known set.""" + + def test_valid_task_names(self): + """Verify the known valid task names.""" + from handlers.testcase_detail import testcase_status_events + valid_names = ( + testcase_status_events.TestcaseStatusInfo.TASK_EVENTS_NAMES + + testcase_status_events.TestcaseStatusInfo.CHROME_TASK_EVENTS_NAMES) + + self.assertIn('analyze', valid_names) + self.assertIn('minimize', valid_names) + self.assertIn('progression', valid_names) + self.assertIn('regression', valid_names) + self.assertIn('variant', valid_names) + self.assertIn('blame', valid_names) + self.assertIn('impact', valid_names) + + def test_injection_payloads_rejected(self): + """Verify that injection payloads are not valid task names.""" + from handlers.testcase_detail import testcase_status_events + valid_names = ( + testcase_status_events.TestcaseStatusInfo.TASK_EVENTS_NAMES + + testcase_status_events.TestcaseStatusInfo.CHROME_TASK_EVENTS_NAMES) + + self.assertNotIn('analyze" OR "1"="1', valid_names) + self.assertNotIn('evil_task', valid_names) + self.assertNotIn('', valid_names) 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..6e6ecc59c70 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 @@ -834,3 +834,34 @@ def test_get_task_log_api_call(self): self.assertLess( result.find('"payload": "log1"'), result.find('"payload": "log2"')) self.assertEqual(result.count('\n'), 5) + + +class SanitizeFilterValueTest(unittest.TestCase): + """Test _sanitize_filter_value for Cloud Logging filter injection.""" + + def test_normal_value(self): + """Test that normal values pass through unchanged.""" + self.assertEqual( + testcase_status_events.TestcaseEventHistory._sanitize_filter_value( + 'analyze'), + 'analyze') + + def test_double_quote_escaped(self): + """Test that double quotes are escaped to prevent filter injection.""" + self.assertEqual( + testcase_status_events.TestcaseEventHistory._sanitize_filter_value( + 'analyze" OR resource.type="global'), + 'analyze\\" OR resource.type=\\"global') + + def test_backslash_escaped(self): + """Test that backslashes are escaped.""" + self.assertEqual( + testcase_status_events.TestcaseEventHistory._sanitize_filter_value( + 'test\\value'), + 'test\\\\value') + + def test_empty_string(self): + """Test empty string.""" + self.assertEqual( + testcase_status_events.TestcaseEventHistory._sanitize_filter_value(''), + '')