Skip to content

fix(server): resolve monorepo favicons and sanitize SVGs - #4424

Closed
Franz1241 wants to merge 3 commits into
pingdotgg:mainfrom
Franz1241:t3code/find-sidebar-favicon-source
Closed

Franz1241 wants to merge 3 commits into
pingdotgg:mainfrom
Franz1241:t3code/find-sidebar-favicon-source

Conversation

@Franz1241

@Franz1241 Franz1241 commented Jul 24, 2026 •

Copy link
Copy Markdown

What Changed

  • Resolve project favicons from one-level workspaces under apps/*, packages/*, and services/*.
  • Reuse the existing favicon candidates and source metadata discovery for each workspace.
  • Preserve root-level favicon precedence and deterministic monorepo search order.
  • Serve project SVG favicons using only the root <svg> document, excluding XML declarations, doctypes, and trailing content.
  • Add focused coverage for monorepo lookup, precedence, SVG extraction, and signed asset resolution.

Why

T3 Code only searched favicon locations relative to the selected project root. Monorepos commonly keep their application favicon under paths such as apps/web/public/favicon.svg, so those projects fell back to the generic folder icon.

Some otherwise valid SVG favicon files also contain XML declarations or doctypes that can prevent them from rendering reliably as sidebar image assets. Extracting the SVG document before serving it keeps the original file untouched while avoiding that failure mode.

UI Changes

No layout or component changes. Projects that previously displayed the fallback folder icon can now display their application favicon when it lives in a supported monorepo workspace.

Verified in an isolated local T3 Code environment with an XML/doctype-prefixed favicon at apps/dashboard/public/favicon.svg; both sidebar image instances loaded successfully at their intrinsic 150×150 size.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes (not applicable)

Note

Medium Risk
Touches signed asset resolution and serves transformed SVG content (mitigates trailing markup) rather than always streaming the on-disk file; favicon precedence behavior changes for monorepos.

Overview
Favicon discovery now walks one-level workspaces under apps/, packages/, and services/ (sorted entries), reusing the same conventional filenames and HTML/metadata icon href resolution per app. Root t3.json, root candidates, and root source metadata still win before any monorepo path; unreadable monorepo directories are skipped.

SVG project favicons are no longer served as raw files when possible: resolveAsset reads .svg favicons, runs new extractSvgDocument to keep only the first root <svg> (drops XML/doctype/comments and trailing junk like extra <script>/second roots), and returns { kind: "text", contentType: "image/svg+xml" }. Non-SVG favicons and unparseable SVGs still use file paths. The asset route serves the text variant with the same private cache and nosniff headers as file responses.

Tests cover SVG extraction edge cases, sanitized favicon resolution, and monorepo precedence/ordering.

Reviewed by Cursor Bugbot for commit 917649a. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix favicon resolution for monorepos and sanitize SVG favicons

  • Extends ProjectFaviconResolver.resolvePath to search monorepo subdirectories (apps, packages, services) for favicons, checking well-known candidate paths and <link rel="icon"> hrefs in source files, in stable sorted order.
  • Adds extractSvgDocument in AssetAccess.ts to extract and sanitize the first complete <svg> element from raw file content, handling nested SVGs, comments, and CDATA.
  • When a .svg favicon is resolved, the server now reads the file, extracts the SVG document, and returns it as an inline text asset with content-type: image/svg+xml instead of serving the raw file.
  • Updates the asset HTTP route in http.ts to handle the new inline text asset variant with proper Cache-Control and X-Content-Type-Options headers.
  • Behavioral Change: callers requesting .svg favicons now receive sanitized inline SVG text; non-SVG favicons and failed extractions continue to return file assets.

Macroscope summarized 917649a.

- Search conventional apps, packages, and services favicon locations
- Extract SVG documents and serve them with secure response headers
@coderabbitai

coderabbitai Bot commented Jul 24, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 61594c7f-8c72-4589-a4c2-d96b14559406

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 24, 2026
Comment thread apps/server/src/assets/AssetAccess.ts
Comment thread apps/server/src/project/ProjectFaviconResolver.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Jul 24, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR introduces custom SVG sanitization logic to strip potentially dangerous content (like <script> tags) and adds new monorepo favicon resolution. Security-sensitive parsing code from a new contributor warrants human review to verify the sanitization is robust against bypasses.

You can customize Macroscope's approvability policy. Learn more.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9c263ae. Configure here.

Comment thread apps/server/src/assets/AssetAccess.ts Outdated
@juliusmarminge

Copy link
Copy Markdown
Member

Closing as part of the open-PR backlog sweep (wave 1).

Reason: Jul conflicted monorepo favicon/SVG work; favicon caching landed in #4767.
Related: #4767

Reopen if this is still wanted and you’re willing to rebase onto current main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants