[core] Make `request` and `response` properties on RestError non-enumerable (#30518)

### Packages impacted by this PR

- `@azure/core-rest-pipeline`
- `@typespec/ts-http-runtime`

### Issues associated with this PR

- See e.g. https://github.com/Azure/azure-sdk-for-js/issues/30247
- Also https://github.com/Azure/azure-sdk-for-js/issues/29910
- And #29630

### Describe the problem that is addressed by this PR

Taken from my last PR #30483:

> `PipelineRequest` and `PipelineResponse` objects may contain secrets
in the request URL or headers. This is problematic since `RestError`
objects are often logged. While we override `util.inspect.custom` to
sanitize log output in Node, this does not work in all situations and
environments. For example:
> - `console.log` in browser does not respect `util.inspect.custom`,
meaning secrets could be logged to the browser console. Other non-Node
environments also may not respect this Node-specific functionality.
> - JSON serialization of `RestError` objects. Calling `JSON.stringify`
on the `RestError` currently results in an object that contains
unsanitized secrets. We have encountered scenarios where the JSON
serialization is logged, for example with vitest.

This PR fixes this issue by making the `request` and `response`
properties **non-enumerable**. This means they are ignored by
JSON.stringify and `Object.entries`/`Object.keys`, but the properties
are still directly accessible. This should prevent any potential
sensitive information being accidentally logged in the majority of
scenarios. I think this is a better balance of not breaking folks versus
improving security than the approach in #30483.

sdk/core/core-rest-pipeline/CHANGELOG.md

@@ -10,6 +10,8 @@
 
 ### Other Changes
 
+- The `request` and `response` properties on `RestError` are now non-enumerable.
+
 ## 1.16.2 (2024-07-10)
 
 ### Bugs Fixed

sdk/core/core-rest-pipeline/src/restError.ts

@@ -56,10 +56,12 @@ export class RestError extends Error {
   public statusCode?: number;
   /**
    * The request that was made.
+   * This property is non-enumerable.
    */
   public request?: PipelineRequest;
   /**
    * The response received (if any.)
+   * This property is non-enumerable.
    */
   public response?: PipelineResponse;
   /**
@@ -72,8 +74,13 @@ export class RestError extends Error {
     this.name = "RestError";
     this.code = options.code;
     this.statusCode = options.statusCode;
-    this.request = options.request;
-    this.response = options.response;
+
+    // The request and response may contain sensitive information in the headers or body.
+    // To help prevent this sensitive information being accidentally logged, the request and response
+    // properties are marked as non-enumerable here. This prevents them showing up in the output of
+    // JSON.stringify and console.log.
+    Object.defineProperty(this, "request", { value: options.request, enumerable: false });
+    Object.defineProperty(this, "response", { value: options.response, enumerable: false });
 
     Object.setPrototypeOf(this, RestError.prototype);
   }
@@ -82,7 +89,13 @@ export class RestError extends Error {
    * Logging method for util.inspect in Node
    */
   [custom](): string {
-    return `RestError: ${this.message} \n ${errorSanitizer.sanitize(this)}`;
+    // Extract non-enumerable properties and add them back. This is OK since in this output the request and
+    // response get sanitized.
+    return `RestError: ${this.message} \n ${errorSanitizer.sanitize({
+      ...this,
+      request: this.request,
+      response: this.response,
+    })}`;
   }
 }
 

sdk/core/core-rest-pipeline/test/restError.spec.ts

@@ -0,0 +1,35 @@
+import { describe, it, assert } from "vitest";
+import { PipelineRequest, PipelineResponse } from "../src/interfaces.js";
+import { createHttpHeaders } from "../src/httpHeaders.js";
+import { RestError } from "../src/restError.js";
+
+describe("RestError", function () {
+  const request: PipelineRequest = {
+    url: "http://example.com/",
+    headers: createHttpHeaders(),
+  } as PipelineRequest;
+  const response: PipelineResponse = {
+    request,
+    status: 500,
+    headers: createHttpHeaders(),
+  };
+
+  it("Request and response properties are accessible", function () {
+    const error = new RestError("error!", { request, response });
+    assert.strictEqual(error.request, request);
+    assert.strictEqual(error.response, response);
+  });
+
+  it("Request and response properties are non-enumerable", function () {
+    const error = new RestError("error!", { request, response });
+    const properties = Object.keys(error);
+    assert.notInclude(properties, "request");
+    assert.notInclude(properties, "response");
+  });
+
+  it("Request and response properties do not appear in JSON serialization", function () {
+    const error = new RestError("error!", { request, response });
+    const json = JSON.stringify(error);
+    assert.equal(json, `{"name":"RestError"}`);
+  });
+});

sdk/core/ts-http-runtime/src/restError.ts

@@ -56,10 +56,12 @@ export class RestError extends Error {
   public statusCode?: number;
   /**
    * The request that was made.
+   * This property is non-enumerable.
    */
   public request?: PipelineRequest;
   /**
    * The response received (if any.)
+   * This property is non-enumerable.
    */
   public response?: PipelineResponse;
   /**
@@ -72,8 +74,13 @@ export class RestError extends Error {
     this.name = "RestError";
     this.code = options.code;
     this.statusCode = options.statusCode;
-    this.request = options.request;
-    this.response = options.response;
+
+    // The request and response may contain sensitive information in the headers or body.
+    // To help prevent this sensitive information being accidentally logged, the request and response
+    // properties are marked as non-enumerable here. This prevents them showing up in the output of
+    // JSON.stringify and console.log.
+    Object.defineProperty(this, "request", { value: options.request, enumerable: false });
+    Object.defineProperty(this, "response", { value: options.response, enumerable: false });
 
     Object.setPrototypeOf(this, RestError.prototype);
   }
@@ -82,7 +89,13 @@ export class RestError extends Error {
    * Logging method for util.inspect in Node
    */
   [custom](): string {
-    return `RestError: ${this.message} \n ${errorSanitizer.sanitize(this)}`;
+    // Extract non-enumerable properties and add them back. This is OK since in this output the request and
+    // response get sanitized.
+    return `RestError: ${this.message} \n ${errorSanitizer.sanitize({
+      ...this,
+      request: this.request,
+      response: this.response,
+    })}`;
   }
 }
 

sdk/core/ts-http-runtime/test/restError.spec.ts

@@ -0,0 +1,38 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+import { describe, it, assert } from "vitest";
+import { PipelineRequest, PipelineResponse } from "../src/interfaces.js";
+import { createHttpHeaders } from "../src/httpHeaders.js";
+import { RestError } from "../src/restError.js";
+
+describe("RestError", function () {
+  const request: PipelineRequest = {
+    url: "http://example.com/",
+    headers: createHttpHeaders(),
+  } as PipelineRequest;
+  const response: PipelineResponse = {
+    request,
+    status: 500,
+    headers: createHttpHeaders(),
+  };
+
+  it("Request and response properties are accessible", function () {
+    const error = new RestError("error!", { request, response });
+    assert.strictEqual(error.request, request);
+    assert.strictEqual(error.response, response);
+  });
+
+  it("Request and response properties are non-enumerable", function () {
+    const error = new RestError("error!", { request, response });
+    const properties = Object.keys(error);
+    assert.notInclude(properties, "request");
+    assert.notInclude(properties, "response");
+  });
+
+  it("Request and response properties do not appear in JSON serialization", function () {
+    const error = new RestError("error!", { request, response });
+    const json = JSON.stringify(error);
+    assert.equal(json, `{"name":"RestError"}`);
+  });
+});