fix: don't log raw Error objects in REST.ts (leaks secrets to hdb.log) (#1734)

[kriszyp]

Summary

Fixes #1734. server/REST.ts's catch-all and WebSocket handlers passed the raw Error object to the logger. HarperLogger extends Console, so the Error is formatted with util.inspect, which dumps every own-enumerable property after the stack. Anything an app or an HTTP client library stashes on a thrown Error — a credential used for an outbound Authorization header, an axios config/request with headers — lands verbatim in hdb.log at the default log level, retrievable via the ops-API read_log operation.

Fix

Add harperLogger.errorForLog(error):

export function errorForLog(error: any) {
	return typeof error?.stack === 'string' ? error.stack : errorToString(error);
}

error.stack is the class name + message + frames — it does not contain custom own-enumerable properties — so logging the stack preserves debuggability while dropping the leak vector. Stackless/plain thrown values fall back to errorToString's "ClassName: message".

All six REST/WS log sites (HTTP catch-all warn/info/error, WS error handler, WS catch warn/info/error) now route through it. The client-response path (problemDetail) is untouched — it already minimizes to message/code/status/detail, so this is a server-log-only fix, distinct from #1421.

Design note

I log the stack rather than the issue's suggested error.message: stack is strictly safer than the current raw-object behavior yet keeps the frames operators rely on for 500s (message-only would regress debuggability). Message is still present — it's inside the stack — consistent with the already-accepted client-response title. This is a name-agnostic sanitizer, not an allowlist scrub, so it can't miss a "new sensitive property name."

Tests

4 new unit tests in harper_logger.test.js covering errorForLog, including an assertion that a Bearer token stashed on error.authorization/error.config does not appear in the output. tsc/oxlint/prettier clean.

🤖 Generated with Claude Code

[claude]

Reviewed; no blockers found.

[gemini-code-assist]

Code Review

This pull request introduces the errorForLog utility function to safely format errors for logging, preventing sensitive stashed properties (such as credentials) from being leaked. The REST server has been updated to use this function across various error-handling paths, and corresponding unit tests have been added. The review feedback highlights a critical issue where passing null or undefined to errorForLog can trigger a TypeError crash in errorToString, and suggests adding defensive checks and additional test cases to handle these scenarios gracefully.

[kriszyp]

Cross-model review (standard: Gemini leg + Harper-domain adjudication)

No blockers. One significant concern addressed at the root, the rest triaged:

Addressed (2nd commit) — error.stack alone dropped the error.cause chain, which the old raw-object logging surfaced via util.inspect. Since Harper convention leans on new Error(msg, { cause }), that's a real diagnostic regression. errorForLog now returns a lazy util.inspect.custom wrapper that:

• appends each cause's stack (cycle-protected), still excluding own-enumerable props; and
• defers stack materialization until the logger's level gate actually writes — which also resolves Gemini's eager-.stack-on-the-discarded-path perf note in the same change.

Verified end-to-end through the real HarperLogger: a Bearer token on both error.authorization and the cause's config.headers.Authorization stays out of the rendered line, while message + stack + caused by: root-cause stack are all present. New unit tests cover the cause chain, the cycle guard, and the leak assertion (6 total).

Accepted / not changed:

• Plain thrown object with no stack/message loses its fields (throw { status, details } → errorToString → "[object Object]"). Left as-is deliberately — rendering arbitrary object keys is the exact leak surface this fixes; Harper throws real Errors in practice. Minor.
• errorForLog(null) would throw in the fallback — not reachable: every callsite dereferences error.statusCode/error.status first, so throw null already throws upstream of the logger.

Caveat: Codex leg didn't run (standard mode = Gemini leg only); domain pass + Gemini covered the logging/secret-leak surface.

[cb1kenobi]

Scope module for consistency.

Suggested change

	import { inspect } from 'util';
	import { inspect } from 'node:util';

[cb1kenobi]

I wonder if the error == null check should be moved to errorToString()? Could that be beneficial elsewhere?