Repository navigation
Conversation
|
@claude review this publishing hardening PR, including Gradle, Python and PowerShell request serialization, credential handling, and compatibility with existing publishing callers. |
|
Claude finished @kshivang's task in 7m 27s —— View job Review: publishing hardening (Gradle / Python / PowerShell)
The direction is right and several of these changes fix real breakage, not just theoretical exposure. My substantive concern is not the security work - it is that the same commit makes publishing failures almost undiagnosable, in a flow whose server side goes out of its way to explain itself. What this genuinely fixes
1. The Kotlin task swallows every message the PR just added
So an operator who points The Python side gets this right: 2. Dropping the server's
|
| Store response | What the publisher now sees |
|---|---|
homepageUrl must be a valid URL (required for publishing) |
HTTP 400 |
Plugin ID already exists |
HTTP 400 |
Version already exists |
HTTP 400 |
SHA-256 mismatch: client reported X, server computed Y... (publish.ts:545) |
HTTP 400 |
Manifest identity mismatch: JAR declares X@1.2.3 but the version row is X@1.2.4... (publish.ts:562) |
HTTP 400 |
The stated rationale is that bodies may embed signed URLs. Note who the parties are: the body is the store's answer to a request the operator authenticated, and that operator already holds the bearer token and the signed upload URL. There is no third party gaining anything from error reaching their own console. The last two rows are the worst loss - they are the diagnostics for "your jar is not what you think it is", and they are the reason a publish fails in a way nothing local explains.
extractJsonValue is still present and already parses through JsonSlurper, so surfacing only the parsed error string (never the raw body) is a small change. Failing that, log it at --info. Fix this →
3. The exact-404 existence gate is right, and compounds #2
GET /{pluginId} does return 404 for an unknown plugin (routes/browse.ts:206), so requiring exactly 404 before creating is correct and an improvement over "any non-200 means create". One case to be aware of: browse.ts resolves through the storefront view, while getPluginForPublish deliberately sees unpublished rows (publish.ts:141-146). An unpublished or org-scoped plugin therefore 404s on the check, falls into create, and gets Plugin ID already exists - which, after #2, reads as a bare HTTP 400. Fixing #2 covers this; no separate change needed.
4. Redirect refusal on the signed upload is unvalidated against the real store
instanceFollowRedirects = false, -MaximumRedirection 0 and NoRedirects are applied to the upload PUT as well as the API calls. For the API calls the reasoning is sound. For the upload there is no credential in a header - only the signed URL - and the old code followed redirects by default. If Supabase storage ever answers a signed upload URL with a 307/308, publishing now dies at the upload step with HTTP 307. The fixtures only cover a local server that is asserted to fail on redirect, so this is exactly the property a local fixture cannot settle. Worth one real publish against the store before merge; if a redirect does occur, the upload needs a same-host redirect allowance rather than a flat refusal.
5. The new Python suite never runs
build.yml wires each script test individually (node scripts/test/test-swipe-nav.js at :105, python3 scripts/test/prepare-totp-migration-fixtures.py at :404, and so on). scripts/test/test-publish-plugin.py has no step, so it will not run in CI - and the PowerShellPublisherTest subclass stays skipped without BOSS_TEST_PWSH. The buildSrc test is covered by the existing ./gradlew -p buildSrc test step (ubuntu-only), so only the Python one needs adding. Fix this →
6. The PowerShell entry point rejects the legacy token only by accident
The bash shim rejects --token explicitly with exit 2 and a message. publish-plugin.ps1 has no such check - test_rejects_legacy_token_arguments passes only if PowerShell's binder happens to refuse the argument. With [CmdletBinding()] (:46) and positional binding left on, only $JarPath declares a Position; if the binder does assign implicit positions, a stray --token <value> can land in -Version and -AnonKey, which would put the store token into the apikey header. Even if PS 7 errors today, the contract should be stated rather than inherited - [CmdletBinding(PositionalBinding=$false)] plus an explicit check matching the shim. Fix this →
Related, same file: Assert-PublishingUrl is called inside the try in Invoke-PluginStoreRequest, so an invalid URL is reported as Request failed with no status - the same message-loss shape as #1. Move it above the try or rethrow it.
7. Smaller points
local.propertiesvalue is not stripped (publish_plugin.py:106-109). The old bashcutwas equally sloppy, but the new\r/\nheader guard turns a CRLF-authored file into a hard failure readingInvalid newline in a publishing header, which names the symptom and not the file..strip()it.- Homepage fallback misses
url. The Kotlin path readsmetadata["homepageUrl"]thenmanifest["Plugin-Url"]; Python readshomepageUrlthenPlugin-Url. The store's own GitHub path acceptsmanifest.url || manifest.homepageUrl(publish.ts:752,:778), andPlugin-Urlappears nowhere else in this repo. If plugin.json files inboss_pluginscarry the homepage underurl, every one of them needs a new flag it did not need before. Cheap to also readurl. python3is now a hard dependency ofscripts/publish-plugin, where before it was curl + shasum + unzip. The failure message is clear, but on a mac without Command Line Tools/usr/bin/python3is a stub that prompts rather than runs. Worth a line in the plugin-publishing docs, which this PR does not touch at all - the env-only token and thehomepageUrlrequirement are both things a plugin author needs told.-PpluginStoreTokennow silently does nothing. Gradle ignores unknown-Pproperties, so a caller passing it and not exporting the env var gets the clear "Set BOSS_PLUGIN_STORE_TOKEN" message - good. But anything configuringauthToken.set(...)fails at configuration time with an unresolved reference. Nothing in this repo does; flagging in case a plugin repo copied the wiring.- Test fragility:
PublishPluginTaskTestasserts on Gradle's exact"Configuration cache entry discarded"wording, which is not a stable contract.codeSource.location.toURI().pathalso yields/C:/...on Windows, harmless today since the buildSrc step is ubuntu-only. - Nits:
PublishPluginTask.kt:10-13interleavesjava.*imports into theorg.gradle.*block (buildSrc is outside theallprojects { }ktlint application, so CI will not catch it, but it reads oddly);localGroovy()pulls the whole Groovy distribution onto the buildSrc compile classpath for two JSON classes thatgradleApi()already provides at runtime.
Verification I could and could not do
- Cross-checked all three clients against
supabase/functions/plugin-store(routes/publish.ts,routes/browse.ts,types/schemas.ts) and against.github/workflows/build.yml. - Confirmed by reading:
homepageUrlis server-required;GET /{pluginId}404s on unknown; buildSrc tests do run in CI and the new Python suite does not; buildSrc is not ktlint-gated. - Not run:
python3 scripts/test/test-publish-plugin.pyand./gradlew -p buildSrc test- both were refused by this environment's sandbox, so I am reporting a read of the tests, not a pass. The PowerShell binder behaviour in security: stop DigiCert signing secrets leaking into public CI logs #6 is reasoning about PS semantics, not an observation; PS 5.1 stays unverified as the PR says.
Nothing here argues against the approach. #1 and #2 are what I would want changed before merge, #4 is one manual publish, and the rest are small.
· branch fix/security-publishing-inputs
|
@Raghav-Bansal-15 Review handoff for #566. Your original publishing hardening remains preserved; review fixes are pushed through Addressed: omitted optional authorName instead of sending null (the store schema rejects null); explicit UTF-8 PowerShell JSON; Python/PowerShell HTTP fixtures wired into CI with PowerShell required; safe Kotlin validation reasons retained with a non-HTTPS regression assertion; plugin.json.url homepage fallback; disabled implicit PowerShell positional binding; actionable PowerShell URL validation text. Claude disposition: findings 1 and 5 are addressed, as are the actionable URL-diagnostic/homepage/positional-binding follow-ups. I did not restore arbitrary server error strings (#2): server error fields can contain exception text and submitted metadata, and publisher output can become shared CI logs. Status-only server errors remain deliberate; a future allowlist of stable safe error codes would be a separate improvement. The CRLF local.properties concern does not reproduce by inspection: Python read_text/splitlines removes line endings. No repeated Claude request is needed. Outstanding validation, not demonstrated author defects:
This is not merge-ready until those validations are recorded. Per the review handoff policy I am converting this unresolved PR to draft. Original contribution credit stays with you; the above review repairs are separate. No merge or auto-merge. |
|
@Raghav-Bansal-15 Final disposition recheck under the newly authorized dev-merge policy: your useful serialized-payload, environment-credential and streamed-upload work remains preserved at Good validation update: final-head Shell Script Tests passed. The actual CI log confirms 10 publisher tests passed with no skips, including Python and PowerShell 7. This resolves the earlier PS7 fixture gap. I cannot merge yet: final-head run https://github.com/risa-labs-inc/BossConsole/actions/runs/34736689568 still has required OS build/code-quality/compatibility/database jobs queued or running. This is pending infrastructure/execution, not evidence of an author defect. No required checks will be bypassed. The prior handoff still documents Windows PowerShell 5.1 and the controlled real-store signed-upload smoke validation; record those outcomes before declaring this fully prepared. Next steps: obtain final-head green CI (including buildSrc tests, ktlint and detekt), verify PS5.1 Unicode payload and legacy-token rejection, and verify a disposable authorized plugin reaches upload/finalize without redirects. Latest Claude findings and rejected suggestions retain the evidence-based disposition in the prior handoff; no new review request or speculative redirect relaxation is needed. Refreshed dev remains |
cd88490 to
5a2e818
Compare
6a26181 to
072a364
Compare
|
Rebased this PR and its reviewer fixes onto current dev, preserving authorship, and marked it ready for review. The current-head full fork build and test run is green. The publishing fixtures retain optional-author omission, UTF-8 payloads, and safe diagnostics. A controlled real-store upload and Windows PowerShell 5.1 remain operational checks, not claimed by this CI result. Upstream CI still requires maintainer approval. I am following the remaining PR checks; nothing has been merged. |
|
The Windows fixtures exposed three concrete problems: PowerShell 5.1 could lose Get-FileHash through inherited module paths, return a redirect without throwing, and receive JSON as bytes. The publisher now hashes with .NET, verifies HTTP status, and decodes byte responses before parsing. All 10 Python/PowerShell fixtures pass locally with PowerShell 7.6.6. Current head 6c9258b is pushed; dedicated Windows 5.1/7 checks remain queued. No real-store upload is claimed. |
|
@Raghav-Bansal-15 Thank you for preserving the serialization/environment-credential hardening and following through on the Windows-specific failures. I independently checked the .NET streamed checksum, explicit non-2xx rejection, and byte-response UTF-8 decoding. Your current-head fork Build & Test run 34874351136 is green, and run 34874351324 really executes and passes five fixtures on each of Windows PowerShell 5.1 and PowerShell 7. The previous PS5.1 validation gap is resolved. Local validation at Remaining merge gate: upstream Build & Test 34874356269 and Publishing regressions 34874356338 were awaiting maintainer approval. I approved both once; they are now queued/running, not green. This is an infrastructure/execution gate, not an author defect, and the green fork run does not bypass it. Please let those current-head checks finish, resolve only actual failures, and refresh/revalidate against the then-current dev before marking ready again. The prior Claude review and accepted repairs remain applicable. Safe guard diagnostics and CI wiring are retained; arbitrary server error text stays suppressed because shared CI logs can expose submitted values. CRLF trimming and speculative redirect relaxation remain unwarranted. No repeated Claude request. The remaining controlled signed-upload/finalize smoke check is still unperformed: use an authorized disposable plugin to verify special-character metadata, omitted author/homepage handling, exact JAR hash and successful upload without redirects. Do not use live credentials in arguments or weaken redirect refusal. No production publishing or desktop visual check was performed here. Under the bounded review policy, I am converting this PR to draft while upstream validation remains incomplete. Your original contribution and subsequent Windows repairs remain credited; prior reviewer repairs are separate. No merge or auto-merge. |
13e5984 to
59eaec9
Compare
|
@claude Please review current head @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 finished @kshivang's task in 5m 16s —— View job Review:
|
| Path | Homepage resolution |
|---|---|
| Kotlin | homepageUrl property → plugin.json url → plugin.json homepageUrl → manifest Plugin-Url (PublishPluginTask.kt:205) |
| Python | --homepage-url → url → homepageUrl → Plugin-Url (publish_plugin.py:133) |
| PowerShell | -HomepageUrl only, otherwise exit 1 (publish-plugin.ps1:379-382) |
So the same JAR against the same store publishes from Gradle, macOS and Linux and hard-fails on Windows with "Provide -HomepageUrl when creating a new plugin", even though its plugin.json declares the homepage. The data is already in hand: the script opens the JAR and parses plugin.json at :422-440 for minBossVersion, just after the creation step that needs it. Hoisting that read above step 3 gives the same chain the other two use. Fix this →
2. A single tag serialises as a JSON string on the PowerShell path
publish-plugin.ps1:386:
$tagsArray = $Tags -split "," | ForEach-Object { $_.Trim() }Assigning a pipeline that emits one object yields a scalar, not a one-element array, so -Tags "devtools" renders "tags": "devtools". The store declares tags: z.array(z.string().max(50)).max(10) (supabase/functions/plugin-store/types/schemas.ts), so that is a validation failure, and with #2's status-only policy the publisher prints a bare HTTP 400 with nothing to go on. Kotlin (split(",").map) and Python (list comprehension) both always produce arrays.
The assignment predates this PR, so it is not a regression - but correct serialisation across all three clients is this PR's stated subject, and the new fixture matrix is where it would have been caught: test-publish-plugin.py:84 always passes three tags (alpha,"quoted",雪), so the array case is green and the scalar case is untested. $tagsArray = @($Tags -split "," | ForEach-Object { $_.Trim() }) fixes it.
Related and also untested: the -Tags "" case sends @() through ConvertTo-Json on Windows PowerShell 5.1, whose empty-collection rendering has historically differed from PS 7. Since 5.1 support is the point of the new Windows job, a fixture that publishes with no tags on both engines is worth having either way. Fix this →
3. The new Windows workflow is pinned to this topic branch
.github/workflows/publishing-regressions.yml:6-7:
on:
push:
branches: ['fix/security-publishing-inputs']Once this merges, that push trigger can never fire again, and the pull_request trigger carries a paths filter limited to scripts/publish-plugin*. The PowerShell 5.1 coverage this PR adds - the validation the handoffs have been chasing - would then run only on PRs that themselves touch the publisher, and never on dev. If that path-scoped-PR-only shape is deliberate, it is worth saying so in the file, since the branch pin reads like leftover topic scaffolding; if it is meant to guard dev, the branch list should name dev/main. Note also that a paths-filtered workflow must not be made a required check, since it will never report on PRs that skip those paths.
I can't edit .github/workflows/** myself (GitHub App permissions), so this is a flag only.
4. The publisher fixture step fails rather than skips when pwsh is absent
.github/workflows/build.yml:
BOSS_TEST_PWSH=$(command -v pwsh)GitHub runs run: blocks under bash -e, and command -v pwsh exits 1 when pwsh is missing, which fails the assignment and therefore the step - with no message explaining why. pwsh is preinstalled on ubuntu-latest today so nothing is broken now, but the suite already has a skip path for this exact case (test-publish-plugin.py:144), and $(command -v pwsh || true) would use it instead of turning an image change into a red build.
5. Redirect refusal on the signed upload is still validated only by reading
This is the one item the handoffs list as outstanding, and the fixtures do not close it. setUp's redirect flag applies to every request, so test_refuses_redirect_without_forwarding_credentials fails at the existence check and asserts exactly 1 request - the PUT is never reached. So instanceFollowRedirects = false (PublishPluginTask.kt:465), -MaximumRedirection 0 (publish-plugin.ps1:472) and NoRedirects (publish_plugin.py:24) on the signed upload remain unexercised in all three clients.
A per-step redirect flag (redirect only the PUT) would settle the client behaviour locally and cheaply. It still would not answer whether Supabase storage ever 307s a signed upload, which is what the real-store smoke check is for - but it would separate "the client refuses correctly" from "the store never asks", which are currently entangled.
6. Smaller points
- Confirmed fixed, not a finding: every guard now throws
GradleException(:151,:189,:206,:438,:461,:463) andPublishPluginTaskTest.kt:110asserts the non-HTTPS reason survives. The generic wrapper at:121-126deliberately attaches no cause, soJsonException's embedded document cannot reach the console - worth keeping that way, since attaching a cause "for debuggability" would silently undo ⬆️ actions:(deps): Bump actions/setup-java from 4 to 5 #2. authTokenis gone from the task (it isdev's:97). Nothing in this repo wires it, and the task class lives in this repo'sbuildSrcso no external repo can reference the type - but a plugin repo carrying its own copy ofplugin-publish.gradle.ktswithauthToken.set(...)fails at configuration time, not with the new "Set BOSS_PLUGIN_STORE_TOKEN" message. Worth a heads-up wherever those repos are tracked.PublishPluginTaskTest.kt:97still asserts Gradle's exact"Configuration cache entry discarded"string.notCompatibleWithConfigurationCacheis the contract; that wording is not.-Ptags=(present but empty) sends[""]from the Kotlin path (:207), where Python guards it withif args.tags else []. Cosmetic.Get-ManifestValue(:151-178) and the step-4plugin.jsonread (:422-440) leak theZipFileon a throw - nofinally, and thecatchswallows. On Windows that holds a lock on the JAR for the rest of the process. Only reachable on a malformed archive.- Manifest-derived values are never exercised. Both suites pass
--plugin-id/-PluginIdexplicitly, so the three manifest readers (Kotlinjava.util.jar.Manifest, Python's hand-rolled continuation unfolding atpublish_plugin.py:80, and the PS regex at:165) are untested - including the 72-byte continuation wrapping that Python is the only one to handle at all. Dropping the explicit ids from one fixture would cover it.
Verification
- Read: all 10 changed files at
59eaec9,origin/dev's priorPublishPluginTask.kt,supabase/functions/plugin-store/types/schemas.ts, and both workflow files. - Confirmed by reading:
tagsisz.array(...)server-side;homepageUrlis required with no.optional(); no doc indocs/or any*.mdreferencespublish-pluginorBOSS_PLUGIN_STORE_*, so the earlier docs finding has nothing in this repo to land on;authTokenexisted on the task indev. - Not run:
python3 scripts/test/test-publish-plugin.pyand./gradlew -p buildSrc testwere both refused by this environment's sandbox, and there is nopwshhere. So findings 1, 2 and 6 are reads of the code, not observed failures - in particular the single-tag serialisation is PowerShell semantics plus the store schema, not something I executed. Your CI evidence of 10 passing fixtures is consistent with all of it: the array-tags case is exactly what those fixtures pass.
Nothing here is a security regression. #1 and #2 are the two I would want before merge; #3 and #4 are CI durability; #5 is a coverage gap on the property still listed as unvalidated.
· branch fix/security-publishing-inputs
|
@Raghav-Bansal-15 Bounded review checkpoint at Validation: 40 buildSrc tests pass; five Python publisher tests pass locally (five PowerShell cases skip because pwsh is absent). Exact-head upstream Publishing regressions 34959531887 passes both Windows PowerShell 5.1 and 7. 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 34959531920 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. |
|
Follow-up at Original and reviewer commits are preserved. Remaining current-head hosted checks and upstream approval are separate gates; no merge or release. |
|
@Raghav-Bansal-15 I reviewed your follow-up 70a112d. The homepage chain now matches Gradle/Python (explicit option, plugin.json url/homepageUrl, manifest), the JSON tags field remains an array for zero/one/many entries, and archive cleanup now runs in finally. New fixture coverage explicitly reaches the signed PUT redirect and verifies no subsequent request or Authorization header. These are your fixes, separately credited from earlier reviewer work. This addresses substantive completed-Claude findings 1 and 2 and improves finding 5's script coverage. Finding 4's missing-pwsh failure is deliberate: hosted publisher fixtures must not silently skip PowerShell. Finding 3 is optional CI durability: the path-scoped pull_request trigger covers publisher edits after merge; the topic push trigger supports fork checks. No required path-filtered check is being introduced. Gradle signed-upload/live-store smoke remains an operational limitation; no claim of deployed redirect behavior is made. Optional broader manifest/parser rewrites remain outside this bounded follow-up. Approved the new exact-head upstream Build & Test 34977965329 and Publishing regressions 34977965334 once. Will require completed green checks before dev merge; no production publishing or bypass. |
|
@Raghav-Bansal-15 Merged into dev as 95fb5e4 after reviewing your homepage/tag follow-up and all final-head checks passed. The eight fixtures pass on each Windows PowerShell 5.1/7 engine, and full upstream Build & Test 34977965329 is green across all platforms, code quality, compatibility and database checks. Under the shared merge lock, refreshed dev/head/checks, verified the merge-tree, and used --squash --match-head-commit 70a112d (separate CLI argument). No bypass or main merge. Your original implementation and follow-up fixes remain explicitly credited; earlier reviewer repairs are separate. Operational real-store upload/gateway checks remain unperformed and are not represented as completed by local HTTP fixtures. |
Publishing assembled request bodies by hand and exposed bearer credentials in command arguments. Some optional metadata and PowerShell payload shapes also failed server validation.
Serialize metadata, read bearer credentials from the environment, stream checksums/uploads, and encode dynamic path values. PowerShell now resolves homepage metadata consistently with Python/Gradle and preserves zero/single/multiple tags as JSON arrays. HTTP fixtures cover metadata, omitted author, exact upload/hash/finalization, credential arguments and redirect refusal, including signed-upload redirects.
Original implementation and later Windows/hash/status/byte-response/homepage/tag fixes: @Raghav-Bansal-15. Prior review-agent repairs remain separately authored (optional-author omission, safe diagnostics, explicit UTF-8 and CI wiring). The review replay onto rebased dev preserved authorship and removed unrelated old-dev integration history; it adds no new contributor implementation.
Validation on 70a112d: eight Python fixtures pass locally, eight PowerShell cases skip locally because pwsh is absent. Exact-head upstream Windows PowerShell 5.1 and 7 publisher fixtures pass. Earlier 40 buildSrc tests remain applicable because the follow-up changes only the PowerShell script and Python fixture; full exact-head CI is checked separately before merge.
No production publishing or secret provisioning performed. Deployed signed-upload/finalization behavior remains an operational check; local fixtures establish client redirect refusal, not what a production gateway returns. Hosted PowerShell presence remains mandatory; the dedicated workflow's path-scoped pull_request trigger preserves Windows fixture coverage after merge.