feat(workspace-server): seed schedules from workspace-template config.yaml #1929

Merged
hongming merged 2 commits from feat/seed-schedules-from-ws-template into main 2026-05-26 23:50:42 +00:00
Owner

Summary

Adds a new template_schedules.go helper that parses a workspace template's config.yaml for its schedules: block and INSERTs each entry into workspace_schedules with source='template'. Mirrors the org/import path (org_import.go) so a workspace created directly from a workspace template lands with the same schedule grid the org/import path would have produced.

Closes the gap the SEO Agent template hit: the template documented a "comprehensive schedule" (in source/.../recommended-schedule.md and the matching config.yaml schedules block landing in molecule-ai-workspace-template-seo-agent#12), but the workspace-server provisioner never consumed schedules: from a workspace template — only the org/import path seeded workspace_schedules.

Wiring

  • New: handlers/template_schedules.go
    • parseTemplateSchedules(templatePath) — minimal YAML parse of schedules: only; returns (nil, nil) when config.yaml is absent or the block is empty. Errors only on read/parse failure of a present file.
    • seedTemplateSchedules(ctx, workspaceID, templatePath, schedules) — per-entry resolve+insert via the canonical orgImportScheduleSQL constant from org.go. Reuses the existing resolvePromptRef + scheduler.ComputeNextRun helpers. Per-row failures are logged and skipped; never fatal.
  • Modified: handlers/workspace.go
    • WorkspaceHandler.Create calls the parser + seeder after templatePath resolves and before provisionWorkspaceAuto runs. Non-fatal — a broken schedules: block can never block workspace provisioning.
    • Schedules are seeded once at workspace creation; Restart does not re-seed (so user-deleted template rows stay deleted, runtime rows are untouched).
  • New: handlers/template_schedules_test.go
    • Table-driven coverage for parseTemplateSchedules: absent file, empty path, no schedules block, happy path (3 entries incl. explicit enabled: false), malformed YAML.

Issue #24 contract preserved

Both the new path and the existing org/import path execute the same orgImportScheduleSQL constant, so the four guarantees stay enforced:

  • Additive (new template rows are INSERTed)
  • Idempotent (existing template rows refreshed via ON CONFLICT DO UPDATE)
  • Runtime-row preservation (WHERE source='template')
  • Never DELETE

TestImport_OrgScheduleSQLShape already gates that SQL's shape against regression.

Test plan

  • go vet ./... — clean
  • go build ./... — clean
  • gofmt -d on changed files — clean
  • go test ./workspace-server/internal/handlers/ — PASS (incl. 5 new unit tests)
  • E2E: spin up a fresh SEO Agent workspace after SEO template PR #12 lands and confirm the Canvas Schedule tab shows the full 12-entry grid automatically.

Limitations / non-goals

  • The org_import.go insert loop is not refactored to share seedTemplateSchedules. The shared piece (the SQL) is already centralized via orgImportScheduleSQL; org_import uses context.Background() (long-running async tx) which differs from Create's request ctx. Keeping the call sites separate keeps this PR focused.
  • Timezone env-var expansion (${TENANT_TIMEZONE}) is not added in this PR. Mirrors current org/import behavior; template authors pick a literal IANA zone (or rely on UTC + per-tenant operator overrides). Tracked as a follow-up.

Companion PR

molecule-ai/molecule-ai-workspace-template-seo-agent#12 — adds the SEO Agent schedules: block this PR teaches the provisioner to consume.

🤖 Generated with Claude Code

## Summary Adds a new `template_schedules.go` helper that parses a workspace template's `config.yaml` for its `schedules:` block and INSERTs each entry into `workspace_schedules` with `source='template'`. Mirrors the org/import path (`org_import.go`) so a workspace created directly from a workspace template lands with the same schedule grid the org/import path would have produced. Closes the gap the SEO Agent template hit: the template documented a "comprehensive schedule" (in `source/.../recommended-schedule.md` and the matching `config.yaml` schedules block landing in [molecule-ai-workspace-template-seo-agent#12](https://git.moleculesai.app/molecule-ai/molecule-ai-workspace-template-seo-agent/pulls/12)), but the workspace-server provisioner never consumed `schedules:` from a workspace template — only the org/import path seeded `workspace_schedules`. ## Wiring - **New:** `handlers/template_schedules.go` - `parseTemplateSchedules(templatePath)` — minimal YAML parse of `schedules:` only; returns `(nil, nil)` when `config.yaml` is absent or the block is empty. Errors only on read/parse failure of a present file. - `seedTemplateSchedules(ctx, workspaceID, templatePath, schedules)` — per-entry resolve+insert via the canonical `orgImportScheduleSQL` constant from `org.go`. Reuses the existing `resolvePromptRef` + `scheduler.ComputeNextRun` helpers. Per-row failures are logged and skipped; never fatal. - **Modified:** `handlers/workspace.go` - `WorkspaceHandler.Create` calls the parser + seeder after `templatePath` resolves and before `provisionWorkspaceAuto` runs. Non-fatal — a broken `schedules:` block can never block workspace provisioning. - Schedules are seeded **once** at workspace creation; Restart does not re-seed (so user-deleted template rows stay deleted, runtime rows are untouched). - **New:** `handlers/template_schedules_test.go` - Table-driven coverage for `parseTemplateSchedules`: absent file, empty path, no `schedules` block, happy path (3 entries incl. explicit `enabled: false`), malformed YAML. ## Issue #24 contract preserved Both the new path and the existing org/import path execute the same `orgImportScheduleSQL` constant, so the four guarantees stay enforced: - Additive (new template rows are INSERTed) - Idempotent (existing template rows refreshed via ON CONFLICT DO UPDATE) - Runtime-row preservation (`WHERE source='template'`) - Never DELETE `TestImport_OrgScheduleSQLShape` already gates that SQL's shape against regression. ## Test plan - [x] `go vet ./...` — clean - [x] `go build ./...` — clean - [x] `gofmt -d` on changed files — clean - [x] `go test ./workspace-server/internal/handlers/` — PASS (incl. 5 new unit tests) - [ ] E2E: spin up a fresh SEO Agent workspace after [SEO template PR #12](https://git.moleculesai.app/molecule-ai/molecule-ai-workspace-template-seo-agent/pulls/12) lands and confirm the Canvas Schedule tab shows the full 12-entry grid automatically. ## Limitations / non-goals - The `org_import.go` insert loop is **not** refactored to share `seedTemplateSchedules`. The shared piece (the SQL) is already centralized via `orgImportScheduleSQL`; org_import uses `context.Background()` (long-running async tx) which differs from Create's request ctx. Keeping the call sites separate keeps this PR focused. - Timezone env-var expansion (`${TENANT_TIMEZONE}`) is **not** added in this PR. Mirrors current org/import behavior; template authors pick a literal IANA zone (or rely on UTC + per-tenant operator overrides). Tracked as a follow-up. ## Companion PR [molecule-ai/molecule-ai-workspace-template-seo-agent#12](https://git.moleculesai.app/molecule-ai/molecule-ai-workspace-template-seo-agent/pulls/12) — adds the SEO Agent `schedules:` block this PR teaches the provisioner to consume. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
hongming added 1 commit 2026-05-26 23:10:56 +00:00
feat(workspace-server): seed schedules from workspace-template config.yaml
ci-arm64-advisory / fast-checks (pull_request) Waiting to run
Lint shellcheck (arm64 pilot) / shellcheck-arm64 (pilot) (pull_request) Successful in 11s
Block internal-flavored paths / Block forbidden paths (pull_request) Successful in 6s
CI / Python Lint & Test (pull_request) Successful in 4s
CI / Detect changes (pull_request) Successful in 8s
E2E Chat / detect-changes (pull_request) Successful in 12s
E2E API Smoke Test / detect-changes (pull_request) Successful in 18s
E2E Peer Visibility (literal MCP list_peers) / E2E Peer Visibility (pull_request) Has been skipped
E2E Staging Canvas (Playwright) / detect-changes (pull_request) Successful in 18s
E2E Peer Visibility (literal MCP list_peers) / E2E Peer Visibility (local) (pull_request) Successful in 1m13s
Handlers Postgres Integration / detect-changes (pull_request) Successful in 11s
Harness Replays / detect-changes (pull_request) Successful in 7s
Lint forbidden tenant-env keys / Scan workspace_secrets writers for forbidden env keys (pull_request) Successful in 5s
Lint no tenant GITEA or GITHUB token write / Scan for repo-host token write into tenant workspace surface (pull_request) Successful in 4s
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 19s
gate-check-v3 / gate-check (pull_request) Successful in 6s
qa-review / approved (pull_request) Failing after 6s
security-review / approved (pull_request) Failing after 5s
sop-checklist / na-declarations (pull_request) N/A: (none)
sop-checklist / all-items-acked (pull_request) Successful in 5s
sop-checklist / review-refire (pull_request) Has been skipped
sop-tier-check / tier-check (pull_request) Successful in 4s
lint-required-no-paths / lint-required-no-paths (pull_request) Successful in 1m7s
CI / Canvas (Next.js) (pull_request) Successful in 22s
CI / Shellcheck (E2E scripts) (pull_request) Successful in 5s
E2E Staging External Runtime / E2E Staging External Runtime (pull_request) Successful in 5m29s
E2E Chat / E2E Chat (pull_request) Successful in 11s
E2E Staging Canvas (Playwright) / Canvas tabs E2E (pull_request) Successful in 6s
E2E API Smoke Test / E2E API Smoke Test (pull_request) Successful in 2m10s
Harness Replays / Harness Replays (pull_request) Successful in 14s
CI / Canvas Deploy Reminder (pull_request) Has been skipped
Handlers Postgres Integration / Handlers Postgres Integration (pull_request) Successful in 3m20s
CI / Platform (Go) (pull_request) Successful in 7m16s
CI / all-required (pull_request) Successful in 16m2s
d8b409f1bc
Adds a new template_schedules.go helper that parses a workspace
template's config.yaml for its `schedules:` block and INSERTs each
entry into workspace_schedules with source='template'. Mirrors the
org/import path (org_import.go) so a workspace created directly from
a workspace template lands with the same schedule grid the
org/import path would have produced.

Closes the gap the SEO Agent template hit: the template documented
a "comprehensive schedule" (in source/.../recommended-schedule.md
and the matching config.yaml schedules: block from
molecule-ai-workspace-template-seo-agent#12), but the workspace-
server provisioner never consumed `schedules:` from a workspace
template — only the org/import path seeded workspace_schedules.

Wiring:
- New: handlers/template_schedules.go
  * parseTemplateSchedules(templatePath) — minimal YAML parse of
    `schedules:` only; returns (nil, nil) when config.yaml is
    absent or the block is empty. Errors only on read/parse
    failure of a present file.
  * seedTemplateSchedules(ctx, workspaceID, templatePath, schedules)
    — per-entry resolve+insert via the canonical
    orgImportScheduleSQL constant from org.go. Reuses the existing
    resolvePromptRef + scheduler.ComputeNextRun helpers. Per-row
    failures are logged and skipped; never fatal.
- Modified: handlers/workspace.go
  * WorkspaceHandler.Create calls parseTemplateSchedules +
    seedTemplateSchedules after the templatePath resolves and
    before provisionWorkspaceAuto runs. Non-fatal — a broken
    schedules: block can never block workspace provisioning.
  * Schedules are seeded once at workspace creation; Restart
    does not re-seed (so user-deleted template rows stay deleted).
- New: handlers/template_schedules_test.go
  * Table-driven coverage for parseTemplateSchedules: absent file,
    empty path, no schedules block, happy path (3 entries inc.
    explicit enabled: false), malformed YAML.

Issue #24 contract preserved (additive, idempotent, runtime-row
preservation, never-DELETE) because both the new path and the
existing org/import path execute the same orgImportScheduleSQL
constant — and TestImport_OrgScheduleSQLShape already gates that
SQL's shape against regression.

Verified locally:
  go vet ./...                 → clean
  go build ./...               → clean
  gofmt -d <changed files>     → clean
  go test ./internal/handlers/ → PASS (incl. 5 new tests)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hongming added 1 commit 2026-05-26 23:31:55 +00:00
fix(template-schedules): apply review findings (ordering + bounds)
ci-arm64-advisory / fast-checks (pull_request) Waiting to run
Block internal-flavored paths / Block forbidden paths (pull_request) Successful in 6s
Lint shellcheck (arm64 pilot) / shellcheck-arm64 (pilot) (pull_request) Successful in 12s
CI / Python Lint & Test (pull_request) Successful in 11s
E2E API Smoke Test / detect-changes (pull_request) Successful in 11s
CI / Detect changes (pull_request) Successful in 15s
E2E Peer Visibility (literal MCP list_peers) / E2E Peer Visibility (pull_request) Has been skipped
E2E Chat / detect-changes (pull_request) Successful in 11s
Handlers Postgres Integration / detect-changes (pull_request) Successful in 12s
E2E Staging Canvas (Playwright) / detect-changes (pull_request) Successful in 14s
Harness Replays / detect-changes (pull_request) Successful in 12s
Lint no tenant GITEA or GITHUB token write / Scan for repo-host token write into tenant workspace surface (pull_request) Successful in 11s
Lint forbidden tenant-env keys / Scan workspace_secrets writers for forbidden env keys (pull_request) Successful in 12s
gate-check-v3 / gate-check (pull_request) Successful in 11s
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 12s
qa-review / approved (pull_request) Failing after 4s
security-review / approved (pull_request) Failing after 5s
sop-checklist / na-declarations (pull_request) N/A: (none)
sop-checklist / all-items-acked (pull_request) Successful in 5s
sop-checklist / review-refire (pull_request) Has been skipped
E2E Peer Visibility (literal MCP list_peers) / E2E Peer Visibility (local) (pull_request) Successful in 56s
sop-tier-check / tier-check (pull_request) Successful in 6s
lint-required-no-paths / lint-required-no-paths (pull_request) Successful in 1m3s
CI / Canvas (Next.js) (pull_request) Successful in 6s
CI / Shellcheck (E2E scripts) (pull_request) Successful in 5s
E2E Chat / E2E Chat (pull_request) Successful in 10s
E2E Staging Canvas (Playwright) / Canvas tabs E2E (pull_request) Successful in 5s
Harness Replays / Harness Replays (pull_request) Successful in 6s
CI / Canvas Deploy Reminder (pull_request) Has been skipped
E2E API Smoke Test / E2E API Smoke Test (pull_request) Successful in 1m42s
Handlers Postgres Integration / Handlers Postgres Integration (pull_request) Successful in 2m14s
E2E Staging External Runtime / E2E Staging External Runtime (pull_request) Successful in 5m12s
CI / Platform (Go) (pull_request) Successful in 4m57s
CI / all-required (pull_request) Successful in 6m14s
audit-force-merge / audit (pull_request) Successful in 5s
b9a3ef4294
Addresses both review subagents' REQUEST_CHANGES verdicts on
PR #1929:

Code review (correctness)
- #1: Move schedule seeding to AFTER provisionWorkspaceAuto
  succeeds so the scheduler never fires cron rows against a
  workspace whose backend never wired. Failed-backend workspaces
  no longer end up with orphan template_schedules rows.
- #2: seedTemplateSchedules now returns (seeded, skipped int) so
  the caller can observe partial-seed states; workspace.go Create
  logs the (seeded, skipped) pair when skipped > 0, surfacing
  silent partial-loss that the prior (int) return masked.

Security review (hostile-template defenses)
- #3 / #4: parseTemplateSchedules reads config.yaml through an
  io.LimitReader bounded by maxTemplateConfigYAMLBytes (1 MiB)
  and rejects files over the cap before yaml.Unmarshal runs.
  Defends against billion-laughs / anchor-explosion DoS.
- #3: schedules slice length capped at maxTemplateSchedules (100,
  10x the largest current production grid). Hostile template with
  50k schedules now rejected at parse time, not after 50k inserts.
- #3: cron_expr length capped at maxScheduleCronExprLen (128) per
  schedule; resolved prompt body capped at maxSchedulePromptBytes
  (16 KiB) per schedule. Oversized entries are skipped (counted
  as `skipped`) so one bad row doesn't break the rest.
- #3: Seed loop honours ctx.Err() so an aborted Create request
  stops further inserts rather than running to completion on a
  dead goroutine.
- #8: Schedule names quoted via %q in all log lines so CRLF in a
  hostile name can't injection-pollute stdout/Loki.

Tests
- TestParseTemplateSchedules_RejectsOversizeFile — gate against
  the LimitReader cap (1 MiB + 1 byte of '#').
- TestParseTemplateSchedules_RejectsTooManySchedules — gate
  against the schedule-count cap (maxTemplateSchedules + 1
  minimal entries).
- Full handlers test suite still green (17.4s).

Non-fix surface
- Code-review #3 (runtime-default fallback also seeds): runtime-
  default templates do not currently ship a schedules: block so
  this is benign in practice; documented behavior in the comment.
- Code-review #4 (files_dir in workspace-template config.yaml):
  not part of the current template_registry schema; flagged for
  follow-up if templates start declaring files_dir.
- Security-review #7 (cron prompt as agent self-message escalation
  vector): out of scope per security reviewer's own note; tracked
  separately. Will file an issue.

Verified locally:
  go vet ./...                 → clean
  go build ./...               → clean
  gofmt -d <changed files>     → clean
  go test ./internal/handlers/ → PASS (7 unit tests for parser,
                                  full suite 17.4s)

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

Self-review summary

I ran a two-axis review on this PR via Claude Code subagents (code-correctness + security) before requesting platform review. Both initial verdicts: REQUEST_CHANGES. Fixes landed in b9a3ef4; current SHA is what the watcher is tracking.

Code-correctness findings (addressed)

  • #1 medium — ordering Seed ran before provisionWorkspaceAuto; failed-backend workspaces could end up with orphan workspace_schedules rows. Fix: seed only when provisionWorkspaceAuto returns true.
  • #2 medium — observability seedTemplateSchedules returned only int seeded; per-row DB errors were silent. Fix: returns (seeded, skipped int); Create logs partial-seed warnings.

Security findings (addressed)

  • #3 / #4 medium — hostile-template DoS No bound on config.yaml size, schedules length, prompt length, or cron expr length. Fix: 1 MiB io.LimitReader on the YAML file (anchor-bomb defense), maxTemplateSchedules=100, maxScheduleCronExprLen=128, maxSchedulePromptBytes=16<<10. Oversized rows are skipped, not committed; ctx.Err() honoured inside the seed loop.
  • #8 informational — log injection Schedule names now logged via %q, neutralizing CRLF in attacker-controlled names.

Findings not addressed in this PR (rationale)

  • Code-review #3 (runtime-default-template fallback also seeds) — runtime-default templates do not currently ship a schedules: block, so benign in practice; documented in the new comment block.
  • Code-review #4 (files_dir in workspace-template config.yaml) — not part of the current template_registry schema; flagged for follow-up.
  • Security #7 (cron prompt → agent self-message as privilege escalation vector) — out of scope per the security reviewer's own note; will file a separate issue.

Defenses preserved

  • Path traversal via prompt_file: routed through resolvePromptRefresolveInsideRoot; existing TestResolvePromptRef_RejectsTraversal still gates.
  • SQL injection via schedule fields: all positional $1..$7 args into canonical orgImportScheduleSQL; no string concat.
  • Issue #24 contract (additive / idempotent / preserve-runtime-rows / never-DELETE): same SQL constant; TestImport_OrgScheduleSQLShape still gates the shape.
  • Cross-workspace influence: ON CONFLICT keyed on (workspace_id, name) + WHERE source='template' — hostile names cannot mutate other workspaces.

Local CI

  • go vet ./... — clean
  • go build ./... — clean
  • gofmt -d <changed> — clean
  • go test ./workspace-server/internal/handlers/ — PASS (7 parser unit tests incl. 2 new bounds tests; full handlers suite green 17.4s)
## Self-review summary I ran a two-axis review on this PR via Claude Code subagents (code-correctness + security) before requesting platform review. Both initial verdicts: **REQUEST_CHANGES**. Fixes landed in `b9a3ef4`; current SHA is what the watcher is tracking. ### Code-correctness findings (addressed) - **#1 medium — ordering** Seed ran before `provisionWorkspaceAuto`; failed-backend workspaces could end up with orphan `workspace_schedules` rows. **Fix**: seed only when `provisionWorkspaceAuto` returns `true`. - **#2 medium — observability** `seedTemplateSchedules` returned only `int seeded`; per-row DB errors were silent. **Fix**: returns `(seeded, skipped int)`; Create logs partial-seed warnings. ### Security findings (addressed) - **#3 / #4 medium — hostile-template DoS** No bound on config.yaml size, schedules length, prompt length, or cron expr length. **Fix**: 1 MiB `io.LimitReader` on the YAML file (anchor-bomb defense), `maxTemplateSchedules=100`, `maxScheduleCronExprLen=128`, `maxSchedulePromptBytes=16<<10`. Oversized rows are skipped, not committed; ctx.Err() honoured inside the seed loop. - **#8 informational — log injection** Schedule names now logged via `%q`, neutralizing CRLF in attacker-controlled names. ### Findings not addressed in this PR (rationale) - Code-review #3 (runtime-default-template fallback also seeds) — runtime-default templates do not currently ship a `schedules:` block, so benign in practice; documented in the new comment block. - Code-review #4 (files_dir in workspace-template config.yaml) — not part of the current template_registry schema; flagged for follow-up. - Security #7 (cron prompt → agent self-message as privilege escalation vector) — out of scope per the security reviewer's own note; will file a separate issue. ### Defenses preserved - Path traversal via `prompt_file`: routed through `resolvePromptRef` → `resolveInsideRoot`; existing `TestResolvePromptRef_RejectsTraversal` still gates. - SQL injection via schedule fields: all positional `$1..$7` args into canonical `orgImportScheduleSQL`; no string concat. - Issue #24 contract (additive / idempotent / preserve-runtime-rows / never-DELETE): same SQL constant; `TestImport_OrgScheduleSQLShape` still gates the shape. - Cross-workspace influence: ON CONFLICT keyed on `(workspace_id, name)` + `WHERE source='template'` — hostile names cannot mutate other workspaces. ### Local CI - `go vet ./...` — clean - `go build ./...` — clean - `gofmt -d <changed>` — clean - `go test ./workspace-server/internal/handlers/` — PASS (7 parser unit tests incl. 2 new bounds tests; full handlers suite green 17.4s)
core-qa approved these changes 2026-05-26 23:39:08 +00:00
core-qa left a comment
Member

QA review — APPROVE

Automated review on behalf of the QA axis. Findings from a code-correctness subagent on the initial commit d8b409f:

  • ordering: seed ran before provisionWorkspaceAuto; orphan-schedule risk on failed-backend workspaces.
  • observability: per-row DB errors swallowed; partial-seed states invisible.

Both addressed in b9a3ef4:

  • seed now runs only when provisionWorkspaceAuto == true
  • seedTemplateSchedules returns (seeded, skipped int); Create logs partial-seed: seeded=N skipped=M total=K when skipped > 0
  • 2 new bounds-gate tests; full handlers suite green (17.4s)
  • TestImport_OrgScheduleSQLShape still passes — the Issue #24 SQL contract (additive / idempotent / preserve-runtime-rows / never-DELETE) is preserved because both paths reuse the same orgImportScheduleSQL constant

Note: this PR review approval is a soft signal from the QA persona — the qa-review / approved CI status check is set by a separate agent path and is not flipped by this review.

core-qa (automated, dispatched by the workspace template-schedule wiring author)

## QA review — APPROVE Automated review on behalf of the QA axis. Findings from a code-correctness subagent on the initial commit `d8b409f`: - **ordering**: seed ran before `provisionWorkspaceAuto`; orphan-schedule risk on failed-backend workspaces. - **observability**: per-row DB errors swallowed; partial-seed states invisible. Both addressed in `b9a3ef4`: - seed now runs only when `provisionWorkspaceAuto == true` - `seedTemplateSchedules` returns `(seeded, skipped int)`; Create logs `partial-seed: seeded=N skipped=M total=K` when `skipped > 0` - 2 new bounds-gate tests; full handlers suite green (17.4s) - `TestImport_OrgScheduleSQLShape` still passes — the Issue #24 SQL contract (additive / idempotent / preserve-runtime-rows / never-DELETE) is preserved because both paths reuse the same `orgImportScheduleSQL` constant Note: this PR review approval is a soft signal from the QA persona — the `qa-review / approved` CI status check is set by a separate agent path and is **not** flipped by this review. — `core-qa` (automated, dispatched by the workspace template-schedule wiring author)
core-security approved these changes 2026-05-26 23:39:08 +00:00
core-security left a comment
Member

Security review — APPROVE

Automated review on behalf of the Security axis. Threat model: workspace-template config.yaml is partially attacker-influenced (templates can be webhook-synced or uploaded via POST /templates/import).

Findings on initial commit d8b409f (REQUEST_CHANGES)

  • DoS via unbounded inputs — no caps on len(schedules), prompt body, cron length, or config.yaml file size.
  • YAML billion-laughs / anchor-bomb — no LimitReader around os.ReadFileyaml.Unmarshal.
  • Log injection — CRLF-bearing sched.Name flowing into log.Printf un-escaped.

Fixes landed in b9a3ef4

  • parseTemplateSchedules now opens the file and reads through io.LimitReader(f, 1<<20) (1 MiB hard cap); rejects oversize before Unmarshal runs.
  • Constants: maxTemplateSchedules=100, maxScheduleCronExprLen=128, maxSchedulePromptBytes=16<<10.
  • seedTemplateSchedules honours ctx.Err() inside the loop (aborted Create stops further inserts).
  • All log lines use %q for sched.Name — CRLF neutralised.
  • 2 new bounds tests: TestParseTemplateSchedules_RejectsOversizeFile, TestParseTemplateSchedules_RejectsTooManySchedules.

Defenses verified intact

  • Path traversal via prompt_file: routed through resolvePromptRefresolveInsideRoot (CWE-22).
  • SQL injection: all $1..$7 positional args into canonical orgImportScheduleSQL (CWE-89).
  • Cross-workspace influence: ON CONFLICT (workspace_id, name) DO UPDATE … WHERE source='template' — hostile names cannot mutate other workspaces.
  • Auth boundary: POST /workspaces gated by middleware.AdminAuth — seeder unreachable without admin token.

Out-of-scope, file follow-up issue

  • Template-author-controlled schedule prompts replayed as agent self-messages can be a privilege-escalation vector (CWE-94/77). Not introduced by this PR; tracked as a separate hardening item.

Note: this PR review approval is a soft signal — the security-review / approved CI status check is set by a separate agent path and is not flipped by this review.

core-security (automated, dispatched by the workspace template-schedule wiring author)

## Security review — APPROVE Automated review on behalf of the Security axis. Threat model: workspace-template `config.yaml` is partially attacker-influenced (templates can be webhook-synced or uploaded via `POST /templates/import`). ### Findings on initial commit `d8b409f` (REQUEST_CHANGES) - **DoS via unbounded inputs** — no caps on `len(schedules)`, prompt body, cron length, or `config.yaml` file size. - **YAML billion-laughs / anchor-bomb** — no `LimitReader` around `os.ReadFile` → `yaml.Unmarshal`. - **Log injection** — CRLF-bearing `sched.Name` flowing into `log.Printf` un-escaped. ### Fixes landed in `b9a3ef4` - `parseTemplateSchedules` now opens the file and reads through `io.LimitReader(f, 1<<20)` (1 MiB hard cap); rejects oversize before `Unmarshal` runs. - Constants: `maxTemplateSchedules=100`, `maxScheduleCronExprLen=128`, `maxSchedulePromptBytes=16<<10`. - `seedTemplateSchedules` honours `ctx.Err()` inside the loop (aborted Create stops further inserts). - All log lines use `%q` for `sched.Name` — CRLF neutralised. - 2 new bounds tests: `TestParseTemplateSchedules_RejectsOversizeFile`, `TestParseTemplateSchedules_RejectsTooManySchedules`. ### Defenses verified intact - **Path traversal via `prompt_file`**: routed through `resolvePromptRef` → `resolveInsideRoot` (CWE-22). - **SQL injection**: all $1..$7 positional args into canonical `orgImportScheduleSQL` (CWE-89). - **Cross-workspace influence**: `ON CONFLICT (workspace_id, name) DO UPDATE … WHERE source='template'` — hostile names cannot mutate other workspaces. - **Auth boundary**: `POST /workspaces` gated by `middleware.AdminAuth` — seeder unreachable without admin token. ### Out-of-scope, file follow-up issue - Template-author-controlled schedule prompts replayed as agent self-messages can be a privilege-escalation vector (CWE-94/77). Not introduced by this PR; tracked as a separate hardening item. Note: this PR review approval is a soft signal — the `security-review / approved` CI status check is set by a separate agent path and is **not** flipped by this review. — `core-security` (automated, dispatched by the workspace template-schedule wiring author)
hongming merged commit 9bcf9d1dfe into main 2026-05-26 23:50:42 +00:00
Sign in to join this conversation.
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#1929