security: sanitize error messages in API routes (CodeQL js/stack-trace-exposure) - #2209
Conversation
* "Claude PR Assistant workflow" * "Claude Code Review workflow"
Bumps [mermaid](https://github.com/mermaid-js/mermaid) from 11.14.0 to 11.15.0. - [Release notes](https://github.com/mermaid-js/mermaid/releases) - [Commits](https://github.com/mermaid-js/mermaid/compare/mermaid@11.14.0...mermaid@11.15.0) --- updated-dependencies: - dependency-name: mermaid dependency-version: 11.15.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…e exposure Replace direct error.message forwarding in HTTP responses with generic messages across OAuth and model-combo-mappings routes (CodeQL js/stack-trace-exposure). Add CodeQL suppression for intentional SHA-256 token hashing in sync/tokens.ts. Affected routes: kiro/social-authorize, kiro/social-exchange, kiro/import, kiro/auto-import, cursor/import, cursor/auto-import, [provider]/[action], model-combo-mappings (list/create/get/update/delete).
There was a problem hiding this comment.
Code Review
This pull request improves error handling and observability across several API routes by replacing detailed client-side error messages with generic ones and upgrading server-side logging from console.log to console.error. It also includes security scanner suppression comments for SHA-256 usage. Feedback includes a suggestion to log a swallowed error in the Cursor auto-import route and a recommendation to use the modern codeql prefix for suppression comments instead of the legacy lgtm format.
| } catch (error) { | ||
| db?.close(); | ||
| return { found: false, error: `Failed to read database: ${(error as any).message}` }; | ||
| return { found: false, error: "Failed to read database" }; |
There was a problem hiding this comment.
The error is being swallowed here without logging. Since this PR aims to improve observability by upgrading console.log to console.error while sanitizing client-side messages, the internal error should be logged server-side for debugging purposes.
console.error("Failed to read Cursor IDE database:", error);
return { found: false, error: "Failed to read database" };| // CodeQL: Intentionally SHA-256, NOT password hashing. Sync tokens are | ||
| // high-entropy random values (osync_ + 32 random bytes) — not user passwords. | ||
| // lgtm[js/insufficient-password-hash] | ||
| return createHash("sha256").update(rawToken).digest("hex"); // nosemgrep: insufficient-password-hash |
There was a problem hiding this comment.
Use the modern codeql[...] prefix for suppression comments instead of the legacy lgtm[...] format. While GitHub currently supports both for backward compatibility, codeql is the standard for GitHub Advanced Security and explicitly matches the tool mentioned in the PR title.
| // CodeQL: Intentionally SHA-256, NOT password hashing. Sync tokens are | |
| // high-entropy random values (osync_ + 32 random bytes) — not user passwords. | |
| // lgtm[js/insufficient-password-hash] | |
| return createHash("sha256").update(rawToken).digest("hex"); // nosemgrep: insufficient-password-hash | |
| // CodeQL: Intentionally SHA-256, NOT password hashing. Sync tokens are | |
| // high-entropy random values (osync_ + 32 random bytes) — not user passwords. | |
| // codeql[js/insufficient-password-hash] | |
| return createHash("sha256").update(rawToken).digest("hex"); // nosemgrep: insufficient-password-hash |
- Add tests/unit/error-message-sanitization.test.ts: covers model-combo-mappings routes (success, 400, 404 paths) and sync token hashing (satisfies PR Test Policy) - Log swallowed error in cursor/auto-import tryIdeAuth() catch block (Gemini #1) - Use codeql[...] prefix instead of legacy lgtm[...] in sync/tokens.ts (Gemini #2)
CI Coverage Report
Coverage artifact was not available for this run. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b57068588f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| id: claude-review | ||
| uses: anthropics/claude-code-action@v1 | ||
| with: | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} |
There was a problem hiding this comment.
Guard Claude PR review when auth secret is unavailable
This workflow runs on every pull_request, but it always passes secrets.CLAUDE_CODE_OAUTH_TOKEN to anthropics/claude-code-action without checking whether the secret exists. On forked or Dependabot PRs, GitHub does not provide repository secrets, so this input becomes empty and the Claude action fails authentication, creating a failing check on those PRs. Add an if guard (or skip logic) for actors/events where secrets are unavailable so external PRs don't fail solely because the token is inaccessible.
Useful? React with 👍 / 👎.
| (github.event_name == 'issue_comment' && contains(github.event.comment.body, '@claude')) || | ||
| (github.event_name == 'pull_request_review_comment' && contains(github.event.comment.body, '@claude')) || | ||
| (github.event_name == 'pull_request_review' && contains(github.event.review.body, '@claude')) || | ||
| (github.event_name == 'issues' && (contains(github.event.issue.body, '@claude') || contains(github.event.issue.title, '@claude'))) |
There was a problem hiding this comment.
Restrict Claude mention triggers to trusted users
The job condition only checks whether a comment/body contains @claude, so any public user can trigger a full workflow run by posting that string. Even if the action later rejects non-writers, the runner and action startup still execute, which enables easy workflow-spam and unnecessary CI spend. Add an author-permission gate (for example, require collaborator/write association) in the if expression before starting the job.
Useful? React with 👍 / 👎.
|
✅ Merged into One minor enhancement was applied during merge conflict resolution: the kiro Generated by Claude Code |
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: #2135 (1proxy settings), #2227 (antigravity project ID), #2238 (Z.AI Search), #2240 (CLI Suite) - fix: #2217, #2218, #2219, #2221, #2222, #2223, #2224, #2231, #2233, #2236, #2242, #2243 - security: #2209 (stack trace exposure) - chore: #2228, #2234 Total contributors updated from 50+ to 55+.
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: diegosouzapw#2135 (1proxy settings), diegosouzapw#2227 (antigravity project ID), diegosouzapw#2238 (Z.AI Search), diegosouzapw#2240 (CLI Suite) - fix: diegosouzapw#2217, diegosouzapw#2218, diegosouzapw#2219, diegosouzapw#2221, diegosouzapw#2222, diegosouzapw#2223, diegosouzapw#2224, diegosouzapw#2231, diegosouzapw#2233, diegosouzapw#2236, diegosouzapw#2242, diegosouzapw#2243 - security: diegosouzapw#2209 (stack trace exposure) - chore: diegosouzapw#2228, diegosouzapw#2234 Total contributors updated from 50+ to 55+.
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: diegosouzapw#2135 (1proxy settings), diegosouzapw#2227 (antigravity project ID), diegosouzapw#2238 (Z.AI Search), diegosouzapw#2240 (CLI Suite) - fix: diegosouzapw#2217, diegosouzapw#2218, diegosouzapw#2219, diegosouzapw#2221, diegosouzapw#2222, diegosouzapw#2223, diegosouzapw#2224, diegosouzapw#2231, diegosouzapw#2233, diegosouzapw#2236, diegosouzapw#2242, diegosouzapw#2243 - security: diegosouzapw#2209 (stack trace exposure) - chore: diegosouzapw#2228, diegosouzapw#2234 Total contributors updated from 50+ to 55+.
Summary
error.messageforwarding in HTTP 500 responses with generic"Internal server error"messages across 9 API routes, fixing CodeQLjs/stack-trace-exposurealertslgtm[js/insufficient-password-hash]) for intentional SHA-256 usage insrc/lib/sync/tokens.ts(hashing high-entropy random tokens, not passwords)console.log→console.errorfor error logging in affected routesAffected files
src/app/api/oauth/kiro/social-authorize/route.tserror.messagein 500 responsesrc/app/api/oauth/kiro/social-exchange/route.tserror.messagein 500 responsesrc/app/api/oauth/kiro/import/route.tserror.messagein 500 responsesrc/app/api/oauth/kiro/auto-import/route.tserror.messagein 500 responsesrc/app/api/oauth/cursor/import/route.tserror.messagein 500 responsesrc/app/api/oauth/cursor/auto-import/route.tserror.messagein 500 response (2 locations)src/app/api/oauth/[provider]/[action]/route.tserror.messagein 500 response (4 locations)src/app/api/model-combo-mappings/route.tserror.messagein 500 responsesrc/app/api/model-combo-mappings/[id]/route.tserror.messagein 500 response (3 locations)src/lib/sync/tokens.tsjs/insufficient-password-hashsuppressionTest plan
Generated by Claude Code