The repo already validates provider base URLs during provider CRUD, but
TTS test-connection built ephemeral providers from user-supplied api_base
without reusing that protection. This change applies the same validation
at test time and adds regression coverage for blocked loopback targets.
Constraint: Must preserve current local-provider SSRF policy semantics from provider validation
Rejected: Restrict test-connection to admins only | larger behavior change than needed for the bug
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Keep TTS test-connection URL validation aligned with validateProviderURL behavior for provider CRUD
Tested: go test ./internal/http -run 'TestTestConnection_(MissingProvider|UnsupportedProvider|MissingAPIKey|BelowOperator|BlockedAPIBase)$' -count=1 -v
Tested: go build ./...
Tested: go build -tags sqliteonly ./...
Not-tested: Full repo race test suite
Co-authored-by: viettranx <viettranx@gmail.com>
The new per-tenant TTS config surface diverged from the established TTS
schema after the audio refactor, dropping UI alias fields and root-level
settings during round-trip. It also returned ok=true even when non-secret
system config writes failed. This patch accepts backward-compatible field
aliases, round-trips timeout and Edge settings, reuses tenant timeout/rate
when building ephemeral providers, and returns errors on failed saves.
Constraint: Must preserve the existing TTS config schema expected by dashboard flows and legacy config docs
Rejected: Fix only the UI payload shape | backend would still break legacy aliases and direct API callers
Confidence: high
Scope-risk: moderate
Reversibility: clean
Directive: When extending per-tenant config handlers, keep request/response structs aligned with the established config schema and do not ignore store write errors
Related: #935
Related: #936
Tested: go test ./internal/http -run 'Test(TTSConfig|TestConnection_)' -count=1 -v
Tested: go build ./...
Tested: go build -tags sqliteonly ./...
Not-tested: Full repo race test suite
* fix(tests): resolve integration test compile errors
- Remove duplicate allowLoopbackForTest in hooks_pipeline_test.go (canonical version lives in v3_test_helper.go)
- Remove unused fakeClient assignment in mcp_grant_revoke_test.go; fakeMCPClient type retained for future use
* ci: bump unit test timeout from 90s to 5m
The internal/hooks/handlers package binary under `-race -coverpkg=./...`
now runs up against the 90s cap because HTTPHandler retry uses a real
`time.After(1 * time.Second)` backoff across three HTTP test cases, and
goja-based memory-bomb sandbox tests have large allocations that run
inline before the sandbox deadline kicks in.
Recent main CI has been red with this timeout firing on different slow
tests each run (TestHTTP_5xxRetriesOnce, TestCorpus_MemoryBombString).
Bumping to 5m keeps the deadlock safety net (still half the 10-minute
Go default) while giving slow-but-non-deadlocked packages room.
Followup: make HTTPHandler backoff configurable so tests can override
with ms-scale delays and the 90s cap can come back.
Also update `make test` in Makefile to match.
* test(mcp): skip RevokeUserGrant test pending Phase 02 implementation
Commit 8b8da3a3 added this test alongside the grant-checker, but the
user-grant revocation semantics were never implemented: ListAccessible's
SQL treats an absent mcp_user_grants row as "allowed by default"
(WHERE mug.id IS NULL OR mug.enabled = true), so RevokeFromUser's DELETE
leaves the server accessible.
The test never actually ran in CI until the prior compile fix in this PR
unblocked the integration test binary. It's safe to skip — the agent-grant
counterpart test still exercises the grant-recheck code path. Re-enable
once user-grant-required semantics land.
Add useLayoutEffect hook to sync textarea height whenever value changes
or component mounts. Prevents browser's default rows=1 height from
leaving a gap above the icons when textarea content is externally
reset (e.g., after send). Adjust icon padding for better vertical
alignment with multi-line textarea.
Add missing ResetStuckSummoning method stub to mock AgentStore
implementations in test files. Follows from recent agent recovery
feature (d7342fbd) which added this method to the AgentStore interface
but missed updating these 4 test mock implementations.
Add pipBuildFailHint helper that suggests root causes when pip wheel build fails.
Detects missing system deps (pg_config for psycopg, libssl for cryptography, etc).
Hints suggest apt/brew/yum package names or pip alias alternatives (psycopg2→psycopg2-binary).
Wired to dep_installer stderr/stdlog post-build-failure for visibility.
Add manifest-origin dependency declarations in SKILL.md frontmatter.
New fields: deps (pip/npm/system) and exclude_deps (override search results).
Validates dependencies against per-category regex allowlists to prevent injection.
Manifest overrides can extend or reduce auto-discovered deps.
Backward compatible: skills without deps:/exclude_deps: behave identically.
Includes regression guard (bundled_smoke_test.go) for 5 bundled skills.
Add pip package alias resolutions for commonly-misnamed or distribution-variant packages.
psycopg2 shipped as sdist-only on PyPI (requires pg_config); aliases to psycopg2-binary.
Other entries handle distribution variants (MySQLdb→mysqlclient, Crypto→pycryptodome, etc).
Follow-up to #942. The HTTP onboarding path (registerInMemory) still diverged
from the startup path in two ways that #942 deliberately deferred:
1. Anthropic: startup wires WithAnthropicRegistry(modelReg) (cmd/gateway_providers.go:349),
HTTP skipped it — forward-compat model resolution + token counting used static
defaults for HTTP-onboarded providers.
2. Claude CLI: startup validates filepath.IsAbs + exec.LookPath before registering
(cmd/gateway_providers.go:290-297), HTTP skipped both — DB could be poisoned
with relative paths (resolves against CWD) or non-existent binaries that would
pass verify but crash on first spawn.
Changes:
- Add ProvidersHandler.modelReg field + SetModelRegistry setter
- Thread modelReg through wireHTTP in cmd/gateway_http_handlers.go
- Anthropic branch: append WithAnthropicRegistry when modelReg non-nil
- Claude CLI branch: mirror startup validation (reject non-absolute, skip if LookPath fails)
Tests added:
- TestProvidersHandlerRegisterInMemorySkipsClaudeCLIWhenBinaryMissing — asserts
provider NOT registered when binary path cannot be resolved
- TestProvidersHandlerRegisterInMemoryAnthropicUsesModelRegistry — reflection-probe
verifies registry wiring (no public getter on AnthropicProvider)
- Updated existing CLI test to use a real temp binary so exec.LookPath succeeds
regardless of whether \`claude\` is installed in the runner's PATH
White text on bg-primary (oklch 0.62) scored ~3:1 and failed WCAG AA.
Switch the user badge to an outline with a soft primary tint so the
label stays readable in light mode while still visually distinct from
the group (secondary) and topic (outline) types.
- user_search: return tenant_user UUID in new "uuid" field so callers
forwarding a tenant_user foreign key receive the primary key instead
of the user_id string (root cause of HTTP 400 "invalid JSON" on
/v1/contacts/merge when linking to an existing tenant user)
- use-user-picker: add opt-in valueMode prop ("user_id" default | "uuid")
so only merge-contacts-dialog commits UUID; other pickers (tenant
add-user, MCP/CLI credentials) keep returning user_id string
- pg.MigrateUserDataOnMerge: split tClauseIns/tClauseDel with correct
placeholder offsets (N+2 for INSERT/UPDATE, N+1 for DELETE) — reusing
one clause caused "no parameter \$N+2" errors that silently skipped
every cleanup DELETE, leaving orphan per-user rows after a merge
Add Cancel button in SummoningModal after 60s threshold. Call POST
/v1/agents/{id}/cancel-summon via getApiClient(). Update agent status
to summon_failed. Add i18n strings for en/vi/zh in desktop locale.
Add Cancel button in SummoningModal after 60s threshold. Call POST
/v1/agents/{id}/cancel-summon via useHttp hook. Update agent status
to summon_failed. Add i18n strings for en/vi/zh in agents locale.
Add ResetStuckSummoning store method (PG + SQLite) + startup sweep to
flip ghost rows to summon_failed on boot. Drop 409 guards on resummon
and regenerate to allow retry. Add POST /v1/agents/{id}/cancel-summon
endpoint + audit event agent.summon_cancelled + i18n (en/vi/zh).
Closes credential-scope bypass where an agent without a `secure_cli_agent_grants` row could still invoke a registered binary via shell fallthrough and pick up inherited env (`$GH_TOKEN`) or on-disk OAuth state. Registration is now an authorization boundary, not only a credential-injection hint.
- Add `SecureCLIStore.IsRegisteredBinary` on PG + SQLite backends — case-insensitive, tenant-scoped.
- New gate branch in `internal/tools/shell.go` (after normalization, before exec approval) with shell-wrapper unwrap depth 3 (sh/bash/zsh/dash -c, env K=V, nohup, stdbuf, timeout), 2s DB-lookup timeout, fail-CLOSED on error. New log events: `security.credentialed_binary_denied`, `security.credentialed_binary_gate_error`, `security.credentialed_binary_wrapper_too_deep`.
- Env scrubbing on host fall-through (`internal/tools/env_scrub.go`) strips static credential keys (GH_TOKEN, AWS_*, OPENAI_API_KEY, …) plus dynamic keys from tenant's registered binaries; preserves HOME/PATH/TERM/LANG/USER/TZ.
- Subagent ExecTool registration (`cmd/gateway_agents.go` → `buildSubagentToolsRegistry`) now receives the same `SecureCLIStore` — parent can't delegate to a child to bypass the gate.
- Binary names lowercased on Create/Update for symmetry with case-insensitive lookup.
- Thread `pgStores.SecureCLI` into `setupSubagents` in `cmd/gateway.go`.
- 25+ unit tests (gate, env-scrub, SQLite IsRegisteredBinary) + 5 PG integration tests (deny-ungranted, allow-granted, unregistered-unchanged, is_global-not-denied regression, shell-wrapper-bypass denied). All green.
- Remove duplicate allowLoopbackForTest in hooks_pipeline_test.go (canonical version lives in v3_test_helper.go)
- Remove unused fakeClient assignment in mcp_grant_revoke_test.go; fakeMCPClient type retained for future use
* fix(providers): use DB name for Anthropic/ClaudeCLI in HTTP onboarding
Commit 7fcf0327 added WithAnthropicName/WithClaudeCLIName on the startup
registration path (cmd/gateway_providers.go) but missed the HTTP onboarding
path (internal/http/providers.go). Providers created via POST /v1/providers
were registered in-memory with the hardcoded default name ("anthropic",
"claude-cli") instead of the user's chosen DB name.
Symptom: user onboards an Anthropic provider named e.g. "my-anthropic",
picks a model, hits verify — handleVerifyProvider looks up via
GetForTenant(tenantID, p.Name) and fails with "provider not registered:
my-anthropic" because the registry key was "anthropic".
Fix: mirror the startup path by passing WithAnthropicName(p.Name) and
WithClaudeCLIName(p.Name) in registerInMemory. Adds two regression tests
asserting custom-named providers are locatable by their DB name and NOT
by the hardcoded default.
Closes incomplete-refactor gap in #742.
* test(providers): add negative assertion for ClaudeCLI default name
Mirrors the Anthropic test's negative guard: when a user picks a custom
provider name, the hardcoded "claude-cli" default must NOT be present in
the registry. Per Claude review on PR #942 — symmetric test coverage.
- Add per-tenant TTS configuration endpoints (GET/POST /v1/tts/config)
with RoleAdmin auth instead of master scope
- Implement TenantTTSResolver for channels to use tenant-specific
TTS providers and auto mode settings
- Add per-agent voice override in channel TTS auto-apply:
- Extend OutboundMessage with AgentID and AgentOtherConfig
- Inject AgentAudioSnapshot in dispatch.go from outbound message
- MaybeApply reads tts_voice_id/tts_model_id from agent context
- Fix events.go to use RunContext.TenantID directly (H3)
- Fix dispatch.go error notification to use sendCtx (H1)
- Update UI to use new /v1/tts/config endpoints
When user sends exact cancel keywords (stop, cancel, abort, thôi, dừng,
hủy, 取消, 停, nevermind) while agent is busy, auto-trigger abort instead
of injecting as follow-up message. Exact match only - "stop now" still
injects normally to avoid false positives.
- Add TTSAutoMode field to LoopConfig, Loop, SystemPromptConfig
- Wire from config.Tts.Auto through resolver to system prompt
- Inject "## Voice Response" section explaining [[tts]] usage
- Update AGENTS.md: voice only when user explicitly requests
- Add media-src 'self' blob: to CSP for TTS audio blob URLs
- Show advanced config section expanded by default
- Add Save button in credentials section
- Show Voice/Playground sections only after credentials saved
- Improve voice picker message when API key not saved yet
- Add Vite proxy timeout for large audio responses
- Add POST /v1/tts/test-connection for testing TTS credentials before saving
- Add UpdateManager() with RWMutex for thread-safe HTTP handler hot-reload
- Add sendVoice() for Telegram voice bubble support via audio_as_voice metadata
- Wire ttsHandler into gateway deps for config reload propagation
- Add GrantChecker interface with cache + event-bus invalidation for revalidating MCP tool grants at execution time (not just dispatch)
- Move toolGroups from package-level global to per-Registry field; Clone() gives each agent Loop isolated tool groups, eliminating cross-agent race condition
- BridgeTool.Execute rechecks grant validity before executing MCP methods; uses serverID field for cache key
- Wire MCPGrantChecker through ResolverDeps to MCP Manager and LoopConfig
- Add integration test for grant revocation scenarios
- Update policy functions to accept *Registry parameter
* feat(packages): add GitHub Releases binary installer
New runtime source `github:owner/repo[@tag]` for installing Linux CLI
binaries from GitHub Releases. Admin-only, SHA256-verified, ELF-validated.
Backend:
- GitHub API client with 10-min cache + rate-limit mapping
- SSRF-guarded streaming downloader (HTTPS + host allowlist, re-validated
on every redirect hop, literal-IP rejection)
- Checksums.txt / SHA256SUMS lookup with constant-time verify
- Archive extract (tar.gz / zip / raw) with path-traversal + zip-bomb
guards, symlink skip
- ELF magic + 64-bit class + runtime-arch validation
- Atomic manifest persistence (temp + rename)
HTTP:
- POST /v1/packages/install accepts github: spec
- GET /v1/packages/github-releases for picker UI (viewer+, arch-filtered)
- Extended InstalledPackages response with github field
- github-bin runtime probe
Infra:
- Dockerfile creates /app/data/.runtime/bin (goclaw:goclaw 0755)
- docker-entrypoint.sh prepends bin dir to PATH
- Env-only config (never config.json): token, max size, org allowlist,
bin dir, manifest path
UI:
- GitHub Binaries section + release picker modal
- Dismissable musl/glibc compatibility warning (localStorage)
- i18n keys across en/vi/zh
Docs: docs/packages-github.md user guide + 14-skills-runtime.md cross-ref.
Closes#741
* refactor(packages): revert validPkgName broadening + drop unused sentinel
Code review cleanup:
- validPkgName regex had `:` added defensively, but github: specs are
validated separately via skills.ParseGitHubSpec before reaching this
check — the broadening was dead attack surface.
- Drop unused ErrUnknownArchive sentinel + the `_ = ErrUnknownArchive`
stub in extractRaw.
* feat(packages): per-user rate limit on /v1/packages/github-releases
Cap picker endpoint at 30 req/min/user (burst 10) to protect the shared
GitHub API quota. Key is userID (header X-GoClaw-User-Id) or remote IP
for anonymous callers. Returns 429 + Retry-After: 60 when tripped.
Standalone token-bucket limiter (stale-entry cleanup every 5 min) lives
in internal/http rather than importing internal/gateway, which would
create a package cycle.
* fix(ui): guard split()[0] for noUncheckedIndexedAccess strict TS
CI pnpm build failed on TS2345: `.split('@')[0]` returns
`string | undefined` under strict index access. Default to empty
string to satisfy the type checker; runtime behaviour unchanged
because the downstream regex rejects empty strings.
* fix(packages): address Claude review — medium + low findings
Medium
- rate limiter: atomic.Int64 lastSeen + amortized sweep replaces
goroutine-based cleanup → fixes data race on lastSeen and the
goroutine leak when tests swap the package-level limiter.
- checksum pipeline: slog.Warn on ReadFile and ParseChecksums failures
(previously silent). "asset not listed" stays warn+proceed but is now
documented as the publisher's choice — ELF validation remains the
final gate.
- downloader: drop http.Client.Timeout (30s capped the whole request
including body read, aborting large downloads on slow links). Context
deadline from install timeout (5 min) is the correct bound.
Low / style / UI
- extractRaw honors maxUncompressed (ErrFileTooLarge on overflow) so
the helper is safe outside the hot path.
- cmd/gateway_github_installer.go: drop the explicit cfg.Defaults()
call — NewGitHubInstaller already invokes it.
- GitHubPackageEntry: remove unpopulated InstalledBy field + document
why.
- owner regex tightened to 39-char GitHub limit (was 40).
- mu lock comment corrected: serializes only the disk-write phase.
- UI: shared stripPrefixAndTag helper + owner regex mirrors the backend
39-char cap; destructure-with-default kills the split()[0] ?? ""
awkwardness while still satisfying noUncheckedIndexedAccess.
Verified: go build (pg + sqliteonly) · go vet · go test -race
./internal/skills ./internal/http · pnpm build.
* fix(packages): address Claude review round 2
Medium
- validRepoPath now rejects trailing hyphens in the owner segment and
caps at 39 chars, matching gitHubSpecRE exactly. Previously a subtle
drift between the two validators could let `foo-/repo` slip to the
GitHub API and surface as a 502 instead of a clean 400.
- handleGitHubReleases no longer forwards raw err.Error() from the
upstream call. Maps sentinel errors:
ErrGitHubRateLimited → 429 + Retry-After
ErrGitHubNotFound → 404
ErrGitHubUnauthorized → 502 "github authentication failed"
default → 502 "failed to fetch releases"
Avoids leaking rate-limit reset timestamps / server internals to
viewer-tier callers.
Low / UX
- Install response now returns the manifest entry for github: specs
(new lookupGitHubEntry helper; nil-safe fallback to {ok:true}). Lets
the UI display "installed: lazygit v0.42.0" without a list refresh.
- gitHubSpecRE tag segment capped at 1..255 chars (git ref-name bound).
UI isValidFullSpec mirrors the same cap.
* fix(packages): address Claude review round 3
Medium
- github_api: URL-encode owner, repo, and tag via url.PathEscape when
building API paths. Previously a tag containing '#' would be stripped
as a URL fragment and '?' would inject a query parameter, silently
hitting the wrong release.
Low / polish
- Uninstall via full "github:owner/repo[@tag]" spec now falls back to
manifest lookup by owner/repo, handling packages whose binary name
differs from the repo name (cli/cli → gh).
- GitHubClient.cache sweeps expired entries opportunistically when the
map grows past 256 entries (prevents theoretical unbounded growth
over long uptime).
- handleInstall for github: specs now calls GitHubInstaller.Install
directly and returns the freshly-created manifest entry, eliminating
the double manifest read via List() from the lookupGitHubEntry helper.
- pickBinaries comment corrected — actual behavior excludes paths
matched by nonBinaryPathRE rather than enforcing a single-depth limit.
* fix(packages): address Claude review round 4 (style + ordering)
All 4 findings are Low severity:
- github_api.go: replace interface{} with any across the cache type,
cacheGet return, cacheSet param, and doJSON out param.
- doJSON: rename local `url` to `apiURL` to avoid shadowing the
"net/url" package import used by GetRelease/ListReleases.
- Uninstall: save the updated manifest BEFORE removing binaries on
disk. If saveManifest fails we now bail out without leaving a
manifest entry that still claims binaries which have been deleted
(a retried Uninstall would otherwise hit ErrPackageNotInstalled
after the first attempt wiped the files). Disk removal stays
best-effort and warn-on-error, which matches the idempotent intent.
- pickBinaries: inline comment corrected to reflect actual behavior —
depth is not enforced; nonBinaryPathRE filter + downstream ELF
validation are the real gates.
* fix(packages): address Claude review round 5
All 3 findings are Low severity:
- handleInstall github fast-path now wraps the context with
skills.InstallTimeout (5 min) before calling gh.Install and emits the
same "skills: installing dep" / "dep installed" / "github install
failed" log lines as the generic InstallSingleDep path, so
operator-observability is identical between github: and pip:/npm:
install flows.
- installTimeout promoted to exported InstallTimeout so the http layer
shares the single source of truth rather than duplicating the
5-minute constant.
- cacheMaxEntries comment clarifies it is a soft sweep trigger, not a
hard cap — when every entry is still within TTL the map can briefly
exceed the threshold by one insert.
* fix(packages): address Claude review round 6 (final Lows)
Both findings are Low severity (reviewer marked the PR "ready to merge"
already):
- github_installer: "no checksum asset available" downgraded from
slog.Warn to slog.Info. Many popular upstream releases (jq, fzf,
older ripgrep, etc.) ship no checksum file at all — that is publisher
policy, not a problem with the install. The suspicious cases
(checksum file unreadable, unparseable, or missing this asset) stay
at Warn so they stand out.
- handleGitHubReleases response now uses a narrow assetPreview DTO
(name + size_bytes) instead of embedding the full GitHubAsset type
which also carried browser_download_url. The picker UI never rendered
the URL; trimming the response keeps the viewer-tier surface minimal.
UI AssetPreview interface realigned to match.
* fix(packages): address Claude review round 7
Narrow the GET /v1/packages GitHub entry to a viewer-safe projection
(repo/tag/binaries/name/installed_at), mirroring the assetPreview fix
from round 6. Strips asset_url, sha256, and asset_name from the list
response — viewer-level callers no longer see CDN download URLs or
checksum metadata for installed packages. UI types realigned; the
removed fields were never rendered.
Finding #2 (install writes binary before manifest save) left as noted —
reviewer confirmed informational only, self-heals on retry, no security
impact since binaries pass ELF validation before being written.
* fix(packages): address Claude review round 8
Map HTTP 429 (GitHub secondary rate limits — abuse detection,
unauthenticated bursts, search) to ErrGitHubRateLimited in the API
client so the picker endpoint renders 429 "rate limit reached" with
Retry-After: 60 instead of falling through to 502 "failed to fetch
releases". Primary rate limits (403 + X-RateLimit-Remaining: 0) were
already handled; this covers the secondary class documented at
https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#about-secondary-rate-limits
* fix(packages): address Claude review round 9
Two defensive hardenings flagged as Very Low:
- ParseChecksums: strip leading `./` from checksum filenames.
`sha256sum ./file` emits `./file` in the name column; the caller
looks up by bare asset basename so `./`-prefixed entries would
silently miss. Real release checksums almost never use this form,
but the guard is essentially free.
- doJSON: cap response body at 8 MiB via io.LimitReader before JSON
decode. Current GitHub list/release payloads are well under this
(~1 MiB at per_page=100). Guards against future call sites or a
misbehaving upstream returning an oversized document.
* fix(cron): eliminate cross-test race on runLoopTickInterval
`Service.Stop()` closes stopChan but does not wait for the runLoop
goroutine to exit. In the test suite, test A's `defer cs.Stop()` can
return before the spawned runLoop has reached
`ticker := time.NewTicker(runLoopTickInterval)`. If test B then calls
`setFastTick()` to mutate the package-level var, the race detector
correctly flags it:
Read at runLoopTickInterval by goroutine A (runLoop ticker init)
Previous write by goroutine B (setFastTick in test B)
Fix: snapshot `runLoopTickInterval` inside `Start()` under the mutex
before spawning the goroutine, and pass the value as a parameter to
`runLoop`. The spawned goroutine no longer reads the package-level
var, so the cross-test window is closed. Production behavior
unchanged.
Verified: `go test -race -count=3 ./internal/cron/...` passes three
times in a row; the CI failure on PR #898 reproduced before the fix
and is gone after.
* fix(packages): address review P0/P1/P2 + new DoS vector
P0.1 — UI uninstall 400: parseAndValidatePackage now accepts
github:<bare-name> (manifest Name form, no owner/repo) in addition
to the full spec. UI sends github:${pkg.name} from the manifest;
dispatcher already tolerated bare names — the HTTP validator was
the only gate rejecting them. Install path re-validates strictly
via ParseGitHubSpec and bare-name install returns 400 now (was 500).
P1.1 — ExtractArchive raw-ELF fallback name: add ExtractArchiveAs(
path, fallbackName, max). Installer passes parsed.Repo so raw
(non-archive) ELF assets no longer end up recorded as
/tmp/goclaw-gh-asset-XXXX.bin — that basename would leak into the
manifest Binaries entry and break PATH lookup.
P1.3 — Archive entry count cap: maxArchiveEntries = 10_000 +
ErrTooManyEntries sentinel. Tar: count ALL headers seen (incl.
symlinks/dirs we skip) to block the gzip-bomb-of-headers DoS —
header bytes don't count against maxUncompressed for zero-size
entries. Zip: pre-check via peekZipEntryCount reads the EOCD
record manually and rejects oversized archives BEFORE
zip.OpenReader allocates []*zip.File of declared capacity (this
was a fresh red-team finding; stdlib would otherwise alloc ~1GB
for a crafted 200MB zip claiming 4M entries).
P1.4 — Rate-limit install/uninstall: packagesWriteLimiter
(10/min/user, burst 3). Admin-only mitigates but a compromised
token could otherwise flood upstream (GitHub/pip/npm) or spam
manifest mutations.
P1.6 — Non-Linux early reject: ErrUnsupportedOS guard at the top
of Install(). Windows/macOS hosts no longer waste bandwidth
fetching a Linux asset just to fail at the ELF machine check.
P1.7 — Manifest fsync: OpenFile → Write → Sync → Close → Rename →
dir Sync, with tmp cleanup on every error path. POSIX doesn't
guarantee durability via rename alone; XFS / ext4 with async
journal can reorder.
P2.1 — Belt-and-suspenders zip runtime break when cumulative bytes
reach the cap (pre-declared check already covers it but the
streaming loop now bails immediately).
P2.6 — Binary-name collision warn: slog.Warn when a different repo
already owns the basename we're about to overwrite. Last-writer-
wins unchanged; operator now gets a signal instead of silence.
Hardening — rate-limit key: rateLimitKeyFromRequest prefers
store.UserIDFromContext over the raw X-GoClaw-User-Id header so
an admin can't rotate the header mid-session to dodge the bucket.
Header/IP fallback retained for pre-auth / test callers.
Tests: 9 new cases on parseAndValidatePackage (github full/bare/
empty/traversal/injection/space/leading-hyphen);
TestExtractArchiveAs_RawELFUsesFallbackName;
TestExtractTarGz_EntryCountCap + TestExtractZip_EntryCountCap;
TestPeekZipEntryCount (DoS pre-check path).
Verified: go build ./... && go build -tags sqliteonly ./... &&
go vet ./... && go test -race ./internal/skills/... ./internal/http/...
---------
Co-authored-by: viettranx <viettranx@gmail.com>
Follow-up to #924:
- Add TestGetUpdates_EmptyResultReturnsNilSlice to cover the no-events
polling branch (API returns {} when no pending updates).
- Extract magic 7s into pollTimeoutHeadroom const for readability.
- A.10 AdminRoleBypass (3 sub-cases): admin, operator, owner roles in
ctx allow write_file and cron in group context even when SenderID is
empty (the bypass short-circuits before the synthetic-sender check).
- A.11 ViewerRoleDoesNotBypass (2 sub-cases): viewer and empty roles
still hit the DENY path, guarding against the RBAC bypass leaking to
unprivileged roles.
Integration suite 11 fixtures, all passing on pgvector pg18.
Dashboard users (and other tenant-authenticated admin/operator/owner
actors) dispatching team tasks to agents in Telegram/Discord groups
previously hit the empty-sender DENY rule when the assigned agent
tried to write files — because dashboard users have tenant identities
(e.g. "viettx") that don't match the numeric file-writer grants stored
per channel.
Two-part fix:
1. CheckFileWriterPermission / CheckCronPermission add an RBAC bypass
at the store layer: if ctx carries RoleFromContext in {admin,
operator, owner}, skip the per-user grant check. Tenant RBAC already
pre-authenticated these callers at the gateway edge; requiring a
redundant per-channel grant blocks legitimate admin work.
2. Role propagation through the full dispatch chain:
ctx (RoleFromContext)
→ RunRequest.Role
→ SubagentTask.OriginRole / AnnounceMetadata.OriginRole
/ DelegateRequest.Role
→ InboundMessage.Metadata[MetaOriginRole]
→ subagentAnnounceRouting.Role / teammateRole
→ RunRequest.Role (re-ingress)
→ store.WithRole(ctx, ...) in loop_context
Covers subagent announce, delegate announce, teammate dispatch
(WS dashboard, tool), session_send, and processNormalMessage's
synthetic-sender override path. team_tasks_create persists
origin_role for deferred dispatches.
Viewer and empty roles fall through to the existing sender-based path
(no bypass).
Regression tests:
- A.10 AdminRoleBypass: admin/operator/owner in ctx allowed even with
empty sender (normally DENY).
- A.11 ViewerRoleDoesNotBypass: viewer and empty roles still hit DENY.
Builds + integration + unit tests green.
Closes the two last unpropagated paths identified by the #915 e2e audit.
processNormalMessage now prefers MetaOriginSenderID when the on-wire
SenderID is a synthetic internal sender (notification:, ticker:,
session_send_tool, system:escalation, etc.). Real user senders pass
through unchanged. Defense-in-depth: MetaOriginSenderID is ignored if
it itself looks synthetic, preventing a propagation bug from turning
into a permission bypass.
teams_tasks_mutations dashboard dispatch now injects MetaOriginSenderID
into the teammate InboundMessage metadata, preferring the WS caller's
sender (live dashboard) and falling back to origin_sender_id stored on
the task (for deferred dispatches). Without this, dashboard-triggered
team tasks in group chats would hit the empty-sender DENY rule.
team_tasks_create persists origin_sender_id into task.Metadata at
creation time so the deferred dashboard dispatch path can reconstruct
the original actor identity.
Prevents two silent regressions flagged by the #915 e2e audit:
Flow Q — private skill invisible to creator in group:
ListAccessible filtered s.owner_id = $2 (userID = group principal).
After the ACTOR migration, new skills get owner_id = sender, so the
publisher could not see their own private skill on the next group turn.
Fixed by matching owner_id and skill_user_grants.user_id against both
userID and ActorIDFromContext(ctx) in the query. The OR collapses in
DM contexts where both resolve to the same value.
Ownership check in skill_manage (patch + delete):
Replaced the two-identity compare with isOwnerOfSkill helper that
matches any of {actor, userID, senderID}. Covers (a) new rows with
actor-based owner, (b) legacy rows with group-scope / tenant-merged
owner, (c) rows from the short pre-merge-aware ActorID window where
DM owners got the raw channel sender. A tenant-merged user reliably
matches all three of their skills regardless of vintage.
Gateway consumer resolves raw channel senders to merged tenant
identities for DMs (e.g. Telegram "386246614" → tenant "viettx"). The
previous ActorIDFromContext unconditionally preferred SenderID, which
broke cross-channel ownership continuity — a user's skills / audit
records published from Telegram would carry the channel-specific
numeric ID instead of the stable tenant identity.
New resolution rule:
- group: / guild: scope → SenderID wins (UserID is the group namespace)
- DM / HTTP / cron → UserID wins (already merge-aware from consumer)
- SenderID fallback → only when UserID is empty
Unit coverage updated: 8 sub-tests covering group, guild, DM-merge,
DM-unmerged, HTTP, empty, sender-only, delimited-sender scenarios.
- agent-identity-conventions.md: new "ActorID vs UserID in Group Chats"
section with helper table, group behavior table, propagation chain
diagram, group permission policy, legacy-data tolerance notes.
- 17-changelog.md: entry covering the security fix, propagation
additions, ACTOR migration list, scope-intentional sites, tests,
and the no-DB-migration decision with its legacy-fallback rationale.
Extends Telegram group write_file permission coverage with four new
fixtures that exercise the edges exposed by the audit:
- A.6 EmptySenderDenied: empty SenderID in group context must deny
(closes silent-bypass BUG-A)
- A.7 SyntheticSendersDenied: 7 sub-cases for subagent:, notification:,
teammate:, system:, ticker:, session_send_tool, subagent:delegate:
(reporter-reported denial BUG-B)
- A.8 PropagatedSenderAllowed: positive case for F2/F3 — once the real
sender is propagated through announce re-ingress, a granted user can
still write files
- A.9 DMEmptySenderPasses: guards against F1's deny-on-empty rule
bleeding into DM context
Full suite: 9 fixtures, all passing on pgvector pg18.
Resolves issue #915 (Telegram group write_file permission denied after
/addwriter) by reconstructing the real sender on announce re-ingress
and migrating ownership/audit call sites off the scope-coupled UserID.
Propagation chain (new MetaOriginSenderID key):
SubagentTask.OriginSenderID
→ AnnounceMetadata.OriginSenderID
→ InboundMessage.Metadata[MetaOriginSenderID]
→ subagentAnnounceRouting.SenderID
→ RunRequest.SenderID
Same chain for delegate_tool.announceToParent, team_tool_dispatch,
sessions_send, team_tasks_blocker escalation.
ACTOR migration (owner/audit now uses ActorIDFromContext):
- publish_skill: skill owner = individual sender
- skill_manage create/patch/delete: owner = sender; ownership check
accepts actor or legacy UserID for pre-migration rows
- delegate_tool: DelegateRequest.UserID + DomainEvent.UserID = actor
- team_tasks_blocker: blocker attribution + escalation = actor
- team_tool_cache: team access-policy check = actor (fixes per-user
allow/deny lists in group chats)
Scope-intentional (unchanged, commented): cron.go, team_tasks_create.go.
CheckFileWriterPermission/CheckCronPermission previously returned nil
on empty SenderID in group/guild context (fail-open). This allowed
subagent/delegate/teammate/dashboard system turns to write files
without any writer grant — silent privilege bypass (#915).
Post-fix: empty or synthetic-prefix senders (subagent:, notification:,
teammate:, system:, ticker:, session_send_tool) are denied in group
context. DM / HTTP contexts remain unchanged. Fail-open still applies
only to DB errors (availability preserved).
bus.IsInternalSender now also recognises subagent: prefix.
