Separate user input errors from internal errors for Sentry reporting (#11308)

* Initial plan

* Add explicit error categorization with isSentryReported property

Co-authored-by: tommoor <380914+tommoor@users.noreply.github.com>

* Rename isSentryReported to isReportable

Co-authored-by: tommoor <380914+tommoor@users.noreply.github.com>

* Update Discord plugin errors to use isReportable

Co-authored-by: tommoor <380914+tommoor@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: tommoor <380914+tommoor@users.noreply.github.com>

plugins/discord/server/errors.ts

@@ -5,6 +5,7 @@ export function DiscordGuildError(
 ) {
   return httpErrors(400, message, {
     id: "discord_guild_error",
+    isReportable: false,
   });
 }
 
@@ -13,5 +14,6 @@ export function DiscordGuildRoleError(
 ) {
   return httpErrors(400, message, {
     id: "discord_guild_role_error",
+    isReportable: false,
   });
 }

server/errors.test.ts

@@ -0,0 +1,60 @@
+import * as errors from "./errors";
+
+describe("errors", () => {
+  describe("InternalError", () => {
+    it("should be marked for Sentry reporting", () => {
+      const error = errors.InternalError();
+      expect(error.isReportable).toBe(true);
+    });
+
+    it("should have status 500", () => {
+      const error = errors.InternalError();
+      expect(error.status).toBe(500);
+    });
+  });
+
+  describe("User input errors", () => {
+    const userInputErrors = [
+      { name: "AuthenticationError", fn: errors.AuthenticationError },
+      { name: "InvalidAuthenticationError", fn: errors.InvalidAuthenticationError },
+      { name: "AuthorizationError", fn: errors.AuthorizationError },
+      { name: "CSRFError", fn: errors.CSRFError },
+      { name: "RateLimitExceededError", fn: errors.RateLimitExceededError },
+      { name: "InviteRequiredError", fn: errors.InviteRequiredError },
+      { name: "DomainNotAllowedError", fn: errors.DomainNotAllowedError },
+      { name: "AdminRequiredError", fn: errors.AdminRequiredError },
+      { name: "InvalidRequestError", fn: errors.InvalidRequestError },
+      { name: "PaymentRequiredError", fn: errors.PaymentRequiredError },
+      { name: "NotFoundError", fn: errors.NotFoundError },
+      { name: "ParamRequiredError", fn: errors.ParamRequiredError },
+      { name: "ValidationError", fn: errors.ValidationError },
+      { name: "IncorrectEditionError", fn: errors.IncorrectEditionError },
+      { name: "EditorUpdateError", fn: errors.EditorUpdateError },
+      { name: "FileImportError", fn: errors.FileImportError },
+      { name: "OAuthStateMismatchError", fn: errors.OAuthStateMismatchError },
+      { name: "TeamPendingDeletionError", fn: errors.TeamPendingDeletionError },
+      { name: "EmailAuthenticationRequiredError", fn: errors.EmailAuthenticationRequiredError },
+      { name: "MicrosoftGraphError", fn: errors.MicrosoftGraphError },
+      { name: "TeamDomainRequiredError", fn: errors.TeamDomainRequiredError },
+      { name: "GmailAccountCreationError", fn: errors.GmailAccountCreationError },
+      { name: "OIDCMalformedUserInfoError", fn: errors.OIDCMalformedUserInfoError },
+      { name: "AuthenticationProviderDisabledError", fn: errors.AuthenticationProviderDisabledError },
+      { name: "UnprocessableEntityError", fn: errors.UnprocessableEntityError },
+      { name: "ClientClosedRequestError", fn: errors.ClientClosedRequestError },
+    ];
+
+    userInputErrors.forEach(({ name, fn }) => {
+      it(`${name} should not be marked for Sentry reporting`, () => {
+        const error = fn();
+        expect(error.isReportable).toBe(false);
+      });
+    });
+  });
+
+  describe("UserSuspendedError", () => {
+    it("should not be marked for Sentry reporting", () => {
+      const error = errors.UserSuspendedError({ adminEmail: "test@example.com" });
+      expect(error.isReportable).toBe(false);
+    });
+  });
+});

server/errors.ts

@@ -3,6 +3,7 @@ import httpErrors from "http-errors";
 export function InternalError(message = "Internal error") {
   return httpErrors(500, message, {
     id: "internal_error",
+    isReportable: true,
   });
 }
 
@@ -13,6 +14,7 @@ export function AuthenticationError(
   return httpErrors(401, message, {
     redirectPath,
     id: "authentication_required",
+    isReportable: false,
   });
 }
 
@@ -23,18 +25,21 @@ export function InvalidAuthenticationError(
   return httpErrors(401, message, {
     redirectPath,
     id: "invalid_authentication",
+    isReportable: false,
   });
 }
 
 export function AuthorizationError(message = "Authorization error") {
   return httpErrors(403, message, {
     id: "authorization_error",
+    isReportable: false,
   });
 }
 
 export function CSRFError(message = "Authorization error") {
   return httpErrors(403, message, {
     id: "csrf_error",
+    isReportable: false,
   });
 }
 
@@ -43,6 +48,7 @@ export function RateLimitExceededError(
 ) {
   return httpErrors(429, message, {
     id: "rate_limit_exceeded",
+    isReportable: false,
   });
 }
 
@@ -51,6 +57,7 @@ export function InviteRequiredError(
 ) {
   return httpErrors(403, message, {
     id: "invite_required",
+    isReportable: false,
   });
 }
 
@@ -59,6 +66,7 @@ export function DomainNotAllowedError(
 ) {
   return httpErrors(403, message, {
     id: "domain_not_allowed",
+    isReportable: false,
   });
 }
 
@@ -67,6 +75,7 @@ export function AdminRequiredError(
 ) {
   return httpErrors(403, message, {
     id: "admin_required",
+    isReportable: false,
   });
 }
 
@@ -83,6 +92,7 @@ export function UserSuspendedError({
       errorData: {
         adminEmail,
       },
+      isReportable: false,
     }
   );
 }
@@ -90,30 +100,35 @@ export function UserSuspendedError({
 export function InvalidRequestError(message = "Request invalid") {
   return httpErrors(400, message, {
     id: "invalid_request",
+    isReportable: false,
   });
 }
 
 export function PaymentRequiredError(message = "Payment required") {
   return httpErrors(402, message, {
     id: "payment_required",
+    isReportable: false,
   });
 }
 
 export function NotFoundError(message = "Resource not found") {
   return httpErrors(404, message, {
     id: "not_found",
+    isReportable: false,
   });
 }
 
 export function ParamRequiredError(message = "Required parameter missing") {
   return httpErrors(400, message, {
     id: "param_required",
+    isReportable: false,
   });
 }
 
 export function ValidationError(message = "Validation failed") {
   return httpErrors(400, message, {
     id: "validation_error",
+    isReportable: false,
   });
 }
 
@@ -122,6 +137,7 @@ export function IncorrectEditionError(
 ) {
   return httpErrors(402, message, {
     id: "incorrect_edition",
+    isReportable: false,
   });
 }
 
@@ -130,12 +146,14 @@ export function EditorUpdateError(
 ) {
   return httpErrors(400, message, {
     id: "editor_update_required",
+    isReportable: false,
   });
 }
 
 export function FileImportError(message = "The file could not be imported") {
   return httpErrors(400, message, {
     id: "import_error",
+    isReportable: false,
   });
 }
 
@@ -144,6 +162,7 @@ export function OAuthStateMismatchError(
 ) {
   return httpErrors(400, message, {
     id: "state_mismatch",
+    isReportable: false,
   });
 }
 
@@ -152,6 +171,7 @@ export function TeamPendingDeletionError(
 ) {
   return httpErrors(403, message, {
     id: "pending_deletion",
+    isReportable: false,
   });
 }
 
@@ -162,6 +182,7 @@ export function EmailAuthenticationRequiredError(
   return httpErrors(400, message, {
     redirectPath,
     id: "email_auth_required",
+    isReportable: false,
   });
 }
 
@@ -170,6 +191,7 @@ export function MicrosoftGraphError(
 ) {
   return httpErrors(400, message, {
     id: "graph_error",
+    isReportable: false,
   });
 }
 
@@ -178,6 +200,7 @@ export function TeamDomainRequiredError(
 ) {
   return httpErrors(400, message, {
     id: "domain_required",
+    isReportable: false,
   });
 }
 
@@ -186,6 +209,7 @@ export function GmailAccountCreationError(
 ) {
   return httpErrors(400, message, {
     id: "gmail_account_creation",
+    isReportable: false,
   });
 }
 
@@ -194,6 +218,7 @@ export function OIDCMalformedUserInfoError(
 ) {
   return httpErrors(400, message, {
     id: "malformed_user_info",
+    isReportable: false,
   });
 }
 
@@ -204,13 +229,17 @@ export function AuthenticationProviderDisabledError(
   return httpErrors(400, message, {
     redirectPath,
     id: "authentication_provider_disabled",
+    isReportable: false,
   });
 }
 
 export function UnprocessableEntityError(
   message = "Cannot process the request"
 ) {
-  return httpErrors(422, message, { id: "unprocessable_entity" });
+  return httpErrors(422, message, {
+    id: "unprocessable_entity",
+    isReportable: false,
+  });
 }
 
 export function ClientClosedRequestError(
@@ -218,5 +247,6 @@ export function ClientClosedRequestError(
 ) {
   return httpErrors(499, message, {
     id: "client_closed_request",
+    isReportable: false,
   });
 }

server/onerror.test.ts

@@ -0,0 +1,107 @@
+import type Koa from "koa";
+import { requestErrorHandler } from "@server/logging/sentry";
+import { InternalError, ValidationError, NotFoundError } from "./errors";
+import onerror from "./onerror";
+
+// Mock the requestErrorHandler from Sentry
+jest.mock("@server/logging/sentry", () => ({
+  requestErrorHandler: jest.fn(),
+}));
+
+describe("onerror", () => {
+  let app: Koa;
+  let ctx: any;
+
+  beforeEach(() => {
+    // Create a mock Koa app
+    app = {
+      context: {},
+    } as any;
+
+    // Apply the onerror middleware
+    onerror(app);
+
+    // Create a mock context
+    ctx = {
+      headers: {},
+      headerSent: false,
+      writable: true,
+      accepts: jest.fn(() => "json"),
+      set: jest.fn(),
+      res: {
+        end: jest.fn(),
+      },
+      status: undefined,
+      type: undefined,
+      body: undefined,
+    };
+
+    // Clear mock calls
+    (requestErrorHandler as jest.Mock).mockClear();
+  });
+
+  it("should report InternalError to Sentry", () => {
+    const error = InternalError("Test internal error");
+
+    app.context.onerror.call(ctx, error);
+
+    expect(requestErrorHandler).toHaveBeenCalledWith(error, ctx);
+    expect(ctx.status).toBe(500);
+  });
+
+  it("should not report ValidationError to Sentry", () => {
+    const error = ValidationError("Test validation error");
+
+    app.context.onerror.call(ctx, error);
+
+    expect(requestErrorHandler).not.toHaveBeenCalled();
+    expect(ctx.status).toBe(400);
+  });
+
+  it("should not report NotFoundError to Sentry", () => {
+    const error = NotFoundError("Test not found error");
+
+    app.context.onerror.call(ctx, error);
+
+    expect(requestErrorHandler).not.toHaveBeenCalled();
+    expect(ctx.status).toBe(404);
+  });
+
+  it("should report unknown errors without isReportable property to Sentry", () => {
+    const error = new Error("Unknown error") as any;
+    error.status = 500;
+
+    app.context.onerror.call(ctx, error);
+
+    expect(requestErrorHandler).toHaveBeenCalledWith(error, ctx);
+  });
+
+  it("should report errors with invalid status codes to Sentry", () => {
+    const error = new Error("Invalid status error") as any;
+    error.status = 999;
+
+    app.context.onerror.call(ctx, error);
+
+    expect(requestErrorHandler).toHaveBeenCalledWith(error, ctx);
+  });
+
+  it("should not report errors explicitly marked with isReportable: false", () => {
+    const error = new Error("Custom error") as any;
+    error.status = 500;
+    error.isReportable = false;
+
+    app.context.onerror.call(ctx, error);
+
+    expect(requestErrorHandler).not.toHaveBeenCalled();
+  });
+
+  it("should report errors explicitly marked with isReportable: true", () => {
+    const error = new Error("Custom error") as any;
+    error.status = 400;
+    error.isReportable = true;
+
+    app.context.onerror.call(ctx, error);
+
+    expect(requestErrorHandler).toHaveBeenCalledWith(error, ctx);
+  });
+});

server/onerror.ts

@@ -29,12 +29,17 @@ export default function onerror(app: Koa) {
       }
     }
 
-    // Push only unknown and 500 status errors to sentry
-    if (
-      typeof err.status !== "number" ||
+    // Push only errors explicitly marked for Sentry reporting.
+    // For unknown errors without isReportable property, report them as well
+    // to ensure we don't miss unexpected errors.
+    const shouldReport =
+      err.isReportable === true ||
+      (err.isReportable !== false &&
+        (typeof err.status !== "number" ||
           !http.STATUS_CODES[err.status] ||
-      err.status === 500
-    ) {
+          err.status === 500));
+
+    if (shouldReport) {
       requestErrorHandler(err, this);
 
       if (!(err instanceof InternalError)) {