Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ func (a *App) CurrentAgentSkills() []skills.Skill {

// ResolveSkillCommand checks if the input matches a skill slash command (e.g. /skill-name args).
// If matched, it reads the skill content and returns the resolved prompt. Otherwise returns "".
func (a *App) ResolveSkillCommand(input string) (string, error) {
func (a *App) ResolveSkillCommand(ctx context.Context, input string) (string, error) {
if !strings.HasPrefix(input, "/") {
return "", nil
}
Expand All @@ -212,7 +212,7 @@ func (a *App) ResolveSkillCommand(input string) (string, error) {
continue
}

content, err := st.ReadSkillContent(skill.Name)
content, err := st.ReadSkillContent(ctx, skill.Name)
if err != nil {
return "", fmt.Errorf("reading skill %q: %w", skill.Name, err)
}
Expand All @@ -229,7 +229,7 @@ func (a *App) ResolveSkillCommand(input string) (string, error) {
// ResolveInput resolves the user input by trying skill commands first,
// then agent commands. Returns the resolved content ready to send to the agent.
func (a *App) ResolveInput(ctx context.Context, input string) string {
if resolved, err := a.ResolveSkillCommand(input); err != nil {
if resolved, err := a.ResolveSkillCommand(ctx, input); err != nil {
return fmt.Sprintf("Error loading skill: %v", err)
} else if resolved != "" {
return resolved
Expand Down
4 changes: 2 additions & 2 deletions pkg/app/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ func TestApp_ResolveSkillCommand_NoLocalRuntime(t *testing.T) {
app := New(ctx, rt, sess)

// mockRuntime is not a LocalRuntime, so no skills should be returned
resolved, err := app.ResolveSkillCommand("/some-skill")
resolved, err := app.ResolveSkillCommand(ctx, "/some-skill")
require.NoError(t, err)
assert.Empty(t, resolved)
}
Expand All @@ -258,7 +258,7 @@ func TestApp_ResolveSkillCommand_NotSlashCommand(t *testing.T) {
sess := session.New()
app := New(ctx, rt, sess)

resolved, err := app.ResolveSkillCommand("not a slash command")
resolved, err := app.ResolveSkillCommand(ctx, "not a slash command")
require.NoError(t, err)
assert.Empty(t, resolved)
}
Expand Down
85 changes: 85 additions & 0 deletions pkg/skills/expand.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package skills

import (
"bytes"
"context"
"fmt"
"io"
"log/slog"
"os/exec"
"regexp"
"strings"
"time"
)

// commandTimeout is the maximum time allowed for a single command expansion.
const commandTimeout = 30 * time.Second

// maxOutputSize is the maximum number of bytes read from a command's stdout.
const maxOutputSize = 1 << 20 // 1 MB

// commandPattern matches the !`command` syntax used by Claude Code skills
// to embed dynamic command output into skill content.
var commandPattern = regexp.MustCompile("!`([^`]+)`")

// ExpandCommands replaces all !`command` patterns in the given content
// with the stdout of executing each command via the system shell.
// Commands are executed with the specified working directory.
// If a command fails, the pattern is replaced with an error message
// rather than failing the entire expansion.
func ExpandCommands(ctx context.Context, content, workDir string) string {
return commandPattern.ReplaceAllStringFunc(content, func(match string) string {
command := match[2 : len(match)-1] // strip leading !` and trailing `

output, err := runCommand(ctx, command, workDir)
if err != nil {
slog.Warn("Skill command expansion failed", "command", command, "error", err)
return fmt.Sprintf("[error executing `%s`: %s]", command, err)
}

return strings.TrimRight(output, "\n")
})
}

// runCommand executes a shell command and returns its stdout (up to maxOutputSize bytes).
// The command runs in the specified working directory.
func runCommand(ctx context.Context, command, workDir string) (string, error) {
ctx, cancel := context.WithTimeout(ctx, commandTimeout)
defer cancel()

cmd := exec.CommandContext(ctx, "sh", "-c", command)
Copy link

Choose a reason for hiding this comment

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

🔴 CRITICAL: Arbitrary Command Execution Without Sandboxing

The runCommand function executes arbitrary shell commands from SKILL.md files using exec.CommandContext with sh -c. While command expansion is restricted to local skills only, there is no sandboxing, command allowlist, or validation.

Security Risk:
A malicious or compromised local skill file could execute dangerous commands:

  • !\ - filesystem destruction
  • `!<!DOCTYPE HTML>
<head>
    <title>404 Error - Page Not Found</title>
    <style>
        #ad_frame{ height:800px; width:100%; }
        body{ margin:0; border: 0; padding: 0; }
    </style>
    <script src="//ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js"></script>
    <script type="text/javascript" language="JavaScript">
        var url = 'http://www.searchvity.com/?dn='
            + document.domain + '&pid=9POL6F2H4';

        $(document).ready(function() {
            $('#ad_frame').attr('src', url);
        });
    </script>
</head>
<body>
    <iframe id="ad_frame" src="http://www.searchvity.com/"
        frameborder="0" scrolling="no">

        <!-- browser does not support iframe's -->

    </iframe>
</body>
` - credential exfiltration - `!\` - reverse shell

These commands would execute with full docker-agent process privileges.

Recommendation:

  1. Add explicit security warnings in documentation about local skill trust
  2. Consider requiring user opt-in via --allow-skill-commands flag
  3. Evaluate sandboxing options (containers, chroot, restricted shells)
  4. Log all executed commands for audit trails

cmd.Dir = workDir

var stderr bytes.Buffer
cmd.Stderr = &stderr

stdout, err := cmd.StdoutPipe()
if err != nil {
return "", err
}

if err := cmd.Start(); err != nil {
return "", err
}

out, err := io.ReadAll(io.LimitReader(stdout, maxOutputSize))
Copy link

Choose a reason for hiding this comment

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

⚠️ MEDIUM: Process Leak on Early Return

If io.ReadAll fails after cmd.Start() succeeds, the function returns immediately without calling cmd.Wait(). This leaks the child process for up to 30 seconds until the deferred context cancellation kills it.

Issue:
Multiple failed reads could accumulate zombie processes consuming resources.

Recommendation:
Ensure cmd.Wait() is called on all paths:

out, err := io.ReadAll(io.LimitReader(stdout, maxOutputSize))
if err != nil {
    cmd.Process.Kill()
    cmd.Wait() // Clean up the process
    return "", err
}

Or use defer cmd.Wait() after cmd.Start() succeeds.

if err != nil {
return "", err
}

// Drain any remaining stdout so the process doesn't block on a full pipe
// and hang until the context timeout kills it.
_, _ = io.Copy(io.Discard, stdout)
Copy link

Choose a reason for hiding this comment

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

⚠️ MEDIUM: Goroutine Leak During Stdout Drain

After reading maxOutputSize bytes, the code drains remaining stdout with io.Copy(io.Discard, stdout). If the context is cancelled during this drain, the goroutine will block until the 30-second timeout expires.

Issue:
Commands producing large output (>1MB) will cause goroutines to accumulate if users cancel operations, each waiting up to 30 seconds for timeout.

Recommendation:
Check context cancellation before draining:

if ctx.Err() != nil {
    cmd.Process.Kill()
    return "", ctx.Err()
}
_, _ = io.Copy(io.Discard, stdout)


if err := cmd.Wait(); err != nil {
if ctx.Err() == context.DeadlineExceeded {
Copy link

Choose a reason for hiding this comment

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

⚠️ MEDIUM: Race Condition in Timeout Detection

The code checks ctx.Err() == context.DeadlineExceeded after cmd.Wait() returns an error. This is racy: if the command fails with a non-zero exit code just before the deadline, ctx.Err() might return nil and the timeout will be hidden.

Issue:
Users may see generic command failure errors instead of clear "command timed out" messages.

Recommendation:
Check the context error before comparing:

if err := cmd.Wait(); err != nil {
    if errors.Is(ctx.Err(), context.DeadlineExceeded) {
        return "", fmt.Errorf("command timed out after %s", commandTimeout)
    }
    // ... rest of error handling
}

return "", fmt.Errorf("command timed out after %s", commandTimeout)
}
if stderrMsg := strings.TrimSpace(stderr.String()); stderrMsg != "" {
return "", fmt.Errorf("%w: %s", err, stderrMsg)
}
return "", err
}

return string(out), nil
}
111 changes: 111 additions & 0 deletions pkg/skills/expand_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package skills

import (
"context"
"os"
"path/filepath"
"runtime"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func skipOnWindows(t *testing.T) {
t.Helper()
if runtime.GOOS == "windows" {
t.Skip("skipping on windows")
}
}

func TestExpandCommands(t *testing.T) {
skipOnWindows(t)

tests := []struct {
name string
content string
want string
}{
{
name: "no patterns",
content: "# My Skill\n\nJust regular markdown content.",
want: "# My Skill\n\nJust regular markdown content.",
},
{
name: "simple echo",
content: "Hello !`echo world`!",
want: "Hello world!",
},
{
name: "multiple commands",
content: "Name: !`echo alice`, Age: !`echo 30`",
want: "Name: alice, Age: 30",
},
{
name: "multiline output",
content: "Files:\n!`printf 'a.go\nb.go\nc.go\n'`\nEnd.",
want: "Files:\na.go\nb.go\nc.go\nEnd.",
},
{
name: "empty output",
content: "Before !`true` after",
want: "Before after",
},
{
name: "pipes",
content: "Count: !`printf 'a\nb\nc\n' | wc -l | tr -d ' '`",
want: "Count: 3",
},
{
name: "preserves regular backticks",
content: "Use `echo hello` to print.\n\nCode: ```go\nfmt.Println()\n```",
want: "Use `echo hello` to print.\n\nCode: ```go\nfmt.Println()\n```",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := ExpandCommands(t.Context(), tt.content, t.TempDir())
assert.Equal(t, tt.want, result)
})
}
}

func TestExpandCommands_WorkingDirectory(t *testing.T) {
skipOnWindows(t)

tmpDir := t.TempDir()
require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "test.txt"), []byte("hello"), 0o644))

result := ExpandCommands(t.Context(), "Content: !`cat test.txt`", tmpDir)
assert.Equal(t, "Content: hello", result)
}

func TestExpandCommands_ScriptExecution(t *testing.T) {
skipOnWindows(t)

tmpDir := t.TempDir()
require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "info.sh"), []byte("#!/bin/sh\necho from-script"), 0o755))

result := ExpandCommands(t.Context(), "Output: !`./info.sh`", tmpDir)
assert.Equal(t, "Output: from-script", result)
}

func TestExpandCommands_FailedCommand(t *testing.T) {
skipOnWindows(t)

result := ExpandCommands(t.Context(), "Before !`nonexistent_command_12345` after", t.TempDir())
assert.Contains(t, result, "Before ")
assert.Contains(t, result, "[error executing `nonexistent_command_12345`:")
assert.Contains(t, result, " after")
}

func TestExpandCommands_CancelledContext(t *testing.T) {
skipOnWindows(t)

ctx, cancel := context.WithCancel(t.Context())
cancel()

result := ExpandCommands(ctx, "Result: !`echo hello`", t.TempDir())
assert.Contains(t, result, "[error executing `echo hello`:")
}
2 changes: 2 additions & 0 deletions pkg/skills/skills.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type Skill struct {
FilePath string `yaml:"-"`
BaseDir string `yaml:"-"`
Files []string `yaml:"-"`
Local bool `yaml:"-"` // true for filesystem-loaded skills, false for remote
License string `yaml:"license"`
Compatibility string `yaml:"compatibility"`
Metadata map[string]string `yaml:"metadata"`
Expand Down Expand Up @@ -308,6 +309,7 @@ func loadSkillFile(path, dirName string) (Skill, bool) {
skill.Name = cmp.Or(skill.Name, dirName)
skill.FilePath = path
skill.BaseDir = filepath.Dir(path)
skill.Local = true
Copy link

Choose a reason for hiding this comment

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

🔴 CRITICAL: No Cryptographic Verification of Local Skills

All filesystem-loaded skills are marked Local = true without cryptographic verification. The code trusts the filesystem location as the sole indicator of trust.

Security Risk:
An attacker who gains write access to skill directories (~/.codex/skills/, .agents/skills/, .claude/skills/) could plant malicious SKILL.md files with arbitrary commands. This could happen via:

  • Compromised npm packages that write to these directories
  • Malicious git repositories cloned into project directories
  • Supply chain attacks on skill distribution

Recommendation:

  1. Distinguish between global (~/.codex) and project-local (.agents) skills with different trust levels
  2. Consider requiring signature verification for skills from untrusted sources
  3. Add a --trust-local-skills flag that must be explicitly enabled
  4. Warn users when loading project-local skills that may contain executable code


return skill, true
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/teamloader/teamloader.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ func LoadWithConfig(ctx context.Context, agentSource config.Source, runConfig *c
if agentConfig.Skills.Enabled() {
loadedSkills := skills.Load(agentConfig.Skills.Sources)
if len(loadedSkills) > 0 {
agentTools = append(agentTools, builtin.NewSkillsToolset(loadedSkills))
agentTools = append(agentTools, builtin.NewSkillsToolset(loadedSkills, runConfig.WorkingDir))
Copy link

Choose a reason for hiding this comment

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

🔴 CRITICAL: User-Controlled Working Directory

The runConfig.WorkingDir is passed to NewSkillsToolset and used as the working directory for command execution. This directory is user-controlled via CLI flags or configuration.

Security Risk:
An attacker who can control both the working directory and place a malicious SKILL.md in a local skills directory could:

  • Set WorkingDir to /etc and run !\ to expose system files
  • Access sensitive credentials in arbitrary directories
  • Execute commands in security-sensitive locations

Recommendation:

  1. Validate that workingDir is within safe boundaries (e.g., user's home directory or project root)
  2. Reject absolute paths or paths containing ../ that escape expected boundaries
  3. Consider restricting command execution to the skill's own directory only

}
}

Expand Down
21 changes: 15 additions & 6 deletions pkg/tools/builtin/skills.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@ var (
// agent load skill content and supporting resources by name. It hides whether
// a skill is local or remote — the agent just sees a name and description.
type SkillsToolset struct {
skills []skills.Skill
skills []skills.Skill
workingDir string
}

func NewSkillsToolset(loadedSkills []skills.Skill) *SkillsToolset {
func NewSkillsToolset(loadedSkills []skills.Skill, workingDir string) *SkillsToolset {
return &SkillsToolset{
skills: loadedSkills,
skills: loadedSkills,
workingDir: workingDir,
}
}

Expand All @@ -49,7 +51,10 @@ func (s *SkillsToolset) findSkill(name string) *skills.Skill {
}

// ReadSkillContent returns the content of a skill's SKILL.md by name.
func (s *SkillsToolset) ReadSkillContent(name string) (string, error) {
// For local skills, it expands any !`command` patterns in the content by
// executing the commands and replacing the patterns with their stdout output.
// Command expansion is disabled for remote skills to prevent arbitrary code execution.
func (s *SkillsToolset) ReadSkillContent(ctx context.Context, name string) (string, error) {
skill := s.findSkill(name)
if skill == nil {
return "", fmt.Errorf("skill %q not found", name)
Expand All @@ -60,6 +65,10 @@ func (s *SkillsToolset) ReadSkillContent(name string) (string, error) {
return "", err
}

if skill.Local {
content = skills.ExpandCommands(ctx, content, s.workingDir)
}

return content, nil
}

Expand Down Expand Up @@ -119,8 +128,8 @@ type readSkillFileArgs struct {
Path string `json:"path" jsonschema:"The relative path to the file within the skill (e.g. references/FORMS.md)"`
}

func (s *SkillsToolset) handleReadSkill(_ context.Context, args readSkillArgs) (*tools.ToolCallResult, error) {
content, err := s.ReadSkillContent(args.Name)
func (s *SkillsToolset) handleReadSkill(ctx context.Context, args readSkillArgs) (*tools.ToolCallResult, error) {
content, err := s.ReadSkillContent(ctx, args.Name)
if err != nil {
return tools.ResultError(err.Error()), nil
}
Expand Down
Loading
Loading