From 6d154a3b3097f52f1125996c63e13084220a811f Mon Sep 17 00:00:00 2001 From: Fred Date: Thu, 29 Oct 2026 13:15:35 -0700 Subject: [PATCH] Revert "feat: restrict task invocation by caller (#220)" This reverts commit c772470d5b0cf505c7805d25b89200b4b1a08df1. --- SPECIFICATION.md | 49 ------ docs/how-to/create-tasks.md | 39 ----- docs/reference/cli.md | 37 +--- main.go | 20 +-- pkg/codingcontext/authorization.go | 47 ------ pkg/codingcontext/authorization_test.go | 158 ------------------ pkg/codingcontext/context.go | 16 +- pkg/codingcontext/markdown/frontmatter.go | 8 - .../markdown/frontmatter_task_test.go | 34 +--- pkg/codingcontext/options.go | 17 +- 10 files changed, 11 insertions(+), 414 deletions(-) delete mode 100644 pkg/codingcontext/authorization.go delete mode 100644 pkg/codingcontext/authorization_test.go diff --git a/SPECIFICATION.md b/SPECIFICATION.md index 07fe42f0..2014a286 100644 --- a/SPECIFICATION.md +++ b/SPECIFICATION.md @@ -581,43 +581,6 @@ expand: false --- ``` -#### 5.2.13 `allowed_surfaces` (optional) -- **Type:** Array of strings -- **Purpose:** Restrict task execution to matching invocation surfaces -- **Default:** Unrestricted when omitted or empty -- **Matching:** Case-insensitive OR logic across entries - -```yaml ---- -allowed_surfaces: - - jira - - github ---- -``` - -The caller must configure its invocation surface with `WithSurface` or the -`-surface` CLI flag. A restricted task rejects invocations with a missing or -non-matching surface before task content is assembled. - -#### 5.2.14 `allowed_requesters` (optional) -- **Type:** Array of strings -- **Purpose:** Restrict task execution to matching requester usernames or email addresses -- **Default:** Unrestricted when omitted or empty -- **Matching:** Case-insensitive OR logic across entries - -```yaml ---- -allowed_requesters: - - alice - - alice@example.com ---- -``` - -The caller must configure a requester with `WithCaller` or the `-caller` and -`-caller-email` CLI flags. A username or email match permits the requester. -When both `allowed_surfaces` and `allowed_requesters` are present, both -restrictions must match. - ### 5.3 Standard Rule Fields #### 5.3.1 `id` (optional) @@ -1591,16 +1554,6 @@ Potential future additions while maintaining backward compatibility: "description": "Enable/disable parameter expansion", "default": true }, - "allowed_surfaces": { - "type": "array", - "items": {"type": "string"}, - "description": "Invocation surfaces permitted to run a task" - }, - "allowed_requesters": { - "type": "array", - "items": {"type": "string"}, - "description": "Requester usernames or email addresses permitted to run a task" - }, "selectors": { "type": "object", "description": "Auto-filter rules", @@ -1681,8 +1634,6 @@ Potential future additions while maintaining backward compatibility: | `timeout` | string | No | Timeout duration | | `selectors` | object | No | Auto-filter rules | | `expand` | boolean | No | Parameter expansion control | -| `allowed_surfaces` | array | No | Permitted invocation surfaces | -| `allowed_requesters` | array | No | Permitted requester usernames or email addresses | ### Rule Fields diff --git a/docs/how-to/create-tasks.md b/docs/how-to/create-tasks.md index ddc8f10d..0c580830 100644 --- a/docs/how-to/create-tasks.md +++ b/docs/how-to/create-tasks.md @@ -169,45 +169,6 @@ coding-context -s priority=high implement-feature **Note:** Language values should be lowercase (e.g., `go`, `python`, `javascript`). -## Restricting Task Invocation - -Use `allowed_surfaces` and `allowed_requesters` to limit who may run a task and -where the invocation may originate: - -```markdown ---- -allowed_surfaces: - - jira - - github -allowed_requesters: - - alice - - alice@example.com ---- -# Deploy Service - -Deploy the requested service update. -``` - -Values within each list use case-insensitive OR logic. When both fields are -present, the invocation must match both restrictions. Omitting a field or -setting it to an empty list leaves that dimension unrestricted. - -Library callers provide trusted invocation data with `WithSurface` and -`WithCaller`. Direct CLI callers use `-surface`, `-caller`, and optionally -`-caller-email`: - -```bash -coding-context \ - -surface jira \ - -caller alice \ - -caller-email alice@example.com \ - deploy-service -``` - -Restricted tasks reject missing or non-matching invocation data before task -content is assembled. Linting validates the task file without enforcing -invocation restrictions. - ## Task Frontmatter Task frontmatter is used for filtering and metadata purposes but is **NOT included in the output**. The frontmatter controls behavior (like auto-filtering rules via the `selectors` field) but only the task content below the frontmatter delimiters is included in the assembled context. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 688f1998..18ebfd14 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -184,7 +184,7 @@ coding-context \ ### `-a ` -**Type:** String +**Type:** String **Default:** (empty) Specify the target agent being used. This is currently used for: @@ -216,41 +216,6 @@ coding-context -a copilot -w implement-feature **See also:** [Targeting a Specific Agent](../../README.md#targeting-a-specific-agent) in README -### `-surface ` - -**Type:** String -**Default:** (empty) - -Set the invocation surface used to authorize a task that declares -`allowed_surfaces`. Matching is case-insensitive. A restricted task rejects an -empty or non-matching surface. - -### `-caller ` - -**Type:** String -**Default:** (empty) - -Set the requester username used to authorize a task that declares -`allowed_requesters`. - -### `-caller-email ` - -**Type:** String -**Default:** (empty) - -Set the requester email address used to authorize a task that declares -`allowed_requesters`. Either the configured username or email may match an -allowed requester. - -**Example:** -```bash -coding-context \ - -surface jira \ - -caller alice \ - -caller-email alice@example.com \ - deploy-service -``` - ### `-r` **Type:** Boolean flag diff --git a/main.go b/main.go index 1ff5f374..8c214352 100644 --- a/main.go +++ b/main.go @@ -18,11 +18,11 @@ import ( ) var ( - errInvalidUsage = errors.New("invalid usage: expected one task name argument and optional user-prompt") - errWriteRulesNoAgent = errors.New("-w flag requires an agent to be specified (via task 'agent' field or -a flag)") - errNoUserRulePath = errors.New("no user rule path available for agent") - errRulesPathEscapesHome = errors.New("rules path escapes home directory") - errAgentFlagsMutExcl = errors.New("-a and -A flags are mutually exclusive") + errInvalidUsage = errors.New("invalid usage: expected one task name argument and optional user-prompt") + errWriteRulesNoAgent = errors.New("-w flag requires an agent to be specified (via task 'agent' field or -a flag)") + errNoUserRulePath = errors.New("no user rule path available for agent") + errRulesPathEscapesHome = errors.New("rules path escapes home directory") + errAgentFlagsMutExcl = errors.New("-a and -A flags are mutually exclusive") ) type cliConfig struct { @@ -37,8 +37,6 @@ type cliConfig struct { searchPaths []string lenientSearchPaths []string manifestURL string - surface string - caller codingcontext.CallerIdentity taskName string userPrompt string } @@ -83,8 +81,6 @@ func run(ctx context.Context, logger *slog.Logger) error { codingcontext.WithUserPrompt(cfg.userPrompt), codingcontext.WithAgent(cfg.agent), codingcontext.WithLenientAgent(cfg.lenientAgent), - codingcontext.WithSurface(cfg.surface), - codingcontext.WithCaller(cfg.caller), ) result, err := cc.Run(ctx, cfg.taskName) @@ -160,12 +156,6 @@ func parseFlags(logger *slog.Logger) (*cliConfig, error) { }) flag.StringVar(&cfg.manifestURL, "m", "", "Go Getter URL to a manifest file containing search paths (one per line). Every line is included as-is.") - flag.StringVar(&cfg.surface, "surface", "", - "Invocation surface used to authorize tasks that declare allowed_surfaces.") - flag.StringVar(&cfg.caller.Username, "caller", "", - "Requester username used to authorize tasks that declare allowed_requesters.") - flag.StringVar(&cfg.caller.Email, "caller-email", "", - "Requester email used to authorize tasks that declare allowed_requesters.") setupUsage(logger) flag.Parse() diff --git a/pkg/codingcontext/authorization.go b/pkg/codingcontext/authorization.go deleted file mode 100644 index 35939be5..00000000 --- a/pkg/codingcontext/authorization.go +++ /dev/null @@ -1,47 +0,0 @@ -package codingcontext - -import ( - "fmt" - "strings" - - "github.com/kitproj/coding-context-cli/pkg/codingcontext/markdown" -) - -// CallerIdentity identifies the requester invoking a task. -type CallerIdentity struct { - Username string - Email string -} - -func (cc *Context) authorizeTaskInvocation(frontMatter markdown.TaskFrontMatter) error { - if !isAllowed(frontMatter.AllowedSurfaces, cc.surface) { - return fmt.Errorf("%w: surface is not permitted", ErrCallerNotAllowed) - } - - if !isAllowed(frontMatter.AllowedRequesters, cc.caller.Username, cc.caller.Email) { - return fmt.Errorf("%w: requester identity is not permitted", ErrCallerNotAllowed) - } - - return nil -} - -func isAllowed(allowlist []string, values ...string) bool { - if len(allowlist) == 0 { - return true - } - - for _, value := range values { - value = strings.TrimSpace(value) - if value == "" { - continue - } - - for _, allowed := range allowlist { - if strings.EqualFold(value, strings.TrimSpace(allowed)) { - return true - } - } - } - - return false -} diff --git a/pkg/codingcontext/authorization_test.go b/pkg/codingcontext/authorization_test.go deleted file mode 100644 index 8e4b28ea..00000000 --- a/pkg/codingcontext/authorization_test.go +++ /dev/null @@ -1,158 +0,0 @@ -package codingcontext - -import ( - "context" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestRun_TaskInvocationAuthorization(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - frontmatter string - opts []Option - wantErr bool - }{ - { - name: "unrestricted task", - }, - { - name: "empty allowlists leave task unrestricted", - frontmatter: `allowed_surfaces: [] -allowed_requesters: []`, - }, - { - name: "surface matches case insensitively", - frontmatter: "allowed_surfaces: [jira, github]", - opts: []Option{WithSurface(" JIRA ")}, - }, - { - name: "requester username matches", - frontmatter: "allowed_requesters: [alice]", - opts: []Option{WithCaller(CallerIdentity{ - Username: "ALICE", - Email: "other@example.com", - })}, - }, - { - name: "requester email matches", - frontmatter: "allowed_requesters: [alice@example.com]", - opts: []Option{WithCaller(CallerIdentity{ - Username: "other", - Email: "Alice@Example.com", - })}, - }, - { - name: "surface and requester both match", - frontmatter: `allowed_surfaces: [github] -allowed_requesters: [alice, alice@example.com]`, - opts: []Option{ - WithSurface("github"), - WithCaller(CallerIdentity{Username: "alice"}), - }, - }, - { - name: "surface mismatch", - frontmatter: "allowed_surfaces: [jira]", - opts: []Option{WithSurface("github")}, - wantErr: true, - }, - { - name: "surface missing", - frontmatter: "allowed_surfaces: [jira]", - wantErr: true, - }, - { - name: "requester mismatch", - frontmatter: "allowed_requesters: [alice]", - opts: []Option{WithCaller(CallerIdentity{Username: "bob", Email: "bob@example.com"})}, - wantErr: true, - }, - { - name: "requester missing", - frontmatter: "allowed_requesters: [alice]", - wantErr: true, - }, - { - name: "surface matches but requester does not", - frontmatter: `allowed_surfaces: [jira] -allowed_requesters: [alice]`, - opts: []Option{ - WithSurface("jira"), - WithCaller(CallerIdentity{Username: "bob"}), - }, - wantErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - dir := t.TempDir() - createTask(t, dir, "restricted", tt.frontmatter, "Authorized task.") - - opts := append([]Option{WithSearchPaths(dir)}, tt.opts...) - result, err := New(opts...).Run(context.Background(), "restricted") - - if tt.wantErr { - require.ErrorIs(t, err, ErrCallerNotAllowed) - require.Nil(t, result) - - return - } - - require.NoError(t, err) - require.Contains(t, result.Prompt, "Authorized task.") - }) - } -} - -func TestRun_AuthorizationPrecedesTaskRendering(t *testing.T) { - t.Parallel() - - dir := t.TempDir() - createTask(t, dir, "restricted", "allowed_surfaces: [jira]", "/missing-command") - - result, err := New( - WithSearchPaths(dir), - WithSurface("github"), - ).Run(context.Background(), "restricted") - - require.ErrorIs(t, err, ErrCallerNotAllowed) - require.Nil(t, result) - require.NotErrorIs(t, err, ErrCommandNotFound) -} - -func TestRun_LenientSearchPathDoesNotSuppressAuthorizationDenial(t *testing.T) { - t.Parallel() - - dir := t.TempDir() - createTask(t, dir, "restricted", "allowed_surfaces: [jira]", "Restricted task.") - - result, err := New( - WithLenientSearchPaths(dir), - WithSurface("github"), - ).Run(context.Background(), "restricted") - - require.ErrorIs(t, err, ErrCallerNotAllowed) - require.Nil(t, result) -} - -func TestLint_RestrictedTaskDoesNotRequireInvocationIdentity(t *testing.T) { - t.Parallel() - - dir := t.TempDir() - createTask(t, dir, "restricted", `allowed_surfaces: [jira] -allowed_requesters: [alice]`, "Lint this task.") - - result, err := New(WithSearchPaths(dir)).Lint(context.Background(), "restricted") - - require.NoError(t, err) - require.NotNil(t, result) - require.Equal(t, []string{"jira"}, result.Task.FrontMatter.AllowedSurfaces) - require.Equal(t, []string{"alice"}, result.Task.FrontMatter.AllowedRequesters) -} diff --git a/pkg/codingcontext/context.go b/pkg/codingcontext/context.go index 545a7f46..fddfbca5 100644 --- a/pkg/codingcontext/context.go +++ b/pkg/codingcontext/context.go @@ -40,8 +40,6 @@ var ( // ErrMultipleAgents is returned when more than one agent option (WithAgent, WithLenientAgent) is used. // These options are mutually exclusive; only one agent may be set. ErrMultipleAgents = errors.New("only one agent option (WithAgent or WithLenientAgent) may be used") - // ErrCallerNotAllowed is returned when a task does not permit the configured invocation surface or caller. - ErrCallerNotAllowed = errors.New("task invocation not allowed") // ErrInvalidTaskNameNamespace is returned when the task name has an empty namespace. ErrInvalidTaskNameNamespace = errors.New("namespace must not be empty") @@ -87,8 +85,6 @@ type Context struct { agentSetCount int // Incremented by WithAgent and WithLenientAgent; >1 means conflict namespace string // Active namespace derived from task name (e.g. "myteam" from "myteam/fix-bug") userPrompt string // User-provided prompt to append to task - surface string - caller CallerIdentity lintMode bool lintCollector *lintCollector } @@ -204,7 +200,7 @@ func (cc *Context) Run(ctx context.Context, taskName string) (*Result, error) { // Get the task by name if err := cc.findTask(taskName); err != nil { - return nil, fmt.Errorf("find task %q: %w", taskName, err) + return nil, fmt.Errorf("task not found: %w", err) } // Log parameters and selectors after task is found @@ -261,13 +257,13 @@ func (cc *Context) visitMarkdownFiles(searchDirFn func(path string) []string, vi for _, dir := range searchDirs { if err := cc.visitMarkdownInDir(dir.path, visitor); err != nil { - if dir.lenient && !errors.Is(err, ErrCallerNotAllowed) { + if dir.lenient { cc.logger.Warn("skipping directory", "path", dir.path, "error", err) continue } - return fmt.Errorf("process markdown search path %q: %w", dir.path, err) + return err } } @@ -407,12 +403,6 @@ func (cc *Context) loadTask(path, taskName string) error { frontMatter.Name = nameFromPath(path) } - if !cc.lintMode { - if err := cc.authorizeTaskInvocation(frontMatter); err != nil { - return fmt.Errorf("authorize task %q: %w", taskName, err) - } - } - // Extract selector labels from task frontmatter and add them to cc.includes. // This combines CLI selectors (from -s flag) with task selectors using OR logic: // rules match if their frontmatter value matches ANY selector value for a given key. diff --git a/pkg/codingcontext/markdown/frontmatter.go b/pkg/codingcontext/markdown/frontmatter.go index 376b0b01..6dfe3bbc 100644 --- a/pkg/codingcontext/markdown/frontmatter.go +++ b/pkg/codingcontext/markdown/frontmatter.go @@ -72,14 +72,6 @@ type TaskFrontMatter struct { // any active selector are included by default. Defaults to true (current behaviour). // Set to false to require an explicit selector match (strict/opt-in mode). IncludeUnmatched *bool `json:"include_unmatched,omitempty" yaml:"include_unmatched,omitempty"` - - // AllowedSurfaces restricts which invocation surfaces may run the task. - // An empty list leaves the task unrestricted by surface. - AllowedSurfaces []string `json:"allowed_surfaces,omitempty" yaml:"allowed_surfaces,omitempty"` - - // AllowedRequesters restricts which requester usernames or email addresses may run the task. - // An empty list leaves the task unrestricted by requester. - AllowedRequesters []string `json:"allowed_requesters,omitempty" yaml:"allowed_requesters,omitempty"` } // populateContent unmarshals raw JSON into the inline Content map. diff --git a/pkg/codingcontext/markdown/frontmatter_task_test.go b/pkg/codingcontext/markdown/frontmatter_task_test.go index 075cdaca..353f1c4e 100644 --- a/pkg/codingcontext/markdown/frontmatter_task_test.go +++ b/pkg/codingcontext/markdown/frontmatter_task_test.go @@ -2,7 +2,6 @@ package markdown import ( "encoding/json" - "slices" "testing" yaml "github.com/goccy/go-yaml" @@ -49,21 +48,12 @@ func TestTaskFrontMatter_Marshal(t *testing.T) { Model: "gpt-4", SingleShot: true, Timeout: "10m", - AllowedSurfaces: []string{ - "jira", - "github", - }, - AllowedRequesters: []string{ - "alice", - "alice@example.com", - }, Selectors: map[string]any{ "stage": "implementation", }, }, want: "name: Full Task\ndescription: A task with all fields\ntask_name: full-task\nagent: cursor\n" + - "languages:\n- go\nmodel: gpt-4\nsingle_shot: true\ntimeout: 10m\nselectors:\n stage: implementation\n" + - "allowed_surfaces:\n- jira\n- github\nallowed_requesters:\n- alice\n- alice@example.com\n", + "languages:\n- go\nmodel: gpt-4\nsingle_shot: true\ntimeout: 10m\nselectors:\n stage: implementation\n", }, { name: "task with multiple languages", @@ -182,12 +172,6 @@ single_shot: true timeout: 10m selectors: stage: implementation -allowed_surfaces: - - jira - - github -allowed_requesters: - - alice - - alice@example.com `, want: TaskFrontMatter{ BaseFrontMatter: BaseFrontMatter{ @@ -203,14 +187,6 @@ allowed_requesters: Model: "gpt-4", SingleShot: true, Timeout: "10m", - AllowedSurfaces: []string{ - "jira", - "github", - }, - AllowedRequesters: []string{ - "alice", - "alice@example.com", - }, Selectors: map[string]any{ "stage": "implementation", }, @@ -277,14 +253,6 @@ func assertTaskFrontMatter(t *testing.T, got, want TaskFrontMatter, err error, w t.Errorf("Timeout = %q, want %q", got.Timeout, want.Timeout) } - if !slices.Equal(got.AllowedSurfaces, want.AllowedSurfaces) { - t.Errorf("AllowedSurfaces = %v, want %v", got.AllowedSurfaces, want.AllowedSurfaces) - } - - if !slices.Equal(got.AllowedRequesters, want.AllowedRequesters) { - t.Errorf("AllowedRequesters = %v, want %v", got.AllowedRequesters, want.AllowedRequesters) - } - switch { case got.IncludeUnmatched == nil && want.IncludeUnmatched == nil: // both unset — ok diff --git a/pkg/codingcontext/options.go b/pkg/codingcontext/options.go index 36eda277..c7e5187c 100644 --- a/pkg/codingcontext/options.go +++ b/pkg/codingcontext/options.go @@ -124,24 +124,9 @@ func WithUserPrompt(userPrompt string) Option { } } -// WithSurface sets the surface from which the task invocation originated. -func WithSurface(surface string) Option { - return func(c *Context) { - c.surface = surface - } -} - -// WithCaller sets the identity of the requester invoking the task. -func WithCaller(caller CallerIdentity) Option { - return func(c *Context) { - c.caller = caller - } -} - // WithLint enables lint mode: skips bootstrap script execution and shell command // expansion (!`cmd`). File access is tracked and non-fatal structural errors are -// collected in LintResult. Caller allowlists are parsed but not enforced. -// Use Lint() instead of Run() to retrieve results. +// collected in LintResult. Use Lint() instead of Run() to retrieve results. func WithLint(lint bool) Option { return func(c *Context) { c.lintMode = lint