Skip to content

Serve a favicon from the tracked config subtree (14/13) - #224

Open
alex-clickhouse wants to merge 2 commits into
config-refactor/13-repo-setupfrom
config-refactor/14-favicon
Open

Serve a favicon from the tracked config subtree (14/13)#224
alex-clickhouse wants to merge 2 commits into
config-refactor/13-repo-setupfrom
config-refactor/14-favicon

Conversation

@alex-clickhouse

@alex-clickhouse alex-clickhouse commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Stacked PR 14/14. Base: config-refactor/13-repo-setup (#216). The diff shown is this branch's own change. Stack overview in #204.

What

Drop favicon.svg, favicon.png or favicon.ico into workspace/config/ and the gateway serves it at /favicon.ico. No setting, no build step, nothing to enable.

Why a convention rather than a setting

The file has to travel with the config repo to be worth anything — a fleet pointed at one repo is exactly the case where telling six locked instances apart in a browser tab matters. workspace/config/ is already synced, already reviewed, and already the surface propose_config_change may touch, so the file gets there by the same route as everything else.

A path setting would add a second thing to keep in step with the file it names, and a filesystem path in shared settings is the mistake gateway.ssl.cert is documented as: a box without that file. Here there is nothing to misconfigure — the file is either tracked or it isn't.

Why not copy it into the web bundle

web/dist is build output in the install tree, not the workspace. Copying there means the daemon mutating its own installation: undone by the next npm run build, refused by a read-only or root-owned image, invisible to an install that never syncs, and stateful in the wrong direction — remove the file and the copy lingers.

Reading it per request costs nothing and is strictly better: it works read-only under lockdown, and a favicon that arrives by sync is served without a restart.

The security-relevant part

The candidate is resolved and required to stay inside config/. Git tracks symlinks, and this route is unauthenticated by design — a browser asks for a favicon before anyone has logged in — so config/favicon.png -> /etc/shadow would otherwise be an unauthenticated read of any file the daemon can open, behind a filename a reviewer has no reason to question.

is_file() follows symlinks and answers True there, so the containment check is the entire guard rather than a belt-and-braces one. There's a test that asserts the secret does not come back in the response body.

A symlink that stays inside config/ is still followed — containment is the rule, not "no symlinks".

Active content in an SVG (added in review)

An SVG fetched through <link rel="icon"> is an image and script in it does not run. The same URL navigated to is a same-origin document, and there it does — the ordinary stored-XSS shape for uploaded SVG. This UI keeps its token in localStorage (web/src/api/client.ts:203), so that reaches the session, not just the page. There was no CSP anywhere in the codebase to fall back on.

What makes it worth a header rather than a note is where the file can come from. SVG is text, so it is the one favicon format that fits through propose_config_change — which this PR documents as the supported route. The effect classifier there judges a file by what it causes the daemon to run, and _is_code_name('favicon.svg') is False, so no notice is attached: the reviewer is shown a graphic and asked to approve it. Against someone who can already commit config/cron/gates/*.py this adds nothing — the case it covers is agent proposes, human approves.

The response now carries default-src 'none'; img-src data:; style-src 'unsafe-inline' and X-Content-Type-Options: nosniff. The two allowances keep ordinary icons rendering; nosniff is for the raster formats, where a favicon.png whose bytes are HTML is the same trick without the SVG.

Incidental fix

/favicon.ico previously fell through to the SPA catch-all, which answers every unmatched path with index.html and a 200 — so the browser was handed markup where it asked for an image. The new route is registered ahead of it and returns a real 404 when nothing is tracked.

Worth noting about the test for this: web/dist is gitignored and absent in CI, so the catch-all isn't registered and an ordering assertion would pass because neither route is there to compare. The test builds a minimal web/dist (cleaned up after), asserts the catch-all is present before comparing, and a second test registers a catch-all behind the favicon route to prove the resolution rather than the list order.

Limitation

Proposals carry text, so an agent can propose an .svg but a .png or .ico needs a human commit. Stated in the nerve-workspace skill so the agent says so instead of submitting something that will be rejected.

Full suite: 2681 passed.

The header test asks the route create_app registers rather than the one the other tests build — a copy of the route body cannot notice the headers being dropped from the real one, which is the drift that would matter. Verified by removing them: that test fails and the other 22 pass.

🤖 Generated with Claude Code

@alex-clickhouse alex-clickhouse changed the title Serve a favicon from the tracked config subtree (14/14) Serve a favicon from the tracked config subtree (14/13) Jul 30, 2026
@alex-clickhouse
alex-clickhouse requested a review from Copilot July 30, 2026 10:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds first-class support for a per-instance favicon that is distributed via the git-tracked workspace config subtree and served by the gateway at /favicon.ico, avoiding mutation of the built frontend bundle and ensuring correct behavior vs the SPA catch-all.

Changes:

  • Added workspace_favicon() lookup (with containment checks) for config/favicon.{svg,png,ico}.
  • Registered an unauthenticated /favicon.ico FastAPI route ahead of the static SPA catch-all to return the favicon or a real 404.
  • Added comprehensive tests for lookup rules, symlink containment, and route ordering; updated docs/templates and the web entrypoint to reference /favicon.ico.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
web/index.html Adds /favicon.ico as the document favicon and documents that it’s served from tracked config.
tests/test_favicon.py New tests covering favicon discovery, symlink containment, response behavior, and route ordering/auth expectations.
nerve/templates/skills/nerve-workspace/SKILL.md Documents that binary favicons can’t be proposed via text-only proposals (SVG can).
nerve/templates/config-repo/README.md Documents optional tracked favicon files in the config repo layout.
nerve/gateway/server.py Adds /favicon.ico route registered before static UI/catch-all so it doesn’t fall through to SPA index.
nerve/config.py Implements the tracked favicon lookup convention with containment protection against symlink escape.
docs/config.md Documents the convention, ordering, symlink containment rule, and proposal limitations.

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

Comment thread nerve/gateway/server.py Outdated
Comment on lines +983 to +987
found = workspace_favicon(get_config().workspace)
if found is None:
return Response(status_code=404)
path, content_type = found
return FileResponse(str(path), media_type=content_type)
Comment thread nerve/gateway/server.py Outdated
Comment on lines 987 to 991
return FileResponse(str(path), media_type=content_type)

# Serve static web UI files if built
web_dist = Path(__file__).parent.parent.parent / "web" / "dist"
if web_dist.exists():
alex-clickhouse and others added 2 commits July 30, 2026 14:43
Drop favicon.svg, favicon.png or favicon.ico into workspace/config/ and the
gateway serves it at /favicon.ico. A fleet pointed at one config repo is the case
this is for: six locked instances are otherwise six identical browser tabs.

A convention, not a setting. The file has to travel with the config repo to be
worth anything, and that subtree is already synced, already reviewed, and already
the surface a proposal may touch — so a path setting would only add a second
thing to keep in step with the file it names, and a machine-local path in shared
settings is the mistake gateway.ssl.cert is documented as. Nothing here can be
misconfigured; the file is either tracked or it isn't.

Nothing is copied into the web bundle. web/dist is build output in the install
tree, so writing there would mean the daemon mutating its own installation — undone
by the next `npm run build`, refused by a read-only image, and invisible to an
instance that never syncs. Reading the file per request instead costs nothing,
works under lockdown, and means a favicon that arrives by sync is served without a
restart.

The route resolves the candidate and requires it to stay inside config/. Git
tracks symlinks, and this route is unauthenticated because a browser asks for a
favicon before anyone has logged in — so config/favicon.png -> /etc/shadow would
otherwise be an unauthenticated read of any file the daemon can open, behind a
filename a reviewer has no reason to question. is_file() follows symlinks and
answers True there, so the containment check is the whole guard.

It is registered ahead of the SPA catch-all, which until now answered
/favicon.ico with index.html and a 200 — handing the browser markup where it asked
for an image. The ordering test builds a web/dist so the catch-all is actually
present, and asserts it is, because without that the comparison passes for the
wrong reason.

Co-Authored-By: Claude <noreply@anthropic.com>
Serve the favicon under `default-src 'none'` with `nosniff`, and correct the
catch-all comment that still named the favicon as something reaching it.

An SVG loaded through `<link rel="icon">` is an image and script in it does not
run. The same URL navigated to is a same-origin document, and there it does — the
ordinary stored-XSS shape for uploaded SVG. This UI keeps its token in
localStorage, so that reaches the session rather than just the page.

What decides it is where the file can come from. SVG is text, so it is the one
favicon format that fits through propose_config_change, which the previous commit
documented as the supported route. The effect classifier there judges a file by
what it causes the daemon to run, and an icon causes nothing, so no notice is
attached: the reviewer is shown a graphic and asked to approve it. Reading an icon
for embedded script is not a thing to ask of a reviewer, so the response carries a
policy instead. Against someone who can already commit a gate plugin this adds
nothing; the case it covers is agent proposes, human approves.

`img-src data:` and `style-src 'unsafe-inline'` keep ordinary icons rendering.
`nosniff` is for the raster formats, where a favicon.png whose bytes are HTML is
the same trick without the SVG.

The header test asks the route create_app registers, not the one the other tests
build. A copy of the route body cannot notice the headers being dropped from the
real one, which is the drift that would matter here.

Co-Authored-By: Claude <noreply@anthropic.com>
@alex-clickhouse
alex-clickhouse force-pushed the config-refactor/14-favicon branch from f98e65d to 1221196 Compare July 30, 2026 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants