# Permission Phase 0 Security Review Findings

**Date:** 2026-04-05
**Branch:** `fix/permission-phase0-security`
**Method:** 10 parallel review agents (design vs implementation) + live API verification on nexuscrm.pingcap.com
**Design Docs:** `DESIGN.md` (853 lines), `SECURITY_REFACTOR_DESIGN.md` (1063 lines)

---

## 1. Review Methodology

### Phase 1: Agent Review (10 agents in parallel)

Each agent reviewed a specific design section against the actual codebase:

| Agent | Section | Files Reviewed |
|-------|---------|---------------|
| Object Access Classification | SECURITY_REFACTOR Section 2 | service.go, metadata.go, table_types.go, system_tables.json |
| RLS Unification | Section 3 | rls_filter.go, rls_test.go, query_service.go, security_validator.go |
| Session Lifecycle | Section 4 | auth_service.go, auth_user_mgmt.go, user_repository.go |
| SecurityContext | Section 5 | security_context.go, persistence_service.go, system_manager.go |
| Permission Resolution | Section 6 | perm_system.go, perm_object_field.go, perm_sets.go |
| Bootstrap & SharingModel | Section 7 | schema.go, metadata_scan_schema.go, enums.go |
| Route Guards | DESIGN Section 5 + Section 8.4 | routes.go, main.go |
| Record Access 7-Layer | DESIGN Section 4 | record_access.go |
| Additional Security | Section 8 | role_hierarchy.go, action_service.go, file_upload_handler.go, persistence_delete.go |
| Test Coverage | Section 14 | All *_test.go files in permissions/, auth/, analytics/ |

### Phase 2: Live API Verification

Tested against deployed production instance at `https://nexuscrm.pingcap.com` (running main branch).

**Auth method:** `client_credentials` grant with `test-gosf-client` / `test-gosf-secret-123`.

---

## 2. Findings Summary

| Severity | Count | Confirmed on Live | Needs Local Verification |
|----------|-------|-------------------|--------------------------|
| CRITICAL | 6 | 1 (C5) | 5 (C1-C4, C6) |
| HIGH | 13 | 1 (SEC-008) | 12 |
| MEDIUM | 8 | 0 | 8 |

---

## 3. CRITICAL Findings (6)

### C1: RLS RecordShare subquery uses wrong column names

- **File:** `rls_filter.go:293-298`
- **Code:** `rs.SobjectType`, `rs.UserOrGroupId`
- **Actual DB columns:** `ObjectApiName`, `ShareWithUserId`, `ShareWithGroupId` (from `system_tables.json:4305-4323`)
- **Impact:** SQL runtime error `Unknown column 'SobjectType'` for every non-admin user on Private-sharing-model objects accessing records via manual shares
- **Why unit tests missed it:** `rls_test.go` tests `BuildRLSResult` struct fields (mock layer), not the SQL string output against real DB. `RLSResultToFilterExpr` tests check pattern shapes, not column name correctness.
- **Live verification:** Not verifiable on main branch (Contact SharingModel=Private, no non-admin user provisioned). Requires PR branch deployment + sales@test.com user.

### C2: RLS TeamMember subquery uses wrong column name

- **File:** `rls_filter.go:303-304`
- **Code:** `tm.SobjectType`
- **Actual DB column:** `ObjectApiName` (from `system_tables.json:4422`)
- **Impact:** Same as C1 — SQL runtime error for team member access path
- **Why unit tests missed it:** Same as C1

### C3: CheckRecordAccess missing ControlledByParent handling

- **File:** `record_access.go:28-38`
- **Code:** OWD switch handles `PublicReadWrite` and `PublicRead` only — no `ControlledByParent` case
- **Design:** Contact should inherit access from parent Account
- **Impact:** Path A (single-record GET) vs Path B (bulk query SOQL) divergence. `BuildRLSResult` in `rls_filter.go:54-56` handles ControlledByParent correctly with cycle guard. `CheckRecordAccess` does not — Contact falls through to ownership check → access denied even when user has Account access.
- **Violates design invariant:** `R visible in SOQL ⟺ CheckRecordAccess(U, R, Read) == true`
- **Why unit tests missed it:** ControlledByParent tests exist for the bulk query path (`rls_test.go:311`) but not for the single-record path (`record_access.go`).
- **Live verification:** Contact SharingModel=Private on deployed main (not ControlledByParent). PR branch's `defaultSharingModel()` not deployed yet.

### C4: DeleteUser does not revoke sessions

- **File:** `auth_user_mgmt.go:194-211`
- **Code:** `DeleteUser()` calls `userRepo.DeleteUser()` but NOT `sessionRepo.RevokeAllSessionsForUser()`
- **Contrast:** Deactivation path at line 182 correctly calls `RevokeAllSessionsForUser`
- **Impact:** Deleted user's JWT remains valid until expiry. `ValidateSession` doesn't check user existence.
- **Why unit tests missed it:** `TestDeleteUser` (auth_user_mgmt_test.go:337) tests happy path (exist→delete) and error paths, but never asserts that `RevokeAllSessionsForUser` was called. Compare with deactivation tests in `session_lifecycle_test.go` which do assert revocation.
- **Live verification:** Requires creating/deleting temp user — not safe on production.

### C5: pprof debug endpoints have zero authentication

- **File:** `main.go:330-347`
- **Code:** `debug := router.Group("/debug/pprof")` — no middleware arguments
- **Additional bug:** `requireAuth` is initialized at line 350, AFTER pprof registration at line 333
- **Impact:** Anyone can access `/debug/pprof/heap` and get full memory dump containing tokens, DB credentials, user data
- **CONFIRMED ON LIVE:**
  ```
  $ curl -s https://nexuscrm.pingcap.com/debug/pprof/heap -o heap.bin
  $ wc -c heap.bin
  1513 bytes   (HTTP 200, no auth required)
  ```
- **Why unit tests missed it:** Zero test coverage for pprof/debug routes

### C6: File download has zero authorization

- **File:** `file_upload_handler.go:136-188`
- **Code:** `Download()` validates path traversal/extension/UUID format, then directly serves file. No `CheckObjectPermission`, no user session check.
- **Live verification:** Unauthenticated request returned 401 (route-level `requireAuth` middleware catches it). However, any authenticated user can download any file regardless of object permissions — the handler never checks object-level access.
- **Why unit tests missed it:** Zero test coverage for file download

---

## 4. HIGH Findings (13)

### H1: hasAnySystemPermission grants SuperUser bypass on BLOCKED sentinel

- **File:** `service.go:122-123`
- **Code:** Returns `true` for any `IsSuperUser` user regardless of permission requested, including `__BLOCKED__`
- **Current safety:** `CheckObjectPermissionWithUser` (line 218-221) checks BLOCKED before SuperUser, so the bypass path is unreachable TODAY
- **Risk:** Any future caller of `hasAnySystemPermission` with BLOCKED permission + SuperUser session silently grants access
- **Fix:** 3-line guard clause at top of `hasAnySystemPermission`

### H2: checkSetupObjectAccess not implemented as designed

- **File:** `service.go:92`
- **Design:** Single `checkSetupObjectAccess(ctx, schema, operation, user) error` function
- **Actual:** Retains old split pattern `requiredSystemPermissionsForSetupObject` (returns `[]string`) + caller orchestrates BLOCKED check separately
- **Root cause of H1:** The split pattern allows BLOCKED sentinel to leak to functions that don't respect it

### H3: ValidateSession doesn't check user lockout/active status

- **File:** `auth_service.go:272-293`
- **Code:** Checks JWT signature + session exists + not revoked. Does NOT check `IsActive` or `LockedUntil`.
- **Risk:** If `RevokeAllSessionsForUser` fails (errors logged but non-fatal at lines 133, 158, 183), locked/deactivated users retain access indefinitely.

### H4: Boolean-column path PSG components bypass muting

- **File:** `perm_system.go:127-187`
- **Code:** `getPermissionSetColumnSystemPermissions` 3rd UNION includes PSG component PermSets without per-PSG muting
- **Contrast:** Normalized path (`getGroupSystemPermissions` at line 264-344) correctly applies per-PSG muting
- **Impact:** MutingPermissionSet ineffective for boolean-column system permissions

### H5: EnableHierarchySharing hardcoded to true

- **File:** `metadata_scan_schema.go:76`
- **Code:** `obj.EnableHierarchySharing = true` (changed from `false` but still not read from DB)
- **Design:** Should read from DB column with `obj.EnableHierarchySharing = enableHierarchy.Valid && enableHierarchy.Bool`
- **Impact:** Admins cannot disable hierarchy sharing per-object

### H6: Role hierarchy grants unconditional R+E+D

- **File:** `record_access.go:99-104`
- **Code:** When `isAbove` is true, returns `true` unconditionally for any operation
- **Design:** Should be "bounded by user's object permissions" — a manager lacking Delete permission should not be able to delete subordinate records

### H7: UpdateRole missing indirect cycle detection

- **File:** `service.go:580-605`
- **Code:** Only prevents self-reference (`parentRoleID == id`), not indirect cycles (A→B→A)
- **Note:** `GetRoleDescendants` and `GetRoleAncestors` in `role_hierarchy.go` have cycle detection during traversal, but `UpdateRole` doesn't prevent creating cycles in the first place

### H8: Delete restriction uses hardcoded list

- **File:** `persistence_delete.go:35-41`
- **Code:** `[]string{"user", "profile", "objectmetadata", "fieldmetadata"}`
- **Design:** Should use `schema.TableType` metadata-driven check

### H9: Auth endpoints lack dedicated rate limiter

- **File:** `routes.go:47,50`
- **Code:** `/token` and `/google/token` share global 100 req/s limiter
- **Design:** Dedicated 5 req/s limiter
- **CONFIRMED ON LIVE:**
  ```
  10 rapid invalid requests → all returned 400, never 429
  ```

### H10: RLSResultToASTCondition not implemented

- **File:** `security_validator.go:204-211`
- **Design:** Direct AST construction from `RLSResult` fields
- **Actual:** String→AST roundtrip via `parseFilterExprToAST` (fragile)

### H11-H13: Test coverage gaps

- **H11:** 5 design-required tests completely missing (including SQL injection prevention test `TestRLSResultToFilterExpr_EscapesSQLStrings`)
- **H12:** 25 tests are `t.Skip` (20% of design verification suite), including all muting scope, immutable field, and defaultSharingModel tests
- **H13:** Subordinate hierarchy test (`rls_test.go:231`) doesn't actually test subordinate expansion — mock returns empty slice, but the mock infrastructure supports it

---

## 5. MEDIUM Findings (8)

| # | Issue | File |
|---|-------|------|
| M1 | `isSecurityInfrastructure` hardcoded safety net not implemented | Design Section 2.2 |
| M2 | `IncrementFailedAndLockIfNeeded` read-back non-atomic + error swallowed | `user_repository.go:466-479` |
| M3 | Raw integer comparison instead of named method (`< SystemContext` vs `BypassesCRUDandFLS()`) | `persistence_service.go:114` |
| M4 | `defaultSharingModel` uses raw strings instead of typed constants | `schema.go:418-433` |
| M5 | `/metrics` and `/swagger/*` endpoints have no authentication | `main.go:301,304` |
| M6 | `accessLevelAllowsOperation` doesn't handle Delete access level | `record_access.go:162-172` |
| M7 | `warmPermissionsAsync` goroutine missing timeout | `oauth_handler.go:51` |
| M8 | GroupMember index column order reversed for RLS query pattern | `system_tables.json:3824-3831` |

---

## 6. Why Unit Tests Missed CRITICAL Issues

| CRITICAL | Test Exists? | Why It Missed |
|----------|-------------|---------------|
| C1+C2 (RLS column names) | `rls_test.go` tests BuildRLSResult struct | Tests mock layer, never hit real DB. SQL string tests check patterns, not column names. |
| C3 (ControlledByParent) | `rls_test.go:311` for bulk path only | Single-record path (`record_access.go`) has zero ControlledByParent test |
| C4 (DeleteUser session) | `auth_user_mgmt_test.go:337` | Tests happy/error paths but never asserts `RevokeAllSessionsForUser` called |
| C5 (pprof) | None | Zero test coverage for debug routes |
| C6 (file download) | None | Zero test coverage for file download |

**Root cause pattern:** Unit tests with mocks verify internal logic correctness but cannot catch:
1. Column name mismatches between code and DB schema
2. Missing functionality (no ControlledByParent case in switch)
3. Missing side effects (no revoke call in DeleteUser)
4. Missing middleware (no auth on route group)

**Solution:** Scenario tests (`tests/contract/scenarios/api/security_*_test.go`) that hit the real system with multiple user roles.

---

## 7. Live API Verification Results

**Server:** `https://nexuscrm.pingcap.com` (main branch, not PR branch)
**Auth:** `client_credentials` with `test-gosf-client`

| Test | Command | Result |
|------|---------|--------|
| C5: pprof | `curl /debug/pprof/heap` (no auth) | **200 OK, 1513 bytes heap dump** |
| SEC-008: rate limit | 10 rapid invalid token requests | **All 400, no 429** |
| Contact SharingModel | SOQL query _System_Object | `Private` (main branch, PR not deployed) |
| Account SharingModel | SOQL query _System_Object | `Private` |
| Account TableType | SOQL query _System_Object | `system_metadata` (incorrect, should be `standard_object`) |
| _System_Session POST | Admin POST | Returns frontend HTML (SPA catch-all, not API) |
| File download (no auth) | GET ContentVersion/.../VersionData | 401 (route-level auth works) |

**Key observation:** The deployed main branch has NOT received the Phase 0 changes (SharingModel not per-object, TableType incorrect). C5 and SEC-008 are pre-existing issues confirmed on production.

---

## 8. Scenario Tests Created

10 new scenario tests registered in `scenarios.yaml` under `SEC-*` prefix:

| ID | Test | File | What It Catches |
|----|------|------|-----------------|
| SEC-001 | Manual share RLS column names | security_rls_test.go | C1 |
| SEC-002 | Team member RLS column names | security_rls_test.go | C2 |
| SEC-003 | ControlledByParent single-record | security_rls_test.go | C3 |
| SEC-004 | Deleted user session revoked | security_session_test.go | C4 |
| SEC-005 | pprof requires auth | security_route_test.go | C5 |
| SEC-006 | File download requires permission | security_route_test.go | C6 |
| SEC-007 | BLOCKED tables deny SuperUser | security_route_test.go | H1 |
| SEC-008 | Auth rate limit | security_route_test.go | H9 |
| SEC-009 | Bulk vs single-record consistency | security_rls_test.go | C3 invariant |
| SEC-010 | Role hierarchy cycle rejected | security_route_test.go | H7 |

**Scenario doc:** `tests/scenarios/security-permission-phase0.md` (with curl reproduction commands)

---

## 9. Positive Findings (cross-agent consensus)

- SecurityContext four-tier system fully implemented, `ContextKeyInternalWrite` and fake admin sessions eliminated
- `BuildRLSResult` 7-layer model well-structured, nil schema → deny-all correct
- Permission Resolution fixes (PSG UNION, spurious JOIN, cache invalidation) all landed correctly
- Route guards (PermApiEnabled, RunReports) correctly added
- Session lifecycle: all 4 revocation triggers implemented (password change self/admin, lockout, deactivation)
- Old code (`buildRLSFilter`, `applyRLSCondition`) properly deleted
- Test infrastructure (mock system) well-designed

---

## 10. Recommended Fix Priority

1. **C5** (pprof auth) — production vulnerability, 5 min fix
2. **C1+C2** (RLS column names) — 5 min fix, blocks all non-admin RLS
3. **C4** (DeleteUser revoke) — 1-line addition
4. **C3** (ControlledByParent in CheckRecordAccess) — port existing pattern from `rls_filter.go`
5. **C6** (file download permission check) — inject permission service
6. **H1** (BLOCKED guard) — 3-line guard clause
7. **H4** (boolean-column muting bypass) — remove PSG UNION from boolean query
8. **H9/SEC-008** (auth rate limit) — production confirmed, moderate effort