fix(skills): stop the public library-status route disclosing source repo URLs (abilityai/trinity-enterprise#334)

[obasilakis]

Summary

GET /api/skills/library/status was gated only by Depends(get_current_user) and returned skills-source repo URLs to every authenticated principal, including agent-scoped MCP keys (which resolve to their owner carrying the owner's role). The sibling GET /api/skills/sources returns the same get_library_status() dict behind require_admin + reject_agent_principal, with a docstring stating why: for a private source, the repo URL is itself sensitive.

One payload, two trust levels — the weaker gate was the bug.

Why an allow-list and not require_admin

The issue offered a fork: gate the route, or strip userinfo from the URLs. Both are wrong, in opposite directions, and this is the decision most worth reviewing:

• require_admin breaks two shipped non-admin surfaces. /library is requiresAuth only (router/index.js:123) and its skills section calls the endpoint for every authenticated visitor; the per-agent Skills tab is gated on can_share — owner, not admin (AgentDetail.vue:885). Both derive their empty-state discriminator from configured/cloned, so a 403 renders a configured library as "not configured", including the non-admin empty states written for exactly those callers ("Ask your admin to configure a skills library").
• Stripping userinfo alone leaves the issue's own "unconditional lesser variant" open: private org/repo names still reach every agent key.

Allow-listing withholds the sensitive field while every caller keeps the state it actually reads. SkillsLibraryStatus is fail-closed by construction — FastAPI serialises through an explicitly-constructed model, so a future sensitive field is invisible over REST until someone names it. Same idiom, same reason, as response_model=List[SkillInfo] on GET /skills/library.

Verified at the HTTP layer, not just model_dump: a TestClient round-trip with a tokenized url and an injected unknown field drops both from the wire body.

What is withheld, and what is not

Withheld — flat url, per-source url, per-source last_error.

last_error was found by /cso and is the non-obvious one: it is git's failure text, which echoes the remote URL — and the clone path's URL carries a spliced PAT. redact() scrubs it going in but under-matches a double-@ authority (ent#347), which is precisely the shape _authenticated_url builds when the stored URL already has userinfo. That combination reliably fails auth, so the leaking branch is the guaranteed one. Its only consumer (SkillSourcesPanel.vue:90) reads the admin-gated sources route, which still returns it — no operator loses the error.

Kept — branch and commit_sha. A ref name and a commit hash are neither credentials nor repo identity, and the Library header renders both. Dropping them was considered and cut: it would be a second, unrelated behaviour change riding a security fix.

strip_url_credentials

New helper in utils/url_validation.py, beside reject_embedded_credentials (that module already owns what userinfo means here). Applied at both service emitters, so every consumer — including the admin route and SkillSourcesPanel — is covered from one place.

• Parse-based, per the house rule _authenticated_url sets: the host is decided by parsing, never by substring. A regex mangles a legitimate ?ref=a@b, and the [^@]+@ classes the existing scrubbers use cannot cross the first @.
• Never-raises by contract. urlparse throws on an unbalanced bracket, and the legacy-adoption path writes rows with no validation at all, while get_library_status must never 500 the panel.
• A protocol-relative leak (//tok@host — the assumed scheme produced https:////tok@host, whose netloc parses empty) was caught in /review and fixed. It's one of the shapes the frontend stripUserinfo enumerates; the two strips have to agree on it.

Changes

File	Why
utils/url_validation.py	new never-raising, parse-based strip_url_credentials
services/skill_service.py	strip at both URL emitters in get_library_status()
models.py	SkillsLibraryStatus / SkillsLibrarySourceStatus allow-list (Invariant #14)
routers/skills.py	response_model on the public status route
SkillsPanel.vue, LibrarySkillsSection.vue	remove the now-dead URL renders
SkillSourcesPanel.vue	apply stripUserinfo where a URL still renders (admin route)
mcp-server/src/tools/skills.ts	drop url from the interface so the type stops advertising it
tests/*	new suite + inverted the assertion that required url
docs/memory/feature-flows/skills-library-sync.md	route snippet + why the allow-list is a security boundary

Test Plan

• 153 passed — test_ent334_status_url_disclosure.py, test_ent237_skill_sources.py, test_ssrf_skills_library.py (-p no:randomly)
• strip_url_credentials over 15 shapes incl. double-@, protocol-relative, git+ssh://, malformed, IDN, tab/newline, None, non-string — 0 leaks, 0 raises
• HTTP-layer allow-list proof incl. an injected unknown field
• Static guard fails if the response_model is ever dropped
• Inverted tests/test_skills.py — it previously required url to be present
• tests/test_skills.py cannot run locally (401 on /token); identical on dev-baseline — environment, not regression. Needs CI.

Deliberately out of scope

• abilityai/trinity-enterprise#346 (P0) — an agent-scoped key can inject a fleet-wide skills source via PUT /api/settings/skills_library_url, bypassing the grant gate ent#237 built. Read half verified live; the write half is inference.
• abilityai/trinity-enterprise#347 (P2) — both free-text scrubbers under-match a double-@ URL.

Both collide with _adopt_legacy_clone, so this lands first.

The issue's final AC ("lands on feature/ent-237-multi-source-skills before it merges") is satisfied by obsolescence — ent#237 merged as 9e98b31c (#1901). Base is fresh dev.

Refs abilityai/trinity-enterprise#334

[vybe]

The central decision — allow-list rather than require_admin or bare userinfo-stripping — is right, and the PR argues it the right way: by naming the two shipped non-admin callers (/library is requiresAuth only; the Skills tab is gated on can_share, owner not admin) and the fact that both derive their empty state from configured/cloned, so a 403 would render a configured library as "not configured" — including in the non-admin empty states written for exactly those callers. That is a concrete regression, not a hypothetical.

response_model as the mechanism is the part that will still be working in a year: it is fail-closed by construction, so a future sensitive field is invisible over REST until someone names it. Verifying it at the HTTP layer with an injected unknown field, rather than via model_dump, is the correct instinct — response_model filtering is FastAPI's serialization layer and a direct handler call proves nothing about the wire. The static AST guard matters for the same reason: dropping the decorator kwarg reopens the leak with no behavioural test failing.

Withholding last_error is the non-obvious catch and the right call — it is git's failure text, which echoes the remote URL, and the clone path's URL carries a spliced PAT. Keeping branch/commit_sha is also right; dropping them would have been a second, unrelated behaviour change riding a security fix.

I ran the cross-check #2041 asked for (it predicted that whichever of these landed second should reconcile its parser against _CREDENTIAL_URL_RE). On the authority boundary the two agree exactly — all seven inputs where both fire produce the same host, including https://a@b@c@github.com/o/r and https://tok@github.com:8443/o/r, and both correctly leave ?ref=a@b alone.

They differ in when they fire, and strip_url_credentials is strictly the more thorough of the two:

input	strip_url_credentials	redact (#2041)
//tok@github.com/o/r	//github.com/o/r	unchanged
tok@github.com/o/r	github.com/o/r	unchanged
git+ssh://tok@github.com/o/r	git+ssh://github.com/o/r	unchanged

_CREDENTIAL_URL_RE is anchored on a literal https://, so it cannot match the protocol-relative, scheme-less or alternate-scheme forms your had_authority branch handles. That is not a defect in this PR — you cover the shapes it misses — but it is a live gap in the free-text scrubbers, reachable because _scrub_pat runs over str(e) of arbitrary exceptions and _adopt_legacy_clone writes rows with no validation at all. Filing it as a follow-up rather than holding this.

Merge conflict resolved (tests/registry.json, against #2031's entry): rebuilt from the merge stages rather than splicing markers — dev's 130 entries plus this branch's one, 131 total, no duplicates, valid JSON.

That surfaced a real cross-PR break, which is the reason it was worth resolving carefully rather than mechanically: skill_service now imports strip_url_credentials, and test_ent183_skill_packages.py stubs utils.url_validation with a fake that lacked it — so the module failed to import at collection, which also failed #1898's guard (it collects that file as the offender). Added the name to the stub, honouring that stub's own comment that it must mirror every name skill_service imports. 116 passed across the four interacting suites, 390 passed across the wider skills/url/ssrf selection.

Cross-tracker ref — setting status-in-dev on ent#334 by hand after merge.