security(CORE-1123): block non-http(s) main-frame navigation in server webview

[jeanfbrito]

Summary

• CORE-1123 (2022 pentest finding: Missing File Handler Protections). The server webview's will-navigate handler only special-cased t.co/twitter.com redirects — a main-frame navigation to a non-web scheme (e.g. file://) inside the webview had no explicit guard.
• Deep links, downloads, and popup/new-window navigation already had proper validation (parseDeepLink, electron-dl Save-As flow, setWindowOpenHandler/isProtocolAllowed) — untouched, out of scope.
• Drag-and-drop file validation and download file-type filtering were evaluated and intentionally NOT added: the guest webview runs with nodeIntegration=false/contextIsolation=true/webSecurity=true, so these vectors don't apply to this app's threat model — adding them would be defending against attacks that can't reach the filesystem or Node from here.
• Fix: will-navigate now preventDefault()s any main-frame navigation whose protocol isn't http:/https:, before the existing t.co/twitter external-open logic runs. Reuses the existing new URL() parse (no duplicate parsing).

Test plan

• npx tsc --noEmit clean
• yarn eslint clean
• New spec (src/ui/main/serverView/index.spec.ts, 6 cases): http allowed, https allowed, file:// denied, unknown scheme denied, t.co external-open preserved (allowed + denied-by-isProtocolAllowed paths) — all passing

Summary by CodeRabbit

• Bug Fixes
  • Improved webview link handling so only safe http and https navigations proceed.
  • Blocked unsupported protocols and custom schemes from opening inside the embedded view.
  • Twitter preview links now open externally when allowed, instead of navigating within the app.
  • Added test coverage for allowed and blocked navigation paths to prevent regressions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0626a166-4bbb-471b-9d79-0e979b0f99a2

📥 Commits

Reviewing files that changed from the base of the PR and between a459f10 and ce081b5.

📒 Files selected for processing (2)

• src/ui/main/serverView/index.spec.ts
• src/ui/main/serverView/index.ts

📜 Recent review details

⏰ Context from checks skipped due to timeout. (2)

• GitHub Check: check (windows-latest)
• GitHub Check: check (macos-latest)

🧰 Additional context used

📓 Path-based instructions (5)

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example: const uid = process.getuid?.() ?? 1000;

Files:

• src/ui/main/serverView/index.ts
• src/ui/main/serverView/index.spec.ts

**/*.{tsx,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from @rocket.chat/fuselage and check Theme.d.ts for valid color tokens
Use React functional components with hooks
Use PascalCase for component file names

Files:

• src/ui/main/serverView/index.ts
• src/ui/main/serverView/index.spec.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and .d.ts files in node_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources

Files:

• src/ui/main/serverView/index.ts
• src/ui/main/serverView/index.spec.ts

**/*.spec.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Use *.spec.ts file naming for Renderer process tests

Files:

• src/ui/main/serverView/index.spec.ts

**/*.{spec.ts,main.spec.ts}

📄 CodeRabbit inference engine (CLAUDE.md)

Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks: process.getuid(), process.getgid(), process.geteuid(), process.getegid()

Files:

• src/ui/main/serverView/index.spec.ts

🔇 Additional comments (2)

src/ui/main/serverView/index.spec.ts (1)

1-156: LGTM!

src/ui/main/serverView/index.ts (1)

487-496: 🩺 Stability & Availability

No change needed — will-navigate receives an absolute URL string here, so new URL(redirectUrl) is safe.

			> Likely an incorrect or invalid review comment.

---

Walkthrough

The will-navigate handler in serverView is updated to parse the redirect URL's protocol first, immediately blocking non-HTTP(S) navigation without checking hostname or protocol allowance, then applying the existing t.co/twitter.com blocklist only for http/https redirects. A new Jest test suite validates this behavior.

Changes

Will-navigate Protocol Guard

Layer / File(s)	Summary
Protocol-based navigation guard logic src/ui/main/serverView/index.ts	Parses redirectUrl protocol once; blocks non-http(s) navigation immediately; applies t.co/twitter.com hostname blocklist and openExternal flow only for http/https redirects.
Test suite for will-navigate guard src/ui/main/serverView/index.spec.ts	Adds mocking setup and tests verifying http/https pass-through, file:// and unknown-scheme blocking, and conditional t.co external-open behavior based on isProtocolAllowed.

Estimated code review effort: 2 (Simple) | ~12 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GuestWebContents
  participant WillNavigateHandler
  participant IsProtocolAllowed
  participant ExternalBrowser

  GuestWebContents->>WillNavigateHandler: will-navigate(redirectUrl)
  WillNavigateHandler->>WillNavigateHandler: parse redirectUrl protocol
  alt non-http(s) protocol
    WillNavigateHandler->>GuestWebContents: preventDefault()
  else http/https protocol
    alt hostname in t.co/twitter.com
      WillNavigateHandler->>GuestWebContents: preventDefault()
      WillNavigateHandler->>IsProtocolAllowed: isProtocolAllowed(redirectUrl)
      IsProtocolAllowed-->>WillNavigateHandler: allowed true/false
      opt allowed
        WillNavigateHandler->>ExternalBrowser: openExternal(redirectUrl)
      end
    end
  end

Loading

Suggested labels: type: bug

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main security change: blocking non-HTTP(S) main-frame navigation in the server webview.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)

• CORE-1123: Request failed with status code 401

---

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.}