feat: migrate octo marketplace service

[kense-lab]

Summary

• migrate the complete Octo Marketplace service into the Mininglamp-OSS organization repository
• include Skill and MCP catalog APIs, authentication, MySQL migrations, object storage, Docker/CI, and project documentation
• include configurable public or signed COS downloads and authenticated JSON download URL support
• exclude the legacy tracked .env; retain .env.example only

Verification

• go test ./...
• git diff --cached --check before commit

Source

Migrated from the current integration branch in kense-lab/octo-marketplace, including the latest upstream MCP catalog merge and resolved conflicts.

Closes #3

[OctoBoooot]

Review: feat: migrate octo marketplace service (#1)

Verdict: Request changes — two majors on the storage/network attack surface (unauthenticated local-storage proxy; SSRF in the MCP probe), both reachable under the shipped default config. The core service (auth chain, SQL layer, secret handling) is well-built and verified clean. Nothing here is unfixable; the majors are gating/config-posture fixes.

Scope note (honest): this is a 118-file / +16,775-line greenfield service migration — too large for a line-by-line read of every file. I deep-read the high-risk surfaces (auth resolver chain + middleware, secret handling, storage/blob + download authz, the repository/SQL layer, config defaults, router wiring) and fanned out focused audits over SQLi, download-authz/SSRF, and secret-leakage. Findings below are byte-verified at head 2794bd91e735; areas I did not exhaustively read are called out at the end.

Risk tier: high (auth, credential material). needs-human-review applied — an org-repo service import of this size warrants a human architectural pass regardless of my verdict.

Major

• internal/api/handler/upload/handler.go:44-50, 283-310 + internal/api/router/router.go:67,129 — unauthenticated local-storage proxy. RegisterLocalProxy mounts PUT /api/v1/_storage/upload/*key and GET /api/v1/_storage/download/*key on the root engine r, which carries only Logger/Recovery/CORS; authenticator.Handler() is applied to the v1 group only. Both handlers (localUploadProxy/localDownloadProxy) do no middleware.Identity check, no token, no signature — unlike every other handler in the file. So anyone can write arbitrary objects and read any stored object by key. path traversal is contained by safePath (storage/local.go:29-47), but there is zero authorization.
  • Reachable by default: STORAGE_DRIVER defaults to local (config.go:106, .env.example:32), so the proxy mounts unless an operator opts into oss. And because it's on the root engine, it stays exposed even with AUTH_ENABLED=true — a production deployment that keeps the local driver still leaks it.
  • Fix: register the proxy under the auth'd v1 group (or gate it behind middleware.Identity + a signed-key check), and/or refuse to mount it when AUTH_ENABLED=true. At minimum, document that local is dev-only and force oss when auth is on.
• internal/service/probe.go:154-178, 349, 565, 629 — SSRF via the MCP probe. POST /api/v1/mcps/probe (authenticated) issues outbound GET/POST to a caller-supplied req.URL; validateProbeURL enforces only http/https scheme and deliberately does not block RFC1918 / link-local / 169.254.169.254 (cloud metadata). The SSE endpoint event can further redirect the follow-up POST (resolveEndpoint), and user Headers are forwarded. Any authenticated tenant can make the server reach internal services or the metadata endpoint.
  • This looks like a knowingly-accepted v1 tradeoff for self-hosted internal MCP servers (per the code comment), which is why it's major not blocker — but it must have a deploy-gated CIDR denylist (at least metadata/link-local) before any shared-hosting or multi-tenant deployment. Recommend an allowlist/denylist config knob, default-deny for RFC1918+metadata.

Minor

• internal/service/parse/service.go:69,119,290 + internal/service/skill/service.go:216,345 — user file_name is concatenated into object keys (skills/{id}/{filename}) with only an extension check, no ..//leading-/ sanitization. Not exploitable for bucket/path escape (OSS scopes to a fixed bucket; the local driver's safePath blocks traversal), but basename-ing the filename at InitUpload is worth it for defense-in-depth.

Verified clean (the load-bearing surfaces)

• Auth chain (middleware/auth.go, auth/resolver.go + bot_resolver/cached_resolver) — fail-closed throughout: nil resolver → 503, resolve error → 503, empty UID → 401, missing auth context → 503, X-Space-Id not in identity.Spaces → 403. The dev-identity bypass is correctly gated on !a.enabled. No fail-open on the enabled path.
• SQL layer (internal/repository/*, internal/db/*) — every caller-controlled value is bound via ?; the two fmt.Sprintf/concatenated query builders (skill/list.go, skill/update.go, mcp.go buildWhere) interpolate only hardcoded fragments, ORDER BY is fixed, LIMIT/OFFSET parameterized and clamped. No injection.
• Secret handling (service/secret.go, config.go) — secret.go is a protective control: token-like keys are redacted at write time (a non-placeholder value rejects the request), never logged, never serialized into a response DTO. All secrets load from env with empty defaults; no committed .env, no real credentials in the tree (the ghp_… hits are deliberate redaction-test fixtures). .env.example is placeholders only.

Notes

• CI is pending at this head — verdict cannot be APPROVE regardless until code checks are green; but the two majors are the substantive gate.
• Areas I did not exhaustively read (flagging honestly, not silently): the MCP parse/worker pipeline internals beyond the SSRF/key surfaces, the full model/DTO layer, migrations SQL correctness beyond the injection check, and the category service. A human architectural review (the needs-human-review gate) should cover these for a service being adopted into the org.
• Squash-merge is reasonable for a migration, but consider whether a service of this size should land as one reviewable unit vs. a documented import commit — that's a maintainer call.

[OctoBoooot]

[major] SSRF via the MCP probe. POST /api/v1/mcps/probe (authenticated) fetches a caller-supplied req.URL; validateProbeURL enforces only http/https scheme and deliberately does not block RFC1918 / link-local / 169.254.169.254 (cloud metadata). The SSE endpoint event can redirect the follow-up POST (resolveEndpoint), and user Headers are forwarded. Any authenticated tenant can reach internal services or the metadata endpoint. Looks like a knowingly-accepted v1 tradeoff for self-hosted internal MCP servers (hence major not blocker), but it needs a deploy-gated CIDR denylist (default-deny RFC1918 + metadata/link-local) before any shared-hosting/multi-tenant deployment.

[Jerry-Xin]

This PR is in scope for octo-marketplace, but it introduces a blocking storage security issue.

🔴 Blocking

• 🔴 Critical: Local storage proxy routes are unauthenticated and unsigned. router.go registers _storage routes on the root engine, outside the authenticated /api/v1 group, and handler.go writes any provided key directly to storage with no token, upload ownership check, expiry check, or body-size limit. handler.go also serves any known object key without auth. Since STORAGE_DRIVER=local is the default, this exposes arbitrary object overwrite/read and disk-filling attacks whenever the service is reachable. The local presigned URLs need an unguessable signed token with expiry and size enforcement, or the proxy must be mounted behind authorization and tied to a pending upload/download grant.

💬 Non-blocking

• 🟡 Warning: System MCP uniqueness is still race-prone. mcp.go checks duplicate system names/slugs before insert, but 20260714-05-mcp-uniqueness.sql cannot enforce space_id=NULL uniqueness. Two concurrent admin creates can both pass the pre-check and insert duplicates. Add a DB-enforced key using a generated non-null scope value, or serialize admin system creates/updates.

✅ Highlights

• go test ./..., go test -race ./..., and go vet ./... all pass.
• The MCP public routes consistently enforce Space-scoped visibility and return non-enumerating 404s for hidden records.

Superseded by re-posted review (removed local-path artifacts from the body).

[Jerry-Xin]

This PR is in scope for octo-marketplace, but it introduces a blocking storage security issue.

🔴 Blocking

• 🔴 Critical: Local storage proxy routes are unauthenticated and unsigned. internal/api/router/router.go:127 registers the _storage routes on the root engine, outside the authenticated /api/v1 group, and internal/api/handler/upload/handler.go:283 (localUploadProxy) writes any provided key directly to storage with no token, upload-ownership check, expiry check, or body-size limit. internal/api/handler/upload/handler.go:297 (localDownloadProxy) also serves any known object key without auth. Since STORAGE_DRIVER=local is the default, this exposes arbitrary object overwrite/read and disk-filling attacks whenever the service is reachable. The local presigned URLs need an unguessable signed token with expiry and size enforcement, or the proxy must be mounted behind authorization and tied to a pending upload/download grant.

💬 Non-blocking

• 🟡 Warning: System MCP uniqueness is still race-prone. internal/service/mcp.go:264 checks duplicate system names/slugs before insert, but migrations/sql/20260714-05-mcp-uniqueness.sql:31 cannot enforce space_id=NULL uniqueness. Two concurrent admin creates can both pass the pre-check and insert duplicates. Add a DB-enforced key using a generated non-null scope value, or serialize admin system creates/updates.
• 🟡 SSRF (documented v1 stance): POST /api/v1/mcps/probe fetches an attacker-supplied URL (internal/service/probe.go validateProbeURL) with only an http/https scheme check — no blocking of RFC1918 / link-local / cloud-metadata (169.254.169.254) targets. It is authenticated and response-bounded (4 MiB / 15s), and the code documents this as intentional for self-hosted internal MCP servers, but in a shared-hosting posture it is an SSRF vector. Add an opt-in egress allow/deny list for private ranges before multi-tenant hosting.
• 🟡 Deploy hardening: Dockerfile.api runs as root (no USER directive), and docker-compose.yaml ships dev DB creds (MYSQL_ROOT_PASSWORD: root). Fine for local dev, but add a non-root runtime user and keep production credentials out of committed compose files.

✅ Highlights

• Token auth is server-side throughout: identity/space resolved from the Octo token, ownership on skill update/delete enforced against the token identity (internal/service/skill/service.go:288,433), and MCP public routes enforce Space-scoped visibility with non-enumerating 404s.
• Admin gate uses crypto/subtle.ConstantTimeCompare and fails closed on an empty token in prod.
• SQL is parameterized: dynamic query assembly only joins static column/condition fragments; all user values ride ? placeholders.
• No real secrets committed — .env (real) is excluded, only .env.example with placeholders; the ghp_* strings are secret-redaction test fixtures. Local-storage safePath blocks absolute paths and .. traversal with a final base-dir prefix check.

Note: the real CI workflow (verify/test-race/vet/build + golangci-lint) has not produced any check run on this head yet; only the sentinel code-review status is present. Recommend confirming CI green before merge.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Request changes — blocking findings below (data-flow traced).

---

Code Review — PR #1 (octo-marketplace)

Reviewed at head 2794bd91. Scope: complete initial migration of the Octo Marketplace service — MCP catalog API, Skill catalog API, authentication, MySQL migrations, object storage, probe subsystem, Docker/CI, and project documentation. 118 files changed.

Summary

This PR establishes the entire Octo Marketplace service from scratch: a Go/Gin API server with MCP catalog CRUD (streamable-http + legacy SSE probe), Skill archive upload/parse/lifecycle, token-based auth (Octo tokens + bot tokens), admin routes with constant-time token comparison, S3-compatible and local object storage, and MySQL schema with soft-delete + generated-column uniqueness. The code is well-structured, heavily commented, and includes integration tests. One P1 blocker was found (unauthenticated local storage proxy), along with several P2 security notes appropriate for a security-sensitive automated review review.

Verification

• ✅ Diff scope — 118 files, all new (initial migration). Merge base 8399a4c6.
• ✅ Auth middleware correctness — internal/middleware/auth.go:56-97: both Handler() and WrapMarket() enforce token + space membership when AUTH_ENABLED=true; dev mode falls back to devIdentity. Bot token path (bf_ prefix) verified at :103-131 with owner-UID stamping and space scoping.
• ✅ Admin auth — internal/middleware/admin.go:71-84: crypto/subtle.ConstantTimeCompare used; empty token closes admin when auth enabled; dev mode bypass consistent.
• ✅ Secret redaction — internal/service/secret.go:31-56: token-like keys (matched by secretKeyPattern) are forced to empty or rejected if non-empty/non-sentinel. Applied on both Create (mcp.go:555-564) and Patch (mcp.go:656-667).
• ✅ SQL injection prevention — all queries use parameterized ? placeholders. escapeLike at internal/repository/mcp.go:262-268 neutralizes % and _ wildcards.
• ✅ Path traversal prevention — internal/storage/local.go:29-45: safePath rejects absolute paths, .. segments, and verifies resolved path stays under baseDir.
• ✅ Probe SSRF bounds — internal/service/probe.go:124-140: scheme restricted to http/https; 15s timeout; 4 MiB response cap; stdio rejected. SSE stream cleanup via goroutine + ctx cancel at :284-290.
• ✅ Soft-delete filtering — all read paths include deleted_at IS NULL predicate (repository/mcp.go:196-200).
• ✅ Visibility scoping — MCP buildWhere at repository/mcp.go:190-207: visibility = 'system' OR (space_id = ? AND (visibility = 'public' OR owner_uid = ?)). Skill canView at skill/service.go:338-349: fail-closed on unknown visibility values.
• ✅ Body size limits — MCP handler: 8 MiB body cap (handler/mcp.go:33); icon upload: 2 MiB (:149-154); skill archive: configurable via STORAGE_MAX_MB.
• ✅ Zip bomb protection — parse/zip.go:31-34: uncompressed size tracked with maxUncompressed limit; zip bomb ratio check at :62-65; symlink/absolute-path/back-traversal rejection at :86-99; file count cap at :76-79.

Static analysis only at head 2794bd91; build and tests not executed in this environment.

Findings

One P1 blocker described below; four P2 items; one nit.

P1 — Unauthenticated local storage proxy endpoints (internal/api/handler/upload/handler.go:283-306)

When STORAGE_DRIVER=local, the upload handler registers PUT /api/v1/_storage/upload/*key and GET /api/v1/_storage/download/*key directly on the raw Gin engine at internal/api/router/router.go:128-129 — outside the v1 group that carries authenticator.Handler() middleware. The proxy handlers (handler.go:284-306) perform no authentication check whatsoever.

Any network-reachable client can upload arbitrary files into the storage directory and download any stored file (including other users' proprietary skill archives). While safePath prevents path traversal, it does not gate access. There is no runtime enforcement that these routes are dev-only — setting AUTH_ENABLED=true does not suppress their registration; only STORAGE_DRIVER != "local" does.

A misconfigured production deployment (STORAGE_DRIVER=local + network exposure) creates a fully unauthenticated file upload/download surface.

Diff-scope: new (initial migration). Severity per R1: a working path (authenticated skill storage) becomes completely bypassable — user data exfiltration and malicious archive injection are both reachable.

Fix direction: either add auth middleware to the RegisterLocalProxy routes, or add a runtime guard that refuses to register them when AUTH_ENABLED=true.

P2 — Probe endpoint enables authenticated SSRF with no IP-range filtering (internal/service/probe.go:124-140)

validateProbeURL restricts schemes to http/https but explicitly skips private-CIDR/link-local filtering. The comment at :127-131 documents this as intentional for self-hosted deployments that point at RFC1918 MCP servers. However, in any multi-tenant or shared-hosting deployment, an authenticated user can use the probe to scan internal services, hit metadata endpoints (e.g. http://169.254.169.254/), or exfiltrate data through server-side requests.

Not a blocker for v1 self-hosted posture, but should be tracked for any shared deployment. Consider adding an opt-in PROBE_ALLOW_PRIVATE_CIDRS=false flag that enables RFC1918/loopback/link-local filtering when set.

P2 — CORS wildcard Access-Control-Allow-Origin: * on credential-bearing API (internal/api/router/router.go:163)

The CORS middleware sets Access-Control-Allow-Origin: * and Access-Control-Allow-Headers: Content-Type,Authorization,Token,X-Space-Id,X-Admin-Token,X-Request-Id. While Access-Control-Allow-Credentials is not set (so cookies won't be sent cross-origin), any website can make requests with custom Token or Authorization headers. If a user's token is exposed to a malicious page (via clipboard, shared localStorage, XSS on a sibling app), that page can call the full API.

Standard practice for token-based APIs, but worth tightening to specific origins in production. The X-Admin-Token header in Allow-Headers means admin token brute-force attempts can also originate cross-domain.

P2 — secretKeyPattern misses credential, auth, bearer, session keys (internal/service/secret.go:14)

The regex (?i)^(authorization|token|.*token|.*key|.*secret|password|pwd|api[-_]?key)$ does not match keys like credential, auth (only authorization), bearer, session, access, private, or cookie. If a user places a secret value under one of these keys in their env/headers config, it passes through unredacted and is persisted to the database.

The redaction is defense-in-depth (the primary contract is "never put real secrets in the create request"), but the gap weakens that defense. Consider broadening to include the specific terms or using a wider pattern like .*credential.*|.*auth.*|.*bearer.*|.*session.*.

P2 — canView for "public" skills requires same-space membership (internal/service/skill/service.go:341-342)

canView maps "public" visibility to row.SpaceID == spaceID, meaning a "public" skill is only visible within its own space — semantically equivalent to "space" visibility. This may be intentional (space-scoped public), but the naming is ambiguous: users creating a "public" skill might expect cross-space visibility. If the intent is same-space-only, consider renaming the visibility level to "space" or documenting the equivalence clearly in the API docs.

Nit — probeSession.client re-created per probe with no connection pooling (internal/service/probe.go:103)

Each Probe() call creates a fresh &http.Client{Timeout: probeTimeout}. For the probe use case (low frequency, create-wizard), this is fine — connection pooling would add complexity without measurable benefit. Noting for completeness only.

Human-verify

1. Production STORAGE_DRIVER enforcement — confirm that deployment configs (Docker Compose, K8s manifests, CI/CD) always set STORAGE_DRIVER=oss for production. The P1 finding is mitigated if local is never deployed in a network-accessible environment. This is not a merge blocker if operational controls are confirmed.

2. Probe SSRF posture — confirm whether the marketplace will be deployed in multi-tenant/shared-hosting mode in the near term. If yes, the probe's lack of IP-range filtering should be addressed before that deployment. This is not a change I'd block the initial migration on.

3. CORS origins — confirm the intended production CORS policy. If the marketplace will be consumed from specific web origins only, Access-Control-Allow-Origin: * should be narrowed before public exposure. This is not a merge blocker for the initial migration.

Things I checked that are fine

• Generated-column uniqueness — migrations/sql/20260714-02-mcp-uniqueness.sql uses name_live = IF(deleted_at IS NULL, name, NULL) stored generated column with UNIQUE index. Avoids the SELECT ... FOR UPDATE deadlock documented in the migration comment. Slug uniqueness follows the same pattern.
• System MCP dup-check — service/mcp.go:479-505 (checkSystemDupes) performs application-level name+slug uniqueness pre-check for system rows where MySQL NULL space_id defeats the UNIQUE index. Correctly called from both CreateSystem and UpdateSystem.
• Parse worker panic recovery — parse/worker.go:48-52: deferred recover() marks the task as failed instead of crashing the worker. Semaphore-bounded pool (5 goroutines) prevents unbounded concurrency.
• Bot token space isolation — auth.go:153-168: bot identity is stamped with the bot's single SpaceID; the X-Space-Id header is ignored for bot tokens, preventing space escalation.
• Soft-delete + uniqueness interaction — deleted rows have name_live = NULL, so MySQL UNIQUE permits many NULLs; the name is reusable after delete. Verified correct.
• Admin empty-token lockdown — admin.go:79-80: when a.token == "", all admin requests are rejected even if authEnabled=true. Empty config closes the door by design.
• Skill parse-task space isolation — skill/service.go:170-172: parse task SpaceID is checked against caller's SpaceID before consumption. Cross-space task hijacking is blocked.
• S3 SigV4 implementation — blob/s3.go:111-143: canonical request construction follows AWS spec; signing key derived correctly through date→region→service chain; payload hash uses sha256Hex(data) (not UNSIGNED-PAYLOAD).

Verdict: CHANGES_REQUESTED

The P1 (unauthenticated local storage proxy) must be addressed before merge. The simplest fix is adding auth middleware to the RegisterLocalProxy routes or suppressing their registration when AUTH_ENABLED=true. The P2 items are non-blocking security notes appropriate for follow-up before production deployment.

[yujiawei]

Code Review — PR #1 (octo-marketplace)

Reviewed at head 2794bd91e7359a4447b5e702e68447f536e12a5a, merge-base 8399a4c6 (initial commit — this is a greenfield service import: 118 files, +16,775). Classified security-sensitive. I deep-read the load-bearing surfaces (auth resolver chain + middleware, secret handling, storage/blob + download authz, the repository/SQL layer, the probe subsystem, config defaults, router wiring) and fanned out focused audits across authz, SSRF, archive-extraction, SQL/data-integrity, and infra/DoS, then adversarially re-verified each finding against the code at this head. Areas not exhaustively read are listed under Coverage.

Verdict: Request changes. One blocking issue (unauthenticated local-storage proxy, reachable in the shipped default configuration and not closed by turning auth on). The core service is well-built — the auth chain is fail-closed, the SQL layer is fully parameterized, secret redaction is a real protective control, and archive extraction is well-defended. Nothing here is unfixable; the blocker is a routing/gating fix plus a cluster of hardening items.

---

Blocking

🔴 Unauthenticated local-storage proxy (internal/api/handler/upload/handler.go:44-50, 283-306 + internal/api/router/router.go:129)

RegisterLocalProxy mounts PUT /api/v1/_storage/upload/*key and GET /api/v1/_storage/download/*key on the root Gin engine r, which carries only Logger/Recovery/CORS. authenticator.Handler() is applied to the v1 group only, so these two routes have no token check, no ownership check, no signed-key/expiry check — unlike every other handler in the file. localUploadProxy writes any client-supplied key straight to storage; localDownloadProxy returns any stored object by key.

Two properties make this blocking rather than a note:

• Reachable by default: STORAGE_DRIVER defaults to local (config.go, .env.example), so the proxy mounts unless an operator explicitly opts into oss.
• Not closed by auth: because it is on the root engine, it stays exposed even with AUTH_ENABLED=true. A production deployment that keeps the local driver leaks arbitrary object read/write (cross-tenant skill-archive exfiltration + malicious archive injection + disk-fill). Path traversal itself is contained by safePath (storage/local.go), but there is zero authorization.

Fix direction: mount these routes under the auth'd v1 group (or behind an unguessable signed token tied to a pending upload/download grant with expiry + size enforcement), and/or refuse to register them when AUTH_ENABLED=true. Also add a request-body size limit on localUploadProxy (currently io.Copy from c.Request.Body is unbounded).

---

Non-blocking (P2 — hardening / correctness)

Security posture

• SSRF via MCP probe (internal/service/probe.go validateProbeURL). POST /api/v1/mcps/probe (authenticated) issues outbound GET/POST to a caller-supplied URL; only the http/https scheme is enforced — RFC1918 / link-local / 169.254.169.254 (cloud-metadata) are deliberately not blocked, and the code documents this as an accepted v1 stance for self-hosted internal MCP servers. It is response-bounded (4 MiB) and time-bounded (15s). For a human to verify before any shared-hosting / multi-tenant deployment: add a deploy-gated egress denylist (at minimum metadata + link-local). Two amplifiers in the same subsystem: (a) the SSE endpoint event is resolved via base.ResolveReference and can redirect the follow-up POST to a different host (resolveEndpoint); (b) the probe http.Client uses the default redirect policy (no CheckRedirect), so a cooperating remote can 302 the request toward an internal target. User-supplied Headers are also forwarded to the target.
• Admin/dev auth posture (internal/middleware/admin.go, internal/middleware/auth.go, config.go). When AUTH_ENABLED=false (the default), both the user and admin middlewares stamp a synthetic identity and bypass token/role checks — intended for local dev, but there is no runtime guard that prevents this default from reaching production. ValidateAPI enforces OCTO_API_URL/ADMIN_OWNER_UID only when auth is already on. Consider a startup assertion that fails closed unless AUTH_ENABLED is explicitly set in a deployed environment.
• Deploy hardening. Dockerfile.api runs as root (no USER); docker-compose.yaml ships dev DB creds. Both are clearly labelled dev-only, but a non-root runtime user is cheap defense-in-depth.
• secretKeyPattern (internal/service/secret.go) misses credential, bearer, session, cookie, bare auth. Redaction is defense-in-depth (real secrets should never be in the create request), but the gap weakens it.
• CORS Access-Control-Allow-Origin: * with Token/X-Admin-Token in Allow-Headers (router.go). Safe today because Allow-Credentials is unset, but worth tightening to explicit origins in production.

Correctness / data-integrity

• UTF-8 byte truncation (internal/service/parse/worker.go sanitizeString, and readme[:1024*1024]; also frontmatter.go Description[:500]). description is validated to ≤1024 runes but then truncated to 1024 bytes — a multibyte (e.g. CJK) description can be split mid-rune, producing invalid UTF-8 that either fails the utf8mb4 insert or stores mojibake. Truncate on rune boundaries.
• Skill name uniqueness is advisory only. The skills table (migrations/sql/20260714-01-skill-marketplace.sql) has no UNIQUE key on (name, space_id, owner_id) — only non-unique indexes. checkNameDuplicate runs in the async worker, and skill.Create/Update let the client override p.Name after parsing, so the dedup is both racy (two concurrent creates) and bypassable (override the validated name). Add a DB-enforced unique key or re-check the final name in the create/update transaction.
• System-MCP uniqueness is similarly race-prone: the pre-insert check in service/mcp.go can’t be backed by the DB UNIQUE because space_id=NULL defeats it (MySQL treats NULLs as distinct). Two concurrent admin creates can both pass.
• Orphaned upload temp objects. After CopyObject relocates the archive to its final key (skill/service.go), the original pt.FileURL object is never deleted — unbounded storage growth over time.

Infra / DoS

• Unbounded goroutine fan-out (internal/service/parse/worker.go Submit): the goroutine is spawned before acquiring the w.sem semaphore, so a burst of triggers spawns N blocked goroutines rather than queueing. Combined with the absence of rate limiting on probe/upload/parse/icon, this is a DoS amplifier. Acquire the semaphore (or use a channel-fed fixed pool) before go.
• Cached-resolver eviction (internal/auth/cached_resolver.go): at capacity, every miss does an O(N) map scan under the lock and lacks a break, and there is no single-flight, so a burst of same-token misses stampedes the upstream auth service. (Negative results are correctly not cached — good.)
• S3 presigned PUT (internal/storage/oss.go PresignPut) carries no content-length-range, so a client can request an upload URL with a small declared file_size and PUT a much larger object directly to the bucket; the worker’s own read is bounded, but the oversized object persists.

---

Verified clean (the load-bearing surfaces)

• Auth chain — fail-closed throughout: nil resolver → 503, resolve error → 503, empty UID → 401, missing auth context → 503, X-Space-Id not in identity.Spaces → 403. Bot-token (bf_) path derives space from the resolver response, not from request headers. Dev bypass is correctly gated on !a.enabled.
• SQL layer — every caller value is bound via ?; the dynamic builders (skill/list.go, skill/update.go, mcp.go buildWhere) join only hardcoded column fragments; ORDER BY is fixed, LIMIT/OFFSET clamped; LIKE wildcards escaped (escapeLike).
• Secret handling — token-like keys are redacted at write time (a non-placeholder value rejects the request), the Authorization header is force-stripped, and secrets are never serialized back into a response DTO. No committed .env; the ghp_… strings are redaction-test fixtures.
• Archive extraction — zip-slip guards (absolute path, .., symlink) plus total-size and per-file SKILL.md LimitReader caps; checksum computed before use; stdio probing rejected (no arbitrary command execution).
• Ownership / visibility — skill Get/Update/Delete and MCP Patch/Delete/UploadIcon enforce owner + same-space; non-visible records return non-enumerating 404s; admin token uses crypto/subtle.ConstantTimeCompare and fails closed on an empty token in prod.

Coverage

Independent multi-lens audit + adversarial re-verification of each finding at this head; one cross-family advisory leg contributed (a second automated advisor was unavailable this run and is recorded as absent, not as tacit approval). Not exhaustively read: the full model/DTO layer, the category service internals, migration SQL correctness beyond the injection/uniqueness checks, and the MCP parse/worker pipeline beyond the SSRF/key/DoS surfaces. Real CI (verify/test-race/vet/build + golangci-lint) had not posted check runs on this head at review time — recommend confirming green before merge. An architectural human pass is warranted for an org-repo import of this size (the needs-human-review gate).

[github-actions]

Dependency Changes Detected

This PR modifies dependency files. Please review whether these changes are intentional.

Changed files:

• go.mod
• go.sum

Maintainer checklist:

• Confirm dependency changes are intentional
• Review package delta if lockfile changed

[OctoBoooot]

Review: feat: migrate octo marketplace service (#1) — delta @ 231375be

Verdict: Request changes — carries forward from 2794bd91. The large refactor in this commit (mux→gin groups, OpenAPI tooling) auth-gated the MCP routes — good — but left my blocker untouched: the _storage proxy is still mounted unauthenticated on the root engine. Verified at the bytes.

Delta: +1 commit, 45 files (mostly OpenAPI tooling + a routing refactor). I re-checked the two files my prior blocker/major lived in.

Still open — Blocker (unfixed)

• internal/api/router/router.go:129 + internal/api/handler/upload/handler.go:61-66 — unauthenticated local-storage proxy, unchanged. RegisterLocalProxy(r) is still called with the root engine r, and it registers PUT/GET /api/v1/_storage/upload|download/*key directly on r — outside the v1 := r.Group(...); v1.Use(authenticator.Handler()) chain (:66-67). The handlers (local_proxy.go localUploadProxy/localDownloadProxy) still perform no identity/token/signature check — arbitrary object write and read by key.
  • Notably, this same commit did fix the MCP surface — registerMCP/registerAdminMCP moved from a root-mounted mux to r.Group("/api/v1/mcps", authenticator.Handler()). So the auth-gating pattern is right there and applied elsewhere; the _storage proxy just wasn't moved with it.
  • Still reachable by default: STORAGE_DRIVER remains local by default (config.go:106), and being on the root engine it stays exposed even with AUTH_ENABLED=true.
  • Fix (unchanged): register the proxy under the authed v1 group (or gate it on identity + a signed key), and/or refuse to mount when AUTH_ENABLED=true. The MCP-route change in this very commit is the template.

Still open — Major (knowingly retained)

• internal/service/probe.go:154-160 — SSRF: validateProbeURL still enforces only http/https scheme, no RFC1918 / link-local / metadata (169.254.169.254) denylist. The updated comment reaffirms this is an intentional v1 tradeoff for self-hosted internal MCP servers — which is a defensible default for single-tenant self-hosting, but it must gain a deploy-gated CIDR denylist (default-deny RFC1918 + metadata) before any shared-hosting/multi-tenant deployment. Keeping at major.

Acknowledged in this delta

• MCP + admin-MCP routes are now properly behind authenticator.Handler() / adminAuth.Handler() gin groups (was a root-mounted http.ServeMux). Clean improvement.
• The prior minors (unsanitized filename in object key; MCP system-uniqueness TOCTOU that Jerry-Xin raised) — I did not re-verify each at this head given the empty-diff fetch; they remain open as previously filed unless the author addressed them. The blocker is the decisive item.

Note

• gh pr diff returned empty at this head (API/size hiccup), so I reviewed via per-file compare patches focused on the finding's files rather than the full diff — flagging that I did not re-scan all 45 changed files this round; the OpenAPI-tooling additions (tools/octo-api/*, docs/openapi/*) were not audited in depth.
• Real code-CI (build/test/vet/golangci-lint) still does not appear to have produced check runs — only check-sprint/pr-title-lint report. APPROVE would require those to run green regardless. needs-human-review remains.

[OctoBoooot]

[blocker] (carried from 2794bd9 — unfixed) Unauthenticated local-storage proxy. RegisterLocalProxy(r) is still called with the ROOT engine r, registering PUT/GET /api/v1/_storage/upload|download/*key (handler.go:65-66) outside the v1 auth group (v1.Use(authenticator.Handler()) at :67). The handlers (local_proxy.go) do no identity/token/signature check → arbitrary object write + read by key. STORAGE_DRIVER still defaults to local (config.go:106), and being on the root engine it's not closed by AUTH_ENABLED=true. Notably THIS commit fixed the MCP routes the right way — moved them into r.Group("/api/v1/mcps", authenticator.Handler()) — so the pattern is right here; the _storage proxy just wasn't moved with it. Fix: register the proxy under the authed v1 group (or gate on identity + a signed key), and/or refuse to mount when AUTH_ENABLED=true.

[OctoBoooot]

[major] (carried) SSRF: validateProbeURL still enforces only http/https scheme, no RFC1918/link-local/metadata (169.254.169.254) denylist — the comment (:156-157) reaffirms this as an intentional v1 tradeoff for self-hosted internal MCP servers. Defensible for single-tenant self-hosting, but needs a deploy-gated CIDR denylist (default-deny RFC1918 + metadata) before any shared-hosting/multi-tenant deployment. Keeping at major.

Superseded by re-review at head 231375b.

[Jerry-Xin]

Summary: The PR is relevant to octo-marketplace, and go test ./... passes, but the default local storage path exposes unauthenticated artifact read/write endpoints that bypass the service’s authorization model.

🔴 Blocking

• 🔴 Critical: Local storage upload/download proxy bypasses authentication and size controls. RegisterLocalProxy mounts /api/v1/_storage/upload/*key and /api/v1/_storage/download/*key directly on the root router, outside v1.Use(authenticator.Handler()), when STORAGE_DRIVER=local is selected. The proxy handlers then write or stream any valid key without checking caller identity, expiry, upload ownership, or requested size. This is especially risky because STORAGE_DRIVER defaults to local, docs present it as a normal backend, and Skill responses expose file_url, so an authorized viewer can learn the object key and later fetch it directly without auth. Uploads can also write unbounded request bodies to disk because WriteObject uses io.Copy without a limit. See router.go, local_proxy.go, local.go, config.go, and service.go. Fix by either not mounting these routes outside local/dev-only mode, or making local “presigned” URLs carry a server-validated, key-bound, expiring token and enforcing MAX_UPLOAD_MB in the proxy. Downloads should not be reachable by raw object key without the same auth decision as GET /skills/:id/download.

💬 Non-blocking

• 🟡 Warning: docs/api/mcp-v1.md still describes older MCP response shapes and error envelopes in several places, while the implementation/OpenAPI use the standard { "data": ... } / { "error": ... } envelopes and snake_case fields. If the OpenAPI contract is now authoritative, the markdown doc should be updated to avoid client integration drift.

✅ Highlights

• The MCP CRUD service keeps visibility checks out of handlers and has explicit cross-Space non-disclosure behavior.
• ZIP parsing validates traversal/symlink cases and avoids extracting arbitrary archive entries.
• Local verification run: go test ./... passed.

Re-posting with corrected file references.

[Jerry-Xin]

Summary: This PR is in scope for octo-marketplace and go test ./... passes, but the previously flagged blocker is unchanged: the default local-storage path still exposes unauthenticated artifact read/write endpoints that bypass the service's authorization model. Re-reviewed at head 231375b; the delta refactored the proxy handlers and MCP routes but did not fix the exposure.

🔴 Blocking (unchanged from prior review)

• 🔴 Critical: Local storage upload/download proxy bypasses authentication and size controls. RegisterLocalProxy (internal/api/handler/upload/handler.go:61) mounts PUT /api/v1/_storage/upload/*key and GET /api/v1/_storage/download/*key directly on the root gin engine at internal/api/router/router.go:129 (uploadH.RegisterLocalProxy(r)), OUTSIDE the authenticated group established at internal/api/router/router.go:66-67 (v1 := r.Group("/api/v1"); v1.Use(authenticator.Handler())). The proxy handlers localUploadProxy / localDownloadProxy (internal/api/handler/upload/local_proxy.go:12,24) take key straight from the URL param and call WriteObject / GetObject with NO middleware.Identity(c) check, no token, no signature, no expiry, and no upload-ownership check — a stark contrast to every other handler, which starts identity, ok := middleware.Identity(c). WriteObject (internal/storage/local.go:109) copies the request body with io.Copy and no size cap, so uploads are unbounded → disk-fill DoS. STORAGE_DRIVER still defaults to local (internal/config/config.go:106, .env.example:32) so this proxy is mounted BY DEFAULT, and because it is on the root engine it remains unauthenticated even when AUTH_ENABLED=true (which only gates the v1 group). Since Skill responses expose file_url, an authorized viewer can learn an object key and later fetch/overwrite it directly without auth.
  • Fix: either do not mount these routes outside a local/dev-only mode, or make local "presigned" URLs carry a server-validated, key-bound, expiring token; verify that token in the proxy handlers before any read/write; and enforce MAX_UPLOAD_MB on uploads. Downloads must not be reachable by raw object key without the same auth decision as GET /skills/:id/download.

💬 Non-blocking

• 🟡 MCP probe SSRF still unmitigated. validateProbeURL (internal/service/probe.go:154-176) only checks the scheme is http/https; it does not reject private / loopback / link-local (169.254.169.254 cloud-metadata) targets, and the probe follows the server-advertised endpoint event via resolveEndpoint. An authenticated caller can drive the server to fetch internal/metadata addresses. This delta only renamed JSON fields (ok→is_ok, serverInfo→server_info); the fetch-target restriction is unchanged. Recommend resolving the host and blocking non-public IP ranges before dialing (and re-validating after redirects / endpoint hand-off).
• 🟡 docs/api/mcp-v1.md still describes older MCP response shapes / error envelopes in places, while the implementation and OpenAPI use the standard { "data": ... } / { "error": ... } envelopes and snake_case fields. Update the doc to avoid client integration drift.

✅ Highlights

• Storage path handling is solid: safePath (internal/storage/local.go:27-45) rejects absolute paths, .. traversal, and any key escaping the base directory — so the earlier path-basename concern is mitigated at the storage layer.
• MCP CRUD keeps visibility checks out of handlers with explicit cross-Space non-disclosure behavior; ZIP parsing validates traversal/symlink cases.
• MCP/admin routes were cleanly migrated from http.ServeMux to gin groups under authenticator.Handler(), with /probe → /_probe deprecation headers.
• Local verification: go test ./... passed.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Request changes — blocking findings below (data-flow traced).

---

Code Review: octo-marketplace PR#1 — feat: migrate octo marketplace service

Reviewer: Octo-Q (automated review)
Head SHA: 231375be49b25ec247c2e674fe10155ccab65f2a
Base: main (8399a4c)
Diff scope: 138 files, +22,442 / −25 (initial service migration)
Complexity routing: security_sensitive → C1–C6 all checked

---

1. Validation Summary

Area	Status
SQL injection (parameterized queries)	✅ All queries use ? placeholders
Zip slip (archive extraction)	✅ validateZipEntry rejects traversal, symlinks, absolute paths
Secret redaction (token leakage)	✅ secretKeyPattern + applyPatch re-validation
Ownership + space isolation	✅ (owner_uid, space_id) scoping; non-owners get NotFound (not Forbidden)
Body size limits (MCP/JSON)	✅ MaxBytesReader 8MiB on MCP handlers
Pagination clamping	✅ limit 100 (MCP), 50 (skills)
Local storage safePath	✅ Multi-layer: rejects absolute, .., verifies resolved path under baseDir
Admin token comparison	✅ subtle.ConstantTimeCompare (timing-safe)
Error messages (no internal leakage in happy path)	✅ Generic "internal error" returned to clients
CI pipeline	✅ gofmt, vet, test -race, golangci-lint, build

---

2. Findings

P1 — Blockers (verdict: REQUEST_CHANGES)

---

F1: Unauthenticated local storage proxy endpoints (auth bypass → arbitrary file read/write)

Diff-scope: New (introduced by this PR)
Severity rationale (R1): Makes stored data corruptible by any network-reachable caller; production-reachable when STORAGE_DRIVER=local (the default).

Paths:

• internal/api/handler/upload/handler.go:64-66 — routes registered on root *gin.Engine, NOT on the v1 group with auth middleware
• internal/api/handler/upload/local_proxy.go:14-35 — no auth check, no body size limit
• internal/api/router/router.go:129 — uploadH.RegisterLocalProxy(r) passes the bare engine

Data flow:

HTTP PUT /api/v1/_storage/upload/*key  (no auth)
  → localObjectKey(c.Param("key"))      // strips leading "/" only
  → localStorage.WriteObject(key, body) // no MaxBytesReader
  → safePath(key)                       // prevents traversal, but writes within baseDir
  → os.Create(full_path)                // arbitrary file write within storage dir

Impact: When STORAGE_DRIVER=local (default), any unauthenticated caller can:

1. Overwrite any stored skill archive or icon (corrupting data for all users)
2. Read any stored file (exfiltrate skill source code before publication)
3. Fill disk (no body size limit on upload proxy)

The safePath() defense prevents escaping baseDir, but the complete lack of authentication makes this an open file read/write endpoint within the storage directory.

Recommendation: Either (a) register these routes on the v1 group with auth middleware, (b) gate them behind a GIN_MODE=debug check, or (c) add explicit auth middleware inline. Also add http.MaxBytesReader on the upload body.

---

F2: SSRF via probe endpoint — no network-level filtering

Diff-scope: New (introduced by this PR)
Severity rationale (R1 + C3): Any authenticated user can make the server issue HTTP requests to arbitrary internal endpoints, including cloud metadata. Authorization boundary violation: user authenticated for marketplace CRUD gains network probe capability.

Paths:

• internal/service/probe.go:162-175 — validateProbeURL checks scheme only
• internal/service/probe.go:166-167 — explicit comment: "Deeper SSRF filtering (private CIDRs, link-local) is intentionally NOT applied"
• internal/api/handler/mcp.go — POST /api/v1/mcps/_probe under auth middleware

Data flow:

HTTP POST /api/v1/mcps/_probe  {"url": "http://169.254.169.254/latest/meta-data/", "transport": "streamable-http"}
  → validateProbeURL(url)     // accepts http/https only
  → http.NewRequestWithContext(ctx, "POST", url, initBody)
  → client.Do(req)            // server makes request to user-supplied URL
  → response body parsed as JSON-RPC, tools returned to caller
  → on error: up to 512 bytes of response body returned in error snippet

Impact:

• Cloud metadata theft: http://169.254.169.254/latest/meta-data/ (AWS), http://metadata.google.internal/ (GCP), etc. Extract IAM credentials.
• Internal service scanning: Enumerate internal hosts, ports, and services. Error messages reveal DNS, TLS, and connectivity details.
• Header injection: User-supplied headers (req.Headers) are forwarded to the target (after sanitizedHeaders strips only the placeholder sentinel).

The code documents this as intentional for self-hosted v1 deployments, but this creates a critical attack surface in any multi-tenant or shared-hosting deployment.

Recommendation: Add an IP allowlist or deny private CIDRs (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16, 127.0.0.0/8, ::1). At minimum, block cloud metadata endpoints. If self-hosted deployments must reach RFC1918, gate behind an explicit config flag (PROBE_ALLOW_PRIVATE=true).

---

P2 — Should-fix (not blocking but high-signal)

---

F3: CORS Access-Control-Allow-Origin: * on authenticated API

Diff-scope: New
Path: internal/api/router/router.go:175-188

Allow-Headers includes Token, Authorization, and X-Admin-Token. While token-based auth (not cookie) means browsers won't auto-send credentials with *, any JS app that programmatically sets these headers will have its responses readable cross-origin. Compounds with F1: a malicious webpage can use the victim's browser to hit the unauthenticated local proxy endpoints.

Recommendation: Replace * with a configurable origin allowlist. At minimum, exclude admin routes from wildcard CORS.

---

F4: LIKE wildcard injection in skill search (parity gap with MCP search)

Diff-scope: New
Path: internal/repository/skill/list.go:84-91

User-supplied f.Query is wrapped as %{query}% without escaping MySQL LIKE metacharacters (%, _, \). The MCP repository at internal/repository/mcp.go:228-235 correctly implements escapeLike(). The skill repository does not.

Impact: q=% matches all rows. q=_ matches any single-character name. Enables data enumeration and expensive full-table LIKE scans.

Recommendation: Apply the same escapeLike() function used in the MCP repository.

---

F5: AUTH_ENABLED=false is the default — disables all auth including admin

Diff-scope: New
Path: internal/config/config.go:76 — AuthEnabled: envBool("AUTH_ENABLED", false)

The insecure path is the path of least resistance. ValidateAPI() only guards when AUTH_ENABLED=true AND AdminToken != "". No warning is emitted at startup beyond a log line. If deployed without explicitly setting AUTH_ENABLED=true, every endpoint (including admin) is open.

Recommendation: Either (a) default to true, (b) require explicit AUTH_ENABLED=false with a DEVMODE=true confirmation, or (c) fail startup in non-debug gin mode when auth is disabled.

---

F6: Internal error details leaked via parse task status

Diff-scope: New
Paths:

• internal/service/parse/worker.go:68-70 — UpdateFailed(ctx, taskID, "INTERNAL_ERROR", "download failed: "+err.Error())
• internal/service/parse/worker.go:73 — panic message stored verbatim
• internal/service/parse/service.go:182-184 — task.ErrorMessage returned to client

Internal errors containing S3 endpoint URLs, bucket names, DNS hostnames, and stack traces are persisted to parse_tasks.error_message and returned verbatim to any authenticated user via GET /uploads/:task_id/status.

Recommendation: Sanitize error messages before persisting: strip URLs, hostnames, and paths. Return a generic code to the client.

---

F7: Probe error messages leak internal network topology

Diff-scope: New
Path: internal/service/probe.go:163-174 — truncateErr(err.Error()) caps at 200 chars but includes DNS errors, TLS cert details, connection refused messages confirming host liveness.

Compounds with F2: the SSRF primitive returns network enumeration data in error responses.

Recommendation: Return only a generic probe-failure code; log the full error server-side.

---

F8: Unsanitized file_name in object key construction

Diff-scope: New
Paths:

• internal/service/parse/service.go:63-64 — fmt.Sprintf("skills/%s/%s", uploadID, fileName)
• internal/service/parse/service.go:113-114 — same pattern in InitReupload
• internal/service/parse/service.go:222 — icon upload

Only .zip / image extension is validated. No check for NUL bytes, path separators, control characters, or excessive length. For S3 (flat keys), .. is not traversal but creates oddly-named keys. For local storage, safePath() catches traversal. Neither backend sanitizes the name itself.

Recommendation: Strip path separators, reject NUL/control chars, cap length, and normalize the filename before embedding in the object key.

---

F9: Missing request body size limits on upload/skill/category handlers

Diff-scope: New
Paths:

• internal/api/handler/upload/handler.go:97-101 — ShouldBindJSON without MaxBytesReader
• internal/api/handler/skill/handler.go:215,296 — same pattern
• internal/api/handler/category/admin.go:74,109 — same pattern

Contrast with MCP handlers (internal/api/handler/mcp.go:374-376) which correctly apply http.MaxBytesReader. Attackers can send arbitrarily large JSON bodies to exhaust server memory.

Recommendation: Apply MaxBytesReader consistently before all ShouldBindJSON calls.

---

F10: Category hard-delete without referential integrity check

Diff-scope: New
Path: internal/repository/category/admin.go:23-29

Hard-deletes a category without checking if any skills reference it. The skills table has no FK constraint on category_id (migration 20260714-01). Deleting a category with associated skills orphans those rows.

Recommendation: Add FK constraint with ON DELETE RESTRICT, or check SkillCountByCategory before delete.

---

F11: Cached resolver full-cache flush bug (thundering herd)

Diff-scope: New
Path: internal/auth/cached_resolver.go:44-49

if r.maxSize > 0 && len(r.entries) >= r.maxSize {
    for key, entry := range r.entries {
        if now.After(entry.expiresAt) || len(r.entries) >= r.maxSize {
            delete(r.entries, key)
        }
    }
}

The len(r.entries) >= r.maxSize condition in the inner if causes all entries to be deleted when the cache is full, not just enough to make room. This triggers a thundering herd on the upstream auth service.

Recommendation: Evict only expired entries first; if still full, evict LRU entries one-by-one until under capacity.

---

F12: MCP Update WHERE clause lacks ownership scoping (defense-in-depth gap)

Diff-scope: New
Path: internal/repository/mcp.go:276-293

UPDATE mcp_servers SET ... WHERE id = ? AND deleted_at IS NULL — no owner_uid or space_id guard. The caller is documented as responsible for ownership verification, but a single missed check at the service layer becomes full privilege escalation.

Recommendation: Add AND owner_uid = ? AND space_id = ? to the WHERE clause as defense-in-depth.

---

F13: Skill visibility accepts arbitrary strings (no enum validation)

Diff-scope: New
Path: internal/service/skill/service.go:280-282 — toVisibility(v string) model.Visibility { return model.Visibility(v) }

Unlike MCP's validateClientVisibility() (restricting to public/private), skill visibility accepts any string. Unknown values default to "not visible" in canView(), creating invisible-to-everyone skills including the owner — silent data loss.

Recommendation: Validate against an enum (public, space, private) at the service boundary.

---

F14: Negative file_size accepted and persisted

Diff-scope: New
Path: internal/api/handler/upload/handler.go:70-73 + internal/service/parse/service.go:63-65

binding:"required" on int64 rejects zero but not negative values. fileSize > maxBytes passes for negative sizes. Corrupts data integrity.

Recommendation: Add fileSize > 0 check.

---

F15: Admin MCP handler missing handler-layer auth on Get/Patch/Delete/Probe

Diff-scope: New
Path: internal/api/handler/admin_mcp.go:83-157

Only Create calls callerFromContext(). The other four methods rely solely on middleware. If middleware is ever misconfigured or removed, these endpoints become unprotected.

Recommendation: Add callerFromContext(c) as defense-in-depth in all admin handler methods.

---

F16: Error envelope key mismatch between auth paths

Diff-scope: New
Paths:

• internal/middleware/auth.go:218 — writeMarketError uses key "error"
• internal/middleware/admin.go:113 — writeAdminError uses key "err"

Clients expecting one format will fail to parse the other, potentially masking auth failures in monitoring.

Recommendation: Unify the error envelope key.

---

F17: WrapMarket ignores bot tokens (inconsistency with Handler)

Diff-scope: New
Path: internal/middleware/auth.go:112-147

WrapMarket (net/http variant) only calls a.resolver.Resolve() and never checks for bf_-prefixed tokens. Handler() (Gin variant) explicitly branches on bot tokens. Currently unused in routing, but a future caller would silently break bot auth.

Recommendation: Add bot token handling to WrapMarket or document it as Gin-only.

---

F18: Dockerfile runs as root

Diff-scope: New
Path: Dockerfile.api — no USER directive

Container process runs as root. If the container is compromised (e.g., via SSRF or file write), the attacker has root within the container.

Recommendation: Add USER nonroot:nonroot and run as unprivileged user.

---

3. Data Flow Tracing

Consumer	Upstream Source	Data Actually Flows?	Notes
safePath(key) in local proxy	URL wildcard /*key → localObjectKey() (strip /)	✅ Yes	Traversal blocked by .. check + resolved path prefix check
objectKey in InitUpload	JSON body file_name → extension check only	✅ Yes	Path separators, NUL, control chars NOT stripped; safePath catches for local, S3 flat keys
Probe HTTP request target	JSON body url → validateProbeURL (scheme check)	✅ Yes	No CIDR/host filtering; reaches cloud metadata, internal hosts
identity.Role in category admin	Upstream Octo API /v1/auth/verify JSON response	✅ Yes	Admin auth fully delegated to upstream; no local verification
task.ErrorMessage in parse status	worker.go error strings (including err.Error())	✅ Yes	Internal details (S3 URLs, hostnames) returned to client
LIKE search term in skill list	Query param q → "%" + q + "%"	✅ Yes	LIKE metacharacters not escaped; MCP repo escapes correctly (parity gap)
Admin token comparison	Header X-Admin-Token → subtle.ConstantTimeCompare	✅ Yes	Timing-safe; empty token correctly rejects
visibility in skill create	JSON body → toVisibility() raw cast	✅ Yes	Any string accepted; unknown values → invisible to all

---

4. Blindspot Checklist (C1–C6) — security_sensitive PR, all items required

Check	Status	Evidence
C1 — Dual-path parity	⚠️ HIT	Handler() vs WrapMarket() bot token handling diverges (F17). MCP repo has escapeLike(), skill repo does not (F4).
C2 — Control-flow ordering / nested reuse	⚠️ HIT	safePath() is correctly applied in local storage but NOT invoked in the presign-put path for object keys (InitUpload constructs key before presign). Cached resolver eviction runs inside the write lock, correct ordering.
C3 — Authorization boundary ≠ capability boundary	🔴 HIT (P1)	Probe endpoint (F2): authenticated user gains network probe capability beyond marketplace CRUD scope.
C4 — Authorization lifecycle / container-member cascade	CLEAR	Admin token is a flat shared secret, not scoped to containers. No parent-child entity relationships in this service.
C5 — Build/note ≠ runtime correctness	⚠️ HIT	Dockerfile runs as root (F18). .env.example contains dev-admin-token — if copy-pasted to production, admin token is publicly known.
C6 — Governance/policy self-consistency	CLEAR	No governance/policy docs in this PR beyond standard README/AGENTS.md. No conflicting security policies.

---

5. Cross-Round Blocker Review (R6)

N/A — First review of this PR.

---

6. Positive Observations

• Parameterized SQL throughout: All repository queries use ? placeholders. No string interpolation of user input into SQL.
• Zip slip protection: validateZipEntry in parse/zip.go correctly rejects absolute paths, .., and symlinks.
• Secret redaction: secretKeyPattern + applyPatch re-validation prevents token leakage in env/headers fields.
• Ownership isolation: Both MCP and skill services enforce (owner_uid, space_id) scoping. Non-owners see NotFound (not Forbidden), preventing enumeration.
• Transactional parse task consumption: CreateSkillAndConsumeTask uses DB transaction with conditional UPDATE ... WHERE status = 'success' to prevent double-consumption.
• Timing-safe admin token comparison: Uses crypto/subtle.ConstantTimeCompare.
• Test coverage: Integration tests (integration_test.go, 759 lines), migration tests, model tests, and handler tests included.

---

7. Recommendations

1. F1 (P1): Add auth middleware to local proxy routes or gate behind GIN_MODE=debug.
2. F2 (P1): Add private CIDR deny list to probe URL validation; block cloud metadata endpoints.
3. F3 (P2): Replace CORS * with configurable origin allowlist.
4. F4 (P2): Apply escapeLike() to skill search (copy from MCP repo).
5. F5 (P2): Default AUTH_ENABLED=true or require explicit dev-mode confirmation.
6. F6/F7 (P2): Sanitize error messages before persisting/returning to clients.
7. F8 (P2): Sanitize fileName: strip path separators, reject NUL/control chars, cap length.
8. F9 (P2): Apply MaxBytesReader before all ShouldBindJSON calls.
9. F11 (P2): Fix cache eviction to not flush entire cache on capacity.
10. F18 (P2): Run container as non-root user.

---

[Octo-Q] verdict: REQUEST_CHANGES

Two P1 blockers: (1) unauthenticated local storage proxy enables arbitrary file read/write when STORAGE_DRIVER=local (the default), and (2) SSRF via probe endpoint allows any authenticated user to reach cloud metadata and internal services. Both are new, production-reachable, and satisfy R1 ("makes a working path produce wrong data or become unavailable"). Per R4, P1 → REQUEST_CHANGES.

16 additional P2 findings identified; most are defense-in-depth gaps and input validation omissions that should be addressed before production deployment.

[OctoBoooot]

Review: feat: migrate octo marketplace service (#1) — delta @ d5ed7d44 — storage-proxy blocker RESOLVED; SSRF major remains

Verdict: Request changes — but the character has shifted: my prior blocker (unauthenticated storage proxy) is resolved for the production case, downgraded to a minor. The remaining blocking item is the pre-existing SSRF major. Verified at the bytes.

Delta from 231375be49b2: +3 commits touching the proxy/storage files + OpenAPI.

Prior blocker (unauth storage proxy) — RESOLVED → minor ✅

• RegisterLocalProxy(r, authEnabled) (handler.go:67-72) now returns early — does not mount the proxy — when authEnabled is true. So the case that made this a blocker (an unauthenticated _storage read/write proxy reachable in a production posture with AUTH_ENABLED=true, because it sat on the root engine outside the v1 auth group) is closed: auth-on and the proxy are now mutually exclusive. This is exactly the authEnabled-gate fix I offered — credit.
• The disk-fill minor is also fixed: localUploadProxy now wraps the body in http.MaxBytesReader (local_proxy.go) → 413 on oversize.
• Residual (minor, not blocker): with AUTH_ENABLED=false (default) + STORAGE_DRIVER=local (default), the proxy still mounts unauthenticated. But with auth off the entire API already runs as a synthetic dev identity with no auth (config.go:24,29), so an unauth storage proxy in that mode is consistent with the explicitly-dev posture, not an added exposure. Harden if you like (document that auth-off is dev-only, or gate the default), but it's no longer a production-reachable hole.

Still open — Major (unchanged)

• internal/service/probe.go:154-160 — MCP-probe SSRF: validateProbeURL still enforces only http/https scheme, no RFC1918 / link-local / metadata (169.254.169.254) denylist (probe.go is not touched in this delta; the comment reaffirms the intentional-v1 tradeoff). Defensible for single-tenant self-hosting, but needs a deploy-gated CIDR denylist (default-deny RFC1918 + metadata) before any shared-hosting/multi-tenant deployment. This is the remaining item holding REQUEST_CHANGES.

Note on peer grading

mochashanyao's CR at this head grades the storage proxy still-🔴 — I judge it on the bytes: the authEnabled gate (handler.go:68) genuinely closes the production case, so it's no longer a blocker. Her SSRF 🔴 and mine (major) converge on "must-harden"; the size-limit 🟡 she raised is already fixed here. We agree the SSRF needs a denylist; we differ on whether the now-gated proxy is still blocker-tier — the gate is real, so I downgrade.

Still not re-audited

Per the prior delta, I focused on the finding's files (proxy/router/probe); the OpenAPI tooling and the broader service weren't re-scanned this round. Real code-CI (build/test/vet/golangci) still hasn't produced check runs — APPROVE would require those green regardless. needs-human-review remains.

[OctoBoooot]

[major] MCP-probe SSRF (unchanged this delta — the remaining item holding REQUEST_CHANGES). validateProbeURL enforces only http/https scheme, no RFC1918 / link-local / metadata (169.254.169.254) denylist; the comment reaffirms the intentional-v1 tradeoff. Defensible for single-tenant self-hosting, but an authenticated tenant can make the server reach internal services / cloud metadata. Needs a deploy-gated CIDR denylist (default-deny RFC1918 + metadata) before any shared-hosting/multi-tenant deployment.

[yujiawei]

Code Review — PR #1 (octo-marketplace)

Reviewed at head SHA 231375be49b25ec247c2e674fe10155ccab65f2a against merge-base 8399a4c (2 commits, 138 files, +22,442). go build ./... and go test ./... both pass locally. This is a security-sensitive migration (auth / middleware / token / storage), so the review is weighted toward the trust boundaries.

Overall this is a well-structured service — clear layering (handler → service → repo), thoughtful visibility model, DB-level uniqueness via generated columns, constant-time admin-token compare, zip-slip/symlink guards, and body-size caps. The issues below are concentrated at a few boundaries and are worth fixing before this ships in a shared environment.

Verdict: Changes Requested. Blocking items are P1-①/②/③/④.

---

Blocking (P1)

① Local storage proxy is mounted outside the auth group

internal/api/router/router.go:129 registers the proxy on the root engine, not the authenticated v1 group:

uploadH.RegisterLocalProxy(r)   // r = root *gin.Engine

// internal/api/handler/upload/handler.go:65-66
r.PUT("/api/v1/_storage/upload/*key", h.localUploadProxy)
r.GET("/api/v1/_storage/download/*key", h.localDownloadProxy)

v1.Use(authenticator.Handler()) (router.go:67) only guards the v1 group, so these two routes have no authentication at all — and the bypass is independent of AUTH_ENABLED. It fires whenever STORAGE_DRIVER=local, which is the default (config.go, .env.example:32). The code comment calls this "development-only," but nothing prevents a local + AUTH_ENABLED=true deployment, in which case anyone can PUT (overwrite) and GET arbitrary storage objects by key. safePath() confines access to the storage dir (no traversal), but every object inside it is exposed.

Please either mount these under the authenticated group, add an explicit auth check in the proxy handlers, or hard-gate registration so they can never appear in a non-dev build. See also ④, which this amplifies.

② SkillItem leaks internal fields (owner/space/object-key) on the wire

internal/service/skill/service.go:47-67 serializes owner_id, space_id, file_url, and file_sha256 directly, and rowToItem() (line 463) applies no filtering. file_url is the raw object key (skills/{id}/v{ver}/{file}), returned to every viewer of a public/space skill. Contrast the MCP DTO, which deliberately hides owner_uid (internal/model/mcp_dto.go).

On its own this is information disclosure. Combined with ①, it becomes a read-any-file primitive: a leaked object key + the unauthenticated GET /api/v1/_storage/download/{key} proxy = fetch another tenant's skill archive without going through the authorized GET /skills/{id}/download path. Recommend dropping owner_id/space_id/file_url/file_sha256 from the read DTO (or gating them to the owner) and never returning raw object keys.

③ SSRF surface in the MCP probe path

internal/service/probe.go:154-178 (validateProbeURL) validates only the URL scheme and explicitly documents that private-CIDR / link-local filtering is omitted:

// Deeper SSRF filtering (private CIDRs, link-local) is intentionally NOT applied in v1

The probe endpoint is authenticated but open to any logged-in user, and it makes a server-side request to the user-supplied URL. Additional amplifiers:

• The http.Client sets only Timeout, so it follows redirects (Go default ≤10) — a public URL can 302 into 169.254.169.254 or an RFC1918 host.
• Upstream response snippets (up to 512 bytes) are echoed back in error messages (probe.go:366-368, 596-597, 642-645), turning failed probes into an information-disclosure channel.

This is acceptable for a single-tenant self-hosted deployment (the stated intent) but is a real SSRF-to-metadata risk in any multi-tenant / shared-hosting posture. This one needs a human decision on the deployment model. If multi-tenant is possible, add a private/link-local/loopback denylist (resolve-then-check to avoid DNS-rebind), disable redirects, and stop echoing response bodies.

④ Two divergent admin models; the role-based one is fragile

There are two independent admin authorizations:

• internal/middleware/admin.go — X-Admin-Token with crypto/subtle constant-time compare (sound).
• internal/api/handler/category/admin.go:38 — if identity.Role != "admin".

model.Identity.Role is only ever populated by unmarshaling the upstream verify response (internal/auth/resolver.go:55); it is never set for the dev identity (main.go:77) or bot identity (middleware/auth.go:166-171). So in production the skill-category admin surface is either (a) entirely unreachable if upstream omits role, or (b) only as trustworthy as the upstream role field. Two admin models with different threat assumptions on the same service is worth reconciling; at minimum, please confirm the upstream contract guarantees role and add a prod-path test (the current tests only hardcode Role:"admin" in dev mode).

---

Non-blocking (P2 / nits)

• Secret-redaction regex gaps — internal/service/secret.go:13 misses credential/credentials, passwd, bare auth, cookie. Low impact for stored records, but consider widening. Separately, sanitizedHeaders() in probe.go:183 forwards real user-supplied Authorization/token headers verbatim to the probe target (only the sentinel is stripped). Documented, but it means a user who pastes a real token instead of the sentinel leaks it to the remote host — a validation warning would help.
• Dead error-envelope inconsistency — writeMarketError (middleware/auth.go:270) emits key "error" while writeAdminError and docs/api/mcp-v1.md §2 use "err". Currently latent: WrapMarket is unwired (routes use authenticator.Handler()). Fix or delete the dead path before it gets wired.
• Defense-in-depth in create txn — CreateSkillAndConsumeTask (internal/repository/skill/create.go:82) consumes the parse task with WHERE id=? AND status='success' only, whereas the update path also re-checks owner_id/space_id/skill_id (parse_task.go:85). Not exploitable (parse-task attributes are immutable and the service layer already checks), but the asymmetry is worth closing.
• fileName flows into object keys unsanitized — parse/service.go:69,119,290 build keys as skills/{id}/{fileName} / icons/{id}/{fileName} after only an extension check. Harmless for the local driver (safePath blocks ..) and for flat S3 keys, but sanitizing to filepath.Base would remove the smell.
• CORS Access-Control-Allow-Origin: * (router.go:182) alongside allowing the Authorization/Token headers. Since auth is header-based (no cookies), there's no ambient-credential CSRF, so this is informational — just flagging it for the security-sensitive context.

---

What I could not fully verify (coverage note)

• Runtime behavior of the upstream octo-server /v1/auth/verify contract (whether role, spaces, context_included are always populated) — reviewed only the client side. ④ depends on this.
• Actual object-storage/CDN ACLs behind the OSS driver (whether object keys alone grant access) — reviewed only the Go signing/URL construction.
• No live/integration run against a real MySQL + OSS; conclusions on migrations and storage are from static reading + the unit/integration suite (which passes).
• The generated docs/openapi/swagger.yaml (2,681 lines) and the tools/octo-api toolchain assets were skimmed, not line-audited.

Superseded: re-reviewed at d5ed7d4; the unauthenticated local storage proxy blocker is resolved (proxy not mounted when AUTH_ENABLED=true; size limit added).

[Jerry-Xin]

Re-reviewed at head d5ed7d44 (delta 231375be..d5ed7d44: 3 commits ahead, 0 behind — linear, no rebase drift). The standing blocker is resolved and no new blocker was introduced. Approving.

✅ Blocker resolved — unauthenticated local storage proxy

Prior review flagged the _storage upload/download proxy as an unauthenticated read/write surface mounted on the root engine, live even under AUTH_ENABLED=true, with no size cap. The fix commit d5ed7d44 closes this:

• RegisterLocalProxy (internal/api/handler/upload/handler.go:67-69) now early-returns and mounts no routes when localStorage == nil OR authEnabled[0] == true. Router wiring passes authenticator.AuthEnabled() (router.go:48 → :134), so under AUTH_ENABLED=true the proxy is not registered at all.
• AUTH_ENABLED=true is the documented production posture (CONFIGURATION.md: "Production deployments must set AUTH_ENABLED=true"); AUTH_ENABLED=false is the local/dev mode (DEV_AUTH_UID / dev-space fallbacks). The residual unauth proxy therefore exists only in explicitly auth-disabled dev, and is now size-limited via http.MaxBytesReader → 413 FileTooLarge (local_proxy.go). Path traversal remains guarded by safePath.
• Covered by tests: TestLocalProxyNotMountedWhenAuthEnabled (404 when auth on) and TestLocalUploadProxyRejectsOversizedBody (413). go build ./... and the touched package tests pass.

💬 Non-blocking

• 🟡 MCP-probe SSRF: validateProbeURL (internal/service/probe.go:160) still validates only the URL scheme (http/https) with no private-CIDR / link-local / metadata-endpoint denylist. This is unchanged in this delta and carries an explicit, reasonable v1 tradeoff comment (self-hosted deployments legitimately target internal RFC1918 MCP servers). Acceptable for the current single-tenant/self-hosted posture; add CIDR filtering before any shared-hosting deployment.
• Minor: LocalStorage.PublicURL returns an /api/v1/_storage/download/... URL, but that route is only mounted when AUTH_ENABLED=false. A local driver + AUTH_ENABLED=true combination would hand back a URL to an unmounted route. This is a non-security dev-config edge (prod uses the OSS driver); worth a doc note or guard eventually.

✅ Highlights

• Clean auth-gating with direct test coverage; size limit + 413 path added.
• New MCP-icon presigned upload (mcp_icon.go / InitMcpIconUpload) enforces a 2 MiB cap and an image-type allowlist, uses a separate mcp-icons/ key prefix, and sits under the authenticated /api/v1 group with @Security Bearer.
• PublicURL added consistently across the Storage interface, local, and OSS drivers; OSS variant errors safely when OSS_PUBLIC_ENDPOINT is unset.

[OctoBoooot]

Review: feat: migrate octo marketplace service (#1) — delta @ 9b510d6f

Verdict: Request changes — carried forward. This commit is a single-file JSON field rename (id → skill_id / skill_version_id in the skill DTOs) and does not touch my remaining finding. The SSRF major is still open (probe.go byte-unchanged). Verified.

Delta from d5ed7d44: +1 commit, only internal/service/skill/service.go.

Still open — Major (unchanged)

• internal/service/probe.go:160 — MCP-probe SSRF: validateProbeURL still enforces only http/https scheme, no RFC1918 / link-local / metadata (169.254.169.254) denylist (byte-confirmed unchanged this delta; the comment still reaffirms the intentional-v1 tradeoff). An authenticated tenant can make the server reach internal services / cloud metadata. Defensible for single-tenant self-hosting, but needs a deploy-gated CIDR denylist (default-deny RFC1918 + metadata) before any shared-hosting/multi-tenant deployment. This is the item holding REQUEST_CHANGES. (Severity note: I grade this major, not blocker — a documented single-tenant tradeoff — a reviewer split with the APPROVE side who grade it 🟡; it holds my CR either way.)

Still open — Minor (unchanged)

• internal/service/skill/service.go SkillItem still exposes file_url / file_sha256 / owner_uid. Not a cross-tenant leak (List is space+visibility-scoped, so only authorized items; file access via the dev-only proxy or prod OSS signed URLs), so minor hardening — the field rename in this commit didn't remove them.

This delta (acknowledged)

• id → skill_id / skill_version_id rename in SkillItem/SkillVersion DTOs, reflected in swagger.yaml (consistent, not a partial rename). A breaking wire-change for existing clients, but this is a pre-GA service migration with no prior consumers, so it's an intentional API-shaping choice, not a defect. Unrelated to the open findings.

Note

• The remaining path to APPROVE is narrow and unchanged: add the probe CIDR denylist (my major), optionally trim the SkillItem fields, and get real code-CI green. probe.go was not addressed this commit. needs-human-review remains — and note there's a live reviewer severity split on the SSRF (major/CR vs 🟡/APPROVE) that the human gate should arbitrate.

[OctoBoooot]

[major] MCP-probe SSRF, still open (probe.go byte-unchanged this delta). validateProbeURL enforces only http/https scheme — no RFC1918 / link-local / metadata (169.254.169.254) denylist; the comment reaffirms the intentional-v1 tradeoff. An authenticated tenant can make the server reach internal services / cloud metadata. Defensible for single-tenant self-hosting, but needs a deploy-gated CIDR denylist (default-deny RFC1918 + metadata) before shared-hosting/multi-tenant. Holds REQUEST_CHANGES. (I grade major, not blocker — documented single-tenant tradeoff; there's a live reviewer split with the 🟡/APPROVE side for the human gate to arbitrate.)

[OctoBoooot]

[minor] SkillItem still exposes file_url / file_sha256 / owner_uid. Not a cross-tenant leak — List is space+visibility-scoped (authorized items only), and file access is via the dev-only proxy (AUTH_ENABLED=false) or prod OSS signed URLs — so this is hardening, not disclosure. This commit's id→skill_id rename didn't remove these; consider trimming file_sha256 / raw object key from the list response.

[yujiawei]

Code Review — PR #1 (octo-marketplace) — re-review at 9b510d6f

Head has advanced since the first pass (231375be → 9b510d6f, +4 commits). Re-reviewed against the current head; earlier approvals do not carry forward. go build ./... and go test ./... both pass at 9b510d6f.

What the new commits changed

• d5ed7d4 — local storage proxy is now skipped when AUTH_ENABLED=true, and the upload proxy gained a MaxBytesReader. This resolves the most serious first-pass blocker for the production threat model.
• 84cb032 / 96cb845 — new presigned MCP icon-upload endpoint (/api/v1/mcp_icon_uploads + admin twin) and a Storage.PublicURL method.
• 9b510d6 — response IDs renamed to skill_id / skill_version_id.

Verdict: Changes Requested. The two data-plane blockers below remain unaddressed at this head.

---

Blocking (P1)

① SSRF surface in the MCP probe path — unchanged

internal/service/probe.go:154-178 still validates scheme only, with the explicit "private CIDRs, link-local … intentionally NOT applied" comment. The http.Client (line ~107) sets only Timeout, so it follows redirects (a public URL can 302 into 169.254.169.254 / RFC1918), and upstream response snippets (up to 512B) are echoed back in errors (probe.go:366-368, 596-597, 642-645). Any authenticated user can drive server-side requests to internal targets. Acceptable for single-tenant self-host (the stated intent), a real metadata-SSRF risk in any multi-tenant/shared posture — this needs an explicit human call on the deployment model. If multi-tenant is possible: deny private/loopback/link-local (resolve-then-check to avoid DNS rebind), disable redirects, stop echoing bodies, optionally gate RFC1918 behind PROBE_ALLOW_PRIVATE=true.

② SkillItem leaks internal fields on the wire — unchanged

internal/service/skill/service.go:47-67 still serializes owner_id, space_id, file_url (raw object key skills/{id}/v{ver}/{file}), and file_sha256 to every viewer of a public/space skill; rowToItem applies no filtering. Contrast the MCP DTO, which deliberately hides owner_uid. With STORAGE_DRIVER=local and AUTH_ENABLED=false (the shipped default), the download proxy is still mounted, so a leaked object key + GET /api/v1/_storage/download/{key} = fetch another member's archive outside the authorized download path. Recommend dropping owner_id/space_id/file_url/file_sha256 from the read DTO (or gating to owner) and never returning raw object keys. (The id→skill_id rename in 9b510d6 did not touch these fields.)

---

Confirmed secondary issues (P2 — not blocking, worth fixing)

• LIKE-wildcard not escaped in skill search — internal/repository/skill/list.go:85 builds "%" + f.Query + "%" with no escaping, while the MCP repo has escapeLike() (mcp.go:289, :303). Real parity gap: q=%/q=_ enable enumeration + full-table scans.
• Skill visibility accepts arbitrary strings — service.go:440 toVisibility() is a raw cast (no enum check), unlike MCP's validateClientVisibility. An unknown value is invisible to everyone incl. the owner (silent data loss). Validate public/space/private at the boundary.
• Negative file_size accepted — parse/service.go:64 checks only fileSize > maxBytes; binding:"required" rejects 0 but not negatives. Add fileSize > 0. (The new MCP icon endpoint already uses binding:"required,gt=0" — apply the same to skill/upload init.)
• Internal error strings persisted + returned — parse/worker.go:68-73 stores err.Error() (S3 endpoint/host/paths) into parse_tasks.error_message, surfaced to the client via poll (service.go:182). Sanitize before persist; keep a generic client code.
• fileName embedded in object keys unsanitized — extension-only check in parse/service.go (skill upload, icon) and in the new InitMcpIconUpload (mcp-icons/{uuid}/{fileName}). Harmless under current drivers (safePath blocks .. locally; flat S3 keys) but strip separators/NUL/control chars and cap length.
• MCP Update/Delete WHERE lacks owner/space scoping — mcp.go update() uses WHERE id=? AND deleted_at IS NULL; ownership is enforced one layer up in the service. Add AND owner_uid=? AND space_id=? as defense-in-depth so a single missed service check isn't privilege escalation.
• Container runs as root — Dockerfile.api has no USER. Add a non-root user; matters more given ①.
• Admin model divergence — X-Admin-Token (constant-time, sound) vs identity.Role=="admin" for skill categories (category/admin.go:38), where Role is only ever populated from the upstream verify JSON (never for dev/bot identities). Confirm the upstream contract guarantees role, and add a prod-path test.
• Dead error-envelope mismatch — writeMarketError emits "error", writeAdminError/doc §2 use "err"; WrapMarket is currently unwired. Fix or delete before it's mounted.

Refuted / overstated from the automated first pass

• Cache "full-flush" thundering herd — refuted. cached_resolver.go:49 re-evaluates len(r.entries) >= r.maxSize each loop iteration, so once one entry is deleted the condition drops and only expired entries are removed thereafter. It evicts one arbitrary live entry + expired, not the whole cache.
• Category hard-delete orphans skills — overstated. No DB FK exists, but the service layer (service/category/admin.go:53) calls SkillCountByCategory and returns CONFLICT/ErrCategoryInUse before deleting. An FK with ON DELETE RESTRICT would still be good defense-in-depth.

---

Coverage note (could not verify)

• Upstream octo-server /v1/auth/verify contract (whether role/spaces/context_included are populated) — client side only; ② depends on it.
• Live OSS/CDN bucket ACLs for the new PublicURL path (whether anonymous GET is actually open) — static read only.
• No live MySQL+OSS integration run; conclusions from static reading + the passing unit/integration suite.
• Generated docs/openapi/swagger.yaml and tools/octo-api assets skimmed, not line-audited.

[kense-lab]

Sprint check is now linked to #3, but the issue is not yet assigned to the private Octo Board Sprint.

Maintainer action required: please add #3 to https://github.com/orgs/Mininglamp-OSS/projects/2 and set Sprint to Sprint W29, then re-run the Check Sprint workflow.

The contributor token has project scope but cannot resolve private Project 2.

Superseded by re-review at head 9b510d6.

[yujiawei]

Code Review — PR #1 (octo-marketplace)

Reviewed at head SHA 3923f2265ff755fa06415ba8895cd1ea76b464f3. This is a large greenfield migration (146 files, ~24k additions) of the Skill & MCP catalog marketplace service (Gin + MySQL + S3/local object storage). Classified security-sensitive; reviewed accordingly, with data flow traced handler → service → repository and each finding independently verified against the code.

Overall the codebase is well-engineered and shows evidence of prior security hardening: go build ./... and go test ./... both pass locally; CI runs go test -race -shuffle, go vet, gofmt, and golangci-lint. SSRF probe protection (same-origin SSE + dialer-level IP re-resolution), zip-slip/symlink guards, local-storage TOCTOU handling, write-time secret redaction, admin constant-time token compare, and CORS deny-by-default are all present and correct. Multi-tenant isolation (space_id + owner scoping, NotFound-on-unauthorized to prevent enumeration) is consistently enforced across the MCP and Skill surfaces.

There is one blocking issue (P1) and a set of non-blocking hardening recommendations (P2). No P0 / security-critical defects were found.

---

Blocking (P1)

1. skills has no DB-level uniqueness on (owner, space, name); the duplicate check is bypassable

Files: migrations/sql/20260714-01-skill-marketplace.sql:12 (table DDL), internal/service/parse/worker.go:277 (checkNameDuplicate), internal/service/skill/service.go:185 (create name override), internal/repository/skill/create.go:75 (CreateSkillAndConsumeTask), internal/repository/skill/update.go:33 (rename).

The skills table defines only plain indexes (idx_owner, idx_space_visibility) — no UNIQUE constraint on (owner_id, space_id, name). The only name-collision guard, checkNameDuplicate, runs exclusively at parse time (worker.go:125); the create transaction (create.go) locks on the parse-task id, not the name, and performs a plain INSERT with no re-check and no MySQL 1062 handling.

This is exploitable through three concrete paths, two of which are deterministic (no race required):

1. Concurrent parse tasks with the same name each pass the parse-time check, then both insert.
2. Create-time name override: CreateRequest.Name (handler/skill/handler.go) is user-controlled with no binding tag; service.go:185 does name := p.Name, overriding the parse-validated name. A client can submit a create whose name collides with an existing skill and was never dup-checked.
3. Rename: UpdateRequest.Name is user-controlled; buildUpdateSets emits SET name = ? with no duplicate check anywhere on the update path.

The sibling mcp_servers table treats this exact requirement as a DB-level UNIQUE KEY uq_owner_space_name_live (migrations/sql/20260714-05-mcp-uniqueness.sql:35), and that migration’s own comment explicitly states the service-layer-only approach "was proven [inadequate]." The skill path still relies on precisely that inadequate approach, so this is a self-documented known-bad pattern shipping for skills. In a marketplace, duplicate skill names within a tenant are a real data-integrity/UX problem (ambiguous install/reference targets, duplicated listings).

Suggested fix: add UNIQUE (owner_id, space_id, name) to skills (mirroring the mcp live/soft-delete NULL trick if a soft-delete column is introduced), and map MySQL error 1062 to a name_taken 409 on both the create and update paths. Confirm the skills soft-delete model before choosing the exact index shape.

---

Non-blocking recommendations (P2)

These are robustness / defense-in-depth / hardening items. None blocks merge on its own; recommend addressing in a fast-follow.

Multi-tenancy / authorization (defense-in-depth — no live vulnerability found):

• Skill visibility not enum-validated (internal/service/skill/service.go:212, 445): the client visibility string is cast straight to the enum with no allowlist (handler field has no binding tag). It fails closed for reads (canView has default: return false), so it is not an escalation, but it lets users persist nonsense/system visibility and diverges from the MCP surface, which validates via validateClientVisibility. Add a {public,space,private} allowlist for parity.
• Skill repo mutations scope only by id (repository/skill/update.go:102, delete.go:15, parse_task.go:105): safe today because the service gates every call with owner_id == userID && space_id == spaceID, but this contradicts the project’s own "scope every query" convention (the MCP repo scopes explicitly). Add AND owner_id = ? AND space_id = ? as belt-and-suspenders so a future caller can’t reintroduce an IDOR.
• Connection URL not secret-redacted (internal/service/mcp.go:667, 513): redactSecrets sanitizes only the env/headers maps; a credential embedded in the connection URL (https://user:token@host or ?api_key=…) is stored verbatim and echoed to same-space viewers of a public MCP (detailForCaller blanks env/headers but not URL). Strip userinfo / known secret query params on write, or document that URLs must not carry credentials.

SSRF / storage / file handling (all verified non-exploitable as-is; hardening only):

• SSE endpoint re-validation is implicit (internal/service/probe.go:505): the server-advertised SSE POST endpoint is guarded only by a string-based same-origin check; the real IP-level SSRF guarantee comes from the shared client’s DialContext re-resolution. Not a bypass, but an explicit validateProbeURL(resolved) (or a comment) would make the invariant local and regression-proof.
• NAT64 guard covers only the well-known prefix (internal/service/probe.go:23): only 64:ff9b::/96 embedded IPv4 is extracted and re-checked; RFC 8215 network-specific NAT64 prefixes are not. Edge-case hardening.
• Zip size accounting uses declared UncompressedSize64 (internal/service/parse/zip.go:54): sums central-directory declared sizes, which a crafted zip can under-report. Not a bomb today because only SKILL.md is ever inflated (bounded by io.LimitReader); becomes real if other entries are ever extracted. Prefer counting bytes actually read.
• Presigned host swap without OSS_SIGNING_HOST (internal/storage/oss.go:205): when OSS_SIGNING_HOST is unset, the presigned URL host is swapped to the public endpoint with no check that the CDN restores the signed Host — a functional footgun (uploads 403 unless the CDN rewrites Host), not a security bypass. Consider requiring OSS_SIGNING_HOST whenever a public endpoint is configured for signed operations.
• Local download/upload proxy has no per-object authz (internal/api/handler/upload/local_proxy.go:16): dev-only (compiled out when AUTH_ENABLED=true) and correctly loopback-gated on RemoteAddr, but any loopback process can read/overwrite arbitrary keys under baseDir. Document as a hard dev-only invariant so it never ships behind a real reverse proxy.

Data layer / correctness:

• User-controlled version in storage key (internal/service/skill/service.go:220): version is unvalidated and interpolated into skills/{id}/v{version}/{file}; OSSStorage.CopyObject does not reject ..// (LocalStorage does). Blast radius is confined by the random ULID id prefix and object stores treating .. as literal, so no cross-tenant escape — but add a version-shape validation and a dotdot guard to OSSStorage for parity.
• Description truncation splits multibyte UTF-8 (internal/service/parse/worker.go:221): validated by rune count (≤1024) but truncated by bytes (s[:1024]), corrupting the tail of long CJK descriptions into invalid UTF-8. Truncate by runes to match validation.
• LIKE predicates omit an explicit ESCAPE clause (internal/repository/skill/list.go:83, internal/repository/mcp.go buildWhere): escapeLike uses backslash, which is only the default under standard sql_mode; under NO_BACKSLASH_ESCAPES the wildcard neutralization silently fails (search-semantics only — values stay bound, no injection). Append ESCAPE '\\' to each predicate.
• Parse worker Submit is unbounded (internal/service/parse/worker.go:49): wg.Add then a goroutine that parks on a cap-5 semaphore, with no admission control at submit time; a burst accumulates parked goroutines. Authenticated + bounded by distinct pending tasks, so P2 — consider a bounded job channel or rejecting Submit.

Config / packaging:

• Dockerfile.api runs as root, base images unpinned (Dockerfile.api:1,9): no USER directive (process runs as uid 0) and bases pinned to mutable tags rather than digests. Add a non-root user and pin by digest before prod deploy.
• Secret-key redaction over-matches access (internal/service/secret.go:14): the anchored pattern matches any key ending in access (e.g. PUBLIC_ACCESS), fail-closed rejecting benign config with secret_leaked. Deliberate and safe, but consider narrowing to access_key/access_token.

---

Items to confirm manually before deploy

• The upstream Octo /v1/auth/verify and /v1/auth/verify-bot contract is trusted for identity.Spaces / bot.SpaceID; multi-tenant isolation depends on that upstream never returning a space the caller isn’t in.
• AUTH_ENABLED=false trusts the client-supplied X-Space-Id and enables the local proxy — correct for dev, dangerous if it ever ships to prod. Defaults are safe (auth on, ProbeAllowPrivate off), but startup does not hard-assert AUTH_ENABLED=true in a production profile.
• Object-store bucket policy for the unsigned-public-download path (anonymous GET restrictions) is infra, not in this repo.

---

Verdict

Spec compliance: PASS — the migration delivers the stated scope (Skill + MCP catalog APIs, auth, MySQL migrations, object storage, Docker/CI, docs), the legacy .env is untracked with only .env.example retained (no real secrets), and the configurable public/signed download toggle is correctly wired end-to-end. No missing, extra, or deviated scope observed.

Code quality: CHANGES REQUESTED — the single P1 (skill name-uniqueness gap with three deterministic exploit paths, contradicting the codebase’s own established mcp_servers convention) should be fixed before merge. The P2 items are non-blocking follow-ups.

Requesting changes on the P1 only.

[OctoBoooot]

Review: migrate octo marketplace service (#1) — 02b28114 (retrigger) — reconciling yujiawei's skill-table findings

Verdict: Comment — verdict unchanged from 5f5fed50. This head is an empty retrigger commit (chore: retrigger review with ci evidence, zero file changes vs 5f5fed50), so the code is byte-identical: my two majors (UTF-8 truncation, pagination contract) remain fixed (verified last pass). Recording yujiawei's three NEW skill-table findings, byte-checked — one minor+question, two minors; none flips the verdict.

yujiawei's findings — assessed on the bytes

• 🔴 (her grade) no (owner_id, space_id, name) unique constraint on skills → I grade minor + question. Byte-confirmed: migrations/sql/20260714-01-skill-marketplace.sql gives skills only non-unique indexes (idx_category/idx_owner/idx_space_visibility/idx_created_at), and CreateSkillAndConsumeTask (repository/skill/create.go) dedups only the parse task (UPDATE...WHERE status='success'), not the name — so two different uploads with the same name in one space both succeed → duplicate skills coexist. Real gap. But I hold it minor, not 🔴: it's data-hygiene (duplicate names), no security/authz/integrity/cross-tenant impact. And whether skill-name uniqueness is an intended invariant isn't assertable from code — the MCP catalog table deliberately has uq_owner_space_name_live, but the skills table may or may not be meant to. Question for the author: is (owner, space, name) uniqueness a product requirement for skills? If yes, add the UNIQUE index (mirroring the MCP table) + the 409 mapping below; if no, it's working as intended. I can't grade it a blocker without that intent.
• 🟡 visibility enum not app-validated — the DB column is ENUM('public','space','private') so a bad value is rejected at the DB (500), but there's no application-layer check for a clean 400. Fair minor (validate before write).
• 🟡 no 409 dup-key mapping for skills — confirmed: the skill repo has no mapDupKey/errno-1062 handling (unlike the MCP repo's). Consistent with there being no unique constraint to violate; if uniqueness is added, this mapping should land with it. Fair minor.

Unchanged from 5f5fed50 (byte-identical code)

• UTF-8 truncation (yujiawei's earlier major) — FIXED (rune-aware sanitizeString + truncateUTF8Bytes, exec-verified).
• Pagination contract (mine) — FIXED (doc aligned to page/page_size + {data,pagination}, categories documented as removed).
• readme sanitize, error_message generic-map, SSRF CGNAT/NAT64, auth_type alignment — all intact.

Why COMMENT

• Still no Go code CI at this head: check-sprint (governance) red, code-review red (the automated review lane mirroring standing CRs — no details, not an independent code gate), label/pr-title-lint green; Build/Test/Vet absent. Per never-approve-on-unrun-code-CI I hold at COMMENT. My two majors are fixed; the skill-table uniqueness question (if the author confirms uniqueness is required) would be the one item to resolve before APPROVE — otherwise this is APPROVE-ready once real Go CI runs green.

Net: verdict steady at COMMENT. Both majors fixed; yujiawei's skill-table items are non-blocking on my grading (pending the author's uniqueness-intent call). needs-human-review remains.

[yujiawei]

Two additional non-blocking (P2) observations

A follow-up pass surfaced two more low-priority items worth noting alongside the review above. Neither is a blocker and neither changes the requested-changes verdict (which stands on the skill name-uniqueness issue).

System MCP name/slug uniqueness is a service-layer check with a TOCTOU window — internal/service/mcp.go:369 (checkSystemDupes) runs SELECT before INSERT/UPDATE, and system rows carry space_id = NULL, which defeats the uq_owner_space_name_live / uq_space_slug_live unique indexes (MySQL treats NULLs as distinct). Two concurrent admin creates can therefore produce duplicate system MCPs. Admin-only and low-frequency, so P2 — but it is the same class of gap as the blocking skill issue, so it would be natural to fix both together (e.g. a dedicated uniqueness constraint scoped to system rows).

Probe failure messages pass transport-layer errors through verbatim — internal/service/probe.go:322 returns truncateErr(err.Error()) in the init_failed body, so responses can echo strings like dial tcp 10.0.0.1:8080: connection refused, revealing internal network topology. Reachable only by authenticated callers and bounded by probeAllowPrivate=false, so P2. Consider mapping to generic error categories before returning to the client.

[OctoBoooot]

Review: migrate octo marketplace service (#1) — delta @ 88f334ea — skill name-uniqueness RESOLVED

Verdict: Comment — this delta cleanly closes yujiawei's skill name-uniqueness finding (the one open item I'd flagged with a uniqueness-intent question). All substantive findings are now resolved; I hold at COMMENT (not APPROVE) only because no Go code CI runs at this head. Built + ran the skill repo/service suites locally: green.

Delta 02b28114→88f334ea ("fix(skill): enforce owner-space name uniqueness"): migration + repo dup-key mapping + service/handler 409 wiring + test.

yujiawei's skill name-uniqueness finding — RESOLVED ✅ (and the intent question answered)

• Constraint: migrations/sql/20260717-01-skill-name-uniqueness.sql adds UNIQUE KEY uq_skill_owner_space_name (owner_id, space_id, name) — so uniqueness IS the intended invariant (answering my prior question), enforced at the DB (race-safe, not a checkable-then-bypassable SELECT).
• Race-safe mapping: mapDuplicateName (repo.go) maps errno-1062 on uq_skill_owner_space_name → ErrNameTaken, wired on ALL three write paths (Create, CreateSkillAndConsumeTask, Update), through the service, to a 409 Conflict in the handler on both create and update. This closes the concurrent-create, create-time-override, and rename races the parse-time check couldn't. Also resolves yujiawei's 🟡#3 (no 409 dup-key mapping) — it's exactly this.
• Migration safety: ADD UNIQUE KEY with no pre-dedup would fail on a table with existing duplicates — but this is a greenfield service: the entire migration set (baseline → this file) is introduced by this PR (skills created in 20260714-01, same PR), so there's no prior deployment with duplicate rows. Safe here. (One forward note: if this constraint ever ships against a DB that already holds dupes, it'd need a dedup step first — not this PR's situation.)
• repo_test.go adds dup-name coverage. Local run: build clean, skill repo + service tests green.

Remaining from yujiawei — one minor, non-blocking

• Visibility enum app-validation (🟡): still no application-layer check before write; the DB ENUM('public','space','private') backstops it (bad value → 500 rather than a clean 400). Fair minor to add; doesn't block.

All prior findings — still fixed

• UTF-8 truncation (rune-aware) and pagination contract (doc↔code aligned) — both majors fixed, unchanged. readme sanitize / error_message / SSRF CGNAT+NAT64 / auth_type — intact.

Why COMMENT not APPROVE

• No Go code CI at this head: check-sprint (governance) red + label/pr-title-lint green + code-review pending; Build/Test/Vet absent (the persisting repo-CI-setup gap). Per never-approve-on-unrun-code-CI I hold at COMMENT — but every substantive finding across this review history is now closed, so this promotes to APPROVE the moment the Go checks run green. The new dup-name test would exercise the fix if the test job runs.

Net: skill name-uniqueness resolved end-to-end (constraint + race-safe 409, both paths); both majors fixed; one non-blocking enum-validation minor remains. Only unrun Go CI holds it short of APPROVE. needs-human-review remains.

[yujiawei]

Code Review — PR #1 (octo-marketplace)

Re-reviewed at head SHA 88f334ea16881db342636bc4655caedb3027a426 against merge-base 8399a4c6. This is a large greenfield migration (148 files, ~24k additions) of the Skill & MCP catalog marketplace service (Gin + MySQL + S3/local object storage), classified security-sensitive. My previous formal review requested changes on a single P1; this pass focuses on the delta since then (fix(parse): preserve utf8…, chore: retrigger, fix(skill): enforce owner-space name uniqueness) plus a full re-verification of the affected subsystems, with data flow traced handler → service → repository and each finding independently checked against the code.

go build ./..., go vet ./..., and go test ./... all pass locally. CI additionally runs go test -race -shuffle, gofmt, and golangci-lint.

1. Spec compliance

PASS. The delta directly and completely addresses the prior blocker (skill name-uniqueness) and the documented pagination contract mismatch. No missing, extra, or deviated scope observed relative to the stated migration goal.

2. Prior blocker — RESOLVED

P1 (skill name-uniqueness): fully closed. Verified end to end:

• DB constraint added — migrations/sql/20260717-01-skill-name-uniqueness.sql:6 adds UNIQUE KEY uq_skill_owner_space_name (owner_id, space_id, name). This is the correct index shape here: skills uses hard delete (internal/repository/skill/delete.go:16, DELETE FROM skills WHERE id = ?, no deleted_at column), so a plain UNIQUE key correctly permits name reuse after deletion — no name_live generated-column trick is needed (that dance is only required for the soft-deleted mcp_servers table).
• All four write paths that set skills.name are guarded — Create (create.go:48), CreateSkillAndConsumeTask (create.go:112), Update (update.go:107), UpdateSkillAndConsumeTask (parse_task.go:108) all wrap Exec errors with mapDuplicateName, which maps MySQL 1062 and a message containing uq_skill_owner_space_name to ErrNameTaken (repo.go:20-26). This closes all three previously-exploitable paths (concurrent parse, create-time name override, rename).
• Surfaced as a clean 409 — service propagates ErrNameTaken (service.go:257,402,429) and the handler returns 409 Conflict for both Create and Update (handler/skill/handler.go).
• Precise mapping — a PRIMARY-key 1062 is not misclassified as name-taken, and a real name collision is not missed; repo_test.go asserts both the positive and negative case and passes.
• No legitimate operation broken — new versions insert into skill_versions (keyed skill_id+version), not a new skills row; re-parse updates the existing row. Neither collides with the new constraint.
• Index length safe — utf8mb4 worst case (128+64+64)*4 = 1024 bytes, well under the InnoDB 3072-byte limit.

UTF-8 truncation (worker.go): correct. sanitizeString now truncates by rune count, matching the rune-count validation gate (validateSkillDescription rejects utf8.RuneCountInString > 1024) — the prior byte-slice corrupted the tail of long CJK descriptions that had passed validation. The new truncateUTF8Bytes is O(n) (not a DoS: strings.ToValidUTF8 runs first, so the boundary backtrack is ≤3 iterations), has no off-by-one, and the ordering ToValidUTF8(replaceNullBytes(s)) guarantees valid, NUL-free UTF-8 reaches the DB. New unit tests cover both.

Docs/API contract: resolved on the primary surface. GET /mcps and GET /mcps/mine now match the shipped docs exactly — query params page/page_size (max 100), response envelope {data, pagination:{total,page,page_size}}, category facets removed from the list response and served by the dedicated GET /api/v1/mcp_categories endpoint. A client following §4.2 will not break.

3. Non-blocking items (P2 — fast-follow, none blocks merge)

• Stale doc prose in docs/api/mcp-v1.md — §9.2 (admin list) still says limit / offset and {items,total,categories} (lines ~680-682), and §7 (perf note, ~599-600) still describes per-request category GROUP BY. The shipped admin handler uses the same page/page_size + {data,pagination} envelope, so this is doc-only drift that contradicts the doc's own realigned §4.2. Worth a one-line sync.
• Duplicate detection depends on MySQL message text (repo.go:20-26) — strings.Contains(msg, "uq_skill_owner_space_name") is version/locale-sensitive. Robust across current MySQL 8 (key name is emitted verbatim in 1062), but a non-English lc_messages or a MariaDB phrasing change would drop it to a raw 500 instead of 409. Consider a defensive fallback.
• The P2 hardening items from my prior review that remain outside this delta (skill visibility enum-validation parity, belt-and-suspenders owner_id/space_id scoping on skill repo mutations, connection-URL secret redaction, zip size accounting, NAT64 prefix coverage, Dockerfile non-root/digest pinning) still stand as fast-follows; none is a merge blocker.

4. Items to verify manually before deploy (security-sensitive)

• Migration on a populated table — 20260717-01 is a bare ADD UNIQUE KEY with no pre-dedup step. It is safe here because skills is greenfield in this same PR series, but if this constraint is ever applied to an environment where skills was already populated, the ALTER will fail if duplicate (owner_id, space_id, name) rows exist. Confirm the target DB is empty or dedup-clean before applying. The full DDL should also be exercised against a real MySQL instance (unit tests use go-sqlmock and never run the real ALTER or exercise a live MySQLError).
• The upstream /v1/auth/verify contract and the AUTH_ENABLED=false dev path (client-supplied X-Space-Id + unauthenticated local proxy) remain trusted-by-config as noted previously — safe defaults, dangerous only if the dev profile ever ships to prod.

5. Verdict

Spec: ✅ — delta delivers exactly the fix scope (DB uniqueness + 409 mapping + UTF-8 truncation + docs realignment), no over/under/deviated build.

Quality: Approved — the sole prior P1 is fully resolved with correct index shape, complete write-path coverage, and passing tests; no P0/P1 remains. Remaining items are P2 fast-follows and one deploy-time ops caveat.

Overall: APPROVE.

Note for the record: two external cross-family advisory passes were attempted for this security-sensitive change but were unavailable this round (one could not fetch the >20k-line PR diff via the API; the other failed on a gateway tool-config error). This verdict rests on my own line-level review plus an independent multi-leg verification of the delta; the advisory legs are treated as absent, not as silent approval.

[kense-lab]

@Jerry-Xin 当前 head 88f334e 已修复 skill 名称唯一性 P1：新增数据库唯一约束，并在 create、rename、带 parse task 的 update 路径统一映射重复名为 409。该 SHA 的 Fork CI（lint/test/vet/build）已全部通过。麻烦有空时复审一下当前 head，谢谢。

[kense-lab]

@OctoBoooot 关于“当前 head 没有 Go code CI”：当前 head 88f334e 的 Fork CI 已实际运行并全部通过，不是本地结果。Run: https://github.com/kense-lab/octo-marketplace-migration/actions/runs/29552361242 。其中 verify job 已通过 Format check、Test、Vet、Build、Validate Compose，lint job 也已通过。PR head 属于 Fork，若只读取上游仓库的 statusCheckRollup 会漏掉这些 head checks；请基于 PR headRef 的 checks 复审并更新结论。

[Jerry-Xin]

Summary: Re-anchored at head 88f334ea. Both prior 🔴 blockers are byte- and execution-verified fixed, and the new skill-name-uniqueness delta (5f5fed506ee4..88f334ea) is correct with no new bug. Flipping to APPROVE — this supersedes my stale REQUEST_CHANGES at a2f92c61.

💬 Non-blocking

• 🟡 README.md still says MCP business APIs are deferred, but the PR ships MCP CRUD/probe/admin/icon paths — update to avoid confusing adopters.
• 🟡 README.md links to docs/github-ci-improvement.md, which is not present in the tree.
• 🟡 internal/middleware/admin.go Wrap renders {"err": ...} while the rest of the API uses {"error": ...}; harmless while Wrap is unused but inconsistent if reused.
• 🟡 Delete handlers in internal/api/handler/mcp.go and internal/api/handler/admin_mcp.go return a 200 empty envelope, while docs/api/mcp-v1.md documents 204 — align contract vs implementation.
• 🟡 The uniqueness migration adds the UNIQUE index with no dedup/backfill step. Safe here because this is the initial migration of a greenfield service (the skills table itself is created earlier in this same PR series, no legacy duplicate rows), but on any dev/staging DB that predates the constraint a pre-existing duplicate would make ALTER TABLE fail. Name matching also inherits the column's default collation (case/whitespace-insensitivity) — worth confirming that is the intended equivalence.

✅ Highlights

• UTF-8 truncation fixed (credit @yujiawei): sanitizeString now runs strings.ToValidUTF8 then caps on []rune (rune count), and README uses truncateUTF8Bytes, which backs off to a valid rune boundary (for end > 0 && !utf8.ValidString(s[:end]) { end-- }). No mid-codepoint truncation.
• MCP pagination contract now consistent: docs/api/mcp-v1.md specifies page/page_size with a {data, pagination} envelope matching the handler, and documents categories as removed from the list response. Doc ↔ code aligned.
• New skill-name uniqueness is enforced at the right layer: a DB UNIQUE KEY uq_skill_owner_space_name (owner_id, space_id, name) (both columns NOT NULL, so correct per-owner/per-Space multi-tenant scope with no NULL-distinctness gap). It relies on the constraint rather than a check-then-insert, closing the concurrent-create / create-time-override / rename TOCTOU races. mapDuplicateName scopes the 1062 mapping to the specific index name and returns a clean ErrNameTaken → 409 on both Create and Update, with a test asserting an unrelated PRIMARY dup does not mis-map.
• Prior hardening intact: publicParseErrorMessage redaction, SSRF denylist (isUnsafeProbeIP incl. 100.64/10 and 64:ff9b::/96), and local-storage EvalSymlinks path hardening.
• Local verification: go test ./..., go vet ./..., and git diff --check pass.

Superseded by APPROVED review at live head 88f334e; the blockers this CR raised are fixed and verified.

[OctoBoooot]

Review: migrate octo marketplace service (#1) — delta @ de362e76 — vulnerable x/crypto bumped

Verdict: Comment — clean security-hardening delta (fix(deps): upgrade vulnerable x crypto modules); go.mod/go.sum only, no source/behavior change. Every substantive finding across this PR's history remains resolved. Still holding at COMMENT (not APPROVE) only because no Go code CI runs at this head. Built + ran the full suite locally with the bumped deps: green (go build ./... clean, go test ./... = 18 packages ok, 0 fail).

This delta — vulnerable transitive dep closed ✅

• golang.org/x/crypto v0.31.0 → v0.54.0 — v0.31.0 is affected by GHSA-v778-237x-gjrc / CVE-2024-45337 (x/crypto/ssh authorization-bypass-via-misuse advisory); v0.54.0 clears it. Transitive alignment: x/net 0.25.0→0.56.0, x/text 0.21.0→0.40.0, x/sys 0.28.0→0.47.0. go 1.25→go 1.25.0 (patch-format normalization). No require-block additions, no source touched. Build + full tests green on the new versions.

All prior findings — still resolved (unchanged)

• Both my majors: UTF-8 rune-safe truncation + pagination doc↔code alignment — fixed.
• Skill name-uniqueness (yujiawei): DB UNIQUE KEY uq_skill_owner_space_name + 1062→ErrNameTaken→409 on create/update, greenfield-safe migration — fixed.
• readme sanitize / error_message generic-map / SSRF CGNAT+NAT64 / auth_type alignment — intact.
• Remaining non-blocking minors (visibility enum app-validation; a few doc/consistency nits from Jerry-Xin) — author's call, none blocking.

Why COMMENT not APPROVE

• No Go code CI at this head: check-sprint (governance) red + label/pr-title-lint green + code-review pending; Build/Test/Vet absent — the repo-CI-setup gap persisting across this PR. Per never-approve-on-unrun-code-CI I hold at COMMENT. Every substantive finding (including now the vulnerable-dep concern) is closed, so this promotes to APPROVE the moment real Go CI runs green.

Net: security dep-bump verified clean (build + tests green, vulnerable x/crypto gone); no new issues; all prior findings resolved. Only unrun Go CI holds it short of APPROVE. needs-human-review remains.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

Code Review: octo-marketplace PR #1 — feat: migrate octo marketplace service

Reviewer: Octo-Q (automated review)
Head SHA: de362e76a60ca28403b17866d947c4a53565342f
Base SHA: 8399a4c64462be9b663fe064f1b3f739c29590b4
Scope: 148 files, ~24k insertions — full service migration (Go/Gin + MySQL)

---

1. Validation Summary

Area	Status	Evidence
Auth middleware (dual-mode)	✅	middleware/auth.go — prod: token resolve + space membership; dev: devIdentity or X-Space-Id
Admin auth	✅	middleware/admin.go — constant-time token compare; empty token + authEnabled → all rejected
Path traversal (local storage)	✅	storage/local.go:safePath() — rejects absolute/.., evaluates symlinks, O_NOFOLLOW, re-check before rename
Zip slip	✅	parse/zip.go:validateZipEntry() — absolute, .., ../, ..\\, symlink all rejected; 50MB/1MB limits
SSRF protection (probe)	✅	service/probe.go:validateProbeURL() + custom DialContext with DNS IP filtering; redirect chain re-validates
Secret redaction	✅	service/secret.go:redactSecrets() — regex key pattern, write-time rejection; detailForCaller blanks for non-owners
Markdown sanitization	✅	markdown/sanitize.go — goldmark AST walk, removes script/iframe/form/etc blocks
SQL injection	✅	All queries parameterized (?); LIKE wildcards escaped via escapeLike() in repository/mcp.go
CORS	✅	router.go — origin echoed only if in allowed list
Local proxy isolation	✅	local_proxy.go — loopback-only middleware, disabled when authEnabled=true
Parse task consumption	✅	Atomic TransitionPendingToParsing + CreateSkillAndConsumeTask/UpdateSkillAndConsumeTask with DB-level guard
Migration safety	✅	db/migrate.go — MySQL advisory lock (GET_LOCK), sequential execution
MCP uniqueness	✅	migrations/20260714-05 — generated column name_live + UNIQUE index, deadlock-free (proven by test)

---

2. Findings

P2-1: Skill Delete leaves orphaned storage objects

• Path: internal/service/skill/service.go:305-315
• Diff-scope: New code (this PR introduces the skill CRUD surface)
• Description: Delete() hard-deletes the DB row but never calls store.DeleteObject() for the file at row.FileURL. Over time, orphaned objects accumulate in S3/OSS with no cleanup path.
• Impact: Storage cost leak; no user-visible breakage.
• Recommendation: Add s.store.DeleteObject(ctx, row.FileURL) after successful DB delete (best-effort with log on failure). Also consider the icon URL if stored as object key.

P2-2: Skill Update accepts unlinked parse tasks (SkillID == "")

• Path: internal/service/skill/service.go:277-280
• Diff-scope: New code
• Description: The condition if pt.SkillID != "" && pt.SkillID != id accepts parse tasks with SkillID=="" for any skill update. A reupload task created via InitReupload is supposed to be linked to a specific skill, but an unlinked task (from InitUpload) also passes. Combined with the file copy happening before the DB transaction, a concurrent pair of updates on different skills with the same unlinked task would both copy the file, but only one would succeed in consuming the task — the other gets a DB error after an unnecessary file overwrite.
• Impact: Narrow race, same user, same space. No data corruption (DB transaction guards). Orphaned/overwritten file at worst.
• Recommendation: Tighten to if pt.SkillID != id { return ErrInvalidParseTask } for the update path, requiring reupload tasks to be explicitly linked. Or add a consumed-status check before the copy.

P2-3: CachedResolver stampede on expired entries

• Path: internal/auth/cached_resolver.go:39-44
• Diff-scope: New code
• Description: When a cached entry expires, the check-and-delete happens under lock, but the upstream resolve happens outside the lock. N concurrent requests with the same expired token all delete the entry, fan out to the upstream auth server, and race to re-cache. No single-flight dedup.
• Impact: Under high concurrency with expired tokens, momentary load spike on the upstream auth API. Functionally correct (all get the same identity).
• Recommendation: Use a singleflight.Group or keep the lock held during resolve (acceptable if upstream timeout is bounded at 5s, which it is).

P2-4: S3 client Put/objectURL does not sanitize key input

• Path: internal/blob/s3.go:62-65, 93-104
• Diff-scope: New code
• Description: escapePath URL-encodes each path segment but does not reject .. or absolute-path patterns in the key. All current callers construct keys from service-controlled patterns (skills/{id}/v{ver}/{filename}, mcp_icon/{partition}/{id}/{ver}/icon), so this is not exploitable today. However, the blob layer has no defense-in-depth if a future caller passes user-controlled keys.
• Impact: No current vulnerability; latent risk.
• Recommendation: Add a validateKey() guard in objectURL rejecting keys containing .., leading /, or empty segments.

P2-5: Probe endpoint — ProbeAllowPrivate bypasses SSRF protection globally

• Path: internal/service/probe.go, gated by config.ProbeAllowPrivate
• Diff-scope: New code
• Description: When PROBE_ALLOW_PRIVATE=true, IP validation is disabled for all probe requests, not scoped to admin-only. Any authenticated user can probe internal/loopback addresses. The .env.example documents this as "trusted deployments only" but the config flag applies globally.
• Impact: In trusted single-tenant deployments, acceptable. In multi-tenant, an authenticated user could probe internal services.
• Recommendation: Consider restricting ProbeAllowPrivate to the admin surface only, or adding a caller-level capability check.

Observation-1: Dev-mode admin routes fully open

• Path: middleware/admin.go, .env.example
• Description: When AUTH_ENABLED=false, admin routes bypass all authentication. The admin middleware returns a synthetic admin identity. This is documented and intentional for development. Risk: if AUTH_ENABLED=false is accidentally deployed to production (.env.example defaults to false), admin endpoints are open.
• Mitigation already in place: config.ValidateAPI() requires OCTO_API_URL when auth is enabled, and ADMIN_OWNER_UID when auth + admin token are both set. Production deployment tooling should enforce AUTH_ENABLED=true.

Observation-2: Skill canView has no "system" visibility case

• Path: internal/service/skill/service.go:322-331
• Description: The skills ENUM only supports ('public','space','private'), so this is correct. MCP servers have a separate system visibility handled by the MCP service/repository. Noted for cross-domain awareness, not a defect.

---

3. Data Flow Backtracking

Consumer	Data	Upstream Source	Verified?
MCP.Get → detailForCaller	m.Connection.Headers/Env	DB config_json column → scanRow → json.Unmarshal	✅ Non-owners get blanked maps
Skill.Get → rowToItem	row.FileURL	DB file_url column → skillrepo.GetByID	✅ json:"-" prevents serialization
Skill.Get → rowToItem	row.ReadmeContent	DB → mdsanitize.Sanitize()	✅ Always sanitized before return
Skill.List → repo query	spaceID, userID	Auth middleware → middleware.SpaceID(c) / middleware.Identity(c)	✅ Repo SQL includes visibility scoping
MCP.List → buildWhere	f.SpaceID, f.CallerUID	Handler callerFromContext → middleware	✅ SQL visibility clause: system OR (same_space AND (public OR owner))
Parse.TriggerParse	task.OwnerID, task.SpaceID	InitUpload stamps from authenticated caller	✅ Ownership + space checked before dispatch
Probe.validateProbeURL	req.URL	User JSON body → decodeJSON	✅ Scheme, credentials, IP literal validated; DNS resolved in DialContext
Admin.CreateSystem → checkSystemDupes	req.Name, req.Slug	SystemNameExists/SystemSlugExists SQL queries	✅ Needed because NULL space_id defeats UNIQUE index

---

4. Blind-Point Checklist (C1–C6)

C1 — Dual-path parity

• MCP create↔delete: Create inserts, Delete soft-deletes. Both check ownership + space. ✅
• Skill create↔delete: Create consumes parse task + inserts. Delete hard-deletes but misses storage cleanup (see P2-1). ⚠️
• Parse upload↔consume: InitUpload creates pending task, TriggerParse transitions atomically with WHERE status = 'pending'. ✅
• Admin create↔delete: CreateSystem/DeleteSystem both operate on system rows only. ✅

C2 — Control-flow ordering / nested reuse

• validateContent is called on create and on patch (for touched fields via applyPatch → validateModelLengths). No double-validation issue. ✅
• redactSecrets called once on create, once per patch for changed fields. No re-redaction of already-redacted values (redacted = empty string, which passes). ✅

C3 — Authorization boundaries

• Who can create MCPs: Any authenticated user with space membership. ✅
• Who can create system MCPs: Only admin-token holders via /admin/* routes. ✅
• Who can probe: Any authenticated user (space-scoped). Admin probe uses same service. ✅ (see P2-5 for allow-private concern)
• Who can upload skills: Any authenticated user; ownership stamped at upload time. ✅
• Who can delete skills: Owner only, same space. ✅
• Skill download: Checks ownership via skill.OwnerID != identity.UID. Non-owners get 403. ✅

C4 — Authorization lifecycle / container-member cascade

• N/A for this service. Space membership is validated per-request by auth middleware, not cached across requests. No container-level disable pathway exists in this service. ✅

C5 — Build/note through ≠ runtime correctness

• CI runs go test -race, go vet, go build, golangci-lint. ✅
• Docker Compose validated in CI (docker compose config). ✅
• Migrations run at startup with advisory lock. ✅
• No build-time artifacts that diverge from runtime behavior. ✅

C6 — Governance/policy self-consistency

• N/A — no governance/policy documents in this PR.

---

5. Cross-Round Blocker Recheck (R6)

N/A — first review round.

---

6. Additional Observations

• Migration quality: 13 migration files with clean Up/Down pairs. The name_live generated column pattern for soft-delete-safe uniqueness is well-designed and documented with deadlock rationale.
• Error handling: Consistent apierr pattern with structured error codes. Internal errors logged server-side, generic messages returned to clients.
• HTTP timeouts: All server timeouts configurable via env vars. Probe HTTP client has 15s timeout, resolver has 5s timeout.
• CI: Comprehensive — format check, race-enabled tests, vet, build, compose validation, golangci-lint.

---

[Octo-Q] verdict: APPROVE — No P0/P1 blockers found. The codebase demonstrates strong security hygiene across auth, path traversal, SSRF, SQL injection, and secret handling surfaces. Five P2 items noted (orphaned storage on skill delete, unlinked parse task acceptance in update, cache stampede, blob key validation gap, probe allow-private scope). These are improvement recommendations, not blockers. Recommend addressing P2-1 (storage cleanup) and P2-2 (parse task linking) before production deployment.

[Jerry-Xin]

Summary: Re-anchoring my APPROVE at the current head — this delta is a dependency-only hardening bump with no code changes; both prior majors remain fixed.

✅ Highlights

• Delta is dependency-only (go.mod/go.sum). All changes are forward upgrades of transitive golang.org/x/* modules: crypto v0.31.0 → v0.54.0, net v0.25.0 → v0.56.0, sys v0.28.0 → v0.47.0, text v0.21.0 → v0.40.0, plus a cosmetic go 1.25 → go 1.25.0. These move the CVE-bearing x/crypto/x/net lines forward (vulnerability-closing), and introduce no downgrades. No source files changed.
• Both prior majors remain resolved (unchanged at this head): UTF-8 truncation is rune-safe (ToValidUTF8 + rune-boundary backoff at every cap), and the pagination contract is consistent between the doc and handler (page/page_size + {data, pagination}, categories documented as removed).
• Skill-name uniqueness is DB-enforced — ADD UNIQUE KEY uq_skill_owner_space_name (owner_id, space_id, name) with 1062 → ErrNameTaken (409) mapping (not a check-then-insert race).

💬 Non-blocking (carried from prior review, doc/consistency only)

• README staleness / missing linked doc file; error-response field-name consistency; delete-endpoint status code vs doc; and confirm the uniqueness index collation (case/accent sensitivity) matches intent. None block.

No blocking items from my review at this head.

Superseded by my COMMENT at the live head de362e7 (pullrequestreview-4719265580). Both majors this CR raised — byte-slice UTF-8 truncation corrupting CJK content, and the list-pagination doc↔code contract mismatch — are byte+execution-verified FIXED at later heads (rune-safe sanitizeString/truncateUTF8Bytes; pagination doc aligned to page/page_size + {data,pagination}). I am NOT converting to APPROVE: I hold at COMMENT because no Go code CI (Build/Test/Vet) has ever run on this PR — never-approve-on-unrun-code-CI. Dismissing this stale CHANGES_REQUESTED so the review gate reflects my true current position (no open blocking finding) rather than a superseded blocker from head 3923f22.

[yujiawei]

Code Review — PR #1 (octo-marketplace)

Independent review of the octo-marketplace service migration. Reviewed at head de362e76a60ca28403b17866d947c4a53565342f (base main, 148 files, +24,278 / −25). This PR was flagged security-sensitive (auth, middleware, object-storage signed URLs, secret redaction, SQL migrations, MCP probe/SSRF, zip extraction, markdown sanitization), so the security surfaces were reviewed line-by-line and each candidate finding was adversarially re-verified against the code before inclusion.

1. Spec compliance

The diff matches the stated scope: Skill + MCP catalog APIs, token/bot auth, MySQL migrations, object storage (local + S3/COS with configurable public-vs-signed downloads), Docker/CI, and docs. The legacy tracked .env is excluded; only .env.example remains. No secrets are committed. No functionality outside the stated migration scope was found. Spec: OK.

2. Local verification

• gofmt -l . — clean
• go build ./... — passes
• go vet ./... — passes
• go test ./internal/... — all packages pass

3. Security assessment

The security-critical surfaces are, on the whole, implemented carefully and hold up under scrutiny. Highlights of things done right (verified, not exploitable):

• SSRF (probe): DNS-rebinding is defended at dial time — the custom DialContext re-resolves and re-checks every candidate IP with isUnsafeProbeIP (loopback/private/link-local/CGNAT 100.64/10/NAT64 64:ff9b::/96/IPv6), redirects are capped and re-validated, URL credentials and stdio transport are rejected, and SSE endpoints are origin-pinned. The Host-header-injection concern does not apply (Go's http client ignores a Host set via the header map).
• Signed URLs / SigV4: presigned URLs are generated against the canonical signing host and only the browser-facing scheme/host is swapped afterward; SigV4 verifies against the Host header, which the CDN restores — so the substitution does not weaken the signature. OSS_SIGNING_HOST fails closed on mismatch. Unsigned public-download mode only returns object keys that were already gated by an authenticated, space-scoped Download handler.
• Access control: MCP and Skill reads enforce space isolation and owner/visibility (canView, isVisible); non-owner MCP reads have env/headers values blanked (detailForCaller); admin token uses crypto/subtle.ConstantTimeCompare and fails closed on an empty configured token in prod.
• Zip handling: zip-slip (absolute paths, .., both separators), symlink entries, per-entry and total size caps, and a 1 MiB bounded read for SKILL.md are all enforced.
• Markdown XSS: raw HTML blocks/inline are either dropped (blocklisted tags, comments) or html.EscapeString-neutralized; sanitized markdown is stored and returned as JSON, never rendered to HTML server-side.
• SQL: all queries are parameterized; dynamic UPDATE SET clauses are built from a fixed column allowlist, not user input.
• Local storage proxy is loopback-only and disabled when auth is enabled.

Non-blocking findings (P2 — hardening suggestions)

These do not block merge; each is defended in depth elsewhere.

1. Auth cache does not cache negative results (internal/auth/cached_resolver.go:43-46). Errors and empty-UID responses are not cached, so repeated invalid/expired tokens each incur an upstream round-trip. Bounded by the 5s client timeout, but briefly caching failures (per OWASP) would add DoS resistance.
2. Bot tokens bypass the resolver cache entirely (internal/middleware/auth.go:70-73, wired in cmd/marketplace-api/main.go:69). The bf_-prefixed path uses HTTPBotResolver directly with no CachedResolver wrapper, so every bot request hits upstream. Consider wrapping it the same way user-token resolution is wrapped. Routing itself is correct — upstream validates the token.
3. Secret-redaction regex misses some multi-segment hyphenated key names (internal/service/secret.go:14). Keys like api-key-id or x-api-key-id don't match the anchored pattern and would pass through un-redacted on write. Impact is limited because non-owner reads already blank env/headers values and list responses omit them entirely, but the write-time redaction is the documented control and should catch these; consider a substring/token-based match instead of the fixed alternation.
4. Multiple SKILL.md in one archive is resolved non-deterministically (internal/service/parse/zip.go:67-77). Root and one-level-deep SKILL.md are both accepted and the last one in zip iteration order silently wins. Not a security issue (downstream fields are validated), but extraction should pick a deterministic precedence or reject ambiguous archives, and gain a test for it.
5. Dev-mode trusts X-Space-Id verbatim (internal/middleware/auth.go:55-61). Only reachable when AUTH_ENABLED=false; every repo query is still space-scoped, so it is not an isolation bypass, but it diverges from the "never trust caller-supplied identity headers" policy and is worth a comment or a dev-only guard.

4. Items a human should manually confirm (security-sensitive deployment)

• The CDN/gateway in front of COS must restore the Host header to OSS_SIGNING_HOST before COS validates SigV4, or signed uploads/downloads will fail — verify in the real deployment.
• Confirm prod sets AUTH_ENABLED=true, a non-empty MARKETPLACE_ADMIN_TOKEN, and ADMIN_OWNER_UID (startup validation enforces the last, which is good).
• The MCP probe is an intentional authenticated outbound-fetch primitive; confirm PROBE_ALLOW_PRIVATE=false in shared environments.

5. Coverage / blind spots

• Reviewed statically; not run against a live MySQL/COS, so migration ordering and runtime COS/CDN Host behavior were not exercised end-to-end (integration tests requiring MySQL were not run here).
• SQL migration files were scanned for destructive/irreversible operations at a high level, not schema-diff-verified against a populated database.

Verdict

No P0/P1 defects were confirmed after adversarial re-verification. The migration is well-structured and the security-critical paths are sound. The five P2 items above are hardening suggestions, not merge blockers. Recommend addressing (1)–(3) in a follow-up given the security-sensitive classification.