feat(skills): GitHub-backed skill marketplace#69
Conversation
Adds a new marketplace surface so users can install community skill packs from public GitHub repos. Each pack ships one or more SKILL.md prompts that get merged into the existing template picker with a namespaced id so they can't collide with the 75 bundled skills. Storage lives outside the repo at ~/.html-anything/skills/<owner>__<repo>/ (overridable via HTML_ANYTHING_USER_SKILLS_DIR for tests), so user installs survive git clean and aren't accidentally committed. Supported repo layouts: - SKILL.md at the repo root (single-skill pack) - skills/<id>/SKILL.md at the repo root (multi-skill pack) The install flow downloads the GitHub tarball, validates each SKILL.md has frontmatter, enforces size caps (256 KB SKILL.md, 2 MB example.html, 32 MB tarball), rejects symlinks, then atomically swaps the package into place. Re-installs are idempotent; the existing install is preserved if validation of the new payload fails.
lefarcen
added
size/XXL
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
Thanks for shipping the marketplace flow — I found two blocking issues in the current implementation that need to be fixed before this is safe to merge.
🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.| ); | ||
| // Reload the global templates cache so the picker picks up new skills. | ||
| try { | ||
| await fetch("/api/templates", { cache: "no-store" }); |
There was a problem hiding this comment.
This fetch does not actually refresh the picker's client-side template registry. useTemplates() in next/src/lib/templates/index.ts keeps a module-level cache, and every later mount returns that cached array without refetching. After a user installs or uninstalls a package here, the Settings UI updates, but the template picker continues to serve the old list until the whole page is reloaded. That breaks the main promise of this feature (the new skill is immediately available, and uninstall should disappear immediately too). Please add a real client invalidation path here — for example, export a cache-reset/notify helper from @/lib/templates and call it after install/uninstall, or move the registry into a refreshable store — and cover both install and uninstall with a regression test.
| const ref = parsed.ref ?? (await fetchDefaultBranch(owner, repo, fetchImpl)); | ||
|
|
||
| const pkgId = packageId(owner, repo); | ||
| const workDir = await fs.mkdtemp(path.join(os.tmpdir(), "ha-skill-install-")); |
There was a problem hiding this comment.
workDir is created under os.tmpdir(), but later we move stageDir into userSkillsDir() with fs.rename(). That only works when both paths live on the same filesystem. On many Linux hosts /tmp is a separate tmpfs/mount, so rename(stageDir, targetDir) will throw EXDEV and every valid marketplace install will fail. Since install is the primary flow for this feature, we should not rely on cross-device rename here. Please stage inside path.dirname(targetDir) (or detect EXDEV and fall back to a copy+swap on the target filesystem) and add a regression test that exercises that failure mode.
…ng, tarball traversal, and gzip bombs Audit of PR nexu-io#69 found three latent merge-time hazards that "MERGEABLE/CLEAN" status hides: 1. DNS rebinding: marketplace POST/DELETE were unauthenticated and would become reachable from any visited website until the sibling host-validation middleware (PR nexu-io#61) lands. Add a per-route Host guard that mirrors nexu-io#61's loopback-default + HTML_ANYTHING_ALLOWED_HOSTS / HTML_ANYTHING_ALLOW_ANY_HOST knobs, so the routes are safe regardless of merge order. 2. Tar extraction: the system `tar` was invoked without `--no-symlinks`, and symlink validation ran *after* extraction — a tarball with `link -> ../etc` followed by `link/passwd` could write through the symlink before any check fired. Add an in-process preflight that parses ustar headers and rejects symlinks/hardlinks/devices, absolute paths, and `..` segments before tar touches disk. 3. Gzip bombs: the 32 MB cap was on compressed bytes only, so a highly- compressible payload could expand to multiple GB and exhaust /tmp during extraction. Decompress via `zlib.gunzipSync` with a 96 MB `maxOutputLength` cap (and a cumulative-size check during header walk) so a bomb is rejected as `tarball_uncompressed_too_large` before extract. Also add `schemaVersion: 1` to the installed manifest so future migrations have a version field to dispatch on. Tests: 15 new (host guard env-knobs and loopback variants, preflight rejection of hardlinks / absolute paths / `..` traversal / gzip bombs / corrupt gzip, schemaVersion in the happy-path manifest, install POST 403 on non-loopback host) on top of the existing 105 — full suite 120/120 green, typecheck clean, build clean, guard clean.
Follow-up: hardening pass on the install pipelinePushed dbbd2a0 on top of this branch to close three latent hazards an audit surfaced. 1. DNS-rebinding window vs. PR #61
Added a per-route guard in 2. Tar extraction — symlinks/hardlinks/traversal validated before extract, not afterThe previous flow extracted with system New 3. Gzip-bomb — cumulative-decompressed-size capThe 32 MB cap was on compressed bytes only. A highly-compressible payload ( Manifest schema versionAdded Verification
Not included (deliberate follow-ups)
Audit false-positive worth recordingA repo-wide overlap scan initially flagged PR #18 ( |
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@wuwangzhang1216 Thanks for the follow-up hardening pass — I re-reviewed the current head and the two main-path blockers below are still present, so I am keeping this in changes for now.
🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.| ); | ||
| // Reload the global templates cache so the picker picks up new skills. | ||
| try { | ||
| await fetch("/api/templates", { cache: "no-store" }); |
There was a problem hiding this comment.
This still does not invalidate the client-side template registry after an install. useTemplates() in next/src/lib/templates/index.ts keeps a module-level cache and serves that array to later mounts without refetching, so calling fetch("/api/templates", { cache: "no-store" }) here only warms the HTTP response and leaves the in-memory cache unchanged. The result is that install/uninstall updates the Marketplace panel but the template picker keeps showing the old skill list until the whole page reloads, which breaks the feature promise that newly installed skills appear immediately. Please add a real client invalidation path here — for example, export a cache reset or listener notification helper from @/lib/templates and call it after both install and uninstall — and cover both transitions with a regression test.
There was a problem hiding this comment.
Fixed in fe4fa94. You're right — clearing inflight orphaned the older promise but its (async () => { … cache = json.templates … notify })() closure was still alive and would commit on resolve. Your minimal repro (start initial fetch → refresh resolves with [new] → original resolves with [old]) lands in cache=[old], exactly as you described.
Switched to a monotonic generation token bumped at the start of every fetchTemplates(). Each in-flight fetch captures its generation; on resolve it only writes cache and notifies listeners if generation hasn't been bumped in the meantime, otherwise it drops the result and returns the current canonical cache (so callers awaiting the stale promise still observe consistent state — no exception thrown for code that didn't ask for the race). The inflight = null cleanup is now reference-guarded too, so an orphaned fetch can't clobber the newer in-flight pointer when it finally settles.
Regression test in refresh.test.ts > "a stale fetch that resolves AFTER a refresh must not clobber the fresh cache" drives your exact repro: kick off the initial fetch, call refreshTemplates() before it resolves, resolve the second one with [post-install], then resolve the first one with [pre-install], assert cache still holds [post-install]. Without the generation guard this test fails — verified by running it against the previous head before staging the fix.
128/128 green, typecheck/build/guard clean.
| await fs.rename(targetDir, backupDir); | ||
| } | ||
| try { | ||
| await fs.rename(stageDir, targetDir); |
There was a problem hiding this comment.
stageDir is still created under workDir, and workDir still comes from os.tmpdir() a few lines above, so this rename(stageDir, targetDir) still assumes both paths live on the same filesystem. On hosts where /tmp is a separate tmpfs or mount, Node throws EXDEV here and every valid marketplace install fails in the primary happy path. Please stage on the destination filesystem, for example under path.dirname(targetDir), or catch EXDEV and fall back to a copy+swap, then add a regression test that exercises that cross-device case.
… client template cache on install/uninstall Addresses the two main-path blockers @mrcfps flagged on PR nexu-io#69: 1. EXDEV on `fs.rename`. The previous flow staged the package under `os.tmpdir()` then renamed it into `~/.html-anything/skills/`. On Linux hosts where `/tmp` is a separate tmpfs/mount, that throws `EXDEV` and every install fails. Tarball download/extract still happens in `/tmp` (transient), but the staged final layout now lives next to the target under a hidden `.stage-<pkgId>-<random>/` directory, so the swap rename is always intra-filesystem. The `.stage-` prefix keeps these dirs out of `listPackages` (which requires a leading alphanumeric segment). 2. Client-side template registry never refreshed. Settings/Marketplace was re-hitting `/api/templates` with `cache: "no-store"`, but the module- level `cache` in `@/lib/templates/index.ts` was untouched, so the picker kept serving the stale list until a full page reload. New `refreshTemplates()` drops both `cache` and any in-flight promise, then re-fetches and notifies every subscribed `useTemplates` consumer. The hook now subscribes unconditionally — previously it early-returned on warm cache and missed notifications. Settings-modal calls `refreshTemplates()` after both install and uninstall. Tests: +6 (cross-device-safe staging: spy on rename to assert intra-FS, simulate EXDEV to verify staging is on the destination FS, hidden stage dirs invisible to listPackages; refreshTemplates: re-fetch + cache flip, uninstall removes entry, failed refresh leaves cache cleared so the next mount re-fetches). Full suite 126/126, typecheck/build/guard clean.
|
@mrcfps Both blockers addressed in 9c3c7fe. Concrete responses inline: Blocker 2 — EXDEV on cross-device rename (
|
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@wuwangzhang1216 Thanks for addressing the earlier blockers — I found one follow-up correctness issue in the client refresh path that is worth tightening before we rely on the immediate picker refresh behavior.
🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.| export async function refreshTemplates(): Promise<TemplateDef[]> { | ||
| cache = null; | ||
| inflight = null; | ||
| return fetchTemplates(); |
There was a problem hiding this comment.
refreshTemplates() clears inflight, but it does not stop the older request that was already created by fetchTemplates() above. That older promise still runs the existing cache = json.templates / listener notification path when it resolves, so a stale /api/templates response can overwrite the fresh post-install list and push mounted pickers back to the pre-install state. A minimal repro is: start the initial fetch, call refreshTemplates(), let the refresh resolve with [new], then let the original request resolve with [old] — the final cache ends up back on [old]. Because install/uninstall is explicitly promising an immediate picker update, this race can surface on a slow first load or any overlapping refresh. Please guard cache writes with a generation/request token (or abort the superseded fetch) so only the latest refresh is allowed to commit, and add a regression test that forces the stale request to resolve after the refreshed one.
There was a problem hiding this comment.
Fixed in fe4fa94. You're right — clearing inflight orphaned the older promise but its (async () => { … cache = json.templates … notify })() closure was still alive and would commit on resolve. Your minimal repro (initial fetch in-flight → refresh resolves with [new] → original resolves with [old]) lands in cache=[old], exactly as you described.
Switched to a monotonic generation token bumped at the start of every fetchTemplates(). Each in-flight fetch captures its generation; on resolve it only writes cache and notifies listeners if generation hasn't been bumped in the meantime, otherwise it drops the result and returns the current canonical cache (so callers awaiting the stale promise still observe consistent state — no undefined flash, no rejection for code that didn't ask for the race). The inflight = null cleanup is now reference-guarded too (if (inflight === myPromise) inflight = null), so an orphaned fetch can't clobber a newer in-flight pointer when it eventually settles.
const myGeneration = ++generation;
const myPromise = (async () => {
// … fetch + parse …
if (myGeneration !== generation) return cache ?? json.templates; // superseded — drop on the floor
cache = json.templates;
for (const l of listeners) l(cache);
return cache;
})();Regression test in refresh.test.ts > "a stale fetch that resolves AFTER a refresh must not clobber the fresh cache (generation guard)" drives your exact repro: hand-rolled deferred fetch with manual resolvers, kick off the initial fetch, call refreshTemplates() before it resolves, resolve the second one with [post-install], then resolve the first one with [pre-install], assert getCachedTemplate("post-install") still wins and getCachedTemplate("pre-install") is undefined. I confirmed the test fails against the pre-fix head, so it's a real regression net.
128/128 green, typecheck/build/guard clean.
…er the post-refresh cache @mrcfps caught a race in the previous refresh path: `refreshTemplates()` cleared `inflight` but didn't cancel the orphaned promise. If a slow initial `/api/templates` fetch was already running when settings-modal called `refreshTemplates()` after install, the stale fetch would still resolve later, overwrite `cache` with the pre-install list, and notify every subscribed `useTemplates` consumer — pushing mounted pickers back to the pre-install state. The race surfaces on a slow first load or any overlapping refresh, and breaks the feature's "immediate picker update" promise. Fix: monotonic `generation` token bumped on every `fetchTemplates()` start. Each in-flight fetch captures its generation and only commits to `cache` / notifies listeners if `generation` hasn't been bumped in the meantime. Older fetches drop their result on the floor and return the current cache so awaiting callers still observe consistent state. The finally block guards `inflight = null` with a reference check so an orphaned fetch can't clobber the newer in-flight pointer. Regression test (`refresh.test.ts`) drives the exact repro: initial fetch in-flight, refresh fires and resolves with `[post-install]`, THEN the original request resolves with `[pre-install]`. Asserts cache still holds `[post-install]`. Without the generation guard, the stale fetch wins and the test fails. Full suite 128/128, typecheck/build/guard clean.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@wuwangzhang1216 Thanks for the follow-up hardening pass — I found one remaining security gap in the new marketplace surface that is worth tightening before this ships.
🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.| export const dynamic = "force-dynamic"; | ||
|
|
||
| /** List every installed marketplace package. */ | ||
| export async function GET() { |
There was a problem hiding this comment.
This new GET /api/marketplace route is the one marketplace endpoint that still skips isHostAllowed(). Because the stopgap host guard here exists specifically to cover the DNS-rebinding window before the global /api/* middleware lands, leaving the read endpoint open still lets an untrusted rebinding origin enumerate the user's installed packages, including repo owners/names/refs, from the local app. That is a real privacy leak even though install/uninstall are now protected. Please apply the same hostRejectedResponse() / isHostAllowed() check here (or land the shared middleware in the same release) so the temporary defense covers the whole marketplace API surface.
There was a problem hiding this comment.
Fixed in ce8c86e. You're right — the per-route guard's whole purpose is to cover the DNS-rebinding window until #61's /api/* middleware lands, so leaving the read endpoint open undermines that goal. Even though listing installed packages is "just" a read, the response includes source.{owner,repo,ref} + installedAt for every package, which is exactly the kind of info a rebinding origin should not be able to enumerate from the user's local app.
Applied the same hostRejectedResponse() / isHostAllowed() pair to GET. New regression test in api.test.ts > "returns 403 when the Host header is not loopback — even for the read endpoint" drives a request from http://evil.example.com/api/marketplace and asserts 403 + host_not_allowed, matching the existing install-route test. The empty-list and post-uninstall happy-path tests now pass a loopback Request.
The whole marketplace surface (GET /api/marketplace, POST /api/marketplace/install, DELETE /api/marketplace/packages/[id]) is now covered by the stopgap guard, and the entire stopgap can be deleted once #61 merges.
129/129 green, typecheck/build/guard clean.
…rebinding privacy leak @mrcfps flagged the read endpoint as the last marketplace surface still skipping `isHostAllowed`. Until the global `/api/*` host-validation middleware (PR nexu-io#61) lands, an untrusted DNS-rebinding origin could call `GET /api/marketplace` against the user's local app and enumerate every installed package — repo owners, names, refs, install timestamps. That is a real privacy leak even though install/uninstall are now protected. Same `hostRejectedResponse()` / `isHostAllowed()` pair already applied to the install + uninstall routes; just adding it here closes the remaining surface. Regression test exercises a request from `http://evil.example.com/api/marketplace` and asserts 403 with `host_not_allowed`, matching the existing install-route test. 129/129 green, typecheck/build/guard clean.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@wuwangzhang1216 I re-reviewed the current head and verified the last follow-ups landed: the marketplace read endpoint now carries the same host guard as the write routes, and the earlier install/refresh hardening stays covered by focused regression tests. I didn't find any remaining actionable issues in the changed ranges. Thanks for pushing the security and correctness fixes through — this looks ready to merge. 🙏
🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.3 participants
Summary
Closes the Skill marketplace (install
<github-repo>) item on the roadmap. Users can now install community skill packs from public GitHub repos directly inside the app, and the installed skills show up in the existing template picker alongside the 75 bundled ones.End-to-end story: paste
owner/repo(orowner/repo#branch, or a full GitHub URL) into Settings → Marketplace → Install from GitHub, hit Install, and the new skill is immediately available in the template picker. Uninstall is one click in the same panel.What's in this PR
New surfaces
Settings → Marketplacetab (settings-modal.tsx) — install form, installed-package list with skill chips, per-package uninstall, inline status / error toasts. Fully i18n'd (EN + ZH).GET /api/marketplace(route.ts) — list installed packages.POST /api/marketplace/install(install/route.ts) — install from a GitHub spec.DELETE /api/marketplace/packages/[id]([id]/route.ts) — uninstall a package.New library surface (
next/src/lib/skills/)paths.ts— resolves~/.html-anything/skills/(overridable viaHTML_ANYTHING_USER_SKILLS_DIR); helpers for thepkg-<owner>__<repo>--<originalId>id namespace so user skills can't collide with bundled ones.registry.ts—listPackages()/listUserSkills()/findUserSkill()walks the user-skills directory and parses each package'spackage.jsonmanifest.install.ts— GitHub spec parser, tarball download viacodeload.github.com,tar -xzfextraction with--strip-components=1, validation, atomic swap-into-place.Loader merge (
templates/loader.ts)listSkills()/loadSkill()/skillHasPreview()now transparently merge bundled skills with user-installed ones. AddsinvalidateSkillsCache()so install/uninstall force a refresh. Existing consumers (/api/templates,/api/templates/[id]/example,/api/convert, etc.) need no changes — they keep callingloadSkill(id)and it routes to the right directory based on the id.Storage layout
User skills appear in the registry as
pkg-<owner>__<repo>--<originalId>(e.g.pkg-nexu-io__roast-skill--roast-skill). Bundled skills keep their plain kebab-case ids unchanged.Supported repo layouts
SKILL.mdat the repo root → installed under the repo's name asoriginalId.skills/<id>/SKILL.mdat the repo root → each direct subdirectory ofskills/becomes one skill.Spec formats accepted
owner/repoowner/repo#ref(branch, tag, or sha)https://github.com/owner/repo[/tree/<ref>]owner/repo.gitWhen no ref is provided, the install hits
api.github.com/repos/<owner>/<repo>to pick up the default branch (falls back tomainif the call fails).Security & safety
The install flow runs in the same Node process that already spawns user-configured CLI agents, so the local-only network surface is unchanged. Inside that boundary:
..), and non-github.comhosts.SKILL.md≤ 256 KB,example.html≤ 2 MB,example.md≤ 512 KB.SKILL.mdmust contain YAML frontmatter — naked README-style files are rejected.os.tmpdir(), thenrename(2)into place. Re-installs back up the existing directory first and restore on failure, so a broken re-install never corrupts an existing working install.DELETErequires a well-formedowner__repoid and refuses anything that doesn't already have a valid manifest on disk.Testing
pnpm -F @html-anything/next test→ 11 files, 105 tests passing (27 net-new tests below).pnpm -F @html-anything/next typecheck→ ✅pnpm -F @html-anything/next build→ ✅ (new routes appear in the route manifest)pnpm exec tsx scripts/guard.ts→ ✅Unit tests (new files under
next/src/lib/skills/__tests__/)paths.test.tspkg-…--…id encode/decode; bundled vs namespaced id discriminationspec.test.tsowner/repo,owner/repo#ref, full URLs,/tree/<ref>(with slashes in ref),.gitsuffix; rejection of path-traversal refs, shell-char owners, empty input, non-github hostsinstall.test.ts.tar.gzfixture, stubs onlyfetch); multi-skillskills/<id>/SKILL.mddiscovery; idempotent reinstall; invalid spec doesn't fetch; repo without any SKILL.md rejected; manifest persisted with correct shape; uninstall removes and reports correctlyinstall-rejections.test.tsSKILL.mdwithout frontmatter;SKILL.md > 256 KB;example.html > 2 MB; symlink inside archive; download 404; oversized declaredcontent-length;api.github.comfailure fallback tomain; failed re-install does not corrupt the prior installapi.test.tsGET /api/marketplaceempty;POST /installinvalid JSON / missing source / invalid spec / success;DELETEmalformed id / unknown id / real removalloader-merge.test.tsloadSkillresolves a namespaced id to its body and frontmatter-derived metadata; unknown namespaced id returns null; bundled ids still resolve unchangedThe install tests do not mock the tar pipeline — they build real
.tar.gzarchives with the systemtarbinary and let the install code'star -xzfextract them. Only the HTTP layer (fetch) is stubbed.Real-network e2e (manual)
Started the dev server with
HTML_ANYTHING_USER_SKILLS_DIR=/tmp/ha-marketplace-e2e/user-skills PORT=3499 pnpm -F @html-anything/next devand ran the full happy-path + error-path matrix viacurl:GET /api/marketplacewith seeded fixtureGET /api/templatesafter seedPOST /installinvalid JSONinvalid_jsonPOST /installmissing sourcemissing_sourcePOST /installinvalid specinvalid_specPOST /installfoo;rm-rf/barPOST /installfoo/bar#../../etc/passwdDELETE ../etc/passwdinvalid_idDELETE ghost__packnot_foundDELETEreal packageGET /api/marketplaceafter deleteReal GitHub install (
nexu-io/roast-skill— a public repo withSKILL.mdat root):POST /api/marketplace/installwith{"source": "nexu-io/roast-skill"}→ 200, manifest returned~/.html-anything/skills/nexu-io__roast-skill/{package.json, skills/roast-skill/SKILL.md}written/api/templatesimmediately shows it withzhName: "牛马锐评"extracted from the real frontmatter/api/convertwithtemplateId: pkg-nexu-io__roast-skill--roast-skillgot past template lookup (failed at the agent stage withunknown agent: nonexistent-agent, confirmingloadSkillresolved successfully)pkg-does__not-exist--ever→400 unknown templateDELETE→ 200, directory removed,GET /api/marketplace→{"packages": []}What's intentionally not in this PR
codeload.github.comandapi.github.comwithout credentials.Test plan
pnpm install --frozen-lockfilepnpm -F @html-anything/next typecheckpnpm -F @html-anything/next testpnpm -F @html-anything/next buildpnpm exec tsx scripts/guard.tspnpm -F @html-anything/next dev, open Settings → Marketplace, installnexu-io/roast-skill, confirm it appears in the template picker