From 27b74aa60f6e590260bbd11371644c8afe8a5355 Mon Sep 17 00:00:00 2001 From: Bishara Hodali Date: Mon, 18 May 2026 21:16:47 +0200 Subject: [PATCH 1/3] api/common: add requires_view decorator for Thrift methods Introduces a @requires_view decorator in api/common.py that wraps a Thrift API method's view-permission check into the method's decorator chain, so the permission requirement is visible in the method's signature rather than buried in the first line of the body. The decorator looks up the handler's existing __require_view() helper via getattr on the name-mangled attribute, so the helper does not need to be renamed and the change is purely additive in report_server.py. This commit applies the decorator to ThriftRequestHandler.getRunData as a design proof. A follow-up commit will migrate the remaining self.__require_view() call sites in report_server.py to use the decorator. --- web/server/codechecker_server/api/common.py | 22 +++++++++++++++++++ .../codechecker_server/api/report_server.py | 5 ++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/web/server/codechecker_server/api/common.py b/web/server/codechecker_server/api/common.py index 2fc699a24f..be46fcf7e2 100644 --- a/web/server/codechecker_server/api/common.py +++ b/web/server/codechecker_server/api/common.py @@ -5,6 +5,8 @@ # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception # # ------------------------------------------------------------------------- +import functools + import sqlalchemy from codechecker_api_shared.ttypes import RequestFailed, ErrorCode @@ -47,3 +49,23 @@ def wrapper(*args, **kwargs): raise RequestFailed(ErrorCode.GENERAL, msg) return wrapper + + +def requires_view(function): + """ + Decorator for Thrift API methods that require view permission on the + current product. Calls into the handler's __require_view() helper + before invoking the wrapped method. + + The helper is accessed via getattr because Python name-mangles + double-underscore attribute access at compile time, and the + decorator lives outside the handler class. Computing the mangled + name from the instance's class lets the same decorator work for any + handler that defines a __require_view() method. + """ + @functools.wraps(function) + def wrapper(self, *args, **kwargs): + mangled = f'_{type(self).__name__}__require_view' + getattr(self, mangled)() + return function(self, *args, **kwargs) + return wrapper diff --git a/web/server/codechecker_server/api/report_server.py b/web/server/codechecker_server/api/report_server.py index 217a613eb1..38568acab0 100644 --- a/web/server/codechecker_server/api/report_server.py +++ b/web/server/codechecker_server/api/report_server.py @@ -74,7 +74,7 @@ Run, RunHistory, RunHistoryAnalysisInfo, RunLock, \ SourceComponent, SourceComponentFile, FilterPreset -from .common import exc_to_thrift_reqfail +from .common import exc_to_thrift_reqfail, requires_view from .thrift_enum_helper import detection_status_enum, \ detection_status_str, report_status_enum, \ review_status_enum, review_status_str, report_extended_data_type_enum @@ -1545,9 +1545,8 @@ def __add_comment(self, bug_id, message, kind=CommentKindValue.USER, date or datetime.now()) @timeit + @requires_view def getRunData(self, run_filter, limit, offset, sort_mode): - self.__require_view() - limit = verify_limit_range(limit) with DBSession(self._Session) as session: From 0d5ba3f638467d91d54490fb2259192914efcff1 Mon Sep 17 00:00:00 2001 From: Bishara Hodali Date: Mon, 18 May 2026 21:27:29 +0200 Subject: [PATCH 2/3] report_server: apply @requires_view to remaining Thrift methods Migrates the remaining 42 Thrift API method bodies in ThriftRequestHandler from starting with a literal self.__require_view() call to using the @requires_view decorator introduced in the prior commit. The decorator inserts the permission check before the method body executes, so behavior is unchanged but the permission requirement is now visible at the method's signature. Performed mechanically: each call site was located, the enclosing def was identified, @requires_view was inserted above the def at matching indentation, and the call line was removed. Net diff is symmetric (42 insertions, 42 deletions). All 31 server unit tests still pass. --- .../codechecker_server/api/report_server.py | 84 +++++++++---------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/web/server/codechecker_server/api/report_server.py b/web/server/codechecker_server/api/report_server.py index 38568acab0..81bed4b224 100644 --- a/web/server/codechecker_server/api/report_server.py +++ b/web/server/codechecker_server/api/report_server.py @@ -1751,11 +1751,11 @@ def deleteFilterPreset(self, preset_id): @exc_to_thrift_reqfail @timeit + @requires_view def getFilterPreset(self, preset_id: int): """ Returns the FilterPreset identified by preset_id. """ - self.__require_view() LOG.info("Returning filter preset by ID: %s", preset_id) with DBSession(self._Session) as session: @@ -1777,11 +1777,11 @@ def getFilterPreset(self, preset_id: int): @exc_to_thrift_reqfail @timeit + @requires_view def listFilterPreset(self): """ Returns all filter presets stored for the product repository """ - self.__require_view() LOG.info("List back filter presets") try: @@ -1810,8 +1810,8 @@ def listFilterPreset(self): @exc_to_thrift_reqfail @timeit + @requires_view def getRunCount(self, run_filter): - self.__require_view() with DBSession(self._Session) as session: query = session.query(Run.id) @@ -1820,9 +1820,9 @@ def getRunCount(self, run_filter): return query.count() # DEPRECATED: use getAnalysisInfo API function instead of this function. + @requires_view def getCheckCommand(self, run_history_id, run_id): """ Get analyzer command based on the given filter. """ - self.__require_view() limit = None offset = 0 @@ -1838,9 +1838,9 @@ def getCheckCommand(self, run_history_id, run_id): @exc_to_thrift_reqfail @timeit + @requires_view def getAnalysisInfo(self, analysis_info_filter, limit, offset): """ Get analysis information based on the given filter. """ - self.__require_view() res: List[ttypes.AnalysisInfo] = [] if not analysis_info_filter: @@ -1910,8 +1910,8 @@ def getAnalysisInfo(self, analysis_info_filter, limit, offset): @exc_to_thrift_reqfail @timeit + @requires_view def getRunHistory(self, run_ids, limit, offset, run_history_filter): - self.__require_view() limit = verify_limit_range(limit) @@ -1950,8 +1950,8 @@ def getRunHistory(self, run_ids, limit, offset, run_history_filter): @exc_to_thrift_reqfail @timeit + @requires_view def getRunHistoryCount(self, run_ids, run_history_filter): - self.__require_view() with DBSession(self._Session) as session: query = session.query(RunHistory.id) @@ -1963,8 +1963,8 @@ def getRunHistoryCount(self, run_ids, run_history_filter): @exc_to_thrift_reqfail @timeit + @requires_view def getReport(self, reportId): - self.__require_view() with DBSession(self._Session) as session: @@ -2006,9 +2006,9 @@ def getReport(self, reportId): @exc_to_thrift_reqfail @timeit + @requires_view def getDiffResultsHash(self, run_ids, report_hashes, diff_type, skip_detection_statuses, tag_ids): - self.__require_view() # FIXME: This getDiffResultsHash() function is returning a set of # reports based on what are they compared to in a "CodeChecker cmd @@ -2119,9 +2119,9 @@ def getDiffResultsHash(self, run_ids, report_hashes, diff_type, @exc_to_thrift_reqfail @timeit + @requires_view def getRunResults(self, run_ids, limit, offset, sort_types, report_filter, cmp_data, get_details): - self.__require_view() limit = verify_limit_range(limit) @@ -2463,8 +2463,8 @@ def __filter_blame_info_on_line( @exc_to_thrift_reqfail @timeit + @requires_view def getReportAnnotations(self, run_ids, report_filter, cmp_data): - self.__require_view() with DBSession(self._Session) as session: filter_expression, join_tables = process_report_filter( @@ -2506,13 +2506,13 @@ def getReportAnnotations(self, run_ids, report_filter, cmp_data): return list(map(lambda x: x[0], result)) @timeit + @requires_view def getRunReportCounts(self, run_ids, report_filter, limit, offset): """ Count the results separately for multiple runs. If an empty run id list is provided the report counts will be calculated for all of the available runs. """ - self.__require_view() limit = verify_limit_range(limit) @@ -2558,8 +2558,8 @@ def getRunReportCounts(self, run_ids, report_filter, limit, offset): @exc_to_thrift_reqfail @timeit + @requires_view def getRunResultCount(self, run_ids, report_filter, cmp_data): - self.__require_view() with DBSession(self._Session) as session: filter_expression, join_tables = process_report_filter( @@ -2585,12 +2585,12 @@ def getRunResultCount(self, run_ids, report_filter, cmp_data): @exc_to_thrift_reqfail @timeit + @requires_view def getReportDetails(self, reportId): """ Parameters: - reportId """ - self.__require_view() with DBSession(self._Session) as session: return get_report_details(session, [reportId])[reportId] @@ -2701,11 +2701,11 @@ def _setReviewStatus(self, session, report_hash, status, @exc_to_thrift_reqfail @timeit + @requires_view def isReviewStatusChangeDisabled(self): """ Return True if review status change is disabled. """ - self.__require_view() with DBSession(self._config_database) as session: product = session.get(Product, self._product.id) @@ -2763,8 +2763,8 @@ def changeReviewStatus(self, report_id, status, message): @exc_to_thrift_reqfail @timeit + @requires_view def getReviewStatusRules(self, rule_filter, sort_mode, limit, offset): - self.__require_view() if not sort_mode: sort_mode = ReviewStatusRuleSortMode( type=ReviewStatusRuleSortType.DATE, @@ -2811,8 +2811,8 @@ def getRules(reportHashes=None): @exc_to_thrift_reqfail @timeit + @requires_view def getReviewStatusRulesCount(self, rule_filter): - self.__require_view() with DBSession(self._Session) as session: q = get_rs_rule_query(session, rule_filter) @@ -2868,11 +2868,11 @@ def addReviewStatusRule(self, report_hash, review_status, message): @exc_to_thrift_reqfail @timeit + @requires_view def getComments(self, report_id): """ Return the list of comments for the given bug. """ - self.__require_view() with DBSession(self._Session) as session: report = session.get(Report, report_id) @@ -2901,11 +2901,11 @@ def getComments(self, report_id): @exc_to_thrift_reqfail @timeit + @requires_view def getCommentCount(self, report_id): """ Return the number of comments for the given bug. """ - self.__require_view() with DBSession(self._Session) as session: report = session.get(Report, report_id) commentCount = 0 @@ -3027,22 +3027,22 @@ def removeComment(self, comment_id): @exc_to_thrift_reqfail @timeit + @requires_view def getCheckerDoc(self, _): """ Parameters: - checkerId """ - self.__require_view() return "" @exc_to_thrift_reqfail @timeit + @requires_view def getCheckerLabels( self, checkers: List[ttypes.Checker] ) -> List[List[str]]: """ Return the list of labels to each checker. """ - self.__require_view() labels = [] for checker in checkers: @@ -3061,12 +3061,12 @@ def getCheckerLabels( @exc_to_thrift_reqfail @timeit + @requires_view def getGuidelineRules( self, guidelines: List[ttypes.Guideline] ): """ Return the list of rules to each guideline that given. """ - self.__require_view() guideline_rules = defaultdict(list) for guideline in guidelines: @@ -3093,6 +3093,7 @@ def getGuidelineRules( @exc_to_thrift_reqfail @timeit + @requires_view def getSourceFileData(self, fileId, fileContent, encoding): """ Parameters: @@ -3100,7 +3101,6 @@ def getSourceFileData(self, fileId, fileContent, encoding): - fileContent - enum Encoding """ - self.__require_view() with DBSession(self._Session) as session: sourcefile = session.get(File, fileId) @@ -3134,9 +3134,9 @@ def getSourceFileData(self, fileId, fileContent, encoding): @exc_to_thrift_reqfail @timeit + @requires_view def getBlameInfo(self, fileId): """ Get blame information for the given file. """ - self.__require_view() with DBSession(self._Session) as session: sourcefile = session.get(File, fileId) @@ -3185,8 +3185,8 @@ def getBlameInfo(self, fileId): @exc_to_thrift_reqfail @timeit + @requires_view def getLinesInSourceFileContents(self, lines_in_files_requested, encoding): - self.__require_view() with DBSession(self._Session) as session: res = defaultdict(lambda: defaultdict(str)) @@ -3225,6 +3225,7 @@ def getLinesInSourceFileContents(self, lines_in_files_requested, encoding): @exc_to_thrift_reqfail @timeit + @requires_view def getCheckerCounts(self, run_ids, report_filter, cmp_data, limit, offset): """ @@ -3232,7 +3233,6 @@ def getCheckerCounts(self, run_ids, report_filter, cmp_data, limit, for all of the runs and in compare mode all of the runs will be used as a baseline excluding the runs in compare data. """ - self.__require_view() limit = verify_limit_range(limit) @@ -3297,8 +3297,8 @@ def getCheckerCounts(self, run_ids, report_filter, cmp_data, limit, @exc_to_thrift_reqfail @timeit + @requires_view def getCheckerStatusVerificationDetails(self, run_ids, report_filter): - self.__require_view() with DBSession(self._Session) as session: max_run_histories = session.query( @@ -3431,6 +3431,7 @@ def getCheckerStatusVerificationDetails(self, run_ids, report_filter): @exc_to_thrift_reqfail @timeit + @requires_view def getAnalyzerNameCounts(self, run_ids, report_filter, cmp_data, limit, offset): """ @@ -3438,7 +3439,6 @@ def getAnalyzerNameCounts(self, run_ids, report_filter, cmp_data, limit, for all of the runs and in compare mode all of the runs will be used as a baseline excluding the runs in compare data. """ - self.__require_view() limit = verify_limit_range(limit) @@ -3494,13 +3494,13 @@ def getAnalyzerNameCounts(self, run_ids, report_filter, cmp_data, limit, @exc_to_thrift_reqfail @timeit + @requires_view def getSeverityCounts(self, run_ids, report_filter, cmp_data): """ If the run id list is empty the metrics will be counted for all of the runs and in compare mode all of the runs will be used as a baseline excluding the runs in compare data. """ - self.__require_view() results = {} with DBSession(self._Session) as session: filter_expression, join_tables = process_report_filter( @@ -3546,6 +3546,7 @@ def getSeverityCounts(self, run_ids, report_filter, cmp_data): @exc_to_thrift_reqfail @timeit + @requires_view def getCheckerMsgCounts(self, run_ids, report_filter, cmp_data, limit, offset): """ @@ -3553,7 +3554,6 @@ def getCheckerMsgCounts(self, run_ids, report_filter, cmp_data, limit, for all of the runs and in compare mode all of the runs will be used as a baseline excluding the runs in compare data. """ - self.__require_view() limit = verify_limit_range(limit) @@ -3607,13 +3607,13 @@ def getCheckerMsgCounts(self, run_ids, report_filter, cmp_data, limit, @exc_to_thrift_reqfail @timeit + @requires_view def getReportStatusCounts(self, run_ids, report_filter, cmp_data): """ If the run id list is empty the metrics will be counted for all of the runs and in compare mode all of the runs will be used as a baseline excluding the runs in compare data. """ - self.__require_view() with DBSession(self._Session) as session: filter_expression, join_tables = process_report_filter( session, run_ids, report_filter, cmp_data) @@ -3672,13 +3672,13 @@ def getReportStatusCounts(self, run_ids, report_filter, cmp_data): @exc_to_thrift_reqfail @timeit + @requires_view def getReviewStatusCounts(self, run_ids, report_filter, cmp_data): """ If the run id list is empty the metrics will be counted for all of the runs and in compare mode all of the runs will be used as a baseline excluding the runs in compare data. """ - self.__require_view() with DBSession(self._Session) as session: filter_expression, join_tables = process_report_filter( session, run_ids, report_filter, cmp_data) @@ -3716,13 +3716,13 @@ def getReviewStatusCounts(self, run_ids, report_filter, cmp_data): @exc_to_thrift_reqfail @timeit + @requires_view def getFileCounts(self, run_ids, report_filter, cmp_data, limit, offset): """ If the run id list is empty the metrics will be counted for all of the runs and in compare mode all of the runs will be used as a baseline excluding the runs in compare data. """ - self.__require_view() limit = verify_limit_range(limit) @@ -3769,6 +3769,7 @@ def getFileCounts(self, run_ids, report_filter, cmp_data, limit, offset): @exc_to_thrift_reqfail @timeit + @requires_view def getRunHistoryTagCounts(self, run_ids, report_filter, cmp_data, limit, offset): """ @@ -3776,7 +3777,6 @@ def getRunHistoryTagCounts(self, run_ids, report_filter, cmp_data, limit, for all of the runs and in compare mode all of the runs will be used as a baseline excluding the runs in compare data. """ - self.__require_view() limit = verify_limit_range(limit) @@ -3859,13 +3859,13 @@ def getRunHistoryTagCounts(self, run_ids, report_filter, cmp_data, limit, @exc_to_thrift_reqfail @timeit + @requires_view def getDetectionStatusCounts(self, run_ids, report_filter, cmp_data): """ If the run id list is empty the metrics will be counted for all of the runs and in compare mode all of the runs will be used as a baseline excluding the runs in compare data. """ - self.__require_view() results = {} with DBSession(self._Session) as session: filter_expression, join_tables = process_report_filter( @@ -3907,13 +3907,13 @@ def getDetectionStatusCounts(self, run_ids, report_filter, cmp_data): @exc_to_thrift_reqfail @timeit + @requires_view def getFailedFilesCount(self, run_ids): """ Count the number of uniqued failed files in the latest storage of each given run. If the run id list is empty the number of failed files will be counted for all of the runs. """ - self.__require_view() # Unfortunately we can't distinct the failed file paths by using SQL # queries because the list of failed files for a run / analyzer are @@ -3923,13 +3923,13 @@ def getFailedFilesCount(self, run_ids): @exc_to_thrift_reqfail @timeit + @requires_view def getFailedFiles(self, run_ids): """ Get files which failed to analyze in the latest storage of the given runs. For each files it will return a list where each element contains information in which run the failure happened. """ - self.__require_view() res = defaultdict(list) with DBSession(self._Session) as session: @@ -3956,8 +3956,8 @@ def getFailedFiles(self, run_ids): # ----------------------------------------------------------------------- @timeit + @requires_view def getPackageVersion(self): - self.__require_view() return self.__package_version @@ -4175,11 +4175,11 @@ def addSourceComponent(self, name, value, description): @exc_to_thrift_reqfail @timeit + @requires_view def getSourceComponents(self, component_filter): """ Returns the available source components. """ - self.__require_view() with DBSession(self._Session) as session: q = session.query(SourceComponent) @@ -4419,8 +4419,8 @@ def storeAnalysisStatistics(self, run_name, b64zip): @exc_to_thrift_reqfail @timeit + @requires_view def getAnalysisStatistics(self, run_id, run_history_id): - self.__require_view() analyzer_statistics = {} @@ -4448,8 +4448,8 @@ def getAnalysisStatistics(self, run_id, run_history_id): @exc_to_thrift_reqfail @timeit + @requires_view def exportData(self, run_filter): - self.__require_view() with DBSession(self._Session) as session: @@ -4600,8 +4600,8 @@ def updateCleanupPlan(self, cleanup_plan_id, name, description, dueDate): @exc_to_thrift_reqfail @timeit + @requires_view def getCleanupPlans(self, cleanup_plan_filter): - self.__require_view() with DBSession(self._Session) as session: q = session \ .query(CleanupPlan) \ From d824b7b0e29dd3e93ecf4f41f86516e0f1a971fd Mon Sep 17 00:00:00 2001 From: Bishara Hodali Date: Tue, 19 May 2026 03:23:55 +0200 Subject: [PATCH 3/3] api: enforce permission check on every Thrift API method Adds a static check that every method declared in the Thrift service codeCheckerDBAccess_v6 has a detectable permission check on the implementing ThriftRequestHandler. A method is considered protected if any of the following holds: * It has the @requires_view decorator (introduced earlier in this branch). * One of its first few statements calls a self.__require_*() or self._require_*() permission helper (admin, access, store, permission, etc.). * It is listed in _DELEGATED_PROTECTION, in which case the check verifies that the named delegate helper itself satisfies one of the above conditions. This handles methods such as massStoreRun that pass the request through __massStoreRun_common, which in turn calls self.__require_store(). The check is wired into two places, both using the same logic so they cannot disagree: * As a unit test (test_thrift_permission_coverage.py), so a PR that introduces an unprotected method fails CI before being merged. * As a startup assertion in start_server(), so even if such a PR landed in master, the server would refuse to start instead of serving an exploitable endpoint. Thrift method names are discovered by introspecting the generated Iface class in codechecker_api.codeCheckerDBAccess_v6, which is present in both development and deployed installations -- no dependency on the .thrift source files. The __require_view helper on ThriftRequestHandler is renamed to _require_view (single underscore). The @requires_view decorator now calls self._require_view() directly. Pylint's unused-private-member checker cannot see through the previous getattr indirection that worked around Python's name mangling, so the original double- underscore form was flagged as dead code despite being live. --- web/server/codechecker_server/api/common.py | 11 +- .../api/permission_coverage.py | 219 ++++++++++++++++++ .../codechecker_server/api/report_server.py | 2 +- web/server/codechecker_server/server.py | 9 + .../unit/test_thrift_permission_coverage.py | 36 +++ 5 files changed, 267 insertions(+), 10 deletions(-) create mode 100644 web/server/codechecker_server/api/permission_coverage.py create mode 100644 web/server/tests/unit/test_thrift_permission_coverage.py diff --git a/web/server/codechecker_server/api/common.py b/web/server/codechecker_server/api/common.py index be46fcf7e2..fc68cdbfd6 100644 --- a/web/server/codechecker_server/api/common.py +++ b/web/server/codechecker_server/api/common.py @@ -54,18 +54,11 @@ def wrapper(*args, **kwargs): def requires_view(function): """ Decorator for Thrift API methods that require view permission on the - current product. Calls into the handler's __require_view() helper + current product. Calls into the handler's _require_view() helper before invoking the wrapped method. - - The helper is accessed via getattr because Python name-mangles - double-underscore attribute access at compile time, and the - decorator lives outside the handler class. Computing the mangled - name from the instance's class lets the same decorator work for any - handler that defines a __require_view() method. """ @functools.wraps(function) def wrapper(self, *args, **kwargs): - mangled = f'_{type(self).__name__}__require_view' - getattr(self, mangled)() + self._require_view() return function(self, *args, **kwargs) return wrapper diff --git a/web/server/codechecker_server/api/permission_coverage.py b/web/server/codechecker_server/api/permission_coverage.py new file mode 100644 index 0000000000..08e42878aa --- /dev/null +++ b/web/server/codechecker_server/api/permission_coverage.py @@ -0,0 +1,219 @@ +# ------------------------------------------------------------------------- +# +# Part of the CodeChecker project, under the Apache License v2.0 with +# LLVM Exceptions. See LICENSE for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +# +# ------------------------------------------------------------------------- +""" +Static enforcement that every Thrift API method on the report server has +some form of permission check. + +This is a security boundary: a Thrift method that lacks any permission +check is reachable by any authenticated user, which would allow an +unprivileged user to invoke functionality reserved for product admins, +superusers, or even unauthenticated callers in principle. + +The check is run from two places: + + * From a unit test, so that a PR that introduces an unprotected method + fails CI before being merged. + + * From the server's start_server() function, so that even if such a PR + were merged anyway, the server refuses to start instead of serving + an exploitable endpoint. + +The same logic is used in both, so they can never disagree. + +A method is considered "protected" if any of the following hold: + + * It has the @requires_view decorator applied. + * One of its first few statements calls into one of the existing + self.__require_*() permission helpers (admin, access, store, + permission, etc.). + * It is listed in _DELEGATED_PROTECTION and the named delegate + helper itself satisfies one of the above conditions. + +The set of Thrift methods to check is read from the generated Thrift +Python module (codechecker_api.codeCheckerDBAccess_v6.codeCheckerDBAccess), +which is shipped with both development and deployed installations. +""" +import ast +import inspect +from pathlib import Path +from typing import List, Set + +from codechecker_api.codeCheckerDBAccess_v6 import codeCheckerDBAccess + + +# Path to the handler implementing codeCheckerDBAccess_v6. +# Resolved relative to this file so it works in both source and built +# layouts. +_HANDLER_PATH = Path(__file__).parent / "report_server.py" + +# Name of the Python class that implements the Thrift Iface. +_HANDLER_CLASS_NAME = "ThriftRequestHandler" + +# Statement positions to inspect for an in-body permission check. +# Most methods place the check as their first non-docstring statement, +# but a handful do per-argument preprocessing first; three is a safe +# limit. +_MAX_PROLOGUE_STATEMENTS = 3 + +# Decorator names that count as a permission check on their own. +_PERMISSION_DECORATORS = frozenset({ + "requires_view", +}) + +# Substrings inside an attribute access that indicate a permission +# helper was called. We match on suffix rather than equality because +# Python's name-mangling rewrites `self.__require_view` to e.g. +# `self._ThriftRequestHandler__require_view` at parse time as well. +_PERMISSION_CHECK_SUFFIXES = ( + "_require_view", + "__require_view", + "__require_admin", + "__require_access", + "__require_store", + "__require_permission", + "__require_supermission", + "__require_privilaged_access", + "__require_permission_view", +) + +# Thrift methods that delegate their permission check to a helper +# rather than performing it as one of their own first statements. +# +# Each entry maps the Thrift method name to the helper method it +# delegates to. The check below verifies that the named helper itself +# performs a permission check; if a future change removes that helper's +# check, the delegation entry will fail and the Thrift method will be +# reported as unprotected. +_DELEGATED_PROTECTION = { + # Both massStoreRun variants share the same upload pipeline, + # __massStoreRun_common, which calls self.__require_store() before + # touching any data. + "massStoreRun": "__massStoreRun_common", + "massStoreRunAsynchronous": "__massStoreRun_common", +} + + +def _thrift_method_names() -> Set[str]: + """Return the names of all methods on the Thrift service Iface.""" + iface = codeCheckerDBAccess.Iface + return { + name + for name, _ in inspect.getmembers(iface, inspect.isfunction) + } + + +def _is_permission_decorator(decorator_node: ast.expr) -> bool: + """Return True if the AST decorator node names a known permission + decorator.""" + if isinstance(decorator_node, ast.Name): + return decorator_node.id in _PERMISSION_DECORATORS + if isinstance(decorator_node, ast.Attribute): + return decorator_node.attr in _PERMISSION_DECORATORS + return False + + +def _statement_is_permission_check(stmt: ast.stmt) -> bool: + """Return True if the statement is a self.__require_*()-style + call.""" + if not isinstance(stmt, ast.Expr): + return False + if not isinstance(stmt.value, ast.Call): + return False + func = stmt.value.func + if not isinstance(func, ast.Attribute): + return False + return any(func.attr.endswith(suffix) + for suffix in _PERMISSION_CHECK_SUFFIXES) + + +def _method_is_protected(method_node: ast.FunctionDef) -> bool: + """Return True if the method has either a permission decorator or + a permission-check call in its first few statements.""" + for decorator in method_node.decorator_list: + if _is_permission_decorator(decorator): + return True + # Skip a leading docstring, then look at the next few statements. + body = method_node.body + if body and isinstance(body[0], ast.Expr) \ + and isinstance(body[0].value, ast.Constant) \ + and isinstance(body[0].value.value, str): + body = body[1:] + for stmt in body[:_MAX_PROLOGUE_STATEMENTS]: + if _statement_is_permission_check(stmt): + return True + return False + + +def _build_method_index(class_node: ast.ClassDef) -> dict: + """Map method name -> FunctionDef node for a class.""" + return { + m.name: m + for m in class_node.body + if isinstance(m, ast.FunctionDef) + } + + +def find_unprotected_methods() -> List[str]: + """Return a list of Thrift method names on ThriftRequestHandler + that have no detectable permission check. Empty list means + everything is protected.""" + thrift_methods = _thrift_method_names() + tree = ast.parse(_HANDLER_PATH.read_text(encoding="utf-8")) + + unprotected: List[str] = [] + for cls in ast.walk(tree): + if not isinstance(cls, ast.ClassDef): + continue + if cls.name != _HANDLER_CLASS_NAME: + continue + + methods_by_name = _build_method_index(cls) + + for member in cls.body: + if not isinstance(member, ast.FunctionDef): + continue + if member.name not in thrift_methods: + continue + + if _method_is_protected(member): + continue + + # Not directly protected; consult the delegation map. + delegate_name = _DELEGATED_PROTECTION.get(member.name) + if delegate_name is not None: + # Look up the delegate by its mangled name as it would + # appear inside the same class (double-underscore + # methods are name-mangled). + mangled = f"_{_HANDLER_CLASS_NAME}{delegate_name}" + delegate_node = ( + methods_by_name.get(delegate_name) + or methods_by_name.get(mangled)) + if delegate_node is not None \ + and _method_is_protected(delegate_node): + continue + # Delegation declared but delegate missing or itself + # unprotected -- fall through, this method is + # unprotected. + + unprotected.append(member.name) + + return sorted(unprotected) + + +def assert_all_thrift_methods_protected() -> None: + """Raise RuntimeError listing any Thrift methods that are not + permission-protected. Called from both the unit test and from + start_server() at server startup.""" + unprotected = find_unprotected_methods() + if unprotected: + raise RuntimeError( + "Thrift API methods without a detectable permission check " + "were found. Add either an @requires_view decorator, an " + "in-body self.__require_*() call, or a " + "_DELEGATED_PROTECTION entry for each:\n " + + "\n ".join(unprotected)) diff --git a/web/server/codechecker_server/api/report_server.py b/web/server/codechecker_server/api/report_server.py index 81bed4b224..d2ecd40e0e 100644 --- a/web/server/codechecker_server/api/report_server.py +++ b/web/server/codechecker_server/api/report_server.py @@ -1528,7 +1528,7 @@ def __require_access(self): def __require_store(self): self.__require_permission([permissions.PRODUCT_STORE]) - def __require_view(self): + def _require_view(self): self.__require_permission([ permissions.PRODUCT_VIEW, permissions.PERMISSION_VIEW diff --git a/web/server/codechecker_server/server.py b/web/server/codechecker_server/server.py index b06ef3a005..59229e051b 100644 --- a/web/server/codechecker_server/server.py +++ b/web/server/codechecker_server/server.py @@ -1110,6 +1110,15 @@ def _cleanup_incomplete_tasks(action: str) -> int: "by a previous server instance matching machine ID '%s'.", dropped_tasks, machine_id) + # Defense in depth: refuse to start if any Thrift API method + # lacks a permission check. The same check is also run as a unit + # test for PR CI; the startup variant guarantees that even if an + # unprotected method somehow lands in master, the server will + # not serve it. + from .api.permission_coverage import ( + assert_all_thrift_methods_protected, + ) + assert_all_thrift_methods_protected() server_clazz = CCSimpleHttpServer if ':' in listen_address: # IPv6 address specified for listening. diff --git a/web/server/tests/unit/test_thrift_permission_coverage.py b/web/server/tests/unit/test_thrift_permission_coverage.py new file mode 100644 index 0000000000..2026db32ff --- /dev/null +++ b/web/server/tests/unit/test_thrift_permission_coverage.py @@ -0,0 +1,36 @@ +# ------------------------------------------------------------------------- +# +# Part of the CodeChecker project, under the Apache License v2.0 with +# LLVM Exceptions. See LICENSE for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +# +# ------------------------------------------------------------------------- +""" +Verify that every Thrift API method on the report server has a +detectable permission check. + +This guards against a class of security bug: a Thrift method that is +exposed to the JavaScript client but lacks a permission check, allowing +an unprivileged user to invoke functionality reserved for product +admins or other higher-privileged roles. + +The same check is also run at server startup; this test exists to fail +PR CI early, before such a regression can be merged. +""" +import unittest + +from codechecker_server.api.permission_coverage import ( + assert_all_thrift_methods_protected, +) + + +class ThriftPermissionCoverageTest(unittest.TestCase): + def test_all_thrift_methods_protected(self): + """Every Thrift method on ThriftRequestHandler is protected by + either an @requires_view decorator, a self.__require_*() call + in its first few statements, or an audited entry in + _DELEGATED_PROTECTION.""" + # Raises RuntimeError listing offenders if any method is + # unprotected; the unittest framework converts the raise into + # a test failure with that message visible. + assert_all_thrift_methods_protected()