paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-04 05:53:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

report_error: Remove API endpoint for client error reporting.

Browse Source
This commit is contained in:
Alex Vandiver
2023-04-10 18:45:14 +00:00
committed by Tim Abbott
parent cb7bc1b7b9
commit e536a14b61
13 changed files with 3 additions and 263 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
pyproject.toml
View File

@@ -83,7 +83,6 @@ module = [
"scrapy.*", # https://github.com/scrapy/scrapy/issues/4041
"social_core.*",
"social_django.*",
"sourcemap.*",
"talon_core.*",
"tlds.*",
"twitter.*",

3
requirements/common.in
View File

@@ -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

4
requirements/dev.txt
View File

@@ -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

4
requirements/prod.txt
View File

@@ -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 \

1
tools/test-backend
View File

@@ -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",

72
zerver/lib/unminify.py
View File

@@ -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

5
zerver/middleware.py
View File

@@ -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:

1
zerver/tests/test_middleware.py
View File

@@ -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/"))

76
zerver/tests/test_report.py
View File

@@ -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:

90
zerver/views/report.py
View File

@@ -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

1
zproject/default_settings.py
View File

@@ -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

2
zproject/prod_settings_template.py
View File

@@ -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"

6
zproject/urls.py
View File

@@ -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",