Fix/error boundaries and security by SweetSophia · Pull Request #31 · MiniMax-AI/OpenRoom

SweetSophia · 2026-04-01T18:43:40Z

Summary

Brief description of the changes.

Changes

Related Issues

Closes #

Checklist

pnpm run lint passes
pnpm build passes
Documentation updated (if applicable)
Self-reviewed the code changes
AI code review passed (auto-triggered on PR)

* refactor: extract ChatPanel into focused modules Decompose the 1472-line ChatPanel god component into four focused files: - toolDefinitions.ts (133 lines): tool defs, system prompt builder, config guard - ChatSubComponents.tsx (178 lines): CharacterAvatar, StageIndicator, ActionsTaken - SettingsModal.tsx (270 lines): LLM + image generation configuration UI - useConversationEngine.ts (341 lines): conversation loop + tool dispatch hook ChatPanel/index.tsx reduced from 1472 → 632 lines (-57%), now a thin shell that wires state to UI. No behavior changes — pure structural refactor. Benefits: - Conversation engine is testable in isolation - Settings modal can be reasoned about independently - Each file has a single clear responsibility - Future PRs can target specific modules without touching the whole * fix: consolidate duplicate ModManager imports in useConversationEngine.ts Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/f4ebc43a-24c2-4853-b135-f234fd25d12b Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * fix: stabilize runConversation identity to prevent listener churn runConversation was recreated on every render, causing the onUserAction subscription effect to unsubscribe/resubscribe on each render. App actions emitted during the cleanup-rebind window could be silently dropped. Since runConversation only reads from refs (no reactive state), wrapping it in useCallback with an empty dependency array makes its identity stable across renders, eliminating the churn. * fix: address code review findings across ChatPanel modules useConversationEngine: - Wrap callbacks in ref to prevent stale closure in stabilized runConversation - Break outer loop after respond_to_user to avoid extra model round-trip - Add console.warn for unparseable tool call arguments - Handle loadMemories rejection with fallback to empty array ChatSubComponents: - Reactivate existing inactive layer instead of skipping it - Use ref for cleanup timeout to prevent stale timeout interference index.tsx: - Move setSessionPath call from render body to useEffect - Reduce reload effect deps to sessionPath only, add cancellation guard SettingsModal: - Sync local state from parent props via useEffect toolDefinitions: - Replace hardcoded emotion examples with generic reference to character keys * fix: address review feedback on types, timeout cleanup, and config guard Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/6cd6bd9b-6a80-474a-b067-8f2ff9f8f32a Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * fix: address Copilot review — tool loop, save snapshot, layer cleanup useConversationEngine: - Remove inner break on respond_to_user so sibling tool calls (e.g. finish_target) still execute before outer loop stops index.tsx: - Guard loadMemories .then/.catch with cancelled flag - Snapshot session/data at debounce schedule time instead of reading refs at fire-time, preventing cross-session data mixing - Remove now-unused sessionPathRef2 ChatSubComponents: - On layer reactivation, cancel pending cleanup timeout and explicitly deactivate all other layers to prevent dual-active state * fix: Prettier import style, stale memory guard, remove redundant ref useConversationEngine: - Break React import to multiline per Prettier config - Guard loadMemories handlers against session change during async gap index.tsx: - Remove unused sessionPathRef_forSet ref — setSessionPath in useEffect alone is sufficient for module-level sync * fix: flush debounced save on cleanup, remove stale dep in handleResetSession index.tsx: - Flush pending saveChatHistory in cleanup instead of discarding, preventing data loss on rapid session switches - Remove modCollection from handleResetSession dependency array (uses refs and functional state updates, no direct read needed) * fix(ChatPanel): useLayoutEffect for setSessionPath; fix debounced save write amplification Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/4ddaf906-91f2-4ac3-ac97-7ce3fd331545 Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * refactor: extract PendingSaveSnapshot type from inline ref type Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/4ddaf906-91f2-4ac3-ac97-7ce3fd331545 Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * fix: ModManager value import, sessionPath out of save deps, redact sensitive args from warn log Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/1954e0a2-8dec-4da9-85f4-92df5e2ddf00 Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * fix: only break loop when respond_to_user is sole tool call Previously the loop broke after respond_to_user even when other tools ran in the same batch. If the model planned to emit follow-up tool calls (e.g. finish_target) in a subsequent round-trip, those were silently skipped. Now the loop only terminates when respond_to_user was the only tool call — batched tool calls get their follow-up round-trip as intended. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>

…state 1. Error Boundaries: Wrap AppWindow and ChatPanel in ErrorBoundary components so crashes don't take down the entire Shell. 2. Path Traversal: Replace regex-based sanitization in session-data and session-reset endpoints with resolve()-based validation that ensures paths stay within SESSIONS_DIR. 3. SSRF/Header Injection: LLM proxy now uses an explicit allowlist for forwarded headers. x-custom- headers are validated to prevent auth injection. Target URLs are validated for protocol. 4. AbortController: Conversation loop accepts AbortSignal. handleSend and processActionQueue cancel previous conversations before starting new ones. 5. Stale State: handleSend reads chatHistory from chatHistoryRef instead of closure, fixing race condition.

SweetSophia · 2026-04-01T18:44:24Z

/review

Copilot

Pull request overview

This PR focuses on improving resilience and safety in the Web UI apps runtime by adding UI error isolation, refactoring ChatPanel into testable/purer units, and tightening dev-server endpoints (session file access + LLM proxy), alongside documentation formatting updates.

Changes:

Added an ErrorBoundary component and wrapped app windows + ChatPanel to prevent a single crash from taking down the Shell.
Refactored ChatPanel into separate modules (conversation engine, tool definitions, settings modal, subcomponents) and added cancellation support plumbing.
Hardened Vite dev-server middleware: session path sanitization and stricter LLM proxy header forwarding / target URL validation behavior.

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
apps/webuiapps/vite.config.ts	Adds session path sanitization and revises LLM proxy header/URL handling.
apps/webuiapps/src/pages/Twitter/twitter_en/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/Twitter/twitter_cn/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/MusicApp/meta/meta_en/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/MusicApp/meta/meta_cn/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/Gomoku/meta/meta_en/guide.md	Markdown table/JSON example formatting.
apps/webuiapps/src/pages/Gomoku/meta/meta_cn/guide.md	Markdown table/JSON example formatting.
apps/webuiapps/src/pages/FreeCell/freecell_en/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/FreeCell/freecell_cn/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/EvidenceVault/evidencevault_en/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/EvidenceVault/evidencevault_cn/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/Email/email_en/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/Email/email_cn/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/Diary/diary_en/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/Diary/diary_cn/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/CyberNews/meta/meta_en/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/CyberNews/meta/meta_cn/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/Chess/chess_en/guide.md	Markdown table/JSON example formatting.
apps/webuiapps/src/pages/Chess/chess_cn/guide.md	Markdown table/JSON example formatting.
apps/webuiapps/src/pages/Album/album_en/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/pages/Album/album_cn/guide.md	Markdown table/line-wrapping formatting.
apps/webuiapps/src/components/Shell/index.tsx	Wrapes AppWindow(s) and ChatPanel with ErrorBoundary.
apps/webuiapps/src/components/ErrorBoundary.tsx	New ErrorBoundary component with retry UI.
apps/webuiapps/src/components/ErrorBoundary.module.scss	Styling for ErrorBoundary fallback UI.
apps/webuiapps/src/components/ChatPanel/useConversationEngine.ts	New extracted conversation loop + tool dispatch.
apps/webuiapps/src/components/ChatPanel/toolDefinitions.ts	New tool defs + system prompt builder (pure).
apps/webuiapps/src/components/ChatPanel/SettingsModal.tsx	New extracted settings modal for LLM + image gen.
apps/webuiapps/src/components/ChatPanel/index.tsx	Refactors ChatPanel to use extracted modules + debounce/save changes.
apps/webuiapps/src/components/ChatPanel/ChatSubComponents.tsx	Extracted UI helpers (avatar, actions taken, stage indicator, renderer).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-01T18:48:18Z

apps/webuiapps/vite.config.ts

+function sanitizeRelativePath(relPath: string, baseDir: string): string | null {
+  // Strip null bytes and normalize
+  const cleaned = relPath.replace(/\0/g, '');
+  // Only allow safe characters: alphanumeric, underscore, hyphen, dot, forward slash
+  const safe = cleaned.replace(/[^a-zA-Z0-9_\-./]/g, '_');
+  // Resolve to absolute and verify it stays within baseDir
+  const resolved = resolve(baseDir, safe);
+  // Normalize both paths for comparison (resolve handles .. and symlinks)
+  const normalizedBase = resolve(baseDir);
+  if (!resolved.startsWith(normalizedBase + sep) && resolved !== normalizedBase) {
+    return null;
+  }
+  // Return the relative portion (stripped of base) for use with join()
+  return resolved.slice(normalizedBase.length + 1) || '';


sanitizeRelativePath() prevents simple ".." traversal, but it can still escape baseDir via symlinks inside the sessions directory (e.g. sessions/link -> /etc). path.resolve() does not resolve symlinks, so the startsWith(normalizedBase + sep) check is insufficient. Consider comparing fs.realpathSync() (or fs.promises.realpath) for both baseDir and the candidate path, or using path.relative(realBase, realCandidate) and rejecting paths that start with ../are absolute.

Copilot · 2026-04-01T18:48:19Z

apps/webuiapps/vite.config.ts

+            // Only forward safe, known headers. Block all others to prevent injection.
+            const allowKeys = new Set([
+              'content-type',
+              'authorization', // LLM API key (Bearer token)
+              'x-api-key', // Anthropic API key
+              'anthropic-version', // Anthropic API version
+            ]);
            for (const [key, val] of Object.entries(req.headers)) {
              if (typeof val !== 'string') continue;
-              if (skipKeys.has(key)) continue;
-              if (key.startsWith('x-custom-')) {
-                headers[key.replace('x-custom-', '')] = val;
-              } else {
+              if (allowKeys.has(key)) {
                headers[key] = val;
+              } else if (key.startsWith('x-custom-')) {
+                // Only forward x-custom- headers that map to safe, non-sensitive names


The new header allowlist in llmProxyPlugin drops headers required by existing clients. For example, Gemini image generation sends x-goog-api-key (apps/webuiapps/src/lib/imageGenClient.ts), but it isn't in allowKeys here, so those requests will fail. Add the needed provider headers to the allowlist (and/or share a single canonical list with the clients).

Copilot · 2026-04-01T18:48:19Z

apps/webuiapps/vite.config.ts

+              if (
+                hostname === 'localhost' ||
+                hostname === '127.0.0.1' ||
+                hostname === '0.0.0.0' ||
+                hostname.startsWith('192.168.') ||
+                hostname.startsWith('10.') ||
+                hostname.startsWith('172.16.') ||
+                hostname === '::1'
+              ) {
+                // Allow for local development (llama.cpp, etc.) but log a warning
+                console.warn('[llm-proxy] Allowing request to internal address:', hostname);


The target URL validation claims to "Block internal/private network addresses to prevent SSRF", but the code only logs a warning and still allows these requests. This leaves the proxy SSRF-capable (and also doesn't cover common targets like 169.254.169.254 or the full 172.16.0.0/12 range). Either hard-block private/loopback/link-local hosts by default (with an explicit dev-only override), or rename/reword the comment/logic to match the intended behavior.

Suggested change

if (

hostname === 'localhost' ||

hostname === '127.0.0.1' ||

hostname === '0.0.0.0' ||

hostname.startsWith('192.168.') ||

hostname.startsWith('10.') ||

hostname.startsWith('172.16.') ||

hostname === '::1'

) {

// Allow for local development (llama.cpp, etc.) but log a warning

console.warn('[llm-proxy] Allowing request to internal address:', hostname);

const isInternalHostname = (host: string): boolean => {

// Loopback and special hostnames

if (

host === 'localhost' ||

host === '127.0.0.1' ||

host === '0.0.0.0' ||

host === '::1'

) {

return true;

}

// IPv4 literals

const match = host.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/);

if (!match) {

return false;

}

const octets = match.slice(1).map(Number);

if (octets.some((o) => Number.isNaN(o) || o < 0 || o > 255)) {

return false;

}

const [a, b] = octets;

// 10.0.0.0/8

if (a === 10) return true;

// 127.0.0.0/8

if (a === 127) return true;

// 192.168.0.0/16

if (a === 192 && b === 168) return true;

// 172.16.0.0/12 (172.16.0.0 – 172.31.255.255)

if (a === 172 && b >= 16 && b <= 31) return true;

// 169.254.0.0/16 (link-local)

if (a === 169 && b === 254) return true;

return false;

};

const isInternal = isInternalHostname(hostname);

if (isInternal) {

// Allow for local development (llama.cpp, etc.) only when explicitly enabled

const allowInternalForDev =

process.env.NODE_ENV === 'development' &&

process.env.VITE_ALLOW_INTERNAL_PROXY === 'true';

if (!allowInternalForDev) {

res.writeHead(400, { 'Content-Type': 'application/json' });

res.end(

JSON.stringify({

error: 'Target URL points to an internal/private network address',

}),

);

return;

}

console.warn(

'[llm-proxy] Allowing request to internal address for development:',

hostname,

);

Copilot · 2026-04-01T18:48:19Z

apps/webuiapps/src/components/ChatPanel/useConversationEngine.ts

+      while (iterations < maxIterations) {
+        if (signal?.aborted) break;
+        iterations++;
+        const response = await chat(currentMessages, tools, cfg);
+
+        if (response.toolCalls.length === 0) {
+          // No tool calls — fallback plain text
+          if (response.content) {
+            callbacksRef.current.addMessage({


runConversation() supports cancellation via AbortSignal, but it doesn't check signal.aborted after the awaited chat() call. If the user triggers a new request while the old fetch is in-flight, the old response can still be processed and update UI/state after cancellation. Add an early if (signal?.aborted) return/break; immediately after await chat(...) (and consider passing the signal down into llmClient.fetch so the request itself is aborted).

Copilot · 2026-04-01T18:48:20Z

apps/webuiapps/src/components/ChatPanel/useConversationEngine.ts

+        // Execute each tool call
+        const hasRespondToUser = response.toolCalls.some(
+          (tc) => tc.function.name === 'respond_to_user',
+        );
+        for (const tc of response.toolCalls) {
+          const result = await executeToolCall(tc, {
+            mm,
+            hasImageGen,
+            pendingToolCallsRef,
+            sessionPathRef,
+            imageGenConfigRef,
+            characterRef,
+            callbacks: callbacksRef.current,
+          });
+          currentMessages = [...currentMessages, result];
+        }


Even with an AbortSignal, tool execution will still run to completion once a response has been received (including file writes / app_action dispatches). To avoid side effects from a cancelled turn, add signal?.aborted checks before executing each tool call and/or inside executeToolCall (and stop app_action/file operations when cancelled).

Copilot · 2026-04-01T18:48:20Z

apps/webuiapps/src/components/ErrorBoundary.tsx

+            {this.props.name ? `${this.props.name} crashed` : 'Something went wrong'}
+          </div>
+          <div className={styles.detail}>{this.state.error?.message}</div>
+          <button className={styles.retryBtn} onClick={this.handleRetry}>


The retry button should explicitly set type="button" to avoid defaulting to submit if this boundary is ever rendered inside a form (which can cause unexpected navigation/submission when clicking Retry).

Suggested change

<button className={styles.retryBtn} onClick={this.handleRetry}>

<button type="button" className={styles.retryBtn} onClick={this.handleRetry}>

SweetSophia and others added 2 commits April 1, 2026 19:44

Copilot AI review requested due to automatic review settings April 1, 2026 18:43

Copilot started reviewing on behalf of SweetSophia April 1, 2026 18:44 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

SweetSophia closed this Apr 1, 2026

SweetSophia deleted the fix/error-boundaries-and-security branch April 1, 2026 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/error boundaries and security#31

Fix/error boundaries and security#31
SweetSophia wants to merge 2 commits intoMiniMax-AI:mainfrom
SweetSophia:fix/error-boundaries-and-security

SweetSophia commented Apr 1, 2026

Uh oh!

SweetSophia commented Apr 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-              if (
-                hostname === 'localhost' ||
-                hostname === '127.0.0.1' ||
-                hostname === '0.0.0.0' ||
-                hostname.startsWith('192.168.') ||
-                hostname.startsWith('10.') ||
-                hostname.startsWith('172.16.') ||
-                hostname === '::1'
-              ) {
-                // Allow for local development (llama.cpp, etc.) but log a warning
-                console.warn('[llm-proxy] Allowing request to internal address:', hostname);
+              const isInternalHostname = (host: string): boolean => {
+                // Loopback and special hostnames
+                if (
+                  host === 'localhost' ||
+                  host === '127.0.0.1' ||
+                  host === '0.0.0.0' ||
+                  host === '::1'
+                ) {
+                  return true;
+                }
+                // IPv4 literals
+                const match = host.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/);
+                if (!match) {
+                  return false;
+                }
+                const octets = match.slice(1).map(Number);
+                if (octets.some((o) => Number.isNaN(o) || o < 0 || o > 255)) {
+                  return false;
+                }
+                const [a, b] = octets;
+                // 10.0.0.0/8
+                if (a === 10) return true;
+                // 127.0.0.0/8
+                if (a === 127) return true;
+                // 192.168.0.0/16
+                if (a === 192 && b === 168) return true;
+                // 172.16.0.0/12 (172.16.0.0 – 172.31.255.255)
+                if (a === 172 && b >= 16 && b <= 31) return true;
+                // 169.254.0.0/16 (link-local)
+                if (a === 169 && b === 254) return true;
+                return false;
+              };
+              const isInternal = isInternalHostname(hostname);
+              if (isInternal) {
+                // Allow for local development (llama.cpp, etc.) only when explicitly enabled
+                const allowInternalForDev =
+                  process.env.NODE_ENV === 'development' &&
+                  process.env.VITE_ALLOW_INTERNAL_PROXY === 'true';
+                if (!allowInternalForDev) {
+                  res.writeHead(400, { 'Content-Type': 'application/json' });
+                  res.end(
+                    JSON.stringify({
+                      error: 'Target URL points to an internal/private network address',
+                    }),
+                  );
+                  return;
+                }
+                console.warn(
+                  '[llm-proxy] Allowing request to internal address for development:',
+                  hostname,
+                );

	<button className={styles.retryBtn} onClick={this.handleRetry}>
	<button type="button" className={styles.retryBtn} onClick={this.handleRetry}>

Conversation

SweetSophia commented Apr 1, 2026

Summary

Changes

Related Issues

Checklist

Uh oh!

SweetSophia commented Apr 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants