Skip to content

Commit 8c37c2c

Browse files
petrmarinecPetr Marinec
authored andcommitted
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.
1 parent 525dd2b commit 8c37c2c

4 files changed

Lines changed: 87 additions & 4 deletions

File tree

src/appengine/handlers/testcase_detail/show.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -640,9 +640,21 @@ class TaskLogHandler(base_handler.Handler):
640640
@handler.get(handler.TEXT)
641641
def get(self):
642642
"""Serve the task log."""
643+
testcase_id = flask.request.args.get('testcase_id')
644+
645+
# Verify the user is authenticated and has access to this testcase.
646+
testcase = access.check_access_and_get_testcase(testcase_id)
647+
643648
task_id = flask.request.args.get('task_id')
644649
task_name = flask.request.args.get('task_name')
645-
testcase_id = flask.request.args.get('testcase_id')
650+
651+
# Validate task_name against the known set to prevent filter injection.
652+
valid_task_names = (
653+
testcase_status_events.TestcaseStatusInfo.TASK_EVENTS_NAMES +
654+
testcase_status_events.TestcaseStatusInfo.CHROME_TASK_EVENTS_NAMES)
655+
if task_name and task_name not in valid_task_names:
656+
raise helpers.EarlyExitError('Invalid task name.', 400)
657+
646658
log_content = testcase_status_events.get_task_log(testcase_id, task_id,
647659
task_name)
648660

src/appengine/handlers/testcase_detail/testcase_status_events.py

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -219,11 +219,21 @@ def _get_time_range_filter(self, days: int) -> str:
219219
start_time = utils.utcnow() - datetime.timedelta(days=days)
220220
return f'timestamp >= "{start_time.isoformat()}Z"'
221221

222+
@staticmethod
223+
def _sanitize_filter_value(value: str) -> str:
224+
"""Sanitize a value for use in a Cloud Logging filter string.
225+
226+
Escapes double quotes and backslashes to prevent filter injection."""
227+
return value.replace('\\', '\\\\').replace('"', '\\"')
228+
222229
def _get_task_log_query_filter(self, task_id: str, task_name: str) -> str:
223230
"""Returns the filter string for querying task logs."""
224-
query = (f'jsonPayload.extras.task_id="{task_id}" AND '
225-
f'jsonPayload.extras.testcase_id="{self._testcase_id}" AND '
226-
f'jsonPayload.extras.task_name="{task_name}"')
231+
safe_task_id = self._sanitize_filter_value(task_id)
232+
safe_task_name = self._sanitize_filter_value(task_name)
233+
safe_testcase_id = self._sanitize_filter_value(str(self._testcase_id))
234+
query = (f'jsonPayload.extras.task_id="{safe_task_id}" AND '
235+
f'jsonPayload.extras.testcase_id="{safe_testcase_id}" AND '
236+
f'jsonPayload.extras.task_name="{safe_task_name}"')
227237
query += f' AND {self._get_time_range_filter(days=31)}'
228238
return query
229239

src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/show_test.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -607,3 +607,33 @@ def test_unreproducible_get(self):
607607
self.assertDictContainsSubset({
608608
'lines': [show.Line(1, 'crash_stacktrace', False)]
609609
}, result['last_tested_crash_stacktrace'])
610+
611+
612+
class TaskLogHandlerValidationTest(unittest.TestCase):
613+
"""Test that TaskLogHandler validates task_name against the known set."""
614+
615+
def test_valid_task_names(self):
616+
"""Verify the known valid task names."""
617+
from handlers.testcase_detail import testcase_status_events
618+
valid_names = (
619+
testcase_status_events.TestcaseStatusInfo.TASK_EVENTS_NAMES +
620+
testcase_status_events.TestcaseStatusInfo.CHROME_TASK_EVENTS_NAMES)
621+
622+
self.assertIn('analyze', valid_names)
623+
self.assertIn('minimize', valid_names)
624+
self.assertIn('progression', valid_names)
625+
self.assertIn('regression', valid_names)
626+
self.assertIn('variant', valid_names)
627+
self.assertIn('blame', valid_names)
628+
self.assertIn('impact', valid_names)
629+
630+
def test_injection_payloads_rejected(self):
631+
"""Verify that injection payloads are not valid task names."""
632+
from handlers.testcase_detail import testcase_status_events
633+
valid_names = (
634+
testcase_status_events.TestcaseStatusInfo.TASK_EVENTS_NAMES +
635+
testcase_status_events.TestcaseStatusInfo.CHROME_TASK_EVENTS_NAMES)
636+
637+
self.assertNotIn('analyze" OR "1"="1', valid_names)
638+
self.assertNotIn('evil_task', valid_names)
639+
self.assertNotIn('', valid_names)

src/clusterfuzz/_internal/tests/appengine/handlers/testcase_detail/testcase_status_events_test.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -834,3 +834,34 @@ def test_get_task_log_api_call(self):
834834
self.assertLess(
835835
result.find('"payload": "log1"'), result.find('"payload": "log2"'))
836836
self.assertEqual(result.count('\n'), 5)
837+
838+
839+
class SanitizeFilterValueTest(unittest.TestCase):
840+
"""Test _sanitize_filter_value for Cloud Logging filter injection."""
841+
842+
def test_normal_value(self):
843+
"""Test that normal values pass through unchanged."""
844+
self.assertEqual(
845+
testcase_status_events.TestcaseEventHistory._sanitize_filter_value(
846+
'analyze'),
847+
'analyze')
848+
849+
def test_double_quote_escaped(self):
850+
"""Test that double quotes are escaped to prevent filter injection."""
851+
self.assertEqual(
852+
testcase_status_events.TestcaseEventHistory._sanitize_filter_value(
853+
'analyze" OR resource.type="global'),
854+
'analyze\\" OR resource.type=\\"global')
855+
856+
def test_backslash_escaped(self):
857+
"""Test that backslashes are escaped."""
858+
self.assertEqual(
859+
testcase_status_events.TestcaseEventHistory._sanitize_filter_value(
860+
'test\\value'),
861+
'test\\\\value')
862+
863+
def test_empty_string(self):
864+
"""Test empty string."""
865+
self.assertEqual(
866+
testcase_status_events.TestcaseEventHistory._sanitize_filter_value(''),
867+
'')

0 commit comments

Comments
 (0)