[Raghav-Bansal-15]

Store identifiers were inserted into URL paths without segment encoding, and download analytics reused authentication-related secret material.

Encode dynamic path segments and use a separate HMAC key for download analytics. Add safe configuration warnings, rotation/provisioning notes, and a fixture that does not leak an auth-refresh timer.

Original implementation and subsequent contributor fixes: @Raghav-Bansal-15. Prior review-agent repairs remain in their separately authored commits. This review replayed only the topic commits onto rebased dev 77c4a4071, preserving author identities and removing unrelated old-dev integration history. No new production implementation was added by this replay.

Current head: 95e53e9825265dc7d898af3c2d301a20fbed6c79. Current-head upstream CI passed at 95e53e98: 🔨 Build & Test. Older fork CI is historical evidence only. No live publishing, gateway rollout, or production secret provisioning was performed.

[kshivang]

@claude review the current head against dev for correctness and regression risks. Please inspect the actual diff and existing tests; do not modify code. This is the current downloads-group review.

[claude]

Claude finished @kshivang's task in 6m 22s —— View job

---

Downloads-group review - PR #571 (fix/security-plugin-store-inputs)

• Read the actual diff (4cb1007, 4 files) against dev
• Trace endpoint() semantics and every Kotlin call site for behaviour change
• Check server-side handling of the encoded segments (Hono route table, req.param decode)
• Trace hashIp return-type widening through both callers, recordDownload, and the RPC/column
• Confirm the new tests actually run in CI and check for coverage gaps
• Look for the same class of issue left unfixed elsewhere

No code changed, as asked.

---

Verdict

The direction is right and the risk to normal operation is low: for a reverse-DNS plugin id (ai.rever.boss.plugin) and a semver version the emitted URL is byte-identical to before, because Ktor's encodeURLParameter preserves -._~ and alphanumerics. I found no correctness regression that breaks a working flow. What I would want addressed before rollout is concentrated in hashIp, where three separate misconfigurations all fail silently.

Things worth affirming (I checked, they hold)

• encodeURLParameter is the correct choice over encodeURLPathPart. The path-part encoder deliberately leaves / alone, which would have made the fix a no-op for the main attack shape. spaceToPlus = false is also right for a path.
• The old scheme was materially weaker than "SHA-256 with a salt". The salt was SUPABASE_SERVICE_ROLE_KEY?.substring(0, 16); a legacy Supabase service key is an HS256 JWT, so its first 16 characters are the constant eyJhbGciOiJIUzI1 for every project. The previous ip_hash was therefore an effectively unsalted SHA-256 of the IP - enumerable across all of IPv4 in seconds by anyone who obtained a row. (Reads were already locked down by fix(plugin-store): restrict plugin_downloads RLS to the server-side pipeline #488, so this is defence in depth, but the fix is substantive rather than cosmetic.) Note also the old code appended the string "undefined" when the env var was absent, so hashes were not even stable across environments.
• No other hashing use of the service-role key remains anywhere under supabase/functions - I grepped; the remaining four hits are all legitimate client construction. The isolation is complete.
• The string | null widening is type-safe end to end: both call sites (routes/download.ts:209, :356) already assign to string | null, recordDownload's ipHash is string | null = null, and record_plugin_download's p_ip_hash TEXT DEFAULT NULL accepts it. Tracking is inside a try/catch that never fails the download, so the "downloads remain available" claim holds.
• Both new tests do run in CI. edge-functions.yml's plugin-store job runs deno test --allow-all with working-directory: supabase/functions/plugin-store, and @std/assert is in that function's deno.json imports; the Kotlin test is in desktopTest, which ./gradlew build → allTests covers for this module.
• The new exception type does not escape to a user. require throws IllegalArgumentException, but every caller is broad: RemotePluginRepository (runCatching), PluginStoreApiKeyProviderImpl (catch (e: Exception)), PluginStoreSetup.fetchStoreSignature (catch (e: Exception), after rethrowing CancellationException), and checkHealth's own try. See the nit below for the one observable difference.
• I could not independently verify the HMAC test vector locally (no interpreter execution available in this sandbox). It is asserted in a CI-run test, so a wrong constant fails loudly rather than silently.

Findings

1. Misconfiguration is invisible, and the key is referenced nowhere but the code that reads it (supabase/functions/plugin-store/services/downloads.ts:34) - medium

PLUGIN_DOWNLOAD_IP_HASH_KEY appears in exactly two files in the repo: the reader and its test. No doc, no deploy workflow, no env template, and recordDownload swallows nothing because nothing is thrown - a missing key just writes ip_hash = null forever with no log line. The realistic outcome of this PR as written is that the secret is never provisioned, the field is null on every row, and nobody notices for months. This repo's own AGENTS.md makes the general case ("Refusals must stay visible"). A single console.warn naming the variable - never its value - plus a line in the deployment notes would close it. Fix this →

2. Two of the three validity checks turn plausible operator mistakes into total silent loss (downloads.ts:35) - medium

/\s/.test(secret) rejects the whole key for a trailing newline, which is the single most likely artifact of pasting a secret into a dashboard or piping a file. And secret.length > 256 has no security purpose at all: HMAC pre-hashes over-long keys, so a 512-character key is perfectly usable and is instead treated as "no analytics". Both mean a provisioned key produces null hashes. Suggest Deno.env.get(...)?.trim() then validate, drop or sharply raise the upper bound, and log on rejection. One rollout note if you take the trim: it changes the key material, so decide before provisioning, not after - trimming later silently rotates every hash.

3. The column comment now contradicts the code, and the two hash generations are indistinguishable (supabase/migrations/20260130000000_plugin_store_tables.sql:204) - low/medium

COMMENT ON COLUMN plugin_downloads.ip_hash IS 'SHA-256 hashed IP for privacy-preserving analytics' is the only in-repo description of this column and it is now wrong. More substantively, rows written before and after the deploy sit in one TEXT column with no discriminator and no backfill, so any count(distinct ip_hash) spanning the deploy boundary double-counts every returning IP. The PR body acknowledges rotation breaks correlation; a comment-update migration would make the boundary discoverable by whoever queries this next.

4. What is hashed is the raw x-forwarded-for header, not a parsed client IP (routes/download.ts:209, :356) - low, pre-existing, but this PR is the place to say so

The header is client-supplied and unnormalised, so a caller can rotate it to look like arbitrarily many distinct "IPs", and the new ip.length > 1024 cap adds a cheap opt-out: send a long enough header and the hash is null. Neither is a regression, and this is the same honest stance AGENTS.md already takes for browser telemetry ("indicative, not measurements"). Worth a comment on hashIp so the next reader does not treat these as unique-visitor counts.

5. isNotEmpty() does not cover the stated intent (PluginStoreClient.kt:36) - low

The PR says "ambiguous empty or dot-only segments are rejected", but a segment of only whitespace passes the guard and is sent as %20. isNotBlank() matches the intent and costs nothing; the new test's listOf("", ".", "..") could gain " ". The failure message also omits which segment failed - fine for not logging raw values, but an index would help debugging.

6. A new exception type on a surface that otherwise throws PluginStoreException (PluginStoreClient.kt:36) - low

No crash escapes today (verified above), but there is one observable change: getPlugin("") used to reach the store and return null, and now throws, so RemotePluginRepository.getPlugin returns Result.failure where it returned Result.success(null). That distinction is load-bearing in the dependency-plan path, where "the store cannot describe this plugin" is documented to contribute null and be attempted-but-not-expanded. A blank plugin id is a nonsense input either way, so this is a note rather than a defect - either wrap in PluginStoreException or state the thrown type in the KDoc.

7. Test coverage gap: the static routes are the untested half of the diff - low, cheap to close

The change rewrote ten literal paths by hand, including three that had to be split correctly (tags/popular, version/finalize, api-keys/:keyId) and three where ordering matters (admin/:pluginId/...). A typo there is a silent 404 at runtime, and nothing in PluginStoreEndpointTest would catch it - both path-asserting tests only exercise dynamic segments. I checked all ten against the route table in index.ts and they match today. The test server is already generic, so pinning list, search, tags/popular, health, publish, version/finalize and api-keys is a few lines. Fix this →

8. Residual risk the tests cannot reach: %2F must survive the deployed gateway - worth one manual check

The whole guarantee is that %2F stays encoded until Hono's router has matched, with req.param decoding afterwards. The test proves the client emits it and that a JDK HttpServer preserves it; it does not prove Supabase's edge gateway does not normalise %2F before routing, which some proxies do. If it decodes, a crafted id can still change the server-side route despite this fix. One curl against the deployed function with an encoded id would settle it, and the answer belongs in a comment either way.

9. Same class of issue left unfixed, out of this PR's scope - informational

buildSrc/src/main/kotlin/PublishPluginTask.kt:254 and :310 still interpolate pluginId straight into store URL paths, and :278 interpolates it unescaped into a hand-built JSON body. Input is a developer's own manifest in a Gradle task, so severity is much lower than the client - but it is now the only remaining instance, and the JSON one is arguably the worse shape.

Suggested gating

Nothing here blocks merge on correctness grounds. I would want 1 and 2 resolved before the secret is provisioned, because after provisioning they are only discoverable by noticing the column is full of nulls, and 8 answered before this is described as closing the route-confusion hole end to end.
| Branch: fix/security-plugin-store-inputs

[kshivang]

Review-agent commit e60f904 disables autoRefreshToken and persistSession only in the fake Supabase client used by download-hash.test.ts. The original new test leaked an auth-refresh interval under Deno test resource sanitization. With the fixture repaired, all 63 plugin-store tests pass using deno test --allow-env --allow-net --allow-read tests. Production URL encoding and separate HMAC remain @Raghav-Bansal-15's contribution. Kotlin tests and root ktlint/detekt are queued under the shared build lock; hosted CI is not yet validated.

[kshivang]

Assessed the completed review at 4cb1007. The existing test timer leak is fixed by e60f904. Bounded follow-up 7c14ae2 adds a once-per-isolate missing/invalid-key warning that names only the variable, tests the exact warning/count, and adds DOWNLOAD_ANALYTICS.md with provisioning, exact key-material semantics, historical SHA vs HMAC boundary, rotation, untrusted forwarded-header limitations and gateway validation scope. All 63 Deno tests pass.

Retained the deliberate 32-256 non-whitespace configuration contract: silently trimming key material would change hashes, and HMAC accepting longer keys does not make a documented configuration limit a correctness defect. Blank path values are encoded safely; only empty/dot segments are rejected for route ambiguity. The reviewer verified static routes correct, so speculative static-path churn and exception-type changes were not applied. The deployed gateway and broader Gradle publishing paths remain operational/cross-group context, not claimed fixed. Original production work remains @Raghav-Bansal-15's; these follow-ups are review-agent work. Kotlin validation queued; hosted fork workflow requires maintainer approval. No live service changed and no repeated review request.

[kshivang]

@Raghav-Bansal-15

Your centralized URL-segment encoding and independent HMAC key materially improve store request integrity and download analytics privacy. Claude found no working-flow correctness regression. Review-agent follow-ups fix the test client's leaked auth timer (e60f904) and add secret-free configuration diagnostics, regression assertions and rollout documentation (7c14ae2). All 63 Deno plugin-store tests pass at the repaired head; your original implementation remains credited and preserved.

Remaining gates are infrastructure/validation, not an unresolved demonstrated production defect. Hosted fork Build & Test requires maintainer approval and has not executed (the preceding repaired-head run 34736500478 is action_required; the current head must independently pass). Local plugin-repository tests and root ktlintCheck/detekt are still waiting for the shared build lock. No approval bypass was attempted.

Next steps: a maintainer must approve the current-head workflow, then require all checks green on 7c14ae2. Inspect the queued 571-gradle.log in the downloads result directory or run the command below through the shared lock. Resolve any actual failures, recheck current dev compatibility, then mark ready. Before rollout provision the documented independent key; preserve the exact key format rather than silently trimming it. A deployed gateway check is still needed before describing client encoding as an end-to-end gateway guarantee; no live service was exercised here.

Validation commands: python3 /Users/kshivang/Development/Boss/.pr-review-sessions/with-build-lock.py ./gradlew :plugin-platform:plugin-repository:desktopTest ktlintCheck detekt; and, from supabase/functions/plugin-store, deno test --allow-env --allow-net --allow-read tests.

Under the updated bounded-review policy I am marking this PR draft pending these gates, preserving the useful change and repaired tests. Workflow authorization and build-queue capacity are maintainer infrastructure issues, not author defects.

[Raghav-Bansal-15]

Review follow-up: Encode dynamic path segments and use a separate HMAC key for download analytics. Add safe configuration warnings, rotation/provisioning notes, and a fixture that does not leak an auth-refresh timer.

Validation: 63 store route tests pass locally. The current-head full fork CI passed. Production HMAC provisioning and deployed gateway behavior remain operational checks.

The PR is ready for review and the existing author/reviewer commits are retained. Upstream CI needs maintainer approval. I am following the latest-head checks and have not merged or released anything.

[kshivang]

@Raghav-Bansal-15 Your shared endpoint encoder and independent HMAC key remain useful, distinct improvements: dynamic IDs cannot change the client-emitted path structure, and analytics no longer derives its hash key from a predictable service-role JWT prefix. I checked all changed route call sites, the nullable download-recording consumers, and the retained warning/provisioning/rotation documentation. These belong in the host store transport and server analytics service; they do not add vault algorithms to the host.

Validation at 9373adae341ff21598d55c4db852824669321b7f: all 63 plugin-store Deno tests pass locally, git diff --check passes, and integration merge-tree against dev 600423381 succeeds. I verified full fork CI 34867351546 is green on that exact head, including OS builds/tests and code quality. The prior implementation and reviewer fixes are preserved; no new production defect or source repair was identified. #566 repairs publishing clients and remains complementary; #488 RLS and #599 health cancellation are separate work.

The remaining merge gate is upstream Build & Test 34867355227. I approved its pending fork workflow once; jobs are now queued, not green. This is maintainer infrastructure/execution, not an author defect. Let current-head upstream CI complete, address any actual failures, then refresh dev and recheck integration before marking ready. No check bypass or repeated Claude request is needed.

The applicable Claude findings retain their documented dispositions: secret-free warning and fixture timer cleanup are present; exact key-material validation is deliberate, and whitespace is safely encoded. Static routes still match the server. Before deployment, provision the documented independent key and verify missing-key downloads still work; a controlled gateway check must show encoded separators survive routing before claiming end-to-end gateway protection. These operational checks were not performed, and no secret or live service was accessed. Manual store detail/install/version/rating/admin UI checks remain unperformed.

Under the bounded review policy I am converting this PR to draft pending upstream validation. Original and prior reviewer contributions remain separately credited. No merge or auto-merge.

[kshivang]

@claude Please review current head 95e53e9825265dc7d898af3c2d301a20fbed6c79 against dev, bounded to this PR's publishing/store files. This is a topic-only replay after dev was rebased; the reviewed topic file contents equal the author's prior head. Read the prior review and handoff dispositions; report actual remaining correctness/regression/security issues, without repeating resolved findings or speculative rewrites. No code edits.

@Raghav-Bansal-15 Your original implementation and follow-up fixes are preserved with authorship. I removed the unrelated old-dev history (including cross-group health/CLI work) so this PR can integrate safely into current dev. Fresh CI is required; pending execution is not an author defect.

[claude]

Claude finished @kshivang's task in 6m 59s —— View job

---

Bounded re-review - PR #571 at 95e53e9

• Read the diff at current head against rebased dev (77c4a40, HEAD is a descendant)
• Re-verify every client path literal against dev's current route table (post-rebase drift check)
• Re-check endpoint() semantics, imports and all call sites
• Re-check downloads.ts hashing, warning gate, type widening and both route callers
• Re-check the two new tests for wiring and fragility
• Report only unresolved issues; no code edits

---

Verdict

The replay is clean. The three topic commits apply on top of current dev with no semantic drift, and I found no new correctness or regression defect in the reviewed files. One new low-severity residual is worth recording (#1 below) because it bears directly on the "route confusion" framing; everything else I checked either holds or was already dispositioned.

Re-verified against current dev (these are the things a rebase could have broken)

• All 16 client paths still match the server route table. I re-read routes/{browse,download,rating,publish,admin,api-keys}.ts and index.ts at this head: /list, /search, /{pluginId}, /tags/popular, /{pluginId}/download, /{pluginId}/download/{version}, /{pluginId}/rate, /health, /publish, /{pluginId}/version, /version/finalize, /admin/{pluginId}/publish, /admin/{pluginId}, /admin/{pluginId}/verify, /api-keys (×2), /api-keys/{keyId}. No literal in the hand-rewritten set is stale. Checked for cross-method shadowing between /admin/{pluginId} and /{pluginId}/download too - the methods differ, so no route is captured.
• Every HTTP call now goes through endpoint(), and the one that does not is correct: uploadJar's httpClient.put(uploadUrl) takes a server-issued signed URL, which must not be re-encoded (PluginStoreClient.kt:320).
• encodeURLParameter resolves via the existing io.ktor.http.* wildcard (PluginStoreClient.kt:12) - no missing import; spaceToPlus = false is still right for a path segment.
• The string | null widening is still type-safe end to end at this head: routes/download.ts:209 and :356 assign into string | null, recordDownload's ipHash: string | null = null (downloads.ts:10), and both calls sit inside try/catch that never fails the download.
• @std/assert is still mapped in supabase/functions/plugin-store/deno.json, so the bare specifier type-checks under deno check; the plugin-store CI job's deno test --allow-all picks the new file up.
• The empty-segment require is outcome-neutral for the real callers. I traced the two that can plausibly receive a blank dynamic value: PluginStoreSetup.fetchStoreSignature (:984) already funnels any throw into StoreSignatureOutcome.Unavailable, which is what a trailing-slash 404 produced before, and RemotePluginRepository's downloadInfoProvider (:45/:47) returns Result.failure either way. The one behaviour change remains getPlugin(""), already dispositioned.
• Dot-segment traversal is genuinely closed, not just for the literal cases the test pins. . and .. are rejected, and anything else that could normalise ("../x", "%2e%2e") survives as a single encoded segment because / and % are both encoded - so no emitted path contains a dot segment a parser could collapse.

Findings

1. Reserved route names are still legal plugin ids, so single-segment route confusion is not closed by encoding - it cannot be (supabase/functions/plugin-store/types/schemas.ts:152) - low, new

PublishPluginRequestSchema constrains pluginId to min(3).max(100).regex(/^[a-z0-9.-]+$/i). Every character in that class is URL-unreserved, so encodeURLParameter is a no-op on it - which is the good news for compatibility, and the reason this one is out of the fix's reach. It also means health, list, doc and openapi are all publishable ids, and getPlugin(id) is the one endpoint whose dynamic segment is the whole path (endpoint(pluginId) → GET /plugin-store/{id}). Hono prefers a static route over a param, so getPlugin("health") gets the health handler's payload and getPlugin("list") gets a PluginListResponse, in both cases decoded as PluginDetailResponse - a SerializationException outside the if (!isSuccess) guard, surfacing as Result.failure rather than the null the dependency-plan path expects for "the store cannot describe this plugin".

Not a privilege issue (both sides are unauthenticated GETs) and not a regression from this PR - but the PR is described as addressing identifiers changing the request path, and this is the shape of that which survives. It is a server-side fix, not a client one: a reserved-name refusal in the publish schema, or a /plugins/{id} prefix. Worth an issue rather than a change here. Fix this →

2. The warning assertion is coupled to module-load order, not just to this test (supabase/functions/plugin-store/tests/download-hash.test.ts:17-19) - low, new, test-only

assertEquals(warnings, [ ... ]) is an exact-array match, and the thing that makes it one element is warnedAboutHashKey, module state in downloads.ts. It is correct today for two reasons that are not visible at the assertion: the warning test is first in its file, and nothing else in the suite calls hashIp. Add a test above it, or a route-level test that exercises routes/download.ts with the key unset, and this fails as a false positive with an empty warnings array and no hint about why. Asserting the last warning, or resetting the flag through an exported test hook, would decouple it. Not worth changing on its own; worth knowing before the next test lands in this file.

Previously dispositioned, re-checked, unchanged

Recorded so nobody re-opens them: the exact 32-256 non-whitespace key contract (deliberate, documented in DOWNLOAD_ANALYTICS.md including the "trimming later silently rotates every hash" consequence); isNotEmpty over isNotBlank (whitespace encodes to %20 and stays one segment); IllegalArgumentException rather than PluginStoreException; the migration column comment addressed by documentation rather than a comment migration; and the deployed-gateway %2F check, which remains an operational item the test suite cannot reach. The doc's scoping of that last one ("the deployed gateway's handling of encoded slashes remains an operational check") is accurate and I would keep the claim worded exactly that narrowly.

buildSrc/src/main/kotlin/PublishPluginTask.kt:254, :278 and :310 still interpolate pluginId raw into paths and into a hand-built JSON body on current dev - unchanged from the prior review, out of this PR's bounds, and #566 territory.

Validation I could not run here

This sandbox blocks Gradle and Deno, so :plugin-platform:plugin-repository:desktopTest, ktlintCheck, detekt and the 63 Deno tests were not executed in this job - nor was the HMAC vector at download-hash.test.ts:46 independently computed (a wrong constant fails that CI-run test loudly rather than silently). Line lengths in both Kotlin files are inside the 140-column .editorconfig limit, but that is not a substitute for ktlint. Upstream CI on this exact head remains the gate.
| Branch: fix/security-plugin-store-inputs

[kshivang]

@Raghav-Bansal-15 Bounded review checkpoint at 95e53e982: your original implementation, prior reviewer fixes, and your follow-up commits are preserved. The replay onto rebased dev removes unrelated old-dev history and leaves only the intended 5 store files. Topic file contents match your submitted head; no new production repair or author defect is attributed to this replay.

Validation: 63 plugin-repository tests, 63 Deno tests, and root ktlintCheck/detekt pass locally under the shared build lock for Gradle. Both scoped heads pass diff checks and integrate cleanly with latest dev (the subsequent doctor-output change does not affect these paths). Prior substantive review findings retain the documented fixes/dispositions. One current-head Claude review was requested and is still running.

Upstream full Build & Test 34959541979 is running after approval. This is pending validation, not a failing author test. Keeping this PR ready/open on dev; no draft conversion, merge or auto-merge. Before merge, inspect the completed current-head review and all required checks, refresh dev under the shared merge lock, and use the exact verified head. The earlier real-store/gateway operational checks remain explicitly unperformed; no production publish, secrets or app operation was used for validation.

[kshivang]

Completed current-head review assessed: no blocking correctness/regression finding. Reserved single-segment route names are a pre-existing server schema/routing limitation, not introduced by the endpoint encoder; this PR only guarantees encoding of dynamic separators on emitted client requests. Changing route naming/schema would expand scope and is not needed for this improvement. The once-per-isolate warning test has no competing hashIp caller in today's suite; exposing/resetting production state for hypothetical future tests is unnecessary. Existing deployment/gateway limitations remain documented. No speculative rewrite or repeat review request.

The exact head 95e53e9 has all upstream CI green (34959541979), and the previous 63 repository / 63 Deno tests plus ktlint/detekt pass. Revalidating the combined current-dev tree before serialized dev merge. Original @Raghav-Bansal-15 contribution and prior reviewer repairs remain separately credited.

[kshivang]

@Raghav-Bansal-15 Merged into dev as e75d1e5 after completed review and green exact-head CI. Under the shared merge lock, rechecked dev/head/state/checks and used squash with --match-head-commit 95e53e9. No main merge or bypass.

Fresh combined-dev repository tests and root ktlint/detekt completed successfully via non-TTY execution; previous 63 Deno tests remain applicable. Original and prior reviewer authorship stays credited. The pre-existing reserved-route-name limitation and documented gateway/HMAC provisioning checks remain operational follow-up context, not claims that this PR solves server routing or deploys secrets.