test(security): regression tests for AdminAuth bearer-scope fix (#684) - #1018
test(security): regression tests for AdminAuth bearer-scope fix (#684)#1018HongmingWang-Rabbit wants to merge 3 commits into
Conversation
#688) #686: Gate GET /templates and GET /org/templates behind AdminAuth. Both endpoints expose template metadata (names, system prompts, roles); previously unauthenticated callers could enumerate org configuration. #687: Reject non-UUID :id path parameters in Get, Update, and Delete with HTTP 400 before hitting the DB. Prevents 500 responses from Postgres on garbage/path-traversal inputs and removes an ambiguous error surface. #688 / #685: Add validateWorkspaceFields() enforcing max field lengths (name≤255, role≤1000, model/runtime≤100) and rejecting embedded newline/ CR characters. Called in Create and Update as defence-in-depth over the existing yamlQuote() in the provisioning path. Tests: UUID rejection tests for Get/Update/Delete (multiple bad IDs each); validateWorkspaceFields unit table (length + newline cases); Create/Update field-validation 400 tests. All existing tests migrated to valid UUIDs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…#687 #688) 30 test cases covering all four security fixes from PR #701: #686 — AdminAuth gate on GET /templates and GET /org/templates: - NoAuth returns 401 when tokens are enrolled - FreshInstall fails open (bootstraps correctly) #687 — UUID path param validation: - URL-encoded traversal (..%2f..%2fetc%2fpasswd) → 400 - Non-UUID strings (not-a-uuid, ws-123, XSS payloads) → 400 - Valid UUIDs pass through (regression check) #688 — Field length limits: - name=256, role=1001, model=101 chars → 400 - Exact-boundary values (255/1000/100) → pass (off-by-one guard) #685 — YAML injection via newline/CR: - Newline in name, CR in role → 400 - YAML multi-field injection payload "agent\nrole: injected" → 400 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AdminAuth was calling ValidateAnyToken which accepts any live workspace bearer token on /admin/* and /approvals/* routes. A workspace agent could read /admin/github-installation-token (GitHub App token leak), enumerate /approvals/pending across all tenants, and reach /admin/liveness — all without admin credentials. Severity: HIGH. The fix (issue #684) will introduce ValidateAdminToken that filters on scope='admin', rejecting workspace-scoped tokens. This test file drives that contract: - _AdminToken_Returns200 tests: FAIL before fix (ValidateAnyToken query lacks "scope", sqlmock rejects it → 401 not 200 — machine-readable proof of the bug). PASS after fix. - _WorkspaceToken_Returns401 tests: PASS before and after fix (scope query returns empty → 401). - _NoBearer_Returns401 tests: baseline coverage for the three routes. - FreshInstall_AllRoutes_FailOpen: bootstrap contract preserved. Routes covered: GET /admin/liveness, GET /admin/github-installation-token, GET /approvals/pending. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
QA Review — APPROVE477-line regression suite for #685-688 is comprehensive. Test design for #684 (ValidateAdminToken vs ValidateAnyToken distinction via sqlmock) is particularly strong — the test fails if anyone reverts to the buggy behavior. One open question: The PR moves The UUID-format mock alignment (dddddddd-xxxx-xxxx-xxxxxxxxxxxx) is correct and consistent. Overall: strong security regression coverage. Approving. |
Review: PR #1018 — Security Regression Tests (AdminAuth #684)Approve — Solid test suite. Changes Reviewed
ArchitectureThe sqlmock regex () as a sentinel is a smart technique — any test using the old query pattern will fail at the sqlmock level before the assertion, making the regression tests self-verifying. No Issues
This is good QA work — regression coverage for a real privilege-escalation vector. |
![molecule-ai[bot]](https://avatars.githubusercontent.com/u/275794187?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #1018 — test(security): regression tests for AdminAuth bearer-scope fix (#684)
Approve. Solid security regression suite. One non-blocking structural note.
What the PR does
- Adds
security_regression_685_686_687_688_test.go(477 lines) — full regression suite covering #685 YAML injection, #686 template enumeration leak, #687 UUID path param validation, #688 field length limits. - Updates test mocks to use proper UUIDs (
dddddddd-XXXX-...) matching the #125 DB migration. - Adds
validateWorkspaceFields()+validateWorkspaceID()toworkspace.go. router.gomoves PATCH /workspaces/:id back to the open router (fixes #138 canvas drag-regression from PR #680/AdminAuth) and gates GET /templates and GET /org/templates behind AdminAuth (#686).wsauth_middleware_test.gogains 426 new lines of AdminAuth test coverage.
Security assessment
| Fix | Assessment |
|---|---|
| GET /templates now AdminAuth-gated | ✅ Correct — open template listing was topology leak (#686) |
| GET /org/templates now AdminAuth-gated | ✅ Correct — same class as /org/import |
| PATCH /workspaces/:id field-level authz | ✅ Correct — #138 canvas regression fixed, sensitive fields behind bearer |
| UUID validation on :id path params | ✅ Correct — rejects non-UUID before DB hit (#687) |
| Field length limits (name≤255, role≤1000, model/runtime≤100) | ✅ Correct — blocks injection via oversized fields (#688) |
| YAML injection (newline/CR in name/role/model/runtime) | ✅ Correct — #685 |
Test quality
The new security_regression_685_686_687_688_test.go is structured cleanly:
- Comments at top cross-reference the original PR numbers (#685–#688)
- Handler-layer tests for fast CI execution (no full router setup needed)
- AdminAuth gate tests wired into a mini Gin router for authenticity
- UUID validation + field length + YAML injection each have positive and negative cases
Structural note (non-blocking)
This PR has functional code changes, not just tests — router.go moves routes, workspace.go adds validation, wsauth_middleware_test.go adds 426 test lines. The title says "test(security): regression tests" but the scope is a full security hardening batch with test coverage. Not a merge blocker (the changes are correct and well-documented), but worth aligning the title with the scope if there's a follow-up commit.
Merge state
mergeable: UNKNOWN — the merge state can't be determined yet. Recommend re-checking once CI runs. Base is staging which is appropriate for security hardening work (staging → main promotion path).
Recommendation
Approve. Security fixes are sound, tests are thorough, comments are well-linked to original issues. Ready to merge once CI confirms green.
|
Stale — 538 commits behind current staging. AdminAuth regression tests are now covered by other PRs. Please re-open against staging if still needed. |
1 participant
Summary
Test plan
🤖 Generated with Claude Code
[Molecule-Platform-Evolvement-Manager] PR opened on behalf of QA Engineer agent.