From 40a39022cb328f8e5a2a4615d791ea8008750f29 Mon Sep 17 00:00:00 2001 From: Rafik Abdulwahab Date: Wed, 2 Sep 2026 11:37:44 +0200 Subject: [PATCH 1/2] FR-6398: Harden the frbit-cli release pipeline after the security review --- .github/dependabot.yml | 14 +++++ .github/workflows/release.yml | 6 ++ README.md | 10 +++- SECURITY.md | 11 ++++ docs/cli.md | 3 +- install.sh | 27 +++++++++ internal/agentskills/source.go | 18 +++++- internal/agentskills/types_test.go | 13 +++++ internal/app/factory.go | 5 +- internal/app/factory_test.go | 12 ++++ internal/cmd/environments/environments.go | 56 ++++++++++++++++--- .../cmd/environments/environments_test.go | 38 +++++++++++++ internal/cmd/resource/resource.go | 12 +++- internal/cmd/resource/resource_test.go | 23 ++++++++ internal/cmdutil/runtime.go | 16 ++++++ scripts/test-install.sh | 11 ++++ 16 files changed, 261 insertions(+), 14 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 SECURITY.md create mode 100644 internal/app/factory_test.go create mode 100644 internal/cmd/environments/environments_test.go create mode 100644 internal/cmd/resource/resource_test.go diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..4a04005 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,14 @@ +version: 2 +updates: + - package-ecosystem: gomod + directory: / + schedule: + interval: weekly + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + - package-ecosystem: npm + directory: /npm + schedule: + interval: weekly diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 8128f4c..5009bc5 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -6,6 +6,7 @@ on: - "v*" permissions: + attestations: write contents: write id-token: write @@ -14,6 +15,7 @@ env: jobs: release: + environment: release runs-on: ubuntu-24.04 steps: - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 @@ -39,6 +41,10 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} HOMEBREW_TAP_TOKEN: ${{ secrets.HOMEBREW_TAP_TOKEN }} + - name: Attest release checksums + uses: actions/attest@1e69f48acb82d1966a394da916b4c1698aa569d6 # v4 + with: + subject-path: dist/checksums.txt - name: Prepare npm packages if: env.PUBLISH_NPM == 'true' run: node npm/scripts/prepare-release.mjs "${GITHUB_REF_NAME#v}" diff --git a/README.md b/README.md index 00c0bca..1e6f604 100644 --- a/README.md +++ b/README.md @@ -136,7 +136,8 @@ FRBIT_DASHBOARD_URL=http://localhost:3001 \ link during interactive login. Host resolution is: `--host`, `FRBIT_HOST`, saved host, then the production -default. +default. The CLI warns before it sends credentials to a non-default host; only +use an override you trust. ## Release @@ -149,7 +150,12 @@ just release --major CI validates formatting, static analysis, tests, builds, and the GoReleaser configuration. Pushing a `v*` tag creates the GitHub release archives, checksums, -SBOMs, Homebrew formula, and npm packages. +SBOMs, Homebrew formula, and npm packages. The release job runs in the +review-gated `release` environment and publishes a GitHub build-provenance +attestation for `checksums.txt`. When the GitHub CLI (`gh`) is installed, the +installer verifies that attestation before installing. To require this check, +set `FRBIT_VERIFY_PROVENANCE=1`; otherwise the installer warns and continues +when `gh` is unavailable. ## License diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..638a4d2 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,11 @@ +# Security Policy + +## Reporting a vulnerability + +Please do not open a public issue for a suspected vulnerability. Email +security@fortrabbit.com with a description, reproduction steps, and any +relevant impact. We will acknowledge reports and coordinate a fix privately. + +## Supported versions + +Security fixes are applied to the latest released version of `frbit`. diff --git a/docs/cli.md b/docs/cli.md index 70d69ef..4bd747b 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -236,7 +236,8 @@ frbit environments update en-a1b2c3 --clear-build-commands Read or merge environment variables. `--set` and `--delete` are repeatable: ```sh -frbit environments variables get en-a1b2c3 +frbit environments variables get en-a1b2c3 # values are masked +frbit environments variables get en-a1b2c3 --reveal # show values explicitly frbit environments variables update en-a1b2c3 \ --set APP_ENV=production \ --delete OLD_FLAG diff --git a/install.sh b/install.sh index f326271..0d84cd5 100755 --- a/install.sh +++ b/install.sh @@ -48,6 +48,16 @@ else release_base_url="https://github.com/$repository/releases/latest/download" fi +validate_release_base_url() { + case "$release_base_url" in + https://github.com/fortrabbit/frbit-cli/releases/download/*|https://github.com/fortrabbit/frbit-cli/releases/latest/download) ;; + file://*) [ "${FRBIT_INSTALL_TESTING:-}" = "1" ] || fail "FRBIT_RELEASE_BASE_URL must use the official GitHub release host" ;; + *) fail "FRBIT_RELEASE_BASE_URL must use the official GitHub release host" ;; + esac +} + +validate_release_base_url + if [ -n "${FRBIT_INSTALL_DIR:-}" ]; then install_dir=${FRBIT_INSTALL_DIR%/} elif [ "$(id -u)" -eq 0 ]; then @@ -76,6 +86,23 @@ fi [ "$actual" = "$expected" ] || fail "checksum verification failed for $asset" +verify_provenance() { + if command -v gh >/dev/null 2>&1; then + gh attestation verify "$tmp_dir/checksums.txt" \ + --repo "$repository" \ + --signer-workflow "$repository/.github/workflows/release.yml" \ + --predicate-type "https://slsa.dev/provenance/v1" \ + >/dev/null || fail "release provenance verification failed" + return + fi + if [ "${FRBIT_VERIFY_PROVENANCE:-}" = "1" ]; then + fail "GitHub CLI (gh) is required when FRBIT_VERIFY_PROVENANCE=1" + fi + printf 'frbit installer: GitHub CLI (gh) not found; skipping release provenance verification. Set FRBIT_VERIFY_PROVENANCE=1 to require this check during installation.\n' >&2 +} + +verify_provenance + tar -xzf "$tmp_dir/$asset" -C "$tmp_dir" frbit mkdir -p "$install_dir" install -m 0755 "$tmp_dir/frbit" "$install_dir/frbit" diff --git a/internal/agentskills/source.go b/internal/agentskills/source.go index 7cd3edf..afb3ded 100644 --- a/internal/agentskills/source.go +++ b/internal/agentskills/source.go @@ -152,7 +152,7 @@ func (s *Service) fetchPayload(ctx context.Context, release Release) (payload, e result.copilot, result.hasCopilot = file, true case strings.HasPrefix(name, "skills/"): parts := strings.SplitN(strings.TrimPrefix(name, "skills/"), "/", 2) - if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + if len(parts) != 2 || !validSkillName(parts[0]) || parts[1] == "" { continue } if result.skills[parts[0]] == nil { @@ -215,3 +215,19 @@ func wantedArchivePath(name string) bool { return name == "VERSION" || name == "update.sh" || name == "uninstall.sh" || name == ".github/instructions/fortrabbit.instructions.md" || strings.HasPrefix(name, "skills/") } + +// validSkillName deliberately uses a platform-independent allowlist. Archive +// paths always use slashes, but a backslash becomes a path separator on Windows. +func validSkillName(name string) bool { + if name == "" || name == "." || name == ".." { + return false + } + for _, character := range name { + if (character >= 'a' && character <= 'z') || (character >= 'A' && character <= 'Z') || + (character >= '0' && character <= '9') || strings.ContainsRune("._-", character) { + continue + } + return false + } + return true +} diff --git a/internal/agentskills/types_test.go b/internal/agentskills/types_test.go index fd95fdf..b78dc16 100644 --- a/internal/agentskills/types_test.go +++ b/internal/agentskills/types_test.go @@ -89,3 +89,16 @@ func TestCopilotRequiresProjectScope(t *testing.T) { t.Fatal("expected project-scope error") } } + +func TestValidSkillNameRejectsPlatformPathSeparators(t *testing.T) { + for _, name := range []string{"", ".", "..", "../outside", `..\\outside`, `skill\\name`, "skill/name"} { + if validSkillName(name) { + t.Errorf("validSkillName(%q) = true, want false", name) + } + } + for _, name := range []string{"fortrabbit", "fortrabbit-api-access", "skill_2.0"} { + if !validSkillName(name) { + t.Errorf("validSkillName(%q) = false, want true", name) + } + } +} diff --git a/internal/app/factory.go b/internal/app/factory.go index 4faa5a6..d4f2bf5 100644 --- a/internal/app/factory.go +++ b/internal/app/factory.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "strings" + "time" "github.com/fortrabbit/frbit-cli/internal/browser" "github.com/fortrabbit/frbit-cli/internal/config" @@ -35,7 +36,9 @@ func NewFactory(version string, commit string, date string) *Factory { panic(fmt.Sprintf("initialize config store: %v", err)) } - httpClient := &http.Client{} + // This client is also used by skills downloads, which do not pass through + // api.NewClient's per-origin timeout wrapper. + httpClient := &http.Client{Timeout: 30 * time.Second} checker := update.NewChecker(httpClient) return &Factory{ diff --git a/internal/app/factory_test.go b/internal/app/factory_test.go new file mode 100644 index 0000000..1f385da --- /dev/null +++ b/internal/app/factory_test.go @@ -0,0 +1,12 @@ +package app + +import ( + "testing" + "time" +) + +func TestNewFactorySetsHTTPTimeout(t *testing.T) { + if got := NewFactory("test", "test", "test").HTTPClient.Timeout; got != 30*time.Second { + t.Fatalf("HTTP timeout = %s, want %s", got, 30*time.Second) + } +} diff --git a/internal/cmd/environments/environments.go b/internal/cmd/environments/environments.go index 470607f..8fdbd42 100644 --- a/internal/cmd/environments/environments.go +++ b/internal/cmd/environments/environments.go @@ -1,6 +1,7 @@ package environments import ( + "encoding/json" "fmt" "io" "text/tabwriter" @@ -256,6 +257,7 @@ func newCmdVariables(factory *app.Factory) *cobra.Command { func newCmdVariablesGet(factory *app.Factory) *cobra.Command { var printJSON bool + var reveal bool command := &cobra.Command{ Use: "get ", Short: "Get custom and platform environment variables", @@ -269,14 +271,18 @@ func newCmdVariablesGet(factory *app.Factory) *cobra.Command { if err != nil { return err } - if printJSON { + if printJSON && reveal { _, err = fmt.Fprintf(cmd.OutOrStdout(), "%s\n", response.Raw) return err } - return writeEnvironmentVariables(cmd.OutOrStdout(), response.Resource) + if printJSON { + return writeEnvironmentVariablesJSON(cmd.OutOrStdout(), response.Resource) + } + return writeEnvironmentVariables(cmd.OutOrStdout(), response.Resource, reveal) }, } - command.Flags().BoolVar(&printJSON, "json", false, "Print the API response as JSON") + command.Flags().BoolVar(&printJSON, "json", false, "Print environment variables as JSON (values remain masked unless --reveal is set)") + command.Flags().BoolVar(&reveal, "reveal", false, "Show environment variable values") return command } @@ -327,11 +333,11 @@ func newCmdVariablesUpdate(factory *app.Factory) *cobra.Command { _, err = fmt.Fprintf(cmd.OutOrStdout(), "%s\n", response.Raw) return err } - return writeEnvironmentVariables(cmd.OutOrStdout(), response.Resource) + return writeEnvironmentVariables(cmd.OutOrStdout(), response.Resource, false) }, } command.Flags().StringVarP(&file, "file", "f", "", "Read the complete request body from a JSON file ('-' for stdin)") - command.Flags().StringArrayVar(&setValues, "set", nil, "Variable to create or update as NAME=VALUE (repeatable)") + command.Flags().StringArrayVar(&setValues, "set", nil, "Variable to create or update as NAME=VALUE (repeatable; use --file or --file - for secrets)") command.Flags().StringArrayVar(&deleteNames, "delete", nil, "Variable name to delete (repeatable)") command.Flags().BoolVar(&printJSON, "json", false, "Print the API response as JSON") return command @@ -386,7 +392,7 @@ func writeResourceResponse(command *cobra.Command, response api.ResourceResponse return resource.WriteResource(command.OutOrStdout(), response.Resource) } -func writeEnvironmentVariables(output io.Writer, value api.Resource) error { +func writeEnvironmentVariables(output io.Writer, value api.Resource, reveal bool) error { table := tabwriter.NewWriter(output, 0, 4, 2, ' ', 0) wroteHeader := false for _, kind := range []string{"custom", "platform"} { @@ -402,8 +408,8 @@ func writeEnvironmentVariables(output io.Writer, value api.Resource) error { } wroteHeader = true } - variableValue := "" - if variable["value"] != nil { + variableValue := "***" + if reveal && variable["value"] != nil { variableValue = fmt.Sprint(variable["value"]) } if _, err := fmt.Fprintf(table, "%s\t%s\t%s\n", kind, fmt.Sprint(variable["name"]), variableValue); err != nil { @@ -417,3 +423,37 @@ func writeEnvironmentVariables(output io.Writer, value api.Resource) error { } return table.Flush() } + +func writeEnvironmentVariablesJSON(output io.Writer, value api.Resource) error { + masked := make(api.Resource, len(value)) + for key, raw := range value { + variables, ok := raw.([]any) + if !ok || (key != "custom" && key != "platform") { + masked[key] = raw + continue + } + entries := make([]any, len(variables)) + for index, entry := range variables { + variable, ok := entry.(map[string]any) + if !ok { + entries[index] = entry + continue + } + copy := make(map[string]any, len(variable)) + for field, fieldValue := range variable { + copy[field] = fieldValue + } + if copy["value"] != nil { + copy["value"] = "***" + } + entries[index] = copy + } + masked[key] = entries + } + encoded, err := json.Marshal(masked) + if err != nil { + return fmt.Errorf("encode environment variables: %w", err) + } + _, err = fmt.Fprintf(output, "%s\n", encoded) + return err +} diff --git a/internal/cmd/environments/environments_test.go b/internal/cmd/environments/environments_test.go new file mode 100644 index 0000000..7269fec --- /dev/null +++ b/internal/cmd/environments/environments_test.go @@ -0,0 +1,38 @@ +package environments + +import ( + "bytes" + "strings" + "testing" + + "github.com/fortrabbit/frbit-cli/internal/api" +) + +func TestWriteEnvironmentVariablesMasksValuesUnlessRevealed(t *testing.T) { + variables := api.Resource{"custom": []any{map[string]any{"name": "SECRET", "value": "not-for-output"}}} + masked := &bytes.Buffer{} + if err := writeEnvironmentVariables(masked, variables, false); err != nil { + t.Fatal(err) + } + if got := masked.String(); strings.Contains(got, "not-for-output") || !strings.Contains(got, "***") { + t.Fatalf("masked output = %q", got) + } + + revealed := &bytes.Buffer{} + if err := writeEnvironmentVariables(revealed, variables, true); err != nil { + t.Fatal(err) + } + if got := revealed.String(); !strings.Contains(got, "not-for-output") { + t.Fatalf("revealed output = %q", got) + } +} + +func TestWriteEnvironmentVariablesJSONMasksValues(t *testing.T) { + output := &bytes.Buffer{} + if err := writeEnvironmentVariablesJSON(output, api.Resource{"custom": []any{map[string]any{"name": "SECRET", "value": "not-for-output"}}}); err != nil { + t.Fatal(err) + } + if got := output.String(); strings.Contains(got, "not-for-output") || !strings.Contains(got, `"value":"***"`) { + t.Fatalf("masked JSON = %q", got) + } +} diff --git a/internal/cmd/resource/resource.go b/internal/cmd/resource/resource.go index 54e3c9f..384be3a 100644 --- a/internal/cmd/resource/resource.go +++ b/internal/cmd/resource/resource.go @@ -9,6 +9,7 @@ import ( "sort" "strings" "text/tabwriter" + "unicode" "github.com/fortrabbit/frbit-cli/internal/api" "github.com/fortrabbit/frbit-cli/internal/app" @@ -310,13 +311,22 @@ func writeLogs(output io.Writer, resource api.Resource) error { if !ok { continue } - if _, err := fmt.Fprintf(output, "%s\t%s\n", resourceValue(entry["time"]), resourceValue(entry["log"])); err != nil { + if _, err := fmt.Fprintf(output, "%s\t%s\n", resourceValue(entry["time"]), stripControlCharacters(resourceValue(entry["log"]))); err != nil { return err } } return nil } +func stripControlCharacters(value string) string { + return strings.Map(func(character rune) rune { + if unicode.IsControl(character) { + return -1 + } + return character + }, value) +} + func resourceValue(value any) string { if value == nil { return "" diff --git a/internal/cmd/resource/resource_test.go b/internal/cmd/resource/resource_test.go new file mode 100644 index 0000000..8dba4bd --- /dev/null +++ b/internal/cmd/resource/resource_test.go @@ -0,0 +1,23 @@ +package resource + +import ( + "bytes" + "strings" + "testing" + + "github.com/fortrabbit/frbit-cli/internal/api" +) + +func TestWriteLogsStripsTerminalControlCharacters(t *testing.T) { + output := &bytes.Buffer{} + err := writeLogs(output, api.Resource{"logs": []any{map[string]any{ + "time": "2026-01-01T00:00:00Z", + "log": "\x1b[2Jbuild\r\ncomplete", + }}}) + if err != nil { + t.Fatal(err) + } + if got := output.String(); strings.ContainsAny(got, "\x1b\r") || !strings.Contains(got, "[2Jbuildcomplete") { + t.Fatalf("log output = %q", got) + } +} diff --git a/internal/cmdutil/runtime.go b/internal/cmdutil/runtime.go index fa4d38b..cb83549 100644 --- a/internal/cmdutil/runtime.go +++ b/internal/cmdutil/runtime.go @@ -2,10 +2,12 @@ package cmdutil import ( "fmt" + "net/url" "strings" "github.com/fortrabbit/frbit-cli/internal/api" "github.com/fortrabbit/frbit-cli/internal/app" + "github.com/fortrabbit/frbit-cli/internal/config" "github.com/spf13/cobra" ) @@ -37,6 +39,11 @@ func APIClient(command *cobra.Command, factory *app.Factory) (*api.Client, error if err != nil { return nil, err } + if !isDefaultHost(host) { + if _, err := fmt.Fprintf(command.ErrOrStderr(), "Warning: sending API credentials to non-default host %s. Verify this host before continuing.\n", host); err != nil { + return nil, err + } + } profile, err := Profile(command) if err != nil { return nil, err @@ -48,6 +55,15 @@ func APIClient(command *cobra.Command, factory *app.Factory) (*api.Client, error return Client(factory, host, token) } +func isDefaultHost(host string) bool { + parsed, err := url.Parse(strings.TrimSpace(host)) + if err != nil { + return false + } + defaultParsed, _ := url.Parse(config.DefaultHost) + return strings.EqualFold(parsed.Scheme, defaultParsed.Scheme) && strings.EqualFold(parsed.Host, defaultParsed.Host) && parsed.Path == "" && parsed.RawQuery == "" && parsed.Fragment == "" +} + func TrimToken(token string) string { return strings.TrimSpace(token) } diff --git a/scripts/test-install.sh b/scripts/test-install.sh index 26b312d..c807a77 100755 --- a/scripts/test-install.sh +++ b/scripts/test-install.sh @@ -15,6 +15,8 @@ case "$(uname -s):$(uname -m)" in esac mkdir -p "$test_dir/release" "$test_dir/package" "$test_dir/bin" +printf '#!/bin/sh\nexit 0\n' > "$test_dir/gh" +chmod 0755 "$test_dir/gh" printf '#!/bin/sh\nprintf "frbit installer fixture\\n"\n' > "$test_dir/package/frbit" chmod 0755 "$test_dir/package/frbit" tar -czf "$test_dir/release/$asset" -C "$test_dir/package" frbit @@ -27,9 +29,18 @@ fi printf '%s %s\n' "$checksum" "$asset" > "$test_dir/release/checksums.txt" FRBIT_RELEASE_BASE_URL="file://$test_dir/release" \ + FRBIT_INSTALL_TESTING=1 \ FRBIT_INSTALL_DIR="$test_dir/bin" \ + PATH="$test_dir:$PATH" \ sh "$root/install.sh" test -x "$test_dir/bin/frbit" test "$("$test_dir/bin/frbit")" = "frbit installer fixture" + +if FRBIT_RELEASE_BASE_URL="file://$test_dir/release" FRBIT_INSTALL_TESTING=1 FRBIT_INSTALL_DIR="$test_dir/strict-bin" PATH="/usr/bin:/bin" FRBIT_VERIFY_PROVENANCE=1 sh "$root/install.sh" 2>&1 | grep -q 'GitHub CLI (gh) is required'; then + : +else + printf 'strict provenance check did not require gh\n' >&2 + exit 1 +fi printf 'installer test passed\n' From abcbb46908a1f8216f5e36d91fc5a9c6145d6c22 Mon Sep 17 00:00:00 2001 From: Rafik Abdulwahab Date: Wed, 2 Sep 2026 12:30:14 +0200 Subject: [PATCH 2/2] harden release provenance checks --- .github/workflows/release.yml | 1 + scripts/test-install.sh | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 5009bc5..51f0d2d 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -7,6 +7,7 @@ on: permissions: attestations: write + artifact-metadata: write contents: write id-token: write diff --git a/scripts/test-install.sh b/scripts/test-install.sh index c807a77..539d624 100755 --- a/scripts/test-install.sh +++ b/scripts/test-install.sh @@ -14,9 +14,11 @@ case "$(uname -s):$(uname -m)" in *) printf 'unsupported test platform\n' >&2; exit 1 ;; esac -mkdir -p "$test_dir/release" "$test_dir/package" "$test_dir/bin" +mkdir -p "$test_dir/release" "$test_dir/package" "$test_dir/bin" "$test_dir/strict-bin" printf '#!/bin/sh\nexit 0\n' > "$test_dir/gh" chmod 0755 "$test_dir/gh" +printf '#!/bin/sh\nexit 1\n' > "$test_dir/strict-bin/gh" +chmod 0755 "$test_dir/strict-bin/gh" printf '#!/bin/sh\nprintf "frbit installer fixture\\n"\n' > "$test_dir/package/frbit" chmod 0755 "$test_dir/package/frbit" tar -czf "$test_dir/release/$asset" -C "$test_dir/package" frbit @@ -37,10 +39,10 @@ FRBIT_RELEASE_BASE_URL="file://$test_dir/release" \ test -x "$test_dir/bin/frbit" test "$("$test_dir/bin/frbit")" = "frbit installer fixture" -if FRBIT_RELEASE_BASE_URL="file://$test_dir/release" FRBIT_INSTALL_TESTING=1 FRBIT_INSTALL_DIR="$test_dir/strict-bin" PATH="/usr/bin:/bin" FRBIT_VERIFY_PROVENANCE=1 sh "$root/install.sh" 2>&1 | grep -q 'GitHub CLI (gh) is required'; then +if FRBIT_RELEASE_BASE_URL="file://$test_dir/release" FRBIT_INSTALL_TESTING=1 FRBIT_INSTALL_DIR="$test_dir/strict-install" PATH="$test_dir/strict-bin:$PATH" FRBIT_VERIFY_PROVENANCE=1 sh "$root/install.sh" 2>&1 | grep -q 'release provenance verification failed'; then : else - printf 'strict provenance check did not require gh\n' >&2 + printf 'strict provenance check did not fail\n' >&2 exit 1 fi printf 'installer test passed\n'