diff --git a/web/server/codechecker_server/api/common.py b/web/server/codechecker_server/api/common.py index 2fc699a24f..fc68cdbfd6 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,16 @@ 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. + """ + @functools.wraps(function) + def wrapper(self, *args, **kwargs): + 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 217a613eb1..d2ecd40e0e 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 @@ -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 @@ -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: @@ -1752,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: @@ -1778,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: @@ -1811,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) @@ -1821,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 @@ -1839,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: @@ -1911,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) @@ -1951,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) @@ -1964,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: @@ -2007,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 @@ -2120,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) @@ -2464,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( @@ -2507,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) @@ -2559,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( @@ -2586,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] @@ -2702,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) @@ -2764,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, @@ -2812,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) @@ -2869,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) @@ -2902,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 @@ -3028,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: @@ -3062,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: @@ -3094,6 +3093,7 @@ def getGuidelineRules( @exc_to_thrift_reqfail @timeit + @requires_view def getSourceFileData(self, fileId, fileContent, encoding): """ Parameters: @@ -3101,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) @@ -3135,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) @@ -3186,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)) @@ -3226,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): """ @@ -3233,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) @@ -3298,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( @@ -3432,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): """ @@ -3439,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) @@ -3495,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( @@ -3547,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): """ @@ -3554,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) @@ -3608,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) @@ -3673,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) @@ -3717,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) @@ -3770,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): """ @@ -3777,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) @@ -3860,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( @@ -3908,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 @@ -3924,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: @@ -3957,8 +3956,8 @@ def getFailedFiles(self, run_ids): # ----------------------------------------------------------------------- @timeit + @requires_view def getPackageVersion(self): - self.__require_view() return self.__package_version @@ -4176,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) @@ -4420,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 = {} @@ -4449,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: @@ -4601,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) \ 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()