fix(auth): Respond with bad json on signed requests [INGEST-407] (#1090)

Responds with "400 Bad Request" and a descriptive error message when
sending invalid JSON payloads on endpoints requiring `SignedJson`.
Previously, this would just result in a misleading "invalid relay
signature" error.

CHANGELOG.md

@@ -6,6 +6,7 @@
 
 - Correctly validate timestamps for outcomes and sessions. ([#1086](https://github.com/getsentry/relay/pull/1086))
 - Run compression on a thread pool when sending to upstream. ([#1085](https://github.com/getsentry/relay/pull/1085))
+- Report proper status codes and error messages when sending invalid JSON payloads to an endpoint with a `X-Sentry-Relay-Signature` header. ([#1090](https://github.com/getsentry/relay/pull/1090))
 
 **Internal**:
 - Add the exclusive time of the transaction's root span. ([#1083](https://github.com/getsentry/relay/pull/1083))

relay-server/src/extractors/signed_json.rs

@@ -1,10 +1,11 @@
 use actix_web::actix::*;
+use actix_web::http::StatusCode;
 use actix_web::{Error, FromRequest, HttpMessage, HttpRequest, HttpResponse, ResponseError};
 use failure::Fail;
 use futures::prelude::*;
 use serde::de::DeserializeOwned;
 
-use relay_auth::RelayId;
+use relay_auth::{RelayId, UnpackError};
 use relay_common::tryf;
 use relay_config::RelayInfo;
 use relay_log::Hub;
@@ -23,18 +24,34 @@ pub struct SignedJson<T> {
 #[derive(Fail, Debug)]
 enum SignatureError {
     #[fail(display = "invalid relay signature")]
-    BadSignature,
+    BadSignature(#[cause] UnpackError),
     #[fail(display = "missing header: {}", _0)]
     MissingHeader(&'static str),
     #[fail(display = "malformed header: {}", _0)]
     MalformedHeader(&'static str),
     #[fail(display = "Unknown relay id")]
     UnknownRelay,
+    #[fail(display = "invalid JSON data")]
+    InvalidJson(#[cause] serde_json::Error),
 }
 
 impl ResponseError for SignatureError {
     fn error_response(&self) -> HttpResponse {
-        HttpResponse::Unauthorized().json(&ApiErrorResponse::from_fail(self))
+        let status = match self {
+            SignatureError::InvalidJson(_) => StatusCode::BAD_REQUEST,
+            _ => StatusCode::UNAUTHORIZED,
+        };
+
+        HttpResponse::build(status).json(&ApiErrorResponse::from_fail(self))
+    }
+}
+
+impl From<UnpackError> for SignatureError {
+    fn from(error: UnpackError) -> Self {
+        match error {
+            UnpackError::BadPayload(json_error) => Self::InvalidJson(json_error),
+            other => Self::BadSignature(other),
+        }
     }
 }
 
@@ -80,7 +97,7 @@ impl<T: DeserializeOwned + 'static> FromRequest<ServiceState> for SignedJson<T>
                     .public_key
                     .unpack(&body, &relay_sig, None)
                     .map(|inner| SignedJson { inner, relay })
-                    .map_err(|_| Error::from(SignatureError::BadSignature))
+                    .map_err(|e| Error::from(SignatureError::from(e)))
             });
 
         Box::new(future)

tests/integration/test_projectconfigs.py

@@ -93,3 +93,38 @@ def test_dynamic_relays(mini_sentry, relay, caller, projects):
     data = resp.json()
     for p in public_keys:
         assert data["configs"][p] is not None
+
+
+def test_invalid_json(mini_sentry, relay):
+    relay = relay(mini_sentry, wait_healthcheck=True)
+
+    body = "{}"  # missing the required `public_keys` field
+    packed, signature = SecretKey.parse(relay.secret_key).pack(body)
+
+    response = relay.post(
+        "/api/0/relays/projectconfigs/?version=2",
+        data=packed,
+        headers={
+            "X-Sentry-Relay-Id": relay.relay_id,
+            "X-Sentry-Relay-Signature": signature,
+        },
+    )
+
+    assert response.status_code == 400  # Bad Request
+    assert "JSON" in response.text
+
+
+def test_invalid_signature(mini_sentry, relay):
+    relay = relay(mini_sentry, wait_healthcheck=True)
+
+    response = relay.post(
+        "/api/0/relays/projectconfigs/?version=2",
+        data='{"public_keys":[]}',
+        headers={
+            "X-Sentry-Relay-Id": relay.relay_id,
+            "X-Sentry-Relay-Signature": "broken",
+        },
+    )
+
+    assert response.status_code == 401  # Unauthorized
+    assert "signature" in response.text