🛡️ Sentinel: [MEDIUM] Fix stack trace exposure in error boundary#214
🛡️ Sentinel: [MEDIUM] Fix stack trace exposure in error boundary#214ldsgroups225 wants to merge 1 commit intomasterfrom
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughBoth Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/core/src/components/default-catch-boundary.tsx (2)
34-40: Consider omitting the "Stack Trace" label when empty.In production, the mailto body will contain
Stack Trace:\n\nwith no actual content. This is harmless but slightly awkward UX. You could conditionally include this section:♻️ Optional: Clean up mailto body in production
const handleReportError = () => { const subject = encodeURIComponent('Error Report') + const stackSection = hasStack ? `\n\nStack Trace:\n${errorStack}` : '' const body = encodeURIComponent( - `An error occurred in the application:\n\nError: ${errorMessage}\n\nStack Trace:\n${errorStack}\n\nPlease describe what you were doing when this error occurred:`, + `An error occurred in the application:\n\nError: ${errorMessage}${stackSection}\n\nPlease describe what you were doing when this error occurred:`, ) window.location.href = `mailto:support@example.com?subject=${subject}&body=${body}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/core/src/components/default-catch-boundary.tsx` around lines 34 - 40, The mailto body in handleReportError currently always inserts the "Stack Trace" label even when errorStack is empty; change body construction so you conditionally append the stack section only when errorStack is non-empty (e.g., build a stackSection variable that is errorStack ? `\n\nStack Trace:\n${errorStack}` : '' and include that in the template with errorMessage), then pass the resulting string through encodeURIComponent before assigning window.location.href; this keeps the subject/body encoding and only shows the stack trace label when there is content.
55-58: Hardcoded UI strings violate i18n guidelines.Unlike the school version which uses
useTranslations(), this component has hardcoded English strings throughout (e.g., "Something went wrong", "Try Again", "Go Back", "Technical Details", "Report Error"). As per coding guidelines, all UI text inapps/core/src/components/**/*.tsxmust useLL.key()for typesafe-i18n.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/core/src/components/default-catch-boundary.tsx` around lines 55 - 58, The component default-catch-boundary.tsx contains hardcoded English UI strings (e.g., the CardTitle "Something went wrong" and other labels like "Try Again", "Go Back", "Technical Details", "Report Error"); replace all literal strings in CardTitle and related UI elements with typesafe-i18n calls using the LL namespace (e.g., LL.someKey()) — import or obtain LL via the existing i18n hook/provider (same pattern as useTranslations() in the school version), add appropriate keys to the i18n resource, and update renderers (CardTitle and any buttons/labels) to call LL.key() instead of hardcoded text so all UI text in apps/core/src/components uses typesafe-i18n.apps/school/src/components/default-catch-boundary.tsx (1)
51-57: Same refinement opportunity as the core app.The "Report Error" button (lines 175-183) is always visible, but in production the mailto body will include an empty
Stack Trace:\n\nsection. Consider conditionally including it:♻️ Optional: Clean up mailto body in production
const handleReportError = () => { const subject = encodeURIComponent('Error Report - Yeko School') + const stackSection = hasStack ? `\n\nStack Trace:\n${errorStack}` : '' const body = encodeURIComponent( - `An error occurred in the application:\n\nError: ${errorMessage}\n\nStack Trace:\n${errorStack}\n\nPlease describe what you were doing when this error occurred:`, + `An error occurred in the application:\n\nError: ${errorMessage}${stackSection}\n\nPlease describe what you were doing when this error occurred:`, ) window.location.href = `mailto:support@yeko.school?subject=${subject}&body=${body}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/school/src/components/default-catch-boundary.tsx` around lines 51 - 57, The mailto body built in handleReportError always includes a "Stack Trace" section even when errorStack is empty; update handleReportError to conditionally append the stack trace only when errorStack is present (and/or when process.env.NODE_ENV !== 'production' if desired), e.g., build bodyParts starting with the errorMessage and push a "Stack Trace:\n{errorStack}" part only when errorStack is truthy, then encodeURIComponent the joined bodyParts before assigning window.location.href; ensure references to handleReportError, errorMessage and errorStack are used so the change is applied in the same function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/core/src/components/default-catch-boundary.tsx`:
- Around line 34-40: The mailto body in handleReportError currently always
inserts the "Stack Trace" label even when errorStack is empty; change body
construction so you conditionally append the stack section only when errorStack
is non-empty (e.g., build a stackSection variable that is errorStack ?
`\n\nStack Trace:\n${errorStack}` : '' and include that in the template with
errorMessage), then pass the resulting string through encodeURIComponent before
assigning window.location.href; this keeps the subject/body encoding and only
shows the stack trace label when there is content.
- Around line 55-58: The component default-catch-boundary.tsx contains hardcoded
English UI strings (e.g., the CardTitle "Something went wrong" and other labels
like "Try Again", "Go Back", "Technical Details", "Report Error"); replace all
literal strings in CardTitle and related UI elements with typesafe-i18n calls
using the LL namespace (e.g., LL.someKey()) — import or obtain LL via the
existing i18n hook/provider (same pattern as useTranslations() in the school
version), add appropriate keys to the i18n resource, and update renderers
(CardTitle and any buttons/labels) to call LL.key() instead of hardcoded text so
all UI text in apps/core/src/components uses typesafe-i18n.
In `@apps/school/src/components/default-catch-boundary.tsx`:
- Around line 51-57: The mailto body built in handleReportError always includes
a "Stack Trace" section even when errorStack is empty; update handleReportError
to conditionally append the stack trace only when errorStack is present (and/or
when process.env.NODE_ENV !== 'production' if desired), e.g., build bodyParts
starting with the errorMessage and push a "Stack Trace:\n{errorStack}" part only
when errorStack is truthy, then encodeURIComponent the joined bodyParts before
assigning window.location.href; ensure references to handleReportError,
errorMessage and errorStack are used so the change is applied in the same
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29c350d6-a579-43e1-8597-0ba583dd0c34
📒 Files selected for processing (3)
apps/core/src/components/default-catch-boundary.tsxapps/school/src/components/default-catch-boundary.tsxpr_desc.txt
1 participant
🚨 Severity: MEDIUM
💡 Vulnerability: The application was exposing full error stack traces in
DefaultCatchBoundaryto the user interface in production.🎯 Impact: This could potentially leak internal application paths, code structure, or dependencies to end-users or attackers during an unexpected error.
🔧 Fix: Used
import.meta.env.DEVto check if we are in development mode and only populated theerrorStackconstant whenDEVis true.✅ Verification: Verify that the error boundary still catches errors correctly and the UI correctly doesn't render stack trace sections when
NODE_ENV/import.meta.env.DEVresolves to false.PR created automatically by Jules for task 9754855999892847221 started by @ldsgroups225
Summary by CodeRabbit