Skip to content

fix(security+scheduler): IPv6 SSRF gap + scheduler unit tests [supersedes #111, #112] - #119

Merged
HongmingWang-Rabbit merged 3 commits into
mainfrom
fix/111-112-clean
Apr 15, 2026
Merged

fix(security+scheduler): IPv6 SSRF gap + scheduler unit tests [supersedes #111, #112]#119
HongmingWang-Rabbit merged 3 commits into
mainfrom
fix/111-112-clean

Conversation

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor

Summary

Why a new branch

PRs #111 and #112 were based on a stale branch (pre-#95, #99, #106) — their diffs vs. main showed -965 deletions of files added by those PRs. Both original commits were clean; this branch cherry-picks them onto a fresh base from main. Original PRs are closed.

Conflict resolutions

scheduler.go: HEAD (PR #95) added LastTickAt() and Healthy() methods; PR #111 independently added the same methods plus tickInterval. Kept HEAD's method bodies (direct mutex access, slightly leaner) and added tickInterval from PR #111. Removed the duplicated method definitions.

registry.go / registry_test.go: PR #112's branch had removed the RFC-1918 and loopback IPv4 blocks (a regression). Cherry-pick resolution: kept all HEAD IPv4 blocked ranges, added only the three new IPv6 entries. Test table updated to match: all existing wantErr: true cases preserved, four new IPv6 cases added.

Test plan

  • go test -race ./internal/scheduler/... — all scheduler tests pass including new ones
  • go test -race ./internal/handlers/...TestValidateAgentURL covers all 4 new IPv6 cases
  • go vet ./... — clean
  • CI platform-build + e2e-api green

🤖 Generated with Claude Code

Backend Engineer and others added 2 commits April 15, 2026 07:42
…un, panic recovery

Added scheduler_test.go with 8 test cases covering all previously untested
security-critical code paths from PR #90:

  TestLastTickAt_zero            — zero time before first tick
  TestHealthy_beforeStart        — false on fresh scheduler (zero lastTickAt)
  TestHealthy_freshTick          — true when lastTickAt == now
  TestHealthy_stale              — false when lastTickAt is 3×pollInterval ago
  TestComputeNextRun_valid       — "0 * * * *" / UTC returns top-of-hour future time
  TestComputeNextRun_invalid     — unparseable expression returns non-nil error
  TestComputeNextRun_invalidTimezone — unrecognised IANA zone returns non-nil error
  TestPanicRecovery              — panicProxy crashes ProxyA2ARequest; scheduler
                                   goroutine recovers and remains Healthy

To support these tests, scheduler.go gained four changes (minimal surface):

1. Added mu sync.RWMutex, lastTickAt time.Time, and tickInterval time.Duration
   fields to Scheduler. tickInterval defaults to pollInterval so production
   behaviour is unchanged; tests can override it directly.

2. Added LastTickAt() and Healthy() methods with read-lock protection.

3. tick() now records lastTickAt after wg.Wait() — a single atomic write under
   the mutex, no hot-path cost.

4. fireSchedule() got a deferred recover() so a panicking A2A proxy cannot
   crash the goroutine pool. Without this, TestPanicRecovery itself crashes
   the test binary — the test passing proves recovery is in place.

Bug fix: ComputeNextRun previously silently fell back to UTC on an invalid
timezone; it now returns a non-nil error. The schedules handler already
validates the timezone before calling ComputeNextRun so this is a no-op for
callers, but it makes the contract explicit and testable.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR #94 blocked 169.254.0.0/16 but left IPv6 equivalents fully open.
Go's (*IPNet).Contains() does not match pure IPv6 addresses against IPv4
CIDRs, so ::1, fe80::*, and fc00::/7 all bypassed the check.

Add three explicit IPv6 entries to blockedRanges:
  - fe80::/10  (IPv6 link-local — cloud metadata analogue)
  - ::1/128    (IPv6 loopback)
  - fc00::/7   (IPv6 ULA — RFC-4193 private)

IPv4-mapped IPv6 (::ffff:169.254.x.x) is already safe: Go normalises
these to IPv4 via To4() before Contains() runs.

Tests: four new cases in TestValidateAgentURL covering all three blocked
IPv6 ranges plus the IPv4-mapped IPv6 auto-normalisation path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor Author

Verified — correct fix, holding for user approval

Reviewed both parts of the diff:

IPv6 SSRF gap (the security side):

  • Adds fe80::/10 (IPv6 link-local), ::1/128 (IPv6 loopback), fc00::/7 (IPv6 ULA / RFC-4193) to the existing blocklist.
  • Comment correctly notes that IPv4-mapped IPv6 (::ffff:169.254.169.254) gets normalised to IPv4 by net.ParseIP.To4() so the existing IPv4 rules catch it — I confirmed this by reading Go's net package source. The test case blocked IPv4-mapped IPv6 link-local exercises this explicitly.
  • Error message upgraded from the generic "private/reserved IP ranges are not permitted" to a per-range label ("url targets a blocked address: IPv6 ULA address" etc.) — better for debugging client-side, no info-leak since the categories are public knowledge.

Scheduler unit tests (the reliability side):

  • Makes tickInterval overridable via the struct so tests can drive fast polls without real-time sleep loops.
  • Moves lastTickAt = time.Now() from Start() to the bottom of tick() — this is more correct (Healthy() now reflects actual completed ticks, not ticker fires).
  • Adds defer recover() to fireSchedule so a panicking A2A proxy call can't crash the whole scheduler goroutine pool (belt-and-braces on top of the tick-level recover from fix(platform): panic-recovering supervisor for every background goroutine (#92) #95).

Why I'm not merging autonomously: touches both SSRF validation (security-critical) and the scheduler core (large blast radius). Per overnight triage rules, both categories need explicit CEO approval.

CI status: same billing block as #110 — 6× FAILURE is a GitHub Actions org spending cap, not a real failure. Verified locally:

  • go test -race ./internal/handlers/TestValidateAgentURL passes all 4 new IPv6 cases
  • go test -race ./internal/scheduler/ — new scheduler tests pass

Ready for CEO merge once billing unblocks or with --admin.

(Supersedes #111 and #112 per the PR title — I haven't verified those branches myself since they're already closed.)

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor Author

🚨 CONFIRMED LIVE (audit cycle-4, 2026-04-15)

Security Auditor ran DAST against bed2f2f (current origin/main):

curl -X POST /registry/register   -d '{"id":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee","url":"http://[fe80::1]:8080","agent_card":"{}"}'
→ 200 {"auth_token":"<token>","status":"registered"}

IPv6 link-local URL bypassed validateAgentURL() entirely. Auth token issued; platform would proxy A2A to fe80::1:8080.

Filed as issue #145.

This PR has the correct fix. The blockedRanges in PR #119's registry.go covers all 8 CIDRs (5 IPv4 + 3 IPv6). Once CI (#136) recovers this must be merged immediately — this vulnerability is live on main.

@HongmingWang-Rabbit
HongmingWang-Rabbit merged commit f065744 into main Apr 15, 2026
1 of 7 checks passed
HongmingWang-Rabbit pushed a commit that referenced this pull request Apr 15, 2026
…port

Cherry-picks the one genuinely new fix from #169 after confirming the
rest of that PR is already covered on main (C1/C3/C5 by wsAuth group,
C6 by #94+#119 SSRF blocklist, C4 ownership by existing WHERE filter).

Pre-existing middleware (WorkspaceAuth on /workspaces/:id/* sub-routes)
proves the caller owns the :id path param. But the body field
source_id was never validated — a workspace authenticated for its own
/activity endpoint could still attribute logs to a different workspace
by setting source_id=<foreign UUID>. Rejected with 403 now.

No schema change, no new middleware. 4-line handler delta. Closes the
only real gap in #169; #169 itself will be closed as superseded.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit pushed a commit that referenced this pull request Apr 15, 2026
…ht sweep

Captures ~27 PRs merged across both repos this session: security
hardening cluster (#94/#99/#106/#110/#119/#162/#155/#167/#185/#200/#203/
#209/#233), data-integrity fixes (#212/#224/#236), CI runner migration
(#186), platform/scheduler reliability (#95/#149/#207/#206), workspace
runtime features (#205/#208/#198/#216/#225/#235/#231), code-review
follow-ups (#228/#232).

Updated counts: 816 Go (+70), 1180 Python (+40), 453 vitest (unchanged
— UI/a11y patches), 97 jest (unchanged).

CLAUDE.md additions:
- Idle Loop section (#205) under Architectural Patterns
- Admin auth middleware variants section linking docs/runbooks/admin-auth.md
- Migration runner section explaining the .down.sql filter (#212)
- Per-route auth notes in the API table (PATCH field-whitelist, CanvasOrBearer
  on PUT /canvas/viewport, AdminAuth on bundles/events/templates-import/
  approvals-pending/admin-liveness)
- Database section updated with workspace_auth_tokens auto-revoke (#110),
  scheduler.error_detail surfacing (#206), workspace_schedules.last_status
  'skipped' state (#207)

PLAN.md additions:
- New Recently launched (overnight sweep) section with full PR/issue index
- Phase status updated (B–G now complete, H partial)
- Live infrastructure deltas (migration fix, token rotation, legal pages)
- Outstanding items consolidated

Edit-history file expanded from the tick-9 stub to a full session record
covering malware cleanup, CI runner migration, security cluster, data
integrity, infra/feature/code-review batches, and outstanding user
actions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@HongmingWang-Rabbit
HongmingWang-Rabbit deleted the fix/111-112-clean branch April 16, 2026 12:30
molecule-ai Bot pushed a commit that referenced this pull request Apr 21, 2026
fix(security+scheduler): IPv6 SSRF gap + scheduler unit tests [supersedes #111, #112]
molecule-ai Bot pushed a commit that referenced this pull request Apr 21, 2026
…port

Cherry-picks the one genuinely new fix from #169 after confirming the
rest of that PR is already covered on main (C1/C3/C5 by wsAuth group,
C6 by #94+#119 SSRF blocklist, C4 ownership by existing WHERE filter).

Pre-existing middleware (WorkspaceAuth on /workspaces/:id/* sub-routes)
proves the caller owns the :id path param. But the body field
source_id was never validated — a workspace authenticated for its own
/activity endpoint could still attribute logs to a different workspace
by setting source_id=<foreign UUID>. Rejected with 403 now.

No schema change, no new middleware. 4-line handler delta. Closes the
only real gap in #169; #169 itself will be closed as superseded.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
molecule-ai Bot pushed a commit that referenced this pull request Apr 21, 2026
…ht sweep

Captures ~27 PRs merged across both repos this session: security
hardening cluster (#94/#99/#106/#110/#119/#162/#155/#167/#185/#200/#203/
#209/#233), data-integrity fixes (#212/#224/#236), CI runner migration
(#186), platform/scheduler reliability (#95/#149/#207/#206), workspace
runtime features (#205/#208/#198/#216/#225/#235/#231), code-review
follow-ups (#228/#232).

Updated counts: 816 Go (+70), 1180 Python (+40), 453 vitest (unchanged
— UI/a11y patches), 97 jest (unchanged).

CLAUDE.md additions:
- Idle Loop section (#205) under Architectural Patterns
- Admin auth middleware variants section linking docs/runbooks/admin-auth.md
- Migration runner section explaining the .down.sql filter (#212)
- Per-route auth notes in the API table (PATCH field-whitelist, CanvasOrBearer
  on PUT /canvas/viewport, AdminAuth on bundles/events/templates-import/
  approvals-pending/admin-liveness)
- Database section updated with workspace_auth_tokens auto-revoke (#110),
  scheduler.error_detail surfacing (#206), workspace_schedules.last_status
  'skipped' state (#207)

PLAN.md additions:
- New Recently launched (overnight sweep) section with full PR/issue index
- Phase status updated (B–G now complete, H partial)
- Live infrastructure deltas (migration fix, token rotation, legal pages)
- Outstanding items consolidated

Edit-history file expanded from the tick-9 stub to a full session record
covering malware cleanup, CI runner migration, security cluster, data
integrity, infra/feature/code-review batches, and outstanding user
actions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant