Merge branch 'master' into users/languy/redact-sensitive-info-from-telemetry

src/Common/ErrorHandlingUtils.ts

@@ -7,16 +7,27 @@ import { HttpStatusCodes } from "./Constants";
 import { logError } from "./Logger";
 import { sendMessage } from "./MessageHandler";
 
-export const handleError = (error: string | ARMError | Error, area: string, consoleErrorPrefix?: string): void => {
+export interface HandleErrorOptions {
+  /** Optional redacted error to use for telemetry logging instead of the original error */
+  redactedError?: string | ARMError | Error;
+}
+
+export const handleError = (
+  error: string | ARMError | Error,
+  area: string,
+  consoleErrorPrefix?: string,
+  options?: HandleErrorOptions,
+): void => {
   const errorMessage = getErrorMessage(error);
   const errorCode = error instanceof ARMError ? error.code : undefined;
 
-  // logs error to data explorer console
+  // logs error to data explorer console (always shows original, non-redacted message)
   const consoleErrorMessage = consoleErrorPrefix ? `${consoleErrorPrefix}:\n ${errorMessage}` : errorMessage;
   logConsoleError(consoleErrorMessage);
 
-  // logs error to both app insight and kusto
-  logError(errorMessage, area, errorCode);
+  // logs error to both app insight and kusto (use redacted message if provided)
+  const telemetryErrorMessage = options?.redactedError ? getErrorMessage(options.redactedError) : errorMessage;
+  logError(telemetryErrorMessage, area, errorCode);
 
   // checks for errors caused by firewall and sends them to portal to handle
   sendNotificationForError(errorMessage, errorCode);

src/Common/dataAccess/queryDocumentsPage.test.ts

@@ -0,0 +1,171 @@
+import { redactSyntaxErrorMessage } from "./queryDocumentsPage";
+
+/* Typical error to redact looks like this (the message property contains a JSON string with nested structure):
+{
+  "message": "{\"code\":\"BadRequest\",\"message\":\"{\\\"errors\\\":[{\\\"severity\\\":\\\"Error\\\",\\\"location\\\":{\\\"start\\\":0,\\\"end\\\":5},\\\"code\\\":\\\"SC1001\\\",\\\"message\\\":\\\"Syntax error, incorrect syntax near 'Crazy'.\\\"}]}\\r\\nActivityId: d5424e10-51bd-46f7-9aec-7b40bed36f17, Windows/10.0.20348 cosmos-netstandard-sdk/3.18.0\"}"
+}
+*/
+
+// Helper to create the nested error structure that matches what the SDK returns
+const createNestedError = (
+  errors: Array<{ severity?: string; location?: { start: number; end: number }; code: string; message: string }>,
+  activityId: string = "test-activity-id",
+): { message: string } => {
+  const innerErrorsJson = JSON.stringify({ errors });
+  const innerMessage = `${innerErrorsJson}\r\n${activityId}`;
+  const outerJson = JSON.stringify({ code: "BadRequest", message: innerMessage });
+  return { message: outerJson };
+};
+
+// Helper to parse the redacted result
+const parseRedactedResult = (result: { message: string }) => {
+  const outerParsed = JSON.parse(result.message);
+  const [innerErrorsJson, activityIdPart] = outerParsed.message.split("\r\n");
+  const innerErrors = JSON.parse(innerErrorsJson);
+  return { outerParsed, innerErrors, activityIdPart };
+};
+
+describe("redactSyntaxErrorMessage", () => {
+  it("should redact SC1001 error message", () => {
+    const error = createNestedError(
+      [
+        {
+          severity: "Error",
+          location: { start: 0, end: 5 },
+          code: "SC1001",
+          message: "Syntax error, incorrect syntax near 'Crazy'.",
+        },
+      ],
+      "ActivityId: d5424e10-51bd-46f7-9aec-7b40bed36f17",
+    );
+
+    const result = redactSyntaxErrorMessage(error) as { message: string };
+    const { outerParsed, innerErrors, activityIdPart } = parseRedactedResult(result);
+
+    expect(outerParsed.code).toBe("BadRequest");
+    expect(innerErrors.errors[0].message).toBe("__REDACTED__");
+    expect(activityIdPart).toContain("ActivityId: d5424e10-51bd-46f7-9aec-7b40bed36f17");
+  });
+
+  it("should redact SC2001 error message", () => {
+    const error = createNestedError(
+      [
+        {
+          severity: "Error",
+          location: { start: 0, end: 10 },
+          code: "SC2001",
+          message: "Some sensitive syntax error message.",
+        },
+      ],
+      "ActivityId: abc123",
+    );
+
+    const result = redactSyntaxErrorMessage(error) as { message: string };
+    const { outerParsed, innerErrors, activityIdPart } = parseRedactedResult(result);
+
+    expect(outerParsed.code).toBe("BadRequest");
+    expect(innerErrors.errors[0].message).toBe("__REDACTED__");
+    expect(activityIdPart).toContain("ActivityId: abc123");
+  });
+
+  it("should redact multiple errors with SC1001 and SC2001 codes", () => {
+    const error = createNestedError(
+      [
+        { severity: "Error", code: "SC1001", message: "First error" },
+        { severity: "Error", code: "SC2001", message: "Second error" },
+      ],
+      "ActivityId: xyz",
+    );
+
+    const result = redactSyntaxErrorMessage(error) as { message: string };
+    const { innerErrors } = parseRedactedResult(result);
+
+    expect(innerErrors.errors[0].message).toBe("__REDACTED__");
+    expect(innerErrors.errors[1].message).toBe("__REDACTED__");
+  });
+
+  it("should not redact errors with other codes", () => {
+    const error = createNestedError(
+      [{ severity: "Error", code: "SC9999", message: "This should not be redacted." }],
+      "ActivityId: test123",
+    );
+
+    const result = redactSyntaxErrorMessage(error);
+
+    expect(result).toBe(error); // Should return original error unchanged
+  });
+
+  it("should not modify non-BadRequest errors", () => {
+    const innerMessage = JSON.stringify({ errors: [{ code: "SC1001", message: "Should not be redacted" }] });
+    const error = {
+      message: JSON.stringify({ code: "NotFound", message: innerMessage }),
+    };
+
+    const result = redactSyntaxErrorMessage(error);
+
+    expect(result).toBe(error);
+  });
+
+  it("should handle errors without message property", () => {
+    const error = { code: "BadRequest" };
+
+    const result = redactSyntaxErrorMessage(error);
+
+    expect(result).toBe(error);
+  });
+
+  it("should handle non-object errors", () => {
+    const stringError = "Simple string error";
+    const nullError: null = null;
+    const undefinedError: undefined = undefined;
+
+    expect(redactSyntaxErrorMessage(stringError)).toBe(stringError);
+    expect(redactSyntaxErrorMessage(nullError)).toBe(nullError);
+    expect(redactSyntaxErrorMessage(undefinedError)).toBe(undefinedError);
+  });
+
+  it("should handle malformed JSON in message", () => {
+    const error = {
+      message: "not valid json",
+    };
+
+    const result = redactSyntaxErrorMessage(error);
+
+    expect(result).toBe(error);
+  });
+
+  it("should handle message without ActivityId suffix", () => {
+    const innerErrorsJson = JSON.stringify({
+      errors: [{ severity: "Error", code: "SC1001", message: "Syntax error near something." }],
+    });
+    const error = {
+      message: JSON.stringify({ code: "BadRequest", message: innerErrorsJson + "\r\n" }),
+    };
+
+    const result = redactSyntaxErrorMessage(error) as { message: string };
+    const { innerErrors } = parseRedactedResult(result);
+
+    expect(innerErrors.errors[0].message).toBe("__REDACTED__");
+  });
+
+  it("should preserve other error properties", () => {
+    const baseError = createNestedError([{ code: "SC1001", message: "Error" }], "ActivityId: test");
+    const error = {
+      ...baseError,
+      statusCode: 400,
+      additionalInfo: "extra data",
+    };
+
+    const result = redactSyntaxErrorMessage(error) as {
+      message: string;
+      statusCode: number;
+      additionalInfo: string;
+    };
+
+    expect(result.statusCode).toBe(400);
+    expect(result.additionalInfo).toBe("extra data");
+
+    const { innerErrors } = parseRedactedResult(result);
+    expect(innerErrors.errors[0].message).toBe("__REDACTED__");
+  });
+});

src/Common/dataAccess/queryDocumentsPage.ts

@@ -4,6 +4,51 @@ import { getEntityName } from "../DocumentUtility";
 import { handleError } from "../ErrorHandlingUtils";
 import { MinimalQueryIterator, nextPage } from "../IteratorUtilities";
 
+// Redact sensitive information from BadRequest errors with specific codes
+export const redactSyntaxErrorMessage = (error: unknown): unknown => {
+  const codesToRedact = ["SC1001", "SC2001"];
+
+  try {
+    // Handle error objects with a message property
+    if (error && typeof error === "object" && "message" in error) {
+      const errorObj = error as { code?: string; message?: string };
+      if (typeof errorObj.message === "string") {
+        // Parse the inner JSON from the message
+        const innerJson = JSON.parse(errorObj.message);
+        if (innerJson.code === "BadRequest" && typeof innerJson.message === "string") {
+          const [innerErrorsJson, activityIdPart] = innerJson.message.split("\r\n");
+          const innerErrorsObj = JSON.parse(innerErrorsJson);
+          if (Array.isArray(innerErrorsObj.errors)) {
+            let modified = false;
+            innerErrorsObj.errors = innerErrorsObj.errors.map((err: { code?: string; message?: string }) => {
+              if (err.code && codesToRedact.includes(err.code)) {
+                modified = true;
+                return { ...err, message: "__REDACTED__" };
+              }
+              return err;
+            });
+
+            if (modified) {
+              // Reconstruct the message with the redacted content
+              const redactedMessage = JSON.stringify(innerErrorsObj) + `\r\n${activityIdPart}`;
+              const redactedError = {
+                ...error,
+                message: JSON.stringify({ ...innerJson, message: redactedMessage }),
+                body: undefined as unknown, // Clear body to avoid sensitive data
+              };
+              return redactedError;
+            }
+          }
+        }
+      }
+    }
+  } catch {
+    // If parsing fails, return the original error
+  }
+
+  return error;
+};
+
 export const queryDocumentsPage = async (
   resourceName: string,
   documentsIterator: MinimalQueryIterator,
@@ -18,7 +63,12 @@ export const queryDocumentsPage = async (
     logConsoleInfo(`Successfully fetched ${itemCount} ${entityName} for container ${resourceName}`);
     return result;
   } catch (error) {
-    handleError(error, "QueryDocumentsPage", `Failed to query ${entityName} for container ${resourceName}`);
+    // Redact sensitive information for telemetry while showing original in console
+    const redactedError = redactSyntaxErrorMessage(error);
+
+    handleError(error, "QueryDocumentsPage", `Failed to query ${entityName} for container ${resourceName}`, {
+      redactedError: redactedError as Error,
+    });
     throw error;
   } finally {
     clearMessage();

test/sql/scaleAndSettings/changePartitionKey.spec.ts

@@ -1,104 +1,142 @@
-// import { expect, test } from "@playwright/test";
-// import { DataExplorer, getDropdownItemByNameOrPosition, TestAccount } from "../../fx";
-// import { createTestSQLContainer, TestContainerContext } from "../../testData";
+import { expect, test } from "@playwright/test";
+import { DataExplorer, TestAccount } from "../../fx";
+import { createTestSQLContainer, TestContainerContext } from "../../testData";
 
-// test.describe("Change Partition Key", () => {
-//   let context: TestContainerContext = null!;
-//   let explorer: DataExplorer = null!;
-//   const newPartitionKeyPath = "newPartitionKey";
-//   const newContainerId = "testcontainer_1";
+test.describe("Change Partition Key", () => {
+  let context: TestContainerContext = null!;
+  let explorer: DataExplorer = null!;
+  const newPartitionKeyPath = "newPartitionKey";
+  const newContainerId = "testcontainer_1";
+  let previousJobName: string | undefined;
 
-//   test.beforeAll("Create Test Database", async () => {
-//     context = await createTestSQLContainer();
-//   });
+  test.beforeAll("Create Test Database", async () => {
+    context = await createTestSQLContainer();
+  });
 
-//   test.beforeEach("Open container settings", async ({ page }) => {
-//     explorer = await DataExplorer.open(page, TestAccount.SQL);
+  test.beforeEach("Open container settings", async ({ page }) => {
+    explorer = await DataExplorer.open(page, TestAccount.SQL);
 
-//     // Click Scale & Settings and open Partition Key tab
-//     await explorer.openScaleAndSettings(context);
-//     const PartitionKeyTab = explorer.frame.getByTestId("settings-tab-header/PartitionKeyTab");
-//     await expect(PartitionKeyTab).toBeVisible();
-//     await PartitionKeyTab.click();
-//   });
+    // Click Scale & Settings and open Partition Key tab
+    await explorer.openScaleAndSettings(context);
+    const PartitionKeyTab = explorer.frame.getByTestId("settings-tab-header/PartitionKeyTab");
+    await expect(PartitionKeyTab).toBeVisible();
+    await PartitionKeyTab.click();
+  });
 
-//   // Delete database only if not running in CI
-//   if (!process.env.CI) {
-//     test.afterEach("Delete Test Database", async () => {
-//       await context?.dispose();
-//     });
-//   }
+  // Delete database only if not running in CI
+  if (!process.env.CI) {
+    test.afterEach("Delete Test Database", async () => {
+      await context?.dispose();
+    });
+  }
 
-//   test("Change partition key path", async () => {
-//     await expect(explorer.frame.getByText("/partitionKey")).toBeVisible();
-//     await expect(explorer.frame.getByText("Change partition key")).toBeVisible();
-//     await expect(explorer.frame.getByText(/To safeguard the integrity of/)).toBeVisible();
-//     await expect(explorer.frame.getByText(/To change the partition key/)).toBeVisible();
+  test("Change partition key path", async ({ page }) => {
+    await expect(explorer.frame.getByText("/partitionKey")).toBeVisible();
+    await expect(explorer.frame.getByText("Change partition key")).toBeVisible();
+    await expect(explorer.frame.getByText(/To safeguard the integrity of/)).toBeVisible();
+    await expect(explorer.frame.getByText(/To change the partition key/)).toBeVisible();
 
-//     const changePartitionKeyButton = explorer.frame.getByTestId("change-partition-key-button");
-//     expect(changePartitionKeyButton).toBeVisible();
-//     await changePartitionKeyButton.click();
+    const changePartitionKeyButton = explorer.frame.getByTestId("change-partition-key-button");
+    expect(changePartitionKeyButton).toBeVisible();
+    await changePartitionKeyButton.click();
 
-//     // Fill out new partition key form in the panel
-//     const changePkPanel = explorer.frame.getByTestId(`Panel:Change partition key`);
-//     await expect(changePkPanel.getByText(context.database.id)).toBeVisible();
-//     await expect(explorer.frame.getByRole("heading", { name: "Change partition key" })).toBeVisible();
-//     await expect(explorer.frame.getByText(/When changing a container/)).toBeVisible();
+    // Fill out new partition key form in the panel
+    const changePkPanel = explorer.frame.getByTestId(`Panel:Change partition key`);
+    await expect(changePkPanel.getByText(context.database.id)).toBeVisible();
+    await expect(explorer.frame.getByRole("heading", { name: "Change partition key" })).toBeVisible();
+    await expect(explorer.frame.getByText(/When changing a container/)).toBeVisible();
 
-//     // Try to switch to new container
-//     await expect(changePkPanel.getByText("New container")).toBeVisible();
-//     await expect(changePkPanel.getByText("Existing container")).toBeVisible();
-//     await expect(changePkPanel.getByTestId("new-container-id-input")).toBeVisible();
+    // Try to switch to new container
+    await expect(changePkPanel.getByText("New container")).toBeVisible();
+    await expect(changePkPanel.getByText("Existing container")).toBeVisible();
+    await expect(changePkPanel.getByTestId("new-container-id-input")).toBeVisible();
 
-//     changePkPanel.getByTestId("new-container-id-input").fill(newContainerId);
-//     await expect(changePkPanel.getByTestId("new-container-partition-key-input")).toBeVisible();
-//     changePkPanel.getByTestId("new-container-partition-key-input").fill(newPartitionKeyPath);
+    changePkPanel.getByTestId("new-container-id-input").fill(newContainerId);
+    await expect(changePkPanel.getByTestId("new-container-partition-key-input")).toBeVisible();
+    changePkPanel.getByTestId("new-container-partition-key-input").fill(newPartitionKeyPath);
 
-//     await expect(changePkPanel.getByTestId("add-sub-partition-key-button")).toBeVisible();
-//     changePkPanel.getByTestId("add-sub-partition-key-button").click();
-//     await expect(changePkPanel.getByTestId("new-container-sub-partition-key-input-0")).toBeVisible();
-//     await expect(changePkPanel.getByTestId("remove-sub-partition-key-button-0")).toBeVisible();
-//     await expect(changePkPanel.getByTestId("hierarchical-partitioning-info-text")).toBeVisible();
-//     await changePkPanel.getByTestId("remove-sub-partition-key-button-0").click();
+    await expect(changePkPanel.getByTestId("add-sub-partition-key-button")).toBeVisible();
+    changePkPanel.getByTestId("add-sub-partition-key-button").click();
+    await expect(changePkPanel.getByTestId("new-container-sub-partition-key-input-0")).toBeVisible();
+    await expect(changePkPanel.getByTestId("remove-sub-partition-key-button-0")).toBeVisible();
+    await expect(changePkPanel.getByTestId("hierarchical-partitioning-info-text")).toBeVisible();
+    await changePkPanel.getByTestId("remove-sub-partition-key-button-0").click();
 
-//     await changePkPanel.getByTestId("Panel/OkButton").click();
+    await changePkPanel.getByTestId("Panel/OkButton").click();
 
-//     await expect(changePkPanel).not.toBeVisible({ timeout: 5 * 60 * 1000 });
+    let jobName: string | undefined;
+    await page.waitForRequest(
+      (req) => {
+        const requestUrl = req.url();
+        if (requestUrl.includes("/dataTransferJobs") && req.method() === "PUT") {
+          jobName = new URL(requestUrl).pathname.split("/").pop();
+          return true;
+        }
+        return false;
+      },
+      { timeout: 120000 },
+    );
 
-//     // Verify partition key change job
-//     const jobText = explorer.frame.getByText(/Partition key change job/);
-//     await expect(jobText).toBeVisible();
-//     await expect(explorer.frame.locator(".ms-ProgressIndicator-itemName")).toContainText("Portal_testcontainer_1");
+    await expect(changePkPanel).not.toBeVisible({ timeout: 5 * 60 * 1000 });
 
-//     const jobRow = explorer.frame.locator(".ms-ProgressIndicator-itemDescription");
-//     // await expect(jobRow.getByText("Pending")).toBeVisible({ timeout: 30 * 1000 });
-//     await expect(jobRow.getByText("Completed")).toBeVisible({ timeout: 5 * 60 * 1000 });
+    // Verify partition key change job
+    const jobText = explorer.frame.getByText(/Partition key change job/);
+    await expect(jobText).toBeVisible();
+    // await expect(explorer.frame.locator(".ms-ProgressIndicator-itemName")).toContainText("Portal_testcontainer_1");
+    await expect(explorer.frame.locator(".ms-ProgressIndicator-itemName")).toContainText(jobName!);
 
-//     const newContainerNode = await explorer.waitForContainerNode(context.database.id, newContainerId);
-//     expect(newContainerNode).not.toBeNull();
+    const jobRow = explorer.frame.locator(".ms-ProgressIndicator-itemDescription");
+    // await expect(jobRow.getByText("Pending")).toBeVisible({ timeout: 30 * 1000 });
+    await expect(jobRow.getByText("Completed")).toBeVisible({ timeout: 5 * 60 * 1000 });
 
-//     // Now try to switch to existing container
-//     await changePartitionKeyButton.click();
-//     await changePkPanel.getByText("Existing container").click();
-//     await changePkPanel.getByLabel("Use existing container").check();
-//     await changePkPanel.getByText("Choose an existing container").click();
+    const newContainerNode = await explorer.waitForContainerNode(context.database.id, newContainerId);
+    expect(newContainerNode).not.toBeNull();
 
-// const containerDropdownItem = await getDropdownItemByNameOrPosition(
-//   explorer.frame,
-//   { name: newContainerId },
-//   { ariaLabel: "Existing Containers" },
-// );
-// await containerDropdownItem.click();
+    // Now try to switch to existing container
+    // Ensure this job name is different from the previously processed job name
+    previousJobName = jobName;
 
-//     await changePkPanel.getByTestId("Panel/OkButton").click();
-//     await explorer.frame.getByRole("button", { name: "Cancel" }).click();
+    await changePartitionKeyButton.click();
+    await changePkPanel.getByText("Existing container").click();
+    await changePkPanel.getByLabel("Use existing container").check();
+    await changePkPanel.getByText("Choose an existing container").click();
 
-//     // Dismiss overlay if it appears
-//     const overlayFrame = explorer.frame.locator("#webpack-dev-server-client-overlay").first();
-//     if (await overlayFrame.count()) {
-//       await overlayFrame.contentFrame().getByLabel("Dismiss").click();
-//     }
-//     const cancelledJobRow = explorer.frame.getByTestId("Tab:tab0");
-//     await expect(cancelledJobRow.getByText("Cancelled")).toBeVisible({ timeout: 30 * 1000 });
-//   });
-// });
+    const containerDropdownItem = await explorer.getDropdownItemByName(newContainerId, "Existing Containers");
+    await containerDropdownItem.click();
+
+    let secondJobName: string | undefined;
+    await Promise.all([
+      page.waitForRequest(
+        (req) => {
+          const requestUrl = req.url();
+          if (requestUrl.includes("/dataTransferJobs") && req.method() === "PUT") {
+            secondJobName = new URL(requestUrl).pathname.split("/").pop();
+            return true;
+          }
+          return false;
+        },
+        { timeout: 120000 },
+      ),
+      changePkPanel.getByTestId("Panel/OkButton").click(),
+    ]);
+
+    const cancelButton = explorer.frame.getByRole("button", { name: "Cancel" });
+    const isCancelButtonVisible = await cancelButton.isVisible().catch(() => false);
+    if (isCancelButtonVisible) {
+      await cancelButton.click();
+
+      // Dismiss overlay if it appears
+      const overlayFrame = explorer.frame.locator("#webpack-dev-server-client-overlay").first();
+      if (await overlayFrame.count()) {
+        await overlayFrame.contentFrame().getByLabel("Dismiss").click();
+      }
+
+      const cancelledJobRow = explorer.frame.getByTestId("Tab:tab0");
+      await expect(cancelledJobRow.getByText("Cancelled")).toBeVisible({ timeout: 30 * 1000 });
+    } else {
+      const jobRow = explorer.frame.locator(".ms-ProgressIndicator-itemDescription");
+      await expect(jobRow.getByText("Completed")).toBeVisible({ timeout: 5 * 60 * 1000 });
+      expect(secondJobName).not.toBe(previousJobName);
+    }
+  });
+});