Browse Source

fix(symbolSources): Deal better with missing hidden secrets (#28996)

This improves handling of someone sending in missing hidden secrets.
This could still be a UI bug though, so do still capture a message but
do not break in an unexpected way that's hard to understand.

Fixes SENTRY-S3C
Floris Bruynooghe 3 years ago
parent
commit
c5e416dfdf

+ 9 - 7
src/sentry/lang/native/symbolicator.py

@@ -19,7 +19,7 @@ from sentry.cache import default_cache
 from sentry.models import Organization
 from sentry.net.http import Session
 from sentry.tasks.store import RetrySymbolication
-from sentry.utils import json, metrics
+from sentry.utils import json, metrics, safe
 
 MAX_ATTEMPTS = 3
 REQUEST_CACHE_TIMEOUT = 3600
@@ -362,7 +362,7 @@ def parse_backfill_sources(sources_json, original_sources):
     try:
         sources = json.loads(sources_json)
     except Exception as e:
-        raise InvalidSourcesError(f"{e}")
+        raise InvalidSourcesError("Sources are not valid serialised JSON") from e
 
     orig_by_id = {src["id"]: src for src in original_sources}
 
@@ -377,15 +377,17 @@ def parse_backfill_sources(sources_json, original_sources):
 
         for secret in secret_fields(source["type"]):
             if secret in source and source[secret] == {"hidden-secret": True}:
-                orig_source = orig_by_id.get(source["id"])
-                # Should just omit the source entirely if it's referencing a previously stored
-                # secret that we can't find
-                source[secret] = orig_source.get(secret, "")
+                secret_value = safe.get_path(orig_by_id, source["id"], secret)
+                if secret_value is None:
+                    sentry_sdk.capture_message("Hidden secret not present in project options")
+                    raise InvalidSourcesError("Sources contain unknown hidden secret")
+                else:
+                    source[secret] = secret_value
 
     try:
         jsonschema.validate(sources, SOURCES_SCHEMA)
     except jsonschema.ValidationError as e:
-        raise InvalidSourcesError(f"{e}")
+        raise InvalidSourcesError("Sources did not validate JSON-schema") from e
 
     return sources
 

+ 35 - 2
tests/sentry/api/endpoints/test_project_details.py

@@ -816,8 +816,41 @@ class ProjectUpdateTest(APITestCase):
                 self.org_slug, self.proj_slug, symbolSources=json.dumps([redacted_source])
             )
             # on save the magic object should be replaced with the previously set password
-            redacted_source["password"] = "beepbeep"
-            assert self.project.get_option("sentry:symbol_sources") == json.dumps([redacted_source])
+            assert self.project.get_option("sentry:symbol_sources") == json.dumps([config])
+
+    @mock.patch("sentry.api.base.create_audit_entry")
+    def test_redacted_symbol_source_secrets_unknown_secret(self, create_audit_entry):
+        with Feature(
+            {"organizations:symbol-sources": True, "organizations:custom-symbol-sources": True}
+        ):
+            config = {
+                "id": "honk",
+                "name": "honk source",
+                "layout": {
+                    "type": "native",
+                },
+                "filetypes": ["pe"],
+                "type": "http",
+                "url": "http://honk.beep",
+                "username": "honkhonk",
+                "password": "beepbeep",
+            }
+            self.get_valid_response(
+                self.org_slug, self.proj_slug, symbolSources=json.dumps([config])
+            )
+            assert self.project.get_option("sentry:symbol_sources") == json.dumps([config])
+
+            # prepare new call, this secret is not known
+            new_source = config.copy()
+            new_source["password"] = {"hidden-secret": True}
+            new_source["id"] = "oops"
+            response = self.get_response(
+                self.org_slug, self.proj_slug, symbolSources=json.dumps([new_source])
+            )
+            assert response.status_code == 400
+            assert json.loads(response.content) == {
+                "symbolSources": ["Sources contain unknown hidden secret"]
+            }
 
 
 class CopyProjectSettingsTest(APITestCase):