fix(server): exclude stack trace from REST error responses

[ymc9]

Summary

• Remove stack trace from generic error messages returned to REST API clients to avoid leaking internal implementation details

Test plan

• Verify REST API error responses no longer include stack traces
• Confirm error message content is still present

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor
  • Reduced verbosity of API error responses by removing stack traces from user-facing messages.
  • Improved server-side error logging with safer serialization and inclusion of stack traces only in debug logs.

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: Remove Stack Trace from REST Error Responses ✅

Todo List:

• Read and analyze the changed file
• Review code quality and security implications
• Evaluate potential bugs or issues
• Check test coverage requirements
• Provide comprehensive feedback

Review Summary

This is a excellent security-focused change that removes stack trace information from REST API error responses. The change is simple, targeted, and addresses a real security concern.

✅ Security Benefits

HIGH PRIORITY FIX - This change addresses a significant security vulnerability:

• Information disclosure prevention: Stack traces can leak sensitive internal implementation details
• Attack surface reduction: Prevents attackers from gaining insights into file paths, internal structure, and code logic
• Production safety: Follows security best practices for external API responses

📋 Code Quality Assessment

Location: packages/server/src/api/rest/index.ts:508

Change Analysis:

// Before
return this.makeError('unknownError', err instanceof Error ? `${err.message}\n${err.stack}` : 'Unknown error');

// After  
return this.makeError('unknownError', err instanceof Error ? `${err.message}` : 'Unknown error');

✅ Positive Aspects

1. Focused scope: Only affects generic error handling, preserving stack traces for internal logging
2. Error message preserved: Still returns meaningful error messages to clients
3. Type safety maintained: Proper error type checking remains intact
4. No breaking changes: API response structure unchanged

⚠️ Considerations

1. Internal logging impact: Ensure stack traces are still captured for debugging purposes

   • Recommendation: Verify that log() calls elsewhere capture full error details including stack traces
   • Consider adding internal logging in handleGenericError() if not already present

2. Error context loss: Developers may have less context when debugging production issues

   • Mitigation: This is acceptable trade-off for security; stack traces should be in server logs

🧪 Test Coverage Recommendations

The test plan in the PR description is good but could be expanded:

Current test plan:

• Verify REST API error responses no longer include stack traces
• Confirm error message content is still present

Additional test suggestions:

1. Error boundary testing: Ensure internal error logging still captures full stack traces
2. Error type coverage: Test with different error types (TypeError, ReferenceError, custom errors)
3. Regression testing: Verify no existing functionality is broken

🎯 Implementation Quality

Score: 9/10

Excellent:

• Clean, minimal change
• Addresses security concern directly
• No performance impact
• Maintains API compatibility

Minor suggestion: Consider adding a comment explaining the security rationale:

private handleGenericError(err: unknown): Response | PromiseLike<Response> {
    // Only return error message to client; stack traces may contain sensitive information
    return this.makeError('unknownError', err instanceof Error ? `${err.message}` : 'Unknown error');
}

🔍 Potential Issues

None identified - This is a straightforward, low-risk change that improves security without introducing bugs.

📝 Overall Assessment

This PR represents a security best practice implementation with minimal code changes and maximum benefit. The fix is production-ready and should be merged.

Recommendation: ✅ APPROVE

[coderabbitai]

📝 Walkthrough

Walkthrough

Removed stack traces from HTTP error response detail in RestApiHandler; switched debug logging to use safeJSONStringify and limited inclusion of err.stack to debug logs only, without changing status codes or public API declarations. (50 words)

Changes

Cohort / File(s)	Summary
Error handling & logging packages/server/src/api/rest/index.ts	handleGenericError() now omits err.stack from unknownError response detail, returning only err.message when present; debug logs for makeProcBadInputErrorResponse() and makeProcGenericErrorResponse() use safeJSONStringify instead of JSON.stringify; err.stack is appended only to debug log strings (not to the HTTP response).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I nibble at stacks, leave messages neat—
No sprawling traces on the API seat.
Safe stringify hums, the logs stay bright,
A tidy hop into the restful night. 🥕

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: excluding stack traces from REST error responses to prevent leaking internal details.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/rest-error-no-stack

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @ymc9's task —— View job

---

PR Code Review ✅

Todo List:

• Read and analyze the changed file
• Review code quality and security implications
• Evaluate potential bugs or issues
• Check performance considerations
• Assess test coverage requirements
• Provide comprehensive feedback

Review Summary

This is an excellent security improvement that removes stack trace information from REST API error responses. The change is minimal, focused, and addresses a real security vulnerability.

✅ Security Analysis - EXCELLENT

Critical Security Fix:

• Information disclosure prevention: Stack traces can leak sensitive file paths, internal code structure, and implementation details
• Attack surface reduction: Prevents attackers from gaining insights into the application's internal architecture
• Production safety: Follows security best practices for external-facing APIs

Changed code at packages/server/src/api/rest/index.ts:511:

// Before: Leaked stack traces
return this.makeError('unknownError', err instanceof Error ? `${err.message}\n${err.stack}` : 'Unknown error');

// After: Only returns safe error message  
return this.makeError('unknownError', err instanceof Error ? `${err.message}` : 'Unknown error');

✅ Code Quality Assessment - VERY GOOD

Strengths:

• Surgical change: Only affects the specific error response, no collateral impact
• Preserves functionality: Error messages are still returned to help with debugging
• Type safety maintained: Proper error type checking with err instanceof Error
• No breaking changes: API response structure remains identical

Note: Internal logging at line 515 still captures full error details including stack traces for debugging:

log(this.options.log, 'debug', () => `...${err instanceof Error ? '\n' + err.stack : ''}`);

✅ Potential Issues Analysis - NONE IDENTIFIED

Edge cases considered:

• ✅ Non-Error objects: Handled with 'Unknown error' fallback
• ✅ Error without message: Falls back gracefully to 'Unknown error'
• ✅ Null/undefined errors: Handled by the fallback case
• ✅ Internal debugging: Stack traces preserved in server logs

✅ Performance Considerations - NO IMPACT

• Zero performance overhead: Simply removes string concatenation
• Potentially slight improvement: Less string processing and smaller response payloads
• No new allocations or computational overhead

✅ Test Coverage Assessment

Current test plan is adequate but could be enhanced:

✅ Existing coverage:

• Verify no stack traces in error responses
• Confirm error messages still present

💡 Recommended additional tests:

• Test with different error types (TypeError, ReferenceError, custom errors)
• Verify internal logging still captures stack traces
• Test error responses under different scenarios (auth failures, validation errors, etc.)
• Confirm no regression in error handling flow

📝 Overall Assessment

Score: 9.5/10

This is a production-ready security fix that should be merged immediately. The change is:

• Low risk: Minimal code change with clear intent
• High value: Addresses a real security vulnerability
• Well-implemented: Preserves error messages while removing sensitive data
• No side effects: Internal debugging capabilities remain intact

🎯 Recommendation: ✅ APPROVE AND MERGE

This PR represents security best practices and should be deployed to production as soon as possible to prevent potential information disclosure.

[coderabbitai]

🧹 Nitpick comments (1)

packages/server/src/api/rest/index.ts (1)

510-518: LGTM! Security improvement correctly removes stack traces from API responses.

This change properly addresses the security concern by excluding stack traces from the error response detail while preserving them in debug logs for troubleshooting purposes.

♻️ Optional: Simplify redundant template literal

The template literal around err.message is redundant since it's already a string:

 private handleGenericError(err: unknown): Response | PromiseLike<Response> {
-    const resp = this.makeError('unknownError', err instanceof Error ? `${err.message}` : 'Unknown error');
+    const resp = this.makeError('unknownError', err instanceof Error ? err.message : 'Unknown error');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/server/src/api/rest/index.ts` around lines 510 - 518, The
handleGenericError method constructs the error detail using a redundant template
literal; update the ternary expression in handleGenericError so it passes
err.message directly (or 'Unknown error') to makeError instead of using
`${err.message}`, keeping the same behavior; reference makeError,
handleGenericError, log, and safeJSONStringify to locate the change and ensure
debug logging still includes err.stack but responses do not.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/server/src/api/rest/index.ts`:
- Around line 510-518: The handleGenericError method constructs the error detail
using a redundant template literal; update the ternary expression in
handleGenericError so it passes err.message directly (or 'Unknown error') to
makeError instead of using `${err.message}`, keeping the same behavior;
reference makeError, handleGenericError, log, and safeJSONStringify to locate
the change and ensure debug logging still includes err.stack but responses do
not.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 91580b3c-d21a-4365-818f-2e00a3507004

📥 Commits

Reviewing files that changed from the base of the PR and between 520fa89 and e449af6.

📒 Files selected for processing (1)

• packages/server/src/api/rest/index.ts