Skip to content

feat(store): buttons install <name | tag:x> from a Source - #190

Merged
bobakemamian merged 3 commits into
mainfrom
feat/store-install
Jun 23, 2026
Merged

feat(store): buttons install <name | tag:x> from a Source#190
bobakemamian merged 3 commits into
mainfrom
feat/store-install

Conversation

@bobakemamian

@bobakemamian bobakemamian commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

What

First slice of the install client (#274), flat — drops the store namespace:

  • internal/store — a Source interface (Index, Fetch) so the CLI is backend-agnostic. LocalSource (a directory) ships now; the registry HTTPSource (MCP 2026-07-28: unknown tool name in tools/call → -32602 (not -32601) #275) is a drop-in later.
  • install resolutionname, name@version, or tag:<x> (every button carrying the tag). Installs each button + its requires deps transitively, and stamps source / version / content_hash into the installed button.json (pinning).
  • cmd/install.gobuttons install <name | tag:x> --source <dir> (or $BUTTONS_SOURCE). Removes the cmd/store.go stub.

Model

Buttons are the atomic unit; collections are tags (install tag:autono-cal); deps come from button requires. No "pack" object.

Test

  • go build ./..., go vet, internal/store tests pass (install-by-name + deps, install-by-tag, no-match error, version parse).
  • Smoke: buttons install tag:demo --source ./pack installs the tagged button with provenance stamped.

Follow-ups (this PR is intentionally scoped to install)

Refs autonoco/autono#274

Summary by CodeRabbit

  • New Features
    • Added an install CLI subcommand to install button specs by name or tag:<tag>, including transitive dependency resolution
    • Added version pinning via name@version
    • Added --source / BUTTONS_SOURCE to select the source directory
    • Added optional JSON output on successful installs
  • Bug Fixes
    • Improved safety checks to prevent path traversal during install/fetch and ensured strict version-pin verification
  • Tests
    • Expanded coverage for installs, tag matching (including no-match), version parsing, and traversal protection

First slice of the install client (#274), flat (no `store` namespace):
- internal/store: a `Source` interface (Index + Fetch) + `LocalSource`
  (a directory) so the CLI is backend-agnostic; the registry HTTPSource
  (#275) is a drop-in later.
- install resolves `name`, `name@version`, or `tag:<x>` (every button
  with the tag), installs each + its button.json `requires` deps
  transitively, and stamps source/version/content_hash into button.json.
- cmd/install.go: `buttons install <name|tag:x> --source <dir>`
  ($BUTTONS_SOURCE). Removes the `cmd/store.go` stub.

Refs autonoco/autono#274

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds the install <name | tag:x> CLI subcommand and a new internal/store package. The package defines a pluggable Source interface with a LocalSource implementation, implements deterministic hashFiles SHA256 hashing, provides path-safety validators (validName, safeJoin), and exports InstallSpec which resolves specs by name (with optional @version pin) or by tag, installs transitive dependencies with deduplication, stamps metadata, and writes files to disk with security hardening against path traversal. The placeholder store subcommand is removed.

Changes

Install Subcommand and Store Layer

Layer / File(s) Summary
Source abstraction and LocalSource
internal/store/source.go
Defines ButtonRef, Bundle, and Source interface; implements hashFiles for deterministic SHA256; adds path-safety validators validName and safeJoin to reject traversal patterns; implements LocalSource.Index() scanning subdirectories and reading button.json metadata; implements LocalSource.Fetch() with traversal-name validation and version-pin enforcement.
InstallSpec, dependency resolution, and file writing
internal/store/install.go
Adds Result type and InstallSpec entry point parsing tag:<x> (expanding to all matching buttons) or name[@Version] (pinning version for root only); implements recursive installWithDeps with slug-based deduplication; implements install which fetches bundles, stamps Source/Version/ContentHash, creates button directories with pressed/ subdirs, and writes files with 0600 defaults except main.* as 0700; adds resolveTag and splitVersion helpers.
install CLI subcommand
cmd/install.go
Adds installCmd resolving source directory from --source flag or BUTTONS_SOURCE environment variable, invoking store.InstallSpec, and outputting results via JSON (config.WriteJSON) or stderr with a next-step hint for buttons list.
Unit tests for source, install, and security validations
internal/store/store_test.go
Adds test helpers writeSourceButton (synthetic button fixture) and installedSpec (verification helper); tests install by name with transitive dependency closure and metadata stamping; tests install by tag with tag filtering; tests tag-no-match error; tests splitVersion parsing; tests traversal-name rejection in Fetch and in safeJoin; tests version-pin mismatch detection and empty-version ("latest") success; tests bundle file traversal rejection via traversalSource fixture.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant installCmd
  participant InstallSpec
  participant LocalSource
  participant Disk

  User->>installCmd: buttons install alpha@1.0 --source ./src
  installCmd->>InstallSpec: spec="alpha@1.0", sourceRef="./src"
  InstallSpec->>InstallSpec: splitVersion → name="alpha", version="1.0"
  InstallSpec->>LocalSource: Fetch("alpha", "1.0")
  LocalSource-->>InstallSpec: Bundle
  InstallSpec->>InstallSpec: stamp button.json (Source, Version, ContentHash)
  InstallSpec->>Disk: mkdir buttons/alpha/pressed, write files
  loop beta in alpha.Requires
    InstallSpec->>LocalSource: Fetch("beta", "")
    LocalSource-->>InstallSpec: Bundle
    InstallSpec->>Disk: write buttons/beta/...
  end
  InstallSpec-->>installCmd: Result{Installed: ["alpha","beta"]}
  installCmd-->>User: JSON or stderr output
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • autonoco/autono#274: This PR directly implements the buttons install <name>[@Version] subcommand, the Source/LocalSource abstraction with version pinning and content-hash verification, InstallSpec with tag-based expansion and recursive dependency resolution, and path-traversal security validations as described in that issue.

Possibly related PRs

  • autonoco/buttons#186: Extended internal/button.Button with Tags, Version, and Requires fields that InstallSpec and LocalSource depend on directly for tag expansion and recursive dependency closure.

Poem

🐇 Hop, hop, a button appears,
From source dir fetched, no fuzz, no fears.
Dependencies bundled, SHA sealed tight,
Path guards stand tall through the night.
buttons install done with delight! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically describes the main change: introducing an install command for the buttons package manager that supports installation by name or tag from a Source.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/store-install

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/install.go`:
- Around line 45-46: The config.WriteJSONError() call is discarding its error
return value, which masks write failures and prevents proper error reporting.
Instead of using the blank identifier to ignore the error from
config.WriteJSONError(), capture the returned error and check if it occurred. If
config.WriteJSONError() returns an error, return that error immediately so the
caller knows about the write failure. Only return errSilent as a fallback when
the JSON write operation succeeds. Apply this fix to all occurrences of the
config.WriteJSONError() pattern mentioned in the comment (including the second
location at lines 55-56).

In `@internal/store/install.go`:
- Around line 105-108: The file mode selection logic in the code block starting
at line 105 currently grants 0700 permissions only to files starting with
"main.", but the repository guideline requires 0700 for all code files and 0600
only for spec/history JSON files. Refactor the permission logic to check if the
file path (rel variable) is a spec/history JSON file instead of checking for the
"main." prefix. If the file is a spec/history JSON file, set mode to 0600,
otherwise default to 0700 to align with the policy that all code files require
executable permissions.
- Around line 104-111: The code in the bundle file installation loop has a path
traversal vulnerability where the relative path variable `rel` is not validated
before being joined with the target directory `dir` in the filepath.Join call.
An attacker could provide paths containing traversal sequences like `../` to
escape the intended installation directory. Add path containment validation
before the os.WriteFile call to ensure the resolved path stays within the target
directory, either by rejecting paths containing `..` or by validating that the
absolute resolved path has the target directory as its prefix.

In `@internal/store/source.go`:
- Around line 90-116: The LocalSource.Fetch method ignores the version parameter
(currently marked with underscore) which breaks install-by-version semantics.
Instead of discarding this parameter, use it to validate that the requested
version matches the version found in the unmarshaled button.Button object after
parsing button.json. After unmarshaling the button.Button, compare the requested
version with b.Version and return an error if they do not match, ensuring that
the method honors version-specific install requests.
- Around line 90-103: The Fetch method in LocalSource has a path traversal
vulnerability where the unvalidated name parameter is directly joined with
s.Root using filepath.Join, allowing attackers to escape the source root with
inputs like ../../. After constructing the dir path by joining s.Root with name,
validate that the resolved path still remains within s.Root by using
filepath.Clean and filepath.Abs to ensure the final path starts with the root
directory. This prevents unauthorized access to files outside the intended
source directory.

In `@internal/store/store_test.go`:
- Around line 17-25: The writeSourceButton test fixture uses non-compliant
permission modes that violate the internal package policy. Change the
os.MkdirAll call to use 0700 instead of 0755 for the directory creation, change
the button.json file creation to use 0600 instead of 0644 since it is a
spec/history JSON file, and change the main.sh file creation to use 0700 instead
of 0644 since it is a code file. These changes ensure all file and directory
permissions comply with the required standards for internal package tests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d9cc7569-e974-4ff2-828d-7155ada4fb32

📥 Commits

Reviewing files that changed from the base of the PR and between cf42102 and 8f85df9.

📒 Files selected for processing (5)
  • cmd/install.go
  • cmd/store.go
  • internal/store/install.go
  • internal/store/source.go
  • internal/store/store_test.go
💤 Files with no reviewable changes (1)
  • cmd/store.go

Comment thread cmd/install.go
Comment on lines +45 to +46
_ = config.WriteJSONError("VALIDATION_ERROR", msg)
return errSilent

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Don’t discard JSON write failures on error paths

If config.WriteJSONError(...) fails, returning errSilent hides the real failure mode. Return the write error first, then fall back to errSilent only when emission succeeds.

Small fix
@@
 			if jsonOutput {
-				_ = config.WriteJSONError("VALIDATION_ERROR", msg)
+				if werr := config.WriteJSONError("VALIDATION_ERROR", msg); werr != nil {
+					return werr
+				}
 				return errSilent
 			}
@@
 			if jsonOutput {
-				_ = config.WriteJSONError("INSTALL_ERROR", err.Error())
+				if werr := config.WriteJSONError("INSTALL_ERROR", err.Error()); werr != nil {
+					return werr
+				}
 				return errSilent
 			}

Also applies to: 55-56

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/install.go` around lines 45 - 46, The config.WriteJSONError() call is
discarding its error return value, which masks write failures and prevents
proper error reporting. Instead of using the blank identifier to ignore the
error from config.WriteJSONError(), capture the returned error and check if it
occurred. If config.WriteJSONError() returns an error, return that error
immediately so the caller knows about the write failure. Only return errSilent
as a fallback when the JSON write operation succeeds. Apply this fix to all
occurrences of the config.WriteJSONError() pattern mentioned in the comment
(including the second location at lines 55-56).

Comment thread internal/store/install.go Outdated
Comment thread internal/store/install.go
Comment on lines +105 to +108
mode := os.FileMode(0600)
if strings.HasPrefix(rel, "main.") {
mode = 0700 // #nosec G302 -- code files need the exec bit to run via sh/python/node
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

File mode policy in internal/** does not fully match repository guideline

Current logic grants 0700 only to main.*, but your guideline requires 0700 for code files and 0600 only for spec/history JSON files. This likely under-permissions non-main.* code artifacts.

As per coding guidelines, "Set file permissions to 0700 for data directories and code files, and 0600 for spec/history JSON files".

Policy-aligned mode selection example
@@
-		mode := os.FileMode(0600)
-		if strings.HasPrefix(rel, "main.") {
-			mode = 0700 // `#nosec` G302 -- code files need the exec bit to run via sh/python/node
-		}
+		mode := os.FileMode(0700) // code/data default in internal/** policy
+		if rel == "button.json" || strings.HasSuffix(rel, ".history.json") {
+			mode = 0600
+		}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
mode := os.FileMode(0600)
if strings.HasPrefix(rel, "main.") {
mode = 0700 // #nosec G302 -- code files need the exec bit to run via sh/python/node
}
mode := os.FileMode(0700) // code/data default in internal/** policy
if rel == "button.json" || strings.HasSuffix(rel, ".history.json") {
mode = 0600
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/install.go` around lines 105 - 108, The file mode selection
logic in the code block starting at line 105 currently grants 0700 permissions
only to files starting with "main.", but the repository guideline requires 0700
for all code files and 0600 only for spec/history JSON files. Refactor the
permission logic to check if the file path (rel variable) is a spec/history JSON
file instead of checking for the "main." prefix. If the file is a spec/history
JSON file, set mode to 0600, otherwise default to 0700 to align with the policy
that all code files require executable permissions.

Source: Coding guidelines

Comment thread internal/store/source.go Outdated
Comment thread internal/store/source.go Outdated
Comment on lines +17 to +25
if err := os.MkdirAll(dir, 0755); err != nil {
t.Fatal(err)
}
data, _ := json.MarshalIndent(&b, "", " ")
if err := os.WriteFile(filepath.Join(dir, "button.json"), data, 0644); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(dir, "main.sh"), []byte("#!/bin/sh\necho hi\n"), 0644); err != nil {
t.Fatal(err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Use repository-required permission modes in test fixtures.

writeSourceButton currently creates directories/files with 0755/0644, which violates the internal/**/*.go permission policy and weakens permission-sensitive coverage.

Suggested patch
-	if err := os.MkdirAll(dir, 0755); err != nil {
+	if err := os.MkdirAll(dir, 0700); err != nil {
 		t.Fatal(err)
 	}
 	data, _ := json.MarshalIndent(&b, "", "  ")
-	if err := os.WriteFile(filepath.Join(dir, "button.json"), data, 0644); err != nil {
+	if err := os.WriteFile(filepath.Join(dir, "button.json"), data, 0600); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.WriteFile(filepath.Join(dir, "main.sh"), []byte("#!/bin/sh\necho hi\n"), 0644); err != nil {
+	if err := os.WriteFile(filepath.Join(dir, "main.sh"), []byte("#!/bin/sh\necho hi\n"), 0700); err != nil {
 		t.Fatal(err)
 	}

As per coding guidelines, "internal/**/*.go: Set file permissions to 0700 for data directories and code files, and 0600 for spec/history JSON files".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if err := os.MkdirAll(dir, 0755); err != nil {
t.Fatal(err)
}
data, _ := json.MarshalIndent(&b, "", " ")
if err := os.WriteFile(filepath.Join(dir, "button.json"), data, 0644); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(dir, "main.sh"), []byte("#!/bin/sh\necho hi\n"), 0644); err != nil {
t.Fatal(err)
if err := os.MkdirAll(dir, 0700); err != nil {
t.Fatal(err)
}
data, _ := json.MarshalIndent(&b, "", " ")
if err := os.WriteFile(filepath.Join(dir, "button.json"), data, 0600); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(dir, "main.sh"), []byte("#!/bin/sh\necho hi\n"), 0700); err != nil {
t.Fatal(err)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/store_test.go` around lines 17 - 25, The writeSourceButton
test fixture uses non-compliant permission modes that violate the internal
package policy. Change the os.MkdirAll call to use 0700 instead of 0755 for the
directory creation, change the button.json file creation to use 0600 instead of
0644 since it is a spec/history JSON file, and change the main.sh file creation
to use 0700 instead of 0644 since it is a code file. These changes ensure all
file and directory permissions comply with the required standards for internal
package tests.

Source: Coding guidelines

Address three CodeRabbit findings on the install path:
- read traversal: LocalSource.Fetch rejects names that aren't a single
  path component (validName), so a CLI spec or a button's `requires`
  can't escape the source root via "../".
- write traversal: install() routes every bundle file key through
  safeJoin, containing writes to the button dir — defense for the
  untrusted HTTPSource registry (#275) behind the Source interface.
- version pin: Fetch honors the version arg, erroring on a mismatch
  instead of silently returning whatever is on disk.

Tests: traversal name/bundle rejection, version mismatch, safeJoin table.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/store/source.go (1)

118-137: 🔒 Security & Privacy | 🔴 Critical

Reject symlinked source files before reading bundle contents.

validName ensures dir stays under s.Root, but os.ReadFile(filepath.Join(dir, e.Name())) at line 133 follows symlinks. If a local source directory contains a symlinked entry like secret.txt -> /etc/passwd, the code will read the target file instead of rejecting it, bypassing the read-containment guarantee.

Proposed fix
 	files := map[string][]byte{}
 	for _, e := range entries {
-		if e.IsDir() {
+		path := filepath.Join(dir, e.Name())
+		info, err := os.Lstat(path)
+		if err != nil {
+			return nil, err
+		}
+		if info.Mode()&os.ModeSymlink != 0 {
+			return nil, fmt.Errorf("button %q: symlink %q is not allowed", name, e.Name())
+		}
+		if info.IsDir() {
 			continue // skip pressed/
 		}
 		// `#nosec` G304 -- dir/name both come from enumerated entries under s.Root.
-		data, err := os.ReadFile(filepath.Join(dir, e.Name()))
+		data, err := os.ReadFile(path)
 		if err != nil {
 			return nil, err
 		}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/source.go` around lines 118 - 137, The Fetch method of
LocalSource does not check for symlinks before reading files with os.ReadFile,
which allows symlinked entries to bypass the containment guarantee by reading
files outside s.Root. In the loop iterating through entries returned by
os.ReadDir, before calling os.ReadFile on each entry, add a check to determine
if the entry is a symlink (using the IsSymlink method available on os.DirEntry)
and skip that entry if it is, ensuring that only regular files within the
designated directory are read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/store/store_test.go`:
- Around line 166-181: The path containment assertion in TestSafeJoin using
strings.HasPrefix(dst, dir) is insufficient because it only checks string prefix
matching, allowing paths like /tmp/install/alpha2/file to incorrectly pass as
contained within /tmp/install/alpha. Update the assertion to use a
separator-aware containment check that ensures the destination path either
equals dir exactly or has dir followed by a path separator, similar to how the
production safeJoin function should validate containment. This prevents
sibling-directory prefix escapes from passing the test.
- Around line 184-189: The install function creates the button directory before
validating all bundle keys for safety, which can leave partial installations
behind when a traversal bundle is rejected. Refactor the install function to
prevalidate all safeJoin results before performing any directory creation or
file writing operations, ensuring that bundle safety checks happen first.
Additionally, extend the TestInstallRejectsTraversalBundle test to assert that
no button directory or related files exist after the rejected traversal bundle
install attempt, verifying that rejected installations do not leave behind any
partial state.

---

Outside diff comments:
In `@internal/store/source.go`:
- Around line 118-137: The Fetch method of LocalSource does not check for
symlinks before reading files with os.ReadFile, which allows symlinked entries
to bypass the containment guarantee by reading files outside s.Root. In the loop
iterating through entries returned by os.ReadDir, before calling os.ReadFile on
each entry, add a check to determine if the entry is a symlink (using the
IsSymlink method available on os.DirEntry) and skip that entry if it is,
ensuring that only regular files within the designated directory are read.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8c23fb56-1e88-4bbf-8be7-721512c783b2

📥 Commits

Reviewing files that changed from the base of the PR and between 8f85df9 and ad3aeb4.

📒 Files selected for processing (3)
  • internal/store/install.go
  • internal/store/source.go
  • internal/store/store_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/store/install.go

Comment on lines +166 to +181
func TestSafeJoin(t *testing.T) {
dir := filepath.Clean("/tmp/install/alpha")
for _, r := range []string{"../evil", "../../etc/passwd", "..", "/abs/path", "sub/../../escape"} {
if _, err := safeJoin(dir, r); err == nil {
t.Errorf("safeJoin(%q) should be rejected", r)
}
}
for _, r := range []string{"button.json", "main.sh", "AGENT.md", "sub/file.txt"} {
dst, err := safeJoin(dir, r)
if err != nil {
t.Errorf("safeJoin(%q) should be allowed: %v", r, err)
}
if !strings.HasPrefix(dst, dir) {
t.Errorf("safeJoin(%q) escaped: %q", r, dst)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Tighten the allowed-path assertion to catch sibling-prefix escapes.

Line 178 accepts any path with prefix dir, so a regression returning /tmp/install/alpha2/file would pass. Mirror the production separator-aware containment check.

Proposed test fix
-		if !strings.HasPrefix(dst, dir) {
+		if !strings.HasPrefix(dst, dir+string(filepath.Separator)) {
 			t.Errorf("safeJoin(%q) escaped: %q", r, dst)
 		}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/store_test.go` around lines 166 - 181, The path containment
assertion in TestSafeJoin using strings.HasPrefix(dst, dir) is insufficient
because it only checks string prefix matching, allowing paths like
/tmp/install/alpha2/file to incorrectly pass as contained within
/tmp/install/alpha. Update the assertion to use a separator-aware containment
check that ensures the destination path either equals dir exactly or has dir
followed by a path separator, similar to how the production safeJoin function
should validate containment. This prevents sibling-directory prefix escapes from
passing the test.

Comment on lines +184 to +189
func TestInstallRejectsTraversalBundle(t *testing.T) {
t.Setenv("BUTTONS_HOME", t.TempDir())
if _, err := install(traversalSource{}, "evil", "", "local:test"); err == nil {
t.Fatal("install should reject a bundle file that escapes the button dir")
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Assert rejected traversal bundles leave no partial install behind.

The supplied install path creates the button directory before all bundle keys are proven safe, and then validates/writes per file. This test can pass while a rejected bundle leaves button.json, pressed/, or other valid files behind; prevalidate all safeJoin results before creating/writing, and extend the test to assert no installed button directory remains. See internal/store/install.go:95-109 in the provided context.

Suggested production shape
+	dsts := make(map[string]string, len(bundle.Files))
+	for rel := range bundle.Files {
+		dst, err := safeJoin(dir, rel)
+		if err != nil {
+			return nil, err
+		}
+		dsts[rel] = dst
+	}
 	if err := os.MkdirAll(filepath.Join(dir, "pressed"), 0700); err != nil {
 		return nil, fmt.Errorf("create button dir: %w", err)
 	}
 	for rel, data := range bundle.Files {
-		dst, err := safeJoin(dir, rel)
-		if err != nil {
-			return nil, err
-		}
+		dst := dsts[rel]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/store_test.go` around lines 184 - 189, The install function
creates the button directory before validating all bundle keys for safety, which
can leave partial installations behind when a traversal bundle is rejected.
Refactor the install function to prevalidate all safeJoin results before
performing any directory creation or file writing operations, ensuring that
bundle safety checks happen first. Additionally, extend the
TestInstallRejectsTraversalBundle test to assert that no button directory or
related files exist after the rejected traversal bundle install attempt,
verifying that rejected installations do not leave behind any partial state.

 rd2)

Follow-up to ad3aeb4 addressing the re-review:
- install() pre-validates every bundle path via safeJoin before creating
  the button dir or writing, so a rejected traversal bundle leaves no
  partial state behind.
- LocalSource.Fetch skips symlink entries instead of following them out of
  the source root (a shared/untrusted pack could symlink to secrets).
- TestSafeJoin uses a separator-aware containment assertion; the reject
  test asserts no dir is left behind; add TestFetchSkipsSymlinks.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@bobakemamian
bobakemamian merged commit 7714460 into main Jun 23, 2026
16 checks passed
bobakemamian added a commit that referenced this pull request Jun 23, 2026
… (#192)

`buttons update` now brings everything current in one command: the CLI
binary (existing self-updater) AND the content of installed buttons,
re-fetched from the source each was installed from.

- internal/store/update.go: UpdateInstalled reconciles every installed
  button against its stamped `source`. Drift is detected by content hash
  (the install-time SHA256 from #190 vs a fresh Fetch). Drifted buttons
  are re-installed (re-verified + re-stamped); un-sourced (hand-authored)
  or not-yet-resolvable (registry, #275) buttons are skipped, never fatal.
  SourceResolver is injectable; DefaultSourceResolver wires `local:<dir>`.
- cmd/update.go: factor the binary path into runBinaryUpdate; add
  --binary / --content scoping (default = both) and make --check report
  available content updates too. A Homebrew-managed binary is now a soft
  skip (was a hard error) so content still updates in a combined run.

Tested: store unit tests (drift detect/apply, --check is read-only,
skip un-sourced + registry-pinned, resolver) + end-to-end install→drift
→update smoke. Stacked on #190 (needs internal/store).

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant