[Goldloli]

Refs #746.

Download in internal/sdk/sdk.go only treated 404 as an error and saved the response body as the archive for every other status, so a mirror answering 403/429/500 ended the install later with Checksum error or zip: not a valid zip file while never saying the server refused the request.

The status check lives in one helper, util.UnexpectedStatusError (internal/shared/util/status.go), used by every path that downloads a file:

• internal/sdk/sdk.go — Download (SDK archives);
• internal/manager.go — downloadPlugin (plugins from a URL or the registry);
• internal/plugin/luai/module/http/http.go — http.download_file (plugins);
• cmd/commands/upgrade.go — downloadFile, and now fetchLatestVersion as well, so a 403/429/500 from the tags page names the status instead of failing with Failed to fetch the version.;
• internal/shared/util/downloader.go — Downloader.Download.

The error reads unexpected HTTP status 403 Forbidden for <url>.

URL credentials in messages and logs. util.RedactSourceURL now masks a user name without a password (****@host, the way pip prints it), keeps user:xxxxx@ for a user name with a password, masks query values while keeping the keys (a query part without =, which carries no key, is masked as a whole), and for a scheme:// string that url.Parse rejects (e.g. a password with a bare %) hides everything between :// and the last @ and drops the query. Local paths and Windows paths pass through unchanged.

The errors that net/http and net/url build print the URL themselves — net/http masks only the password and net/url quotes the input it rejects — so internal/shared/util/status.go gets two helpers: util.RedactURLError returns a copy of a *url.Error with the URL redacted and, for a parse error whose URL had something to hide, a reason that does not quote the rejected input, and util.RedactError applies it to any error and passes other errors through. Every call site uses util.RedactError: Manager.fetchPluginManifest, Manager.downloadPlugin, Manager.Available, Sdk.Download (including its request build error), moveRemoteFile, the Lua http.get / http.head / http.download_file, and Downloader.Download.

The places that format a URL, or a name taken from it, no longer print secrets either: installPluginToTemp prints the extension of the redacted path, http.download_file labels the progress bar from req.URL.Path, moveLocalFile redacts the progress line and the *fs.PathError path, the offline development transport redacts the request URL, and downloadPlugin names the saved file from the URL path — the last path element only when it already ends with the accepted extension, plugin plus that extension otherwise. That rule also fixes a panic: installPluginToTemp accepts a URL by the extension at the end of the whole URL, so a URL whose .zip lives only in the query (/dl?token=...&file=plugin.zip) used to save the file as dl and crash on a nil decompressor in util.NewDecompressor(...).Decompress(...). archiver.decompress now returns unsupported archive type: <ext> for a name no unpacker is registered for instead of panicking the same way.

Update via manifestUrl. Manager.Update fell back to the manifestUrl manifest when the registry answered 404 but then returned the registry's ManifestNotFound anyway (the du, err := shadowed err), so a plugin installed from outside the registry could never update. The fallback manifest now goes into the variable that holds the registry result and the version check runs once for both; the error wording stays as on main — not support update only for a plugin without a manifestUrl, manifest not found when the fallback 404s too. Covered by TestUpdateInstallsManifestUrlUpdate (registry 404, manifest offers 9.9.9, installed demo@0.0.1 ends with 9.9.9) plus tests for each other outcome of the block: no manifestUrl, fallback 404, already the latest.

AGENTS.md "Shared utilities" records the rules for later changes: a download checks the status with util.UnexpectedStatusError before the file is created, a URL that goes into an error, a message or a --debug line goes through util.RedactSourceURL, and the error of a request or of url.Parse goes through util.RedactError.

Tests, all local (httptest, an in-memory RoundTripper, or a refused local port):

• internal/shared/util/status_test.go: TestRedactSourceURL table (password, user name without password, query, query parts without =, password with query, nothing to hide, URLs url.Parse rejects including a leading-digit scheme, /tmp/plugin.zip, ./plugin.zip, /tmp/100%/plugin.zip, a local path containing ://, C:\plugins\plugin.zip, C:/plugins/plugin.zip, empty), TestRedactURLError (connection error, parse error, parse error without secrets keeps the reason) and TestRedactError (plain, wrapped and non-URL errors); downloader_test.go adds redacted request/parse errors;
• internal/manager_test.go: fetchPluginManifest, downloadPlugin (request and connection errors), Available and installPluginToTemp each with a user name, password and query token in the URL; downloadPlugin file naming from the URL path and the plugin fallback; manager_update_test.go adds the Update-via-manifestUrl test above plus TestInstallPluginToTempNamesFileWithAcceptedExtension, which installs a plugin zip through installPluginToTemp for /plugin.zip, /dl/?file=plugin.zip, /dl.php?file=plugin.zip, /download.php?id=7&name=plugin.zip and /dl?token=abc123&file=plugin.zip (four of them panic without the fix);
• internal/plugin/luai/module/archiver/archiver_test.go: decompress of an unknown extension returns unsupported archive type;
• internal/sdk/download_test.go: Download request build error and connection error, moveRemoteFile parse error, moveLocalFile progress line and *fs.PathError redaction (captured via pterm.SetDefaultOutput);
• internal/plugin/luai/module/http/http_test.go: get/head/download_file request and connection errors redacted, and the progress-bar label taken from the URL path;
• internal/plugin/development_test.go: the offline transport redacts the URL;
• cmd/commands/upgrade_download_test.go: fetchLatestVersion names a 429.

Verification on this machine:

go build ./...                                                                                    # passes
go test ./internal/shared/util ./internal ./internal/sdk ./internal/plugin \
  ./internal/plugin/luai/module/http ./cmd/commands                                               # pass

Full go test ./... still hits two pre-existing, environment-dependent groups: internal/plugin/luai/module/http TestGetRequest/TestHeadRequest fetch the live plugin site and time out from this network, and internal/shared/printer waits on a keyboard listener in a non-interactive shell.

New Go files carry the Apache 2.0 header.

Not addressed here: #746 also reports that SDK downloads use Go-http-client/1.1 while plugin requests send vfox/<version> <plugin>/<version>. That is a separate behavior change, so this PR uses Refs #746 and leaves the issue open for it.

[codecov]

Codecov Report

❌ Patch coverage is 97.24138% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.83%. Comparing base (cdf0747) to head (3c8b4cb).

Files with missing lines	Patch %	Lines
internal/manager.go	89.74%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #747      +/-   ##
==========================================
+ Coverage   51.87%   54.83%   +2.96%     
==========================================
  Files         111      112       +1     
  Lines        8276     8368      +92     
==========================================
+ Hits         4293     4589     +296     
+ Misses       3561     3326     -235     
- Partials      422      453      +31     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[linghengqian]

Thanks for the PR. Rejecting anything but 200 before the file is opened is the right fix, and the new tests do what the description says: on main the 403, 429 and 500 cases fail with error = nil, on this branch they pass. With the repro from #746, those three now stop at the download and name the status and the URL (403 also without a checksum), and 404 is unchanged.

I'm requesting changes. Everything below should go into this PR instead of a follow-up.

1. The new error prints the URL with its password. sdk.go:1087 formats u with %s. If PreInstall returns http://user:s3cret@127.0.0.1:18080/403/repro.zip?token=abc123, the message ends with:

   server returned 403 Forbidden for http://user:s3cret@127.0.0.1:18080/403/repro.zip?token=abc123
   

   When the connection fails instead, the net/http error for the same URL masks the password (http://user:***@127.0.0.1:18080/...). Use u.Redacted() here, and add a test with userinfo in the URL that checks the password is not in the message. If you want the query gone too, sanitizeSourceURL in receipt.go does that.

2. The status assertion in download_test.go:50 is too loose:

   if !strings.Contains(err.Error(), http.StatusText(status)) && !strings.Contains(err.Error(), string(rune('0'+status/100))) {

   The second operand is the character 4 or 5, and the random port in the test server's URL often contains one. With the message changed to fmt.Errorf("download failed for %s", u), which has no status in it, the test still passed in 6 of 30 runs. Compare against the whole status line, fmt.Sprintf("%d %s", status, http.StatusText(status)), and drop the fallback.

3. The same bug is in four more places. With a 403 from an httptest server, each of these returns no error and leaves the page of HTML in the file:

   • Manager.downloadPlugin, internal/manager.go:455 (plugins added or updated from a URL or the registry)
   • http.download_file, internal/plugin/luai/module/http/http.go:228 (returns nil to the plugin)
   • downloadFile in cmd/commands/upgrade.go:98 (vfox upgrade; it checks no status at all)
   • Downloader.Download, internal/shared/util/downloader.go:47 (nothing calls it yet, same hole)

   Add the same check to each (the existing 404 messages can stay) and a test for each. TestDownloadFile in http_test.go goes to the live plugin site, so use an httptest server there, as TestUserAgentDefault does. A small helper in internal/shared/util would keep the message and the redaction identical in all five places.

4. Fixes #746 closes the issue on merge, but #746 also reports that the download goes out as Go-http-client/1.1 while plugin requests send vfox/<version> vfox-<plugin>/<version>. This PR doesn't change that: with the repro, the server log still shows Go-http-client/1.1 for repro.zip. Either fix it here (ComputeUserAgent in internal/plugin/luai/context.go builds the string, and a User-Agent returned from PreInstall headers should still win) or change Fixes to Refs so the issue stays open.

5. Smaller things:

   • download_test.go has no Apache 2.0 header. The other test files in internal/sdk have one, except sdk_test.go, and AGENTS.md asks for it in new Go files.
   • failed to download ... file, err:download failed: server returned ... says "download failed" twice. #746 asked for unexpected HTTP status 403 Forbidden; unexpected HTTP status 403 Forbidden for <url> reads better after moveRemoteFile has wrapped it.

I'll take another look once these are in.

[Goldloli]

All points are addressed in c040d87:

1. The error now uses the shared helper with url.Redacted(), so a password in the URL becomes user:xxxxx@; TestSdk_Download_ErrorStatusRedactsPassword asserts the password is absent and the redacted userinfo present.
2. The test compares the whole status line: fmt.Sprintf("%d %s", status, http.StatusText(status)), no fallback character match.
3. util.UnexpectedStatusError (internal/shared/util/status.go) is now used by Download, Manager.downloadPlugin, http.download_file, vfox upgrade's downloadFile (which now creates the file only after the status check) and Downloader.Download, with a test for each path as requested; TestDownloadFile in http_test.go now uses httptest instead of the live plugin site.
4. I used Refs #746 instead of Fixes #746: the Go-http-client/1.1 user-agent report is a separate behavior change and the issue should stay open for it.
5. internal/sdk/download_test.go and the other new Go files carry the Apache 2.0 header; the error is now unexpected HTTP status 403 Forbidden for <url>, so it no longer repeats "download failed" inside the wrapper.

New tests are httptest-based and pass locally (internal/sdk, internal/shared/util, internal, cmd/commands, and TestDownloadFile in the Lua http module); go build ./... passes. Full go test ./... still trips the pre-existing live-site TestGetRequest/TestHeadRequest on this network and the printer keyboard test in a non-interactive shell, both unrelated to these changes.

[Goldloli]

The macOS leg of the CI run failed with a Go module digest-mismatch during dependency download — an infra/cache issue rather than a test failure. All other legs (ubuntu, windows, MSIX, fish, interrupted-install) are green, and the full test suite passes locally on macOS:

go test ./internal/sdk/ -run TestSdk_Download -v -count=1   # PASS (incl. ErrorStatusRedactsPassword)
go test ./internal/shared/util/ -v -count=1                  # PASS

A re-run of that job should clear it.

[linghengqian]

Thanks, the points from the first review are all in and check out on c040d87.

One thing is left, in two parts. Both were already like this before the PR, but the PR now redacts the status error in downloadPlugin, so a plugin URL with credentials is shown in clear text in some places and redacted in others.

First, the 404 message prints the URL as given (internal/manager.go:457):

return "", fmt.Errorf("plugin not found at %s", downloadUrl)

$ vfox add --source http://user:s3cret@127.0.0.1:18080/404/plugin.zip
download wrapper error: plugin not found at http://user:s3cret@127.0.0.1:18080/404/plugin.zip

A 403 for the same kind of URL prints user:xxxxx@, so whether the password shows up depends on the status code. Please redact the URL in the 404 message with the same code the status error uses (redactSourceURL is unexported, so it needs exporting), and add a test next to TestDownloadPluginKeepsNotFoundMessage with userinfo in the URL that checks the password is not in the error.

Second, the --debug output. vfox --debug add --source http://user:s3cret@127.0.0.1:18080/403/plugin.zip prints the password in five debug lines before the request is made: Adding plugin: name=..., url=... (manager.go:494), Installing plugin to temp from (527 and 584), Plugin from (594) and Downloading plugin from (432). On a 404 there is also Plugin not found (404) (456). The lines that print a manifest or update URL (167, 330, 394 and 420) print it as given too. Debug output tends to end up in issues and CI logs, so please run the URL through the same function in all of these. The SDK install path doesn't need anything: vfox --debug install with credentials in the PreInstall URL doesn't print the password.

[Goldloli]

Thanks for the careful re-review. Both points are addressed in 0d9e155:

1. 404 message redaction — redactSourceURL is now exported as util.RedactSourceURL (same semantics: u.Redacted() for absolute URLs, pass-through for local paths), and the user-visible 404 error at manager.go now goes through it: plugin not found at http://user:xxxxx@host/.... Added TestDownloadPluginNotFoundRedactsPassword next to TestDownloadPluginKeepsNotFoundMessage: it downloads from an httptest URL carrying user:s3cret@ userinfo and asserts the password is absent from the error while the redacted user:xxxxx@ form is present.

2. Debug output redaction — every cited line now passes its URL through util.RedactSourceURL: the manifest/update lines (167, 330, 394, 420), the download path lines (432, 456), and the add/install-to-temp lines (494, 527, 584, 594). Local paths pass through unchanged, so --source /path/to/plugin.zip output is unaffected. I also applied it to two sibling lines of the same class that print URLs in debug output: the manifest-404 line in fetchPluginManifest and the "Official plugin download URL" line in Add.

Verification:

• go build ./... — ok
• gofmt -l on changed files — clean
• go test ./internal/ -run TestDownloadPlugin -v -count=1 — 3/3 pass (incl. the new redaction test)
• CI=1 go test ./... -count=1 — all packages ok (CI env is needed because TestSelect_Show in internal/shared/printer requires a TTY and self-skips under CI, matching upstream CI)

[linghengqian]

Thanks, the 404 message and the debug lines listed in the last review check out on 0d9e155.

Two debug lines in Manager.Update still print their URL as given. They were not on that list. With registry.address: http://user:s3cret@127.0.0.1:18747/registry in config.yaml, a plugin with manifestUrl = "http://user:s3cret@127.0.0.1:18747/manifest/demo.json", and the registry answering 404 for the plugin (so Update falls back to manifestUrl), vfox --debug update demo prints:

Fetching plugin demo from http://user:s3cret@127.0.0.1:18747/registry/demo.json...
Fetching plugin manifest from: http://user:xxxxx@127.0.0.1:18747/registry/demo.json
Manifest not found (404): http://user:xxxxx@127.0.0.1:18747/registry/demo.json
Fetching plugin demo from http://user:s3cret@127.0.0.1:18747/manifest/demo.json...
Fetching plugin manifest from: http://user:xxxxx@127.0.0.1:18747/manifest/demo.json

The first and the fourth line come from manager.go:302 (address) and manager.go:307 (pluginMetadata.ManifestUrl). Please pass both through util.RedactSourceURL, like the line at 330 in the same function. With 403 and 404 responses, the --debug output of add --source, add <name> from the registry, the automatic plugin install in vfox install, update through updateUrl, and install of an SDK with credentials in the PreInstall URL contains no password.

The description still says "Existing 404 messages are unchanged" and mentions the redaction only for the status error. The plugin 404 message and the debug lines are redacted now too, so please update the description to say so.

[Goldloli]

Both are fixed in 2601d98:

• Manager.Update now passes the registry address (manager.go:302) and the fallback pluginMetadata.ManifestUrl (manager.go:307) through util.RedactSourceURL before the debug line, matching the download line at manager.go:330.
• go build ./... passes; go test ./internal ./internal/shared/util ./internal/sdk ./cmd/commands passes.
• I also updated the PR description: it now says the plugin 404 messages and the Fetching plugin … from … update debug lines are redacted, instead of claiming 404 messages are unchanged.

Thanks for the careful reproduction; the description no longer overstates the untouched messages.

[linghengqian]

Thanks, the two Manager.Update debug lines and the description check out on 2601d98.

Eight things are left. They go into this PR too.

1. Manager.Update never installs an update that it finds through manifestUrl. When the registry answers 404 and the manifest at manifestUrl is newer, downloadUrl is set at internal/manager.go:316, and the return err at line 321 then returns the registry's ManifestNotFound (the err of du, err := at line 308 lives only in the inner block):

   Manifest found, name: demo, version: 9.9.9,  downloadUrl: http://user:xxxxx@127.0.0.1:18747/403/demo.zip
   manifest not found
   

   The block has had this shape since 654aa18 (#151). It is the block where this PR edits the debug lines, and while it stays like this the manifestUrl flow never reaches the download that the rest of the PR is about. Fetching the fallback manifest into the variable that holds the registry result, and checking the version once for both, removes it. Please add a test: an httptest server whose registry path answers 404 and whose manifest offers a newer version with a downloadUrl to a plugin zip, an installed plugin whose manifestUrl points to that manifest, and Update returning nil with the new version in the plugin directory. On this branch it fails with Update() error = manifest not found.

2. RedactSourceURL hides the password of user:password@ and nothing else. A 403 is the answer to a rejected credential, so the new message is where one gets echoed. With a PreInstall URL of http://tok3n@127.0.0.1:18747/403/demo-1.0.0.zip and of http://127.0.0.1:18747/403/demo-1.0.0.zip?token=abc123:

   failed to download demo@1.0.0 file, err:unexpected HTTP status 403 Forbidden for http://tok3n@127.0.0.1:18747/403/demo-1.0.0.zip
   failed to download demo@1.0.0 file, err:unexpected HTTP status 403 Forbidden for http://127.0.0.1:18747/403/demo-1.0.0.zip?token=abc123
   

   sanitizeSourceURL in internal/sdk/receipt.go already names "credentials, tokens in the query" as what must not be stored, pip prints accesstoken@example.com as ****@example.com, and git strips the user name and password from the URLs it shows (transport_anonymize_url). Please mask in RedactSourceURL a user name that has no password (a user name with a password stays visible, as TestSdk_Download_ErrorStatusRedactsPassword expects) and the values of the query (the keys stay). A string that starts with scheme:// but that url.Parse rejects, such as a password with a bare % or /, is returned as given today, password included. Hide everything between :// and its last @ and drop its query instead.

3. The errors that net/http and net/url build print the URL themselves. net/http masks only the password, and net/url quotes the input that it rejects. With the same URLs and nothing listening on the port:

   failed to download demo@1.0.0 file, err:Get "http://user:***@127.0.0.1:1/demo-1.0.0.zip?token=abc123": dial tcp 127.0.0.1:1: connect: connection refused
   failed to download demo@1.0.0 file, err:Get "http://tok3n@127.0.0.1:1/demo-1.0.0.zip": dial tcp 127.0.0.1:1: connect: connection refused
   parse "http://user:Zq9%ss@127.0.0.1:1/demo-1.0.0.zip": invalid URL escape "%ss"
   parse "http://user:A/b+c==@127.0.0.1:1/demo-1.0.0.zip": invalid port ":A" after host
   

   The same lines show up for add --source, for a registry address with credentials in config.yaml (add, update, available) and in the text that a plugin gets from http.get, http.head and http.download_file. Please add a helper next to RedactSourceURL that returns a copy of a *url.Error with URL passed through RedactSourceURL and, for a parse error whose URL had something to hide, a reason without the quoted input. Use it where these errors are returned or logged: internal/sdk/sdk.go:368 and :1077, internal/manager.go:398-399, :436-437, :449-450 and :656, internal/plugin/luai/module/http/http.go:97, :114, :151, :167, :211 and :225, and internal/shared/util/downloader.go:38 and :42.

4. Five places format a URL, or a name taken from it, themselves:

   • installPluginToTemp takes the extension of the whole URL (internal/manager.go:586), so vfox add --source 'http://host/demo.zip?token=abc123' fails with unsupported .zip?token=abc123 type wrapper to install, only support .lua or .zip (lines 588 and 589). Print the extension of util.RedactSourceURL(path).
   • http.download_file uses filepath.Base(urlStr.String()) as the label of the progress bar (internal/plugin/luai/module/http/http.go:245-246), so a download of .../file.zip?token=abc123 that succeeds prints file.zip?token=abc123 100% [...]. Take it from req.URL.Path.
   • A PreInstall URL with a scheme other than http or https is handled as a local path, and moveLocalFile (internal/sdk/sdk.go:357-359) prints it, in the progress line and in the error: Moving ftp://user:s3cret@127.0.0.1:1/demo-1.0.0.zip?token=abc123 to ... and failed to move file, err:stat ftp://user:s3cret@127.0.0.1:1/demo-1.0.0.zip?token=abc123: no such file or directory. Pass the path through RedactSourceURL in both; the error is an *fs.PathError, so its Path field is the string.
   • The offline transport of vfox plugin run --offline formats req.URL itself (internal/plugin/development_http.go:45), so the password is not masked: Get "http://user:***@127.0.0.1:1/releases?token=abc123": HTTP is disabled offline: GET http://user:s3cret@127.0.0.1:1/releases?token=abc123. Use util.RedactSourceURL(req.URL.String()).
   • downloadPlugin saves the plugin under filepath.Base(downloadUrl) (internal/manager.go:465), so a URL that passes the extension check, such as http://host/dl?token=abc123&file=plugin.zip, prints the query in Saving plugin to (466), Downloading dl?token=abc123&file=plugin.zip... (473), Plugin downloaded successfully (479), Unpacking wrapper (624) and Validating (632). Take the file name from the path of the URL, and use plugin plus the extension when the path has none.

5. The branch of RedactSourceURL that returns its input (internal/shared/util/status.go:43) is not run by any test. It is the only block of that file that go test ./... -coverprofile reports with a count of 0, and the Codecov comment lists the same file. Dropping u.Scheme == "" || u.Host == "" from the condition leaves go test ./... green, although that check keeps a Windows path intact (url.Parse("C:\\plugins\\plugin.zip") returns scheme c, and Redacted() would print c:\plugins\plugin.zip), and Add and installPluginToTemp call the function for every local --source. Please add a table test next to TestUnexpectedStatusError: a password, a user name without a password, a query, a password with a query, a URL with nothing to hide, a URL that url.Parse rejects, and /tmp/plugin.zip, ./plugin.zip, /tmp/100%/plugin.zip, C:\plugins\plugin.zip, C:/plugins/plugin.zip and the empty string. The helper from 3 needs a test with a connection error and one with a parse error. Each place in 3 and 4 needs a test that puts a user name token, a password and a query token in the URL and checks that none of them is in the error, the log line or the label.

6. fetchLatestVersion (cmd/commands/upgrade.go:43) is the only request in this file that still accepts any status. With a transport that answers 403, 429 or 500 with an HTML page, it returns

   Failed to fetch the version.
   

   because the tag regexp finds nothing in the error page. vfox upgrade then fails without naming the status, which is what #746 reports for the download. Please add the same util.UnexpectedStatusError check after the defer resp.Body.Close() at line 48, with a test (a RoundTripper that answers 429 is enough, the URL is fixed), and mention it in the description.

7. AGENTS.md calls itself the single project-wide guide, and #744 updated it together with its change. This PR adds rules for later changes, and the URL lines were found one round at a time, so the next change can easily print a URL as given. Please add them under "Shared utilities": a download checks its status with util.UnexpectedStatusError before the file is created, a URL that goes into an error, a message or a --debug line goes through util.RedactSourceURL, and the error of a request or of url.Parse goes through the helper from 3.

8. The release notes are built from commit titles (.goreleaser.yaml), and the squash commit of a PR with several commits takes the PR title. With the points above in, the title should say what users will notice, for example fix: name the status of a refused download, hide URL credentials in messages, update via manifestUrl.

[Goldloli]

All eight points are in.

1. Manager.Update now fetches the manifestUrl manifest into registryManifest itself and checks the version once for both sources — the shadowed err is gone. TestUpdateInstallsManifestUrlUpdate covers the flow: the registry answers 404, the manifest at manifestUrl offers 9.9.9 with a downloadUrl to a plugin zip, and the installed demo@0.0.1 ends with the new version in the plugin directory. Without the fix it fails with Update() error = manifest not found.

2. RedactSourceURL masks a user name without a password as ****@host, keeps user:xxxxx@ with a password, masks query values and keeps the keys, and for a scheme:// string that url.Parse rejects hides everything between :// and the last @ and drops the query. The u.Scheme == "" || u.Host == "" check stays, so C:\plugins\plugin.zip, C:/plugins/plugin.zip, /tmp/plugin.zip and friends pass through unchanged.

3. util.RedactURLError sits next to RedactSourceURL: it returns a copy of the *url.Error with the URL passed through RedactSourceURL, and for a parse error whose URL had something to hide it replaces the reason with one that does not quote the rejected input. Applied at internal/sdk/sdk.go (moveRemoteFile's url.Parse, Download), internal/manager.go (fetchPluginManifest, both request errors in downloadPlugin, Available), all six sites in the Lua http module (get/head/download_file, request creation and execution), and internal/shared/util/downloader.go.

4. The five places: installPluginToTemp prints filepath.Ext(util.RedactSourceURL(path)); http.download_file takes the label from req.URL.Path; moveLocalFile passes the progress line and the *fs.PathError.Path through RedactSourceURL; the offline transport uses util.RedactSourceURL(req.URL.String()); and downloadPlugin names the file from the URL path, falling back to plugin plus the extension when the path has none.

5. status_test.go has the table test with the listed cases; TestRedactURLError covers the connection error, the parse error, and a parse error without secrets keeping its reason. Each place in 3 and 4 has a test that puts a user name token, a password and a query token in the URL and asserts none of them is in the error, the printed line or the label (the moveLocalFile print is captured with pterm.SetDefaultOutput, the progress bar with the module's Options.Output).

6. fetchLatestVersion checks util.UnexpectedStatusError right after defer resp.Body.Close(), and TestFetchLatestVersionNamesTheStatus drives it with a 429 RoundTripper. The description mentions it.

7. AGENTS.md "Shared utilities" has the three rules.

8. Title updated.

go build ./... passes and go test on the affected packages (internal/shared/util, internal, internal/sdk, internal/plugin, internal/plugin/luai/module/http, cmd/commands) is green.

[linghengqian]

Thanks, the eight points of the last review are in and check out on bf59e05, apart from what is listed below.

Seven things are left. They go into this PR too.

1. vfox add --source panics for a URL whose .zip is only in the query. installPluginToTemp accepts a URL by the extension of the whole URL (internal/manager.go:606), so http://host/dl?token=abc123&file=plugin.zip passes. pluginFileName (internal/manager.go:428) then saves the file under the last element of the URL path, which is dl. util.NewDecompressor picks the unpacker from the file name and returns nil for a name it does not know (internal/shared/util/decompressor.go:729), and internal/manager.go:648 calls Decompress on that nil:

   $ vfox add --source 'http://127.0.0.1:18748/dl?token=abc123&file=plugin.zip'
   Downloading dl...
   panic: runtime error: invalid memory address or nil pointer dereference
   ...
   github.com/version-fox/vfox/internal.(*Manager).installPluginToTemp(...)
   	internal/manager.go:648
   

   On main the same command installs the plugin, because the file is saved as dl?token=abc123&file=plugin.zip, which ends in .zip. With a server that answers every path with a plugin zip, /dl/?file=plugin.zip, /dl.php?file=plugin.zip and /download.php?id=7&name=plugin.zip install on main and panic on this branch, and /plugin.zip installs on both. No test runs installPluginToTemp on such a URL: TestDownloadPluginNamesFileFromURLPath calls only downloadPlugin and asserts the name dl (internal/manager_test.go:464).

   The saved name has to end with the extension that installPluginToTemp accepted. Please use the last element of the URL path only when it does, and plugin plus that extension otherwise:

   ext := filepath.Ext(downloadUrl)
   if u, err := url.Parse(downloadUrl); err == nil {
   	if name := path.Base(u.Path); name != "." && name != "/" && strings.HasSuffix(name, ext) {
   		return name
   	}
   }
   return "plugin" + ext

   With that, all five paths above install, and TestDownloadPluginNamesFileFromURLPath has to expect plugin.zip for its first URL. Please add a test that goes through installPluginToTemp with an httptest server that answers with a plugin zip (updateTestPlugin in manager_update_test.go builds one) for the five paths. On this branch it fails with invalid memory address or nil pointer dereference for four of them.

2. The rewrite of the fallback in Manager.Update changed two things that the fix does not need. When the registry and the manifestUrl both answer 404, the command now ends with demo plugin not support update (internal/manager.go:312). On main it ends with manifest not found, and not support update was only for a plugin without a manifestUrl:

   registry 404, manifestUrl 404
   main:   manifest not found
   branch: demo plugin not support update
   

   Please keep the check for the missing manifestUrl inside the first 404 branch:

   registryManifest, err := m.fetchPluginManifest(address)
   if errors.Is(err, ManifestNotFound) {
   	if pluginMetadata.ManifestUrl == "" {
   		return fmt.Errorf("%s plugin not support update", pluginName)
   	}
   	logger.Debugf("Fetching plugin %s from %s...\n", pluginName, util.RedactSourceURL(pluginMetadata.ManifestUrl))
   	registryManifest, err = m.fetchPluginManifest(pluginMetadata.ManifestUrl)
   }
   if err != nil {
   	return err
   }

   The second change is the color of is already the latest version on the registry path: pterm.LightBlue on main, pterm.Blue at internal/manager.go:318. Please keep LightBlue there.

   Neither the not support update return (line 312) nor the already the latest return (lines 318-319) is run by any test; go test ./... -coverprofile reports both with a count of 0. Please add a test for each outcome of the block: registry 404 without a manifestUrl, registry 404 with a manifestUrl that answers 404, and a manifest that offers the installed version.

3. Sdk.Download returns the error of http.NewRequest as it is (internal/sdk/sdk.go:1080-1082). The same call in Downloader.Download (internal/shared/util/downloader.go:46-48) and in downloadPlugin (internal/manager.go:451-455) goes through the redaction. With the URL built the way TestDownloaderRedactsRequestErrors builds it (Host: "in valid", a user name and password, a query token):

   parse "http://user:s3cret@in%20valid/repro.zip?token=abc123": invalid URL escape "%20"
   

   moveRemoteFile is the only caller and has parsed the string by then, so the command line does not reach this. Downloader.Download, which nothing calls, is handled the same way in this PR. Please redact it here too and add a case next to TestSdk_Download_RedactsConnectionError.

4. RedactURLError takes a *url.Error, so every caller needs an errors.As around it, and the same wrapper was written four times: redactURLError in internal/manager.go:439 and internal/sdk/sdk.go:375, redactError in internal/shared/util/downloader.go:37 and in internal/plugin/luai/module/http/http.go:58. The fallback for other errors in each copy is run by no test (internal/manager.go:444, internal/sdk/sdk.go:380, internal/shared/util/downloader.go:42, internal/plugin/luai/module/http/http.go:63), and the rule added to AGENTS.md says the error of a request goes through util.RedactURLError, which no call site does literally. Please add one function next to it, delete the four copies and call it from the sites:

   // RedactError passes a *url.Error found in err through RedactURLError and
   // returns any other error unchanged.
   func RedactError(err error) error {
   	var urlErr *url.Error
   	if errors.As(err, &urlErr) {
   		return RedactURLError(urlErr)
   	}
   	return err
   }

   The AGENTS.md rule then names util.RedactError, and the function needs a test with a *url.Error and one with another error, next to TestRedactURLError.

5. A query part without = is printed as it is, because redactQueryValues masks only what follows an = (internal/shared/util/status.go:81). A token passed that way stays visible:

   RedactSourceURL("http://example.com/file.zip?abc123")     = http://example.com/file.zip?abc123
   RedactSourceURL("http://example.com/file.zip?abc123&b=2") = http://example.com/file.zip?abc123&b=xxxxx
   

   sanitizeSourceURL in internal/sdk/receipt.go drops the whole query. Please mask a part without = as a whole (?xxxxx&b=xxxxx) and add the two inputs to TestRedactSourceURL.

6. go test ./... -coverprofile reports three blocks of internal/shared/util/status.go with a count of 0: the case of isURLScheme for a digit, +, - or . (lines 117-119), its default return (line 121) and return s in RedactSourceURL (line 75). isURLScheme is what keeps a local path that contains :// from being rewritten, as its comment says, and no row of TestRedactSourceURL reaches those branches. Please add /tmp/100%/dir://x@y and 1http://user:Zq9%ss@host/file.zip to the table; both come back unchanged. The return s cannot be reached: a URL with a scheme and a host prints as scheme://host, and Parse lowercases the scheme, so :// starts at len(u.Scheme). Please drop the search and the fallback:

   s := u.String()
   n := len(u.Scheme) + len("://")
   return s[:n] + userinfo + s[n:]

7. archiver.decompress has the same nil unpacker (internal/plugin/luai/module/archiver/archiver.go:33). Of the callers of NewDecompressor, internal/sdk/sdk.go:405 checks for nil and cmd/commands/upgrade.go:185 always passes vfox_latest.zip or vfox_latest.tar.gz, but this one passes a name that the plugin chooses. For a name that no unpacker is registered for, the call fails with a nil pointer dereference instead of returning an error string like its other failures:

   local archiver = require("vfox.archiver")
   archiver.decompress("/tmp/not-an-archive.dat", "/tmp/out")
   -- runtime error: invalid memory address or nil pointer dereference

   Please check for nil there, push an error such as unsupported archive type: .dat, and add a test.

The description needs to follow points 1, 4 and 5: the rule for the saved file name (it says the name falls back to plugin plus the extension when the path has none), the helper name, and the query rule.

[Goldloli]

Thanks — all seven points are in 7882fa4.

1. pluginFileName now returns the last element of the URL path only when it ends with the extension the whole URL was accepted by, and plugin plus that extension otherwise — exactly the shape you suggested. TestDownloadPluginNamesFileFromURLPath expects plugin.zip for the first URL now, and the new TestInstallPluginToTempNamesFileWithAcceptedExtension goes through installPluginToTemp with an httptest server answering a plugin zip for /plugin.zip, /dl/?file=plugin.zip, /dl.php?file=plugin.zip, /download.php?id=7&name=plugin.zip and /dl?token=abc123&file=plugin.zip. Without the fix the last four panic with the nil dereference; with it all five install.

2. The not support update check is back inside the first 404 branch, so registry 404 + manifestUrl 404 ends with manifest not found as on main, and the registry path keeps pterm.LightBlue. Three new tests in manager_update_test.go cover the block: TestUpdatePluginWithoutManifestUrlNotSupported, TestUpdateManifestUrlAlsoNotFound, TestUpdateAlreadyLatestFromRegistry.

3. Sdk.Download passes the http.NewRequest error through the redaction too, with TestSdk_Download_RedactsRequestBuildError next to the connection-error test, using the Host: "in valid" URL.

4. util.RedactError sits next to RedactURLError and the four private copies (redactURLError ×2, redactError ×2) are gone; every site calls util.RedactError, and the AGENTS.md rule names it. TestRedactError covers a plain *url.Error, a wrapped one, another error and nil.

5. redactQueryValues masks a part without = as a whole: ?abc123 → ?xxxxx, ?abc123&b=2 → ?xxxxx&b=xxxxx. Both rows are in TestRedactSourceURL.

6. /tmp/100%/dir://x@y and 1http://user:Zq9%ss@host/file.zip are in the table and come back unchanged, and the unreachable scheme search is dropped — RedactSourceURL now splices at len(u.Scheme) + len("://"). go test -coverprofile reports every block of status.go covered.

7. archiver.decompress checks the nil unpacker and pushes unsupported archive type: .dat; TestDecompressUnsupportedType covers it.

The description follows points 1, 4 and 5: the saved-name rule, util.RedactError, and the query rule. go build ./... passes and go test is green for internal, internal/shared/util, internal/sdk, internal/plugin/luai/module/http, internal/plugin/luai/module/archiver and cmd/commands.

[linghengqian]

Thanks, the seven points of the last review are in and check out on 7882fa4.

One thing is left. It goes into this PR too.

1. pluginFileName takes the last element of the decoded URL path with path.Base (internal/manager.go:433), which does not split at a backslash. On Windows filepath.Join does (internal/manager.go:478), so a backslash in the URL path, literal or as %5C, moves the download out of the plugin directory. installPluginToTemp removes the downloaded file when it returns (internal/manager.go:616-618), so a file that already sits at that place is gone afterwards. With a Windows build of the tests run under Wine, the plugin directory at <root>\a\b\plugins and a server that answers every path, downloadPlugin saves the file here:

   URL path              main                                     this branch
   /..\..\evil.zip       <root>\a\b\plugins\evil.zip              <root>\a\evil.zip
   /..%5C..%5Cevil.zip   <root>\a\b\plugins\..%5C..%5Cevil.zip    <root>\a\evil.zip
   /sub/..%5Cevil.zip    <root>\a\b\plugins\..%5Cevil.zip         <root>\a\b\evil.zip
   

   With a victim.zip placed next to the plugin directory and <server>/..\victim.zip as the URL, installPluginToTemp returns nil on both, victim.zip is untouched on main and gone on this branch. installPluginToTemp also receives the downloadUrl of a registry manifest and the updateUrl of a plugin, so a registry can supply such a URL as well as vfox add --source can.

   Main used filepath.Base on the raw URL string, and the SDK path uses it on the decoded path (internal/sdk/sdk.go:1104, internal/shared/util/downloader.go:55). Please do the same here and drop the path import:

   if name := filepath.Base(u.Path); name != "." && name != "/" && strings.HasSuffix(name, ext) {

   With that, the three URLs above are saved in the plugin directory as evil.zip, and TestDownloadPluginNamesFileFromURLPath and TestInstallPluginToTempNamesFileWithAcceptedExtension pass on the same Windows build.

   Linux and macOS do not show this, because a backslash is an ordinary file name character there, and no test uses one, so CI is green. Please add a test next to TestDownloadPluginNamesFileFromURLPath that downloads from an httptest server for /..\evil.zip, /..%5Cevil.zip and /sub/..%5C..%5Cevil.zip and checks that the saved file is directly in the plugin directory:

   if rel, err := filepath.Rel(plugins, saved); err != nil || rel != filepath.Base(saved) {
   	t.Fatalf("downloadPlugin() saved %q, want a file directly in %q", saved, plugins)
   }

   It passes on Linux either way. On the Windows build it fails for all three paths on this branch and passes with filepath.Base.

[Goldloli]

Done in 3c8b4cb.

pluginFileName now takes the last element with filepath.Base(u.Path) and the path import is gone, matching the SDK path in internal/sdk/sdk.go and internal/shared/util/downloader.go. On Windows a literal or %5C backslash in the URL path now collapses to the file name, so /..\evil.zip, /..%5C..%5Cevil.zip and /sub/..%5Cevil.zip all land directly in the plugin directory instead of escaping it; on Linux and macOS the backslash stays an ordinary file name character, which is contained as well.

Added TestDownloadPluginKeepsBackslashPathsInPluginDir next to TestDownloadPluginNamesFileFromURLPath: it serves /..\evil.zip, /..%5Cevil.zip and /sub/..%5C..%5Cevil.zip from an httptest server and asserts each saved file sits directly in the plugin directory via the filepath.Rel check you suggested.

Local verification: go build ./... and go test on the affected packages (internal, internal/sdk, internal/shared/util, internal/env, internal/config, internal/pathmeta) all pass, including the new test. The full ./internal/... run only fails in internal/shared/printer, which hangs in atomicgo.dev/keyboard.Listen without an interactive terminal — untouched by this change; the Windows CI build covers the new assertions.

[Goldloli]

CI note on 3c8b4cb: the build (macos-latest) job was cancelled by the runner — it never executed any steps (started 15:46Z, cancelled ~15m later), while build (ubuntu-latest), build (windows-latest), the Fish completion integration, and the Windows MSIX tests all passed. This looks like a runner-side cancellation rather than a build problem. Could you re-run that job when you get a chance? Thanks!

[linghengqian]

LGTM