From d0bd3ec3d9d6dcb6a5566d75285977982fe61ece Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Fri, 26 Jun 2026 22:31:59 -0400 Subject: [PATCH] fix: address verified gadfly P3 review (3-cloud fleet) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 3 cloud models converged on a real access-control bug; fixed it + the other genuine findings (the false-positives were dropped): Security (HIGH — all 3 models): - create_file_url skipped ValidateScope: a same-skill caller could mint a PUBLIC url for a file scoped to another user/run. Now runs ValidateScope (admin-aware), skipped only for the descendant-grant case — mirroring the read tools. Other real fixes: - ValidateScope hard-coded `false` at every call site (admin branch dead) -> pass inv.CallerIsAdmin (the executor sets it via the host AdminPolicy; still false/fail-closed when no admin). Stale "no admin flag" comment corrected. - create_file_url: ExpiresInSeconds clamped BEFORE the *time.Second multiply (huge values overflowed to a negative duration that slipped under the cap, minting already-expired tokens); swallowed json.Marshal error now returned. - RegisterMeta: build the default budget WITH the configured MaxPerRun (was NewInMemorySearchBudget(nil) -> hardcoded 10, ignoring MetaDeps.MaxPerRun). - classify: all-zero scores no longer return a false-positive top-1 winner; coerceClassifyScore uses strconv.ParseFloat (rejects trailing garbage like "50extra" that fmt.Sscanf silently accepted). - file_delete: honor the descendant grant (parent can clean up a worker's artifacts) — was the lone cross-skill-reject-outright file tool. - meta tools: input caps truncate at a UTF-8 rune boundary (truncateUTF8), not mid-rune. - think: removed the dead `var _ = fmt.Errorf` import-keeper; file_save default aligned to 16 MiB (matched RegisterStore). Co-Authored-By: Claude Opus 4.8 (1M context) --- tools/classify.go | 20 ++++++++++++++------ tools/create_file_url.go | 36 +++++++++++++++++++++++++++++++----- tools/extract_entities.go | 2 +- tools/file_delete.go | 15 ++++++++++++--- tools/file_get.go | 2 +- tools/file_get_metadata.go | 2 +- tools/file_get_text.go | 2 +- tools/file_list.go | 2 +- tools/file_save.go | 4 ++-- tools/file_search.go | 2 +- tools/kv_delete.go | 2 +- tools/kv_get.go | 2 +- tools/kv_list.go | 2 +- tools/kv_set.go | 2 +- tools/scope_validate.go | 2 +- tools/summarize.go | 4 ++-- tools/think.go | 2 -- tools/tools.go | 13 ++++++++++--- tools/truncate.go | 18 ++++++++++++++++++ 19 files changed, 100 insertions(+), 34 deletions(-) create mode 100644 tools/truncate.go diff --git a/tools/classify.go b/tools/classify.go index e2a28d4..95f1f3a 100644 --- a/tools/classify.go +++ b/tools/classify.go @@ -25,6 +25,7 @@ import ( "context" "encoding/json" "fmt" + "strconv" "strings" "gitea.stevedudenhoeffer.com/steve/executus/llmmeta" @@ -111,7 +112,7 @@ func NewClassify(helper *llmmeta.Helper, cfg ClassifyConfig, budget SearchBudget } if len(text) > classifyMaxInputBytes { - text = text[:classifyMaxInputBytes] + text = truncateUTF8(text, classifyMaxInputBytes) } // Per-run budget gate. @@ -255,10 +256,14 @@ func coerceClassifyScore(raw any) (float64, bool) { case int64: return float64(v), true case string: - s := strings.TrimSuffix(strings.TrimSpace(v), "%") - var f float64 - if _, err := fmt.Sscanf(s, "%f", &f); err == nil { - if strings.HasSuffix(strings.TrimSpace(v), "%") { + trimmed := strings.TrimSpace(v) + hasPct := strings.HasSuffix(trimmed, "%") + s := strings.TrimSuffix(trimmed, "%") + // strconv.ParseFloat (unlike fmt.Sscanf %f) rejects trailing garbage, + // so "50extra" / "0.5x" are refused instead of silently parsed as 50/0.5. + f, err := strconv.ParseFloat(strings.TrimSpace(s), 64) + if err == nil { + if hasPct { f = f / 100.0 } return f, true @@ -292,7 +297,10 @@ func selectClassifyLabels(scores map[string]float64, categories []string, multiL bestCat = c } } - if bestCat == "" { + // No category fit: an all-zero score set must not yield a false-positive + // top-1 (the first category trivially beats the -1.0 sentinel). Returning + // no label keeps "nothing matched" distinguishable from "category A won". + if bestCat == "" || bestScore <= 0 { return nil } return []string{bestCat} diff --git a/tools/create_file_url.go b/tools/create_file_url.go index bf1b1db..3762f2b 100644 --- a/tools/create_file_url.go +++ b/tools/create_file_url.go @@ -124,14 +124,37 @@ func NewCreateFileURL(minter FileTokenMinter, fileStorage FileStorage, baseURL s } return "", fmt.Errorf("create_file_url: %w", err) } - if meta.SkillID != inv.SkillID && !descendantFileGrant(ctx, fileStorage, inv, meta.SkillID) { - return "", fmt.Errorf("create_file_url: file_id %q does not belong to this skill (cross-skill refs rejected)", args.FileID) + grantedViaDescendant := false + if meta.SkillID != inv.SkillID { + if !descendantFileGrant(ctx, fileStorage, inv, meta.SkillID) { + return "", fmt.Errorf("create_file_url: file_id %q does not belong to this skill (cross-skill refs rejected)", args.FileID) + } + grantedViaDescendant = true + } + // Scope gate — this is a PUBLICATION primitive (it mints an + // unauthenticated link), so it must enforce the same per-user/per-run + // scope isolation the read tools do: a same-skill caller must not be + // able to publish a file scoped to another user/run. Skipped only for + // the descendant-grant case (the worker's file scope is the worker's + // run, not the caller's). + if !grantedViaDescendant { + if err := ValidateScope(inv, meta.Scope, inv.CallerIsAdmin); err != nil { + return "", fmt.Errorf("create_file_url: %w", err) + } } - // Resolve expiry. + // Resolve expiry. Clamp the caller's seconds BEFORE the multiply so a + // huge value can't overflow int64 nanoseconds into a negative + // duration that slips under the max-expiry cap (minting an + // already-expired token). expiry := DefaultFileURLExpiry if args.ExpiresInSeconds > 0 { - expiry = time.Duration(args.ExpiresInSeconds) * time.Second + maxSecs := int(MaxFileURLExpiry / time.Second) + secs := args.ExpiresInSeconds + if secs > maxSecs { + secs = maxSecs + } + expiry = time.Duration(secs) * time.Second } if expiry > MaxFileURLExpiry { expiry = MaxFileURLExpiry @@ -178,7 +201,10 @@ func NewCreateFileURL(minter FileTokenMinter, fileStorage FileStorage, baseURL s if maxViews != nil { res.MaxViews = *maxViews } - b, _ := json.Marshal(res) + b, err := json.Marshal(res) + if err != nil { + return "", fmt.Errorf("create_file_url: marshal: %w", err) + } return string(b), nil }, ) diff --git a/tools/extract_entities.go b/tools/extract_entities.go index 92bc07a..bfc42ea 100644 --- a/tools/extract_entities.go +++ b/tools/extract_entities.go @@ -121,7 +121,7 @@ func NewExtractEntities(helper *llmmeta.Helper, cfg ExtractEntitiesConfig, budge } if len(text) > extractEntitiesMaxInputBytes { - text = text[:extractEntitiesMaxInputBytes] + text = truncateUTF8(text, extractEntitiesMaxInputBytes) } // Per-run budget gate. diff --git a/tools/file_delete.go b/tools/file_delete.go index 3c560bc..4a7d45d 100644 --- a/tools/file_delete.go +++ b/tools/file_delete.go @@ -49,11 +49,20 @@ func NewFileDelete(storage FileStorage) tool.Tool { } return "", fmt.Errorf("file_delete: %w", err) } + // Honor the descendant grant like the read tools do, so a parent + // orchestrator can clean up a worker's artifacts (gadfly flagged the + // asymmetry: delete previously rejected cross-skill outright). + grantedViaDescendant := false if meta.SkillID != inv.SkillID { - return "", fmt.Errorf("file_delete: file does not belong to this skill") + if !descendantFileGrant(ctx, storage, inv, meta.SkillID) { + return "", fmt.Errorf("file_delete: file does not belong to this skill") + } + grantedViaDescendant = true } - if err := ValidateScope(inv, meta.Scope, false); err != nil { - return "", fmt.Errorf("file_delete: %w", err) + if !grantedViaDescendant { + if err := ValidateScope(inv, meta.Scope, inv.CallerIsAdmin); err != nil { + return "", fmt.Errorf("file_delete: %w", err) + } } if err := storage.FileDelete(ctx, args.FileID); err != nil { diff --git a/tools/file_get.go b/tools/file_get.go index dba16e2..a097cf1 100644 --- a/tools/file_get.go +++ b/tools/file_get.go @@ -82,7 +82,7 @@ func NewFileGet(storage FileStorage) tool.Tool { // row gates access (e.g. user:bob's file is unreadable by // alice). The descendant grant stands in for it — the file's // scope is the WORKER's run, never the caller's. - if err := ValidateScope(inv, meta.Scope, false); err != nil && !grantedViaDescendant { + if err := ValidateScope(inv, meta.Scope, inv.CallerIsAdmin); err != nil && !grantedViaDescendant { return "", fmt.Errorf("file_get: %w", err) } diff --git a/tools/file_get_metadata.go b/tools/file_get_metadata.go index a45cbbe..481367c 100644 --- a/tools/file_get_metadata.go +++ b/tools/file_get_metadata.go @@ -70,7 +70,7 @@ func NewFileGetMetadata(storage FileStorage) tool.Tool { grantedViaDescendant = true } if !grantedViaDescendant { - if err := ValidateScope(inv, meta.Scope, false); err != nil { + if err := ValidateScope(inv, meta.Scope, inv.CallerIsAdmin); err != nil { return "", fmt.Errorf("file_get_metadata: %w", err) } } diff --git a/tools/file_get_text.go b/tools/file_get_text.go index 00a0a1d..dc176ec 100644 --- a/tools/file_get_text.go +++ b/tools/file_get_text.go @@ -77,7 +77,7 @@ func NewFileGetText(storage FileStorage) tool.Tool { grantedViaDescendant = true } if !grantedViaDescendant { - if err := ValidateScope(inv, meta.Scope, false); err != nil { + if err := ValidateScope(inv, meta.Scope, inv.CallerIsAdmin); err != nil { return "", fmt.Errorf("file_get_text: %w", err) } } diff --git a/tools/file_list.go b/tools/file_list.go index 7cbacc0..2073420 100644 --- a/tools/file_list.go +++ b/tools/file_list.go @@ -41,7 +41,7 @@ func NewFileList(storage FileStorage) tool.Tool { if storage == nil { return "", fmt.Errorf("file_list: not configured") } - if err := ValidateScope(inv, args.Scope, false); err != nil { + if err := ValidateScope(inv, args.Scope, inv.CallerIsAdmin); err != nil { return "", fmt.Errorf("file_list: %w", err) } // root_run is a KV-only scope (v1) — see file_save's guard. diff --git a/tools/file_save.go b/tools/file_save.go index 79c7663..1ebe747 100644 --- a/tools/file_save.go +++ b/tools/file_save.go @@ -43,7 +43,7 @@ import ( "gitea.stevedudenhoeffer.com/steve/executus/tool" ) -const defaultFileMaxBytes = 10 * 1024 * 1024 // 10 MiB +const defaultFileMaxBytes = 16 * 1024 * 1024 // 10 MiB type fileSaveArgs struct { Scope string `json:"scope" description:"Storage scope: 'skill' (shared across all callers of this skill), 'user:' (per-caller), or 'run:' (this run's scratchpad)."` @@ -84,7 +84,7 @@ func NewFileSave(storage FileStorage, quota QuotaProvider, maxFileBytes int) too if storage == nil { return "", fmt.Errorf("file_save: not configured") } - if err := ValidateScope(inv, args.Scope, false); err != nil { + if err := ValidateScope(inv, args.Scope, inv.CallerIsAdmin); err != nil { return "", fmt.Errorf("file_save: %w", err) } // root_run is a KV-only scope (v1): file storage partitions diff --git a/tools/file_search.go b/tools/file_search.go index 4db75c9..b8b586a 100644 --- a/tools/file_search.go +++ b/tools/file_search.go @@ -98,7 +98,7 @@ func NewFileSearch(searcher FileSearcher) tool.Tool { } scope := args.Scope if scope != "" { - if err := ValidateScope(inv, scope, false); err != nil { + if err := ValidateScope(inv, scope, inv.CallerIsAdmin); err != nil { return "", fmt.Errorf("file_search: %w", err) } } diff --git a/tools/kv_delete.go b/tools/kv_delete.go index b00a429..d44b00a 100644 --- a/tools/kv_delete.go +++ b/tools/kv_delete.go @@ -33,7 +33,7 @@ func NewKVDelete(storage KVStorage) tool.Tool { if storage == nil { return "", fmt.Errorf("kv_delete: not configured") } - if err := ValidateScope(inv, args.Scope, false); err != nil { + if err := ValidateScope(inv, args.Scope, inv.CallerIsAdmin); err != nil { return "", fmt.Errorf("kv_delete: %w", err) } if args.Key == "" { diff --git a/tools/kv_get.go b/tools/kv_get.go index 59b303d..8e1fcac 100644 --- a/tools/kv_get.go +++ b/tools/kv_get.go @@ -43,7 +43,7 @@ func NewKVGet(storage KVStorage) tool.Tool { if storage == nil { return "", fmt.Errorf("kv_get: not configured") } - if err := ValidateScope(inv, args.Scope, false); err != nil { + if err := ValidateScope(inv, args.Scope, inv.CallerIsAdmin); err != nil { return "", fmt.Errorf("kv_get: %w", err) } if args.Key == "" { diff --git a/tools/kv_list.go b/tools/kv_list.go index 6436b6c..be82d71 100644 --- a/tools/kv_list.go +++ b/tools/kv_list.go @@ -49,7 +49,7 @@ func NewKVList(storage KVStorage) tool.Tool { if storage == nil { return "", fmt.Errorf("kv_list: not configured") } - if err := ValidateScope(inv, args.Scope, false); err != nil { + if err := ValidateScope(inv, args.Scope, inv.CallerIsAdmin); err != nil { return "", fmt.Errorf("kv_list: %w", err) } diff --git a/tools/kv_set.go b/tools/kv_set.go index f9cfc17..43754b7 100644 --- a/tools/kv_set.go +++ b/tools/kv_set.go @@ -59,7 +59,7 @@ func NewKVSet(storage KVStorage, quota QuotaProvider, maxValueBytes int) tool.To if storage == nil { return "", fmt.Errorf("kv_set: not configured") } - if err := ValidateScope(inv, args.Scope, false); err != nil { + if err := ValidateScope(inv, args.Scope, inv.CallerIsAdmin); err != nil { return "", fmt.Errorf("kv_set: %w", err) } if args.Key == "" { diff --git a/tools/scope_validate.go b/tools/scope_validate.go index e0f27e8..02779b1 100644 --- a/tools/scope_validate.go +++ b/tools/scope_validate.go @@ -18,7 +18,7 @@ // vocabulary evolves and one place for the test matrix. // // Why the isAdmin parameter: the v4 Invocation does NOT carry an -// admin flag — production tools always pass isAdmin=false. The +// admin flag — the executor sets inv.CallerIsAdmin via the host AdminPolicy; tools pass it through // parameter exists for tests (which exercise the admin paths) and for a // future Invocation extension that adds an admin signal without // breaking this helper's signature. diff --git a/tools/summarize.go b/tools/summarize.go index 99f6a5b..dc8783c 100644 --- a/tools/summarize.go +++ b/tools/summarize.go @@ -198,7 +198,7 @@ func loadSummarizeInput(ctx context.Context, inv tool.Invocation, args summarize if meta.SkillID != inv.SkillID { return "", false, fmt.Errorf("summarize: file does not belong to this skill") } - if err := ValidateScope(inv, meta.Scope, false); err != nil { + if err := ValidateScope(inv, meta.Scope, inv.CallerIsAdmin); err != nil { return "", false, fmt.Errorf("summarize: %w", err) } return capInput(string(content)) @@ -210,7 +210,7 @@ func capInput(text string) (string, bool, error) { if len(text) <= summarizeMaxInputBytes { return text, false, nil } - return text[:summarizeMaxInputBytes], true, nil + return truncateUTF8(text, summarizeMaxInputBytes), true, nil } // buildSummarizePrompt composes the user message handed to the LLM. diff --git a/tools/think.go b/tools/think.go index d0dd2c0..7af45c7 100644 --- a/tools/think.go +++ b/tools/think.go @@ -16,7 +16,6 @@ package tools import ( "context" - "fmt" "strings" "gitea.stevedudenhoeffer.com/steve/executus/tool" @@ -69,4 +68,3 @@ func NewThink() tool.Tool { // switch back to json.Marshal — but until then, the literal is the // idiom that matches the tool's "do nothing" intent. var _ = thinkResponse{} // declared so vet doesn't flag the unused struct -var _ = fmt.Errorf diff --git a/tools/tools.go b/tools/tools.go index e9f15be..f3a06e7 100644 --- a/tools/tools.go +++ b/tools/tools.go @@ -55,15 +55,22 @@ func RegisterMeta(reg tool.Registry, d MetaDeps) error { if d.Helper == nil { return errors.New("tools: MetaDeps.Helper is required for the meta tools") } - if d.Budget == nil { - d.Budget = NewInMemorySearchBudget(nil) - } if d.MaxPerRun <= 0 { d.MaxPerRun = 10 } if d.MaxWords <= 0 { d.MaxWords = 200 } + if d.Budget == nil { + // Build the default budget WITH the configured per-run cap so + // MetaDeps.MaxPerRun is honored — an empty caps map would fall back to + // the budget's hardcoded default and silently ignore MaxPerRun. + d.Budget = NewInMemorySearchBudget(map[string]int{ + "classify": d.MaxPerRun, + "extract_entities": d.MaxPerRun, + "summarize": d.MaxPerRun, + }) + } cfg := fixedMetaConfig{maxPerRun: d.MaxPerRun, maxWords: d.MaxWords} return registerAll(reg, NewClassify(d.Helper, cfg, d.Budget), diff --git a/tools/truncate.go b/tools/truncate.go new file mode 100644 index 0000000..dd2429b --- /dev/null +++ b/tools/truncate.go @@ -0,0 +1,18 @@ +package tools + +import "unicode/utf8" + +// truncateUTF8 returns s truncated to at most maxBytes, backing off to the last +// complete UTF-8 rune boundary so a multibyte rune (CJK, emoji, …) is never +// split — a byte-boundary cut would hand the LLM invalid UTF-8 / replacement +// chars. Used by the meta tools' input caps. +func truncateUTF8(s string, maxBytes int) string { + if len(s) <= maxBytes { + return s + } + s = s[:maxBytes] + for len(s) > 0 && !utf8.ValidString(s) { + s = s[:len(s)-1] + } + return s +}