Serve a favicon from the tracked config subtree (14/13) - #224
Open
alex-clickhouse wants to merge 2 commits into
Open
Serve a favicon from the tracked config subtree (14/13)#224alex-clickhouse wants to merge 2 commits into
alex-clickhouse wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
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) forconfig/favicon.{svg,png,ico}. - Registered an unauthenticated
/favicon.icoFastAPI 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 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 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(): |
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
force-pushed
the
config-refactor/14-favicon
branch
from
July 30, 2026 12:44
f98e65d to
1221196
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.pngorfavicon.icointoworkspace/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 surfacepropose_config_changemay 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.certis 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/distis build output in the install tree, not the workspace. Copying there means the daemon mutating its own installation: undone by the nextnpm 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 — soconfig/favicon.png -> /etc/shadowwould 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 answersTruethere, 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 inlocalStorage(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')isFalse, so no notice is attached: the reviewer is shown a graphic and asked to approve it. Against someone who can already commitconfig/cron/gates/*.pythis 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'andX-Content-Type-Options: nosniff. The two allowances keep ordinary icons rendering;nosniffis for the raster formats, where afavicon.pngwhose bytes are HTML is the same trick without the SVG.Incidental fix
/favicon.icopreviously fell through to the SPA catch-all, which answers every unmatched path withindex.htmland 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/distis 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 minimalweb/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
.svgbut a.pngor.iconeeds a human commit. Stated in thenerve-workspaceskill so the agent says so instead of submitting something that will be rejected.Full suite: 2681 passed.
The header test asks the route
create_appregisters 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