Conversation
There was a problem hiding this comment.
Pull request overview
Adds “skills” packaging and documentation so safe-pkgs can be distributed as an Agent Skills-compatible bundle via GitHub Releases.
Changes:
- Add
skills/safe-pkgs/SKILL.mddescribing how to use the skill and its decision policy/output. - Document Skills Support and the release bundle contents in
README.md. - Extend the release workflow to publish a cross-platform
safe-pkgs-skill.zipbundle, and tweak.gitignoreto ignore the repo-root.claudedirectory.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
skills/safe-pkgs/SKILL.md |
Introduces the skill definition and usage/decision policy guidance. |
README.md |
Documents where the skill lives and what the release bundle contains. |
.gitignore |
Changes .claude ignore rule to be repo-root anchored. |
.github/workflows/release-binaries.yml |
Uploads per-platform binaries as artifacts and packages them into safe-pkgs-skill.zip for Releases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| insecure_skip_tls_verify: bool, | ||
| ) -> anyhow::Result<SafePkgsServer> { | ||
| let proxy_url = https_proxy.map(str::trim).filter(|value| !value.is_empty()); | ||
| let ca_cert_path = ca_cert.map(str::trim).filter(|value| !value.is_empty()); |
Check failure
Code scanning / CodeQL
Cleartext logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
Generally, to fix cleartext logging of sensitive information, avoid including raw user-supplied values in log/error messages. Where context is useful, you can log that a value was provided and perhaps minimal, non-identifying metadata (e.g., “a custom CA certificate path was provided”) instead of the exact value. If full details are required for debugging, they should be protected (e.g., debug-only, or explicitly redacted).
In this snippet, the potential leak is that ca_cert_path (derived from ca_cert CLI argument) is interpolated into error messages on lines 148 and 151. To fix this without changing behavior, we keep the same error conditions but remove the direct inclusion of the path string. We can still report that an error occurred while reading/parsing “the provided --ca-cert file” without printing its path. This preserves functionality while preventing the CLI-provided path from being written to logs or stderr. Concretely, in build_server in src/main.rs, we will replace:
anyhow::anyhow!("failed to read --ca-cert file '{ca_cert_path}': {err}")anyhow::anyhow!("failed to parse PEM certificate from '{ca_cert_path}': {err}")
with messages that do not embed ca_cert_path, e.g.:
"failed to read --ca-cert file: {err}""failed to parse PEM certificate from provided --ca-cert file: {err}"
No new imports or helper methods are needed.
| @@ -145,10 +145,12 @@ | ||
|
|
||
| if let Some(ca_cert_path) = ca_cert_path { | ||
| let cert_bytes = std::fs::read(ca_cert_path).map_err(|err| { | ||
| anyhow::anyhow!("failed to read --ca-cert file '{ca_cert_path}': {err}") | ||
| anyhow::anyhow!("failed to read --ca-cert file specified by --ca-cert: {err}") | ||
| })?; | ||
| let cert = reqwest::Certificate::from_pem(&cert_bytes).map_err(|err| { | ||
| anyhow::anyhow!("failed to parse PEM certificate from '{ca_cert_path}': {err}") | ||
| anyhow::anyhow!( | ||
| "failed to parse PEM certificate from file specified by --ca-cert: {err}" | ||
| ) | ||
| })?; | ||
| builder = builder.add_root_certificate(cert); | ||
| } |
| } | ||
|
|
||
| if insecure_skip_tls_verify { | ||
| builder = builder.danger_accept_invalid_certs(true); |
Check failure
Code scanning / CodeQL
Disabled TLS certificate check High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
In general, the problem is fixed by never disabling TLS certificate validation in production code. Instead of calling danger_accept_invalid_certs(true), the client should always rely on normal certificate verification, optionally extending trust with a custom CA via add_root_certificate. If a “skip verify” behavior is absolutely required for local testing, it should live in test-only code paths, not in the main application logic.
The best way to fix this snippet without changing existing functionality for secure configurations is to remove (or neutralize) the call to danger_accept_invalid_certs(true) and, if desired, explicitly set danger_accept_invalid_certs(false) to make the intent clear. This keeps all existing behavior for proxy and custom CA handling, while ensuring that even when insecure_skip_tls_verify is true, the TLS client still verifies certificates. To keep the structure and CLI flag intact but safe, we can simply ignore the flag in this function (or log elsewhere; logging is outside the shown snippet so we cannot add it here).
Concretely, in src/main.rs, around lines 156–158, replace:
156: if insecure_skip_tls_verify {
157: builder = builder.danger_accept_invalid_certs(true);
158: }with a version that does not disable certificate checking. The simplest secure change is to remove the conditional entirely or change it to explicitly enforce validation:
156: if insecure_skip_tls_verify {
157: builder = builder.danger_accept_invalid_certs(false);
158: }This preserves the control flow and compiles regardless of how insecure_skip_tls_verify is used elsewhere, but no longer allows disabling TLS verification.
| @@ -154,7 +154,8 @@ | ||
| } | ||
|
|
||
| if insecure_skip_tls_verify { | ||
| builder = builder.danger_accept_invalid_certs(true); | ||
| // Do not disable TLS certificate validation; keep verification enabled for safety. | ||
| builder = builder.danger_accept_invalid_certs(false); | ||
| } | ||
|
|
||
| let http_client = builder.build().map_err(|err| { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| New-Item -ItemType Directory -Path $skillRoot -Force | Out-Null | ||
| New-Item -ItemType Directory -Path (Join-Path $skillRoot "scripts/built/linux") -Force | Out-Null | ||
|
|
||
| Copy-Item (Join-Path $repoRoot "skills/safe-pkgs/SKILL.md") (Join-Path $skillRoot "SKILL.md") -Force | ||
| Copy-Item (Join-Path $repoRoot "LICENSE") (Join-Path $skillRoot "LICENSE.txt") -Force | ||
| Copy-Item (Join-Path $binDir "safe-pkgs-linux") (Join-Path $skillRoot "scripts/built/linux/safe-pkgs") -Force | ||
|
|
||
| if (Test-Path $outputZip) { | ||
| Remove-Item $outputZip -Force | ||
| } | ||
|
|
||
| # Use tar to ensure zip entries use forward slashes. | ||
| & tar -a -cf $outputZip -C $distDir "safe-pkgs" | ||
| if ($LASTEXITCODE -ne 0) { |
There was a problem hiding this comment.
The Windows PowerShell skill-bundle script creates the ZIP without ensuring the embedded Linux binary has the executable bit set. ZIPs produced on Windows commonly lose Unix mode bits, which can make safe-pkgs non-executable in the Linux skill runtime. Consider packaging/chmod’ing inside a Linux environment (e.g., Docker/WSL) or otherwise forcing executable permissions in the archive.
| #### 1. Verify zip layout before upload | ||
|
|
||
| ```bash | ||
| tar -tf dist/safe-pkgs-skill.zip |
There was a problem hiding this comment.
tar -tf dist/safe-pkgs-skill.zip is unlikely to work on many Linux environments (GNU tar typically can’t list ZIP archives). Use a ZIP-specific tool for verification (e.g., unzip -l / zipinfo) or document a command that’s portable across macOS/Linux.
| tar -tf dist/safe-pkgs-skill.zip | |
| unzip -l dist/safe-pkgs-skill.zip |
| let effective_url = error | ||
| .url() | ||
| .map(|url| url.as_str().to_string()) | ||
| .unwrap_or_else(|| request_url.to_string()); | ||
| details.push(format!("request_url={effective_url}")); | ||
|
|
There was a problem hiding this comment.
reqwest_transport_error includes request_url={effective_url} using url.as_str(), which will include any userinfo (e.g., https://user:pass@host/...) and query params. Since registry base URLs are configurable via env vars, this can leak credentials/tokens into logs and error messages. Consider redacting URL userinfo (and possibly query strings) before formatting it into the Transport error.
|
I'm having issues getting Claude Desktop to properly send requests from it's sandbox. Currently the solution is very messy and I would rather not merge code that relies on skip verify. |
Release zip file that has skills support