From e536a14b617eaef864f903617c6c6dbb635b0cd7 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Mon, 10 Apr 2023 18:45:14 +0000 Subject: [PATCH] report_error: Remove API endpoint for client error reporting. --- pyproject.toml | 1 - requirements/common.in | 3 - requirements/dev.txt | 4 -- requirements/prod.txt | 4 -- tools/test-backend | 1 - zerver/lib/unminify.py | 72 ------------------------ zerver/middleware.py | 5 +- zerver/tests/test_middleware.py | 1 - zerver/tests/test_report.py | 76 +------------------------ zerver/views/report.py | 90 +----------------------------- zproject/default_settings.py | 1 - zproject/prod_settings_template.py | 2 - zproject/urls.py | 6 -- 13 files changed, 3 insertions(+), 263 deletions(-) delete mode 100644 zerver/lib/unminify.py diff --git a/pyproject.toml b/pyproject.toml index f7cc39c389..029c5845be 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -83,7 +83,6 @@ module = [ "scrapy.*", # https://github.com/scrapy/scrapy/issues/4041 "social_core.*", "social_django.*", - "sourcemap.*", "talon_core.*", "tlds.*", "twitter.*", diff --git a/requirements/common.in b/requirements/common.in index 67d6b2ab0d..4961244092 100644 --- a/requirements/common.in +++ b/requirements/common.in @@ -82,9 +82,6 @@ https://github.com/andersk/zoneinfo/archive/f9687abaea8453be1c8d0e21544bd557d65a # Needed for Redis redis -# Needed to parse source maps for error reporting -sourcemap - # Tornado used for server->client push system tornado diff --git a/requirements/dev.txt b/requirements/dev.txt index b4e7e56fe5..24cd48aa4b 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -2111,10 +2111,6 @@ soupsieve==2.4 \ # via # -r requirements/common.in # beautifulsoup4 -sourcemap==0.2.1 \ - --hash=sha256:be00a90185e7a16b87bbe62a68ffd5e38bc438ef4700806d9b90e44d8027787c \ - --hash=sha256:c448a8c48f9482e522e4582106b0c641a83b5dbc7f13927b178848e3ea20967b - # via -r requirements/common.in sphinx==6.1.3 \ --hash=sha256:0dac3b698538ffef41716cf97ba26c1c7788dba73ce6f150c1ff5b4720786dd2 \ --hash=sha256:807d1cb3d6be87eb78a381c3e70ebd8d346b9a25f3753e9947e866b2786865fc diff --git a/requirements/prod.txt b/requirements/prod.txt index b1c48a37c8..6179d342d7 100644 --- a/requirements/prod.txt +++ b/requirements/prod.txt @@ -1467,10 +1467,6 @@ soupsieve==2.4 \ # via # -r requirements/common.in # beautifulsoup4 -sourcemap==0.2.1 \ - --hash=sha256:be00a90185e7a16b87bbe62a68ffd5e38bc438ef4700806d9b90e44d8027787c \ - --hash=sha256:c448a8c48f9482e522e4582106b0c641a83b5dbc7f13927b178848e3ea20967b - # via -r requirements/common.in sqlalchemy==1.4.47 \ --hash=sha256:03be6f3cb66e69fb3a09b5ea89d77e4bc942f3bf84b207dba84666a26799c166 \ --hash=sha256:048509d7f3ac27b83ad82fd96a1ab90a34c8e906e4e09c8d677fc531d12c23c5 \ diff --git a/tools/test-backend b/tools/test-backend index f887e6ac50..556c70319a 100755 --- a/tools/test-backend +++ b/tools/test-backend @@ -103,7 +103,6 @@ not_yet_fully_covered = [ "zerver/lib/queue.py", "zerver/lib/sqlalchemy_utils.py", "zerver/lib/storage.py", - "zerver/lib/unminify.py", "zerver/lib/utils.py", "zerver/lib/zephyr.py", "zerver/lib/templates.py", diff --git a/zerver/lib/unminify.py b/zerver/lib/unminify.py deleted file mode 100644 index e859a2bc89..0000000000 --- a/zerver/lib/unminify.py +++ /dev/null @@ -1,72 +0,0 @@ -import os -import re -from typing import Dict, List, Optional - -import sourcemap - -from zerver.lib.pysa import mark_sanitized - - -class SourceMap: - """Map (line, column) pairs from generated to source file.""" - - def __init__(self, sourcemap_dirs: List[str]) -> None: - self._dirs = sourcemap_dirs - self._indices: Dict[str, sourcemap.SourceMapDecoder] = {} - - def _index_for(self, minified_src: str) -> Optional[sourcemap.SourceMapDecoder]: - """Return the source map index for minified_src, loading it if not - already loaded.""" - - # Prevent path traversal - assert ".." not in minified_src and "/" not in minified_src - - if minified_src not in self._indices: - for source_dir in self._dirs: - filename = os.path.join(source_dir, minified_src + ".map") - if os.path.isfile(filename): - # Use 'mark_sanitized' to force Pysa to ignore the fact that - # 'filename' is user controlled. While putting user - # controlled data into a filesystem operation is bad, in - # this case it's benign because 'filename' can't traverse - # directories outside of the pre-configured 'sourcemap_dirs' - # (due to the above assertions) and will always end in - # '.map'. Additionally, the result of this function is used - # for error logging and not returned to the user, so - # controlling the loaded file would not be useful to an - # attacker. - with open(mark_sanitized(filename)) as fp: - self._indices[minified_src] = sourcemap.load(fp) - break - - return self._indices.get(minified_src) - - def annotate_stacktrace(self, stacktrace: str) -> str: - out: str = "" - for ln in stacktrace.splitlines(): - out += ln + "\n" - match = re.search(r"/webpack-bundles/([^:]+):(\d+):(\d+)", ln) - if match: - # Get the appropriate source map for the minified file. - minified_src = match.groups()[0] - index = self._index_for(minified_src) - if index is None: - out += " [Unable to look up in source map]\n" - continue - - gen_line, gen_col = list(map(int, match.groups()[1:3])) - # The sourcemap lib is 0-based, so subtract 1 from line and col. - try: - result = index.lookup(line=gen_line - 1, column=gen_col - 1) - display_src = result.src - if display_src is not None: - webpack_prefix = "webpack:///" - if display_src.startswith(webpack_prefix): - display_src = display_src[len(webpack_prefix) :] - out += f" = {display_src} line {result.src_line+1} column {result.src_col+1}\n" - except IndexError: - out += " [Unable to look up in source map]\n" - - if ln.startswith(" at"): - out += "\n" - return out diff --git a/zerver/middleware.py b/zerver/middleware.py index 58d199e35b..e848b9430d 100644 --- a/zerver/middleware.py +++ b/zerver/middleware.py @@ -112,10 +112,7 @@ def format_timedelta(timedelta: float) -> str: def is_slow_query(time_delta: float, path: str) -> bool: if time_delta < 1.2: return False - is_exempt = path in [ - "/activity", - "/json/report/error", - ] or path.startswith(("/realm_activity/", "/user_activity/")) + is_exempt = path == "/activity" or path.startswith(("/realm_activity/", "/user_activity/")) if is_exempt: return time_delta >= 5 if "webathena_kerberos" in path: diff --git a/zerver/tests/test_middleware.py b/zerver/tests/test_middleware.py index 21a37adb65..17184be8e2 100644 --- a/zerver/tests/test_middleware.py +++ b/zerver/tests/test_middleware.py @@ -31,7 +31,6 @@ class SlowQueryTest(ZulipTestCase): self.assertTrue(is_slow_query(2, "/some/random/url")) self.assertTrue(is_slow_query(5.1, "/activity")) self.assertFalse(is_slow_query(2, "/activity")) - self.assertFalse(is_slow_query(2, "/json/report/error")) self.assertFalse(is_slow_query(2, "/realm_activity/whatever")) self.assertFalse(is_slow_query(2, "/user_activity/whatever")) self.assertFalse(is_slow_query(9, "/accounts/webathena_kerberos_login/")) diff --git a/zerver/tests/test_report.py b/zerver/tests/test_report.py index 72299cd674..45011b239a 100644 --- a/zerver/tests/test_report.py +++ b/zerver/tests/test_report.py @@ -1,20 +1,10 @@ -from typing import Callable, ContextManager, Dict, List, Tuple +from typing import Callable, ContextManager, List, Tuple from unittest import mock -import orjson -from django.test import override_settings - from zerver.lib.test_classes import ZulipTestCase -from zerver.lib.test_helpers import mock_queue_publish from zerver.lib.utils import statsd -def fix_params(raw_params: Dict[str, object]) -> Dict[str, str]: - # A few of our few legacy endpoints need their - # individual parameters serialized as JSON. - return {k: orjson.dumps(v).decode() for k, v in raw_params.items()} - - class StatsMock: def __init__(self, settings: Callable[..., ContextManager[None]]) -> None: self.settings = settings @@ -132,70 +122,6 @@ class TestReport(ZulipTestCase): ] self.assertEqual(stats_mock.func_calls, expected_calls) - @override_settings(BROWSER_ERROR_REPORTING=True) - def test_report_error(self) -> None: - user = self.example_user("hamlet") - self.login_user(user) - self.make_stream("errors", user.realm) - - params = fix_params( - dict( - message="hello", - stacktrace="trace", - user_agent="agent", - href="href", - log="log", - more_info=dict(foo="bar", draft_content="**draft**"), - ) - ) - - version_mock = mock.patch("zerver.views.report.ZULIP_VERSION", spec="1.2.3") - with mock_queue_publish("zerver.views.report.queue_json_publish") as m, version_mock: - result = self.client_post("/json/report/error", params) - self.assert_json_success(result) - - report = m.call_args[0][1]["report"] - for k in set(params) - {"more_info"}: - self.assertEqual(report[k], params[k]) - - self.assertEqual(report["more_info"], dict(foo="bar", draft_content="'**xxxxx**'")) - self.assertEqual(report["user"]["user_email"], user.delivery_email) - - # Test with no more_info - del params["more_info"] - with mock_queue_publish("zerver.views.report.queue_json_publish") as m, version_mock: - result = self.client_post("/json/report/error", params) - self.assert_json_success(result) - - with self.settings(BROWSER_ERROR_REPORTING=False): - result = self.client_post("/json/report/error", params) - self.assert_json_success(result) - - # If js_source_map is present, then the stack trace should be annotated. - # DEVELOPMENT=False and TEST_SUITE=False are necessary to ensure that - # js_source_map actually gets instantiated. - with self.settings(DEVELOPMENT=False, TEST_SUITE=False), mock.patch( - "zerver.lib.unminify.SourceMap.annotate_stacktrace" - ) as annotate, self.assertLogs(level="INFO") as info_logs, version_mock: - result = self.client_post("/json/report/error", params) - self.assert_json_success(result) - # fix_params (see above) adds quotes when JSON encoding. - annotate.assert_called_once_with('"trace"') - self.assertEqual( - info_logs.output, ["INFO:root:Processing traceback with type browser for None"] - ) - - # Now test without authentication. - self.logout() - with self.settings(DEVELOPMENT=False, TEST_SUITE=False), mock.patch( - "zerver.lib.unminify.SourceMap.annotate_stacktrace" - ) as annotate, self.assertLogs(level="INFO") as info_logs: - result = self.client_post("/json/report/error", params) - self.assert_json_success(result) - self.assertEqual( - info_logs.output, ["INFO:root:Processing traceback with type browser for None"] - ) - def test_report_csp_violations(self) -> None: fixture_data = self.fixture_data("csp_report.json") with self.assertLogs(level="WARNING") as warn_logs: diff --git a/zerver/views/report.py b/zerver/views/report.py index cbe3dbc04f..9e5d03ac2c 100644 --- a/zerver/views/report.py +++ b/zerver/views/report.py @@ -1,48 +1,26 @@ # System documented in https://zulip.readthedocs.io/en/latest/subsystems/logging.html import logging -from typing import Any, Mapping, Optional, Union -from urllib.parse import SplitResult +from typing import Union -from django.conf import settings from django.contrib.auth.models import AnonymousUser from django.http import HttpRequest, HttpResponse from django.views.decorators.csrf import csrf_exempt from django.views.decorators.http import require_POST -from version import ZULIP_VERSION from zerver.context_processors import get_valid_realm_from_request from zerver.decorator import human_users_only -from zerver.lib.markdown import privacy_clean_markdown -from zerver.lib.queue import queue_json_publish from zerver.lib.request import REQ, RequestNotes, has_request_variables from zerver.lib.response import json_success -from zerver.lib.storage import static_path -from zerver.lib.unminify import SourceMap from zerver.lib.utils import statsd, statsd_key from zerver.lib.validator import ( WildValue, check_bool, - check_dict, check_string, to_non_negative_int, to_wild_value, ) from zerver.models import UserProfile -js_source_map: Optional[SourceMap] = None - - -# Read the source map information for decoding JavaScript backtraces. -def get_js_source_map() -> Optional[SourceMap]: - global js_source_map - if not js_source_map and not (settings.DEVELOPMENT or settings.TEST_SUITE): - js_source_map = SourceMap( - [ - static_path("webpack-bundles"), - ] - ) - return js_source_map - @human_users_only @has_request_variables @@ -117,72 +95,6 @@ def report_unnarrow_times( return json_success(request) -@has_request_variables -def report_error( - request: HttpRequest, - maybe_user_profile: Union[AnonymousUser, UserProfile], - message: str = REQ(), - stacktrace: str = REQ(), - user_agent: str = REQ(), - href: str = REQ(), - log: str = REQ(), - web_version: Optional[str] = REQ(default=None), - more_info: Mapping[str, Any] = REQ(json_validator=check_dict([]), default={}), -) -> HttpResponse: - """Accepts an error report and stores in a queue for processing. The - actual error reports are later handled by do_report_error""" - if not settings.BROWSER_ERROR_REPORTING: - return json_success(request) - more_info = dict(more_info) - - js_source_map = get_js_source_map() - if js_source_map: - stacktrace = js_source_map.annotate_stacktrace(stacktrace) - - server_version = str(ZULIP_VERSION) - - # Get the IP address of the request - remote_ip = request.META["REMOTE_ADDR"] - - # For the privacy of our users, we remove any actual text content - # in draft_content (from drafts rendering exceptions). See the - # comment on privacy_clean_markdown for more details. - if more_info.get("draft_content"): - more_info["draft_content"] = privacy_clean_markdown(more_info["draft_content"]) - - if maybe_user_profile.is_authenticated: - user = { - "user_email": maybe_user_profile.delivery_email, - "user_full_name": maybe_user_profile.full_name, - "user_role": maybe_user_profile.get_role_name(), - } - else: - user = None - - queue_json_publish( - "error_reports", - dict( - type="browser", - report=dict( - host=SplitResult("", request.get_host(), "", "", "").hostname, - ip_address=remote_ip, - user=user, - server_path=settings.DEPLOY_ROOT, - server_version=server_version, - web_version=web_version, - user_agent=user_agent, - href=href, - message=message, - stacktrace=stacktrace, - log=log, - more_info=more_info, - ), - ), - ) - - return json_success(request) - - @csrf_exempt @require_POST @has_request_variables diff --git a/zproject/default_settings.py b/zproject/default_settings.py index e27a82e80f..3318a536aa 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -126,7 +126,6 @@ EMAIL_GATEWAY_EXTRA_PATTERN_HACK: Optional[str] = None # Error reporting ERROR_REPORTING = True -BROWSER_ERROR_REPORTING = False LOGGING_SHOW_MODULE = False LOGGING_SHOW_PID = False diff --git a/zproject/prod_settings_template.py b/zproject/prod_settings_template.py index cce35745cc..6daf655bd6 100644 --- a/zproject/prod_settings_template.py +++ b/zproject/prod_settings_template.py @@ -652,8 +652,6 @@ SOCIAL_AUTH_SAML_SUPPORT_CONTACT = { ## Controls whether or not error reports (tracebacks) are emailed to the ## server administrators. # ERROR_REPORTING = True -## For frontend (JavaScript) tracebacks -# BROWSER_ERROR_REPORTING = False ## Controls the DSN used to report errors to Sentry.io # SENTRY_DSN = "https://aaa@bbb.ingest.sentry.io/1234" diff --git a/zproject/urls.py b/zproject/urls.py index b5685ff2cd..d3c2109a7a 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -132,7 +132,6 @@ from zerver.views.registration import ( ) from zerver.views.report import ( report_csp_violations, - report_error, report_narrow_times, report_send_times, report_unnarrow_times, @@ -491,11 +490,6 @@ v1_api_and_json_patterns = [ # These endpoints are for internal error/performance reporting # from the browser to the web app, and we don't expect to ever # include in our API documentation. - rest_path( - "report/error", - # Logged-out browsers can hit this endpoint, for portico page JS exceptions. - POST=(report_error, {"allow_anonymous_user_web", "intentionally_undocumented"}), - ), rest_path("report/send_times", POST=(report_send_times, {"intentionally_undocumented"})), rest_path( "report/narrow_times",