# CODEBASE-REVIEW — @servicelabsco/slabs-access-manager

## Snapshot

- **Branch:** `main`
- **HEAD:** `983e5aa5ea1714c98d8ca1074b124d5294fc90d3` (Merge PR #343, custom-field update endpoint)
- **Worktree:** clean at audit start
- **Version:** 1.0.73
- **Scope:** first-party TypeScript under `src/` — 1,265 non-migration files + 265 migration files; `test/` e2e; root config. `node_modules`, `dist`, `coverage`, `graphify-out` out of scope (generated/third-party).

## Repository map → audit buckets

| Bucket | Scope | Files (approx.) |
|---|---|---|
| B1 access-api | `src/access/controllers`, `src/access/services` | 114 |
| B2 access-usecases | `src/access/libraries`, `src/access/jobs`, `src/access/commands` | 235 |
| B3 access-data | `src/access/entities`, `src/access/subscribers`, `src/access/dtos`, `src/access/enums` | 422 |
| B4 access-utility | `src/accessUtility/**` | 200 |
| B5 access-workflow | `src/accessWorkflow/**` | 86 |
| B6 auth-boundary | `src/access/middlewares`, `src/development/middlewares`, `src/auth.controller.ts`, `src/login.service.ts`, `src/app.module.ts` | ~15 |
| B7 mcp | `src/mcp/**` | ~135 |
| B8 platform | `src/development/**` (minus middlewares), `src/config/**`, `src/main.ts`, `src/cli.ts`, `src/console.ts`, `src/app.controller.ts`, `src/app.service.ts`, `src/migrations` (spot-check) | ~300 |
| B9 package-quality | `package.json`, tsconfig(s), `es6.classes.ts` wiring completeness, jest config, `test/`, export surface | ~10 + cross-checks |

Every first-party `src/` file belongs to exactly one of B1–B8; B9 is cross-cutting metadata.

## Contracts and boundaries (established Stage 1)

- Publishable npm package **and** runnable NestJS server; public surface = dist exports, controllers, DTOs, entities (CLAUDE.md / AGENTS.md).
- Auth prefixes: `api/*` (JWT + business), `internal/*` (server-to-server), `v1/*` (API key + idempotency), `ai-server/*` (client-connect), `mcp` (soft gate + rate limit), `mcp/admin` + `development/*` (DevelopmentMiddleware). `Auth.user()` canonical principal; tenant = `Auth.user().auth_attributes.business_id`.
- Two Postgres datasources (`default`, `read`), optional Mongo (`MONGO_URL`), BullMQ, Redis WebSocket adapter.
- `es6.classes.ts` barrels are the DI/TypeORM registry — unregistered classes silently don't run.
- **Stage-1 flag (to verify in B9):** `package.json` `main: dist/index.js` but no `src/index.ts` exists; `dist/index.js` present only as a stale artifact and `nest build` uses `deleteOutDir: true`.

## Findings

Nine parallel read-only audits completed; findings below are deduplicated and reconciled. IDs are stable: **S**=security, **B**=bug/correctness, **A**=architecture/release, **E**=efficiency/quality. "Buckets" lists every audit that independently reported the issue (≥3 ⇒ **systemic**). Lead verified S1, S2, S3, S9, and A1 directly against source.

### Systemic (reported by ≥3 buckets)

**S1 — SQL injection across the listing/find/report framework — CRITICAL.** Free-text `str`/`search`, `order`, `limit`, and `ids` are string-interpolated into SQL with no binding or escaping; the shared `SqlService.guardQuery` only blocks a narrow "high-confidence" set (`pg_sleep`, `pg_read_file`, long hex) and does **not** stop quote-breakout, `UNION SELECT`, `--`, or `;`. Verified at `src/access/libraries/process.db.find.ts:75,90,95,107`. Same pattern in `process.common.list.ts` (ORDER BY via `getOrder`), `process.report.data.ts:185,190,208,274`, `process.dashboard.report.data.ts:139,190`, `accessWorkflow/libraries/process.authority.delegation.list.ts:90`, `process.limit.config.list.ts:71`. Reachable from 22+ authenticated `api/*` controllers (B1) plus B4/B5 endpoints; `business_id` tenant scoping is AND-ed and thus bypassable once a `UNION`/`OR` is injected → cross-tenant read of secrets. Buckets: B1, B2, B4, B5. Root fix: parameterize `str`/`limit`/`ids` in the shared processors via `SqlService.boundRead`, whitelist `order` columns with `isSafeOrderBy`.

**S2 — Raw-SQL passthrough DTO fields `filter_query`/`injected_query`/`clauses` — CRITICAL.** These are declared `@Expose() @IsOptional()` on `CommonListFilterDto` (so `whitelist:true` keeps them) and concatenated verbatim into report SQL WHERE clauses (`process.report.data.ts:190,208`; `process.dashboard.report.data.ts:139,190`) with no guard — unlike `listing.service.ts:344`/`process.listing.page.query.ts:21`, which DO call `assertSafeFilterQuery`. Worse, `dashboard.report.controller.ts:42,61` bind `@Body() body: any`, bypassing the DTO entirely. Payload `{"filter_query":"1=1 union select client_secret,client_id from bz_user_secrets --"}` exfiltrates secrets. Buckets: B1, B3, B5 (and cross-noted by B7 for the MCP report tools). Fix: remove these from the client DTO or force every consumer through `assertSafeFilterQuery`; stop binding report bodies as `any`.

**S3 — Unsandboxed `eval()` of stored scripts (RCE) — HIGH.** Business/email-rule/event-trigger/bulk/PDF scripts run through bare `eval` (or utility `CodeEvaluator.execute` = `await eval`) in-process with full `require`/`process`/DB access, no sandbox. Sites: `access/libraries/execute.business.script.ts:30`, `evaluate.email.rule.ts:53`, `jobs/capture.event.trigger.job.ts:114,153`, `accessUtility/libraries/read.xls.file.ts:645`, `process.manual.pdf.document.ts:38`, `accessUtility/services/pdf.document.service.ts:99`. Whoever can author a script (see S6/S7 for how a plain business user can) gets cross-tenant RCE on the shared worker. Buckets: B2, B4. Fix: run in `isolated-vm` with a frozen minimal context + timeout; gate script authorship to trusted operators.

**S4 — `X-Forwarded-For` spoofing defeats every IP-based control — HIGH.** Security decisions read the raw, client-controlled `x-forwarded-for` header (leftmost entry) instead of the framework-computed `req.ip` (which is correct under `trust proxy=1`). A stolen IP-restricted API key is usable from anywhere by sending `X-Forwarded-For: <allowed ip>`; the dev-console IP allowlist and the anonymous MCP rate-limit bucket fall the same way. Sites: `external.access.middleware.ts:19`, `mcp.access.middleware.ts:22`, `mcp.rate.limit.middleware.ts:75`, `development/middlewares/development.middleware.ts:52`, `credential.resolve.service.ts:122`, enforced in `api.account.service.ts:30`/`user.business.access.service.ts:33`. Buckets: B6, B7, B8. Fix: derive client IP from `req.ip` with a known proxy hop count everywhere; never parse raw XFF for auth.

### Security — single/dual bucket

**S5 — Unauthenticated root ops endpoints; `/clean-jobs` wipes all queues — CRITICAL.** Verified: `app.controller.ts` is `@Controller()` (root, no prefix); `RestrictedMiddleware` is mounted only on `api/*`, and the global Jwt/Basic middlewares are parse-only. So `GET /clean-jobs` (`:102`) drops all delayed/waiting/active/completed/failed BullMQ jobs (silent data loss + DoS), `GET /queue` + `/failed-jobs` leak job payloads (business data), `POST /sets` writes a DB row — all anonymous. Buckets: B6, B8. Fix: move under `internal/*` or add an admin guard on `AppController`; make `/clean-jobs` POST; delete dead `/set`,`/sets`.

**S6 — Device-auth approval mints a personal key for a non-member business — HIGH.** Verified `device.auth.service.ts:150-158`: `resolveBusinessForApprove` loads the business by client-supplied `business_id` (`device.decide.dto.ts:20`) with **no** `assertActiveMembership` — the OAuth sibling (`oauth.as.service.ts:322`) does check. An authenticated user approves a device login with an arbitrary `business_id`, minting a `bz_user_secrets` key scoped to a tenant they don't belong to; downstream services trust `auth_attributes.business_id`. Buckets: B1, B3. Fix: call `assertActiveMembership(user.id, businessId)` in both `resolveBusinessForApprove` and `mintPersonalKey`.

**S7 — Any business user can create/overwrite GLOBAL executable form/menu config — HIGH.** (a) `form.script.service.ts:86-89` explicitly exempts `business_id === null` from its ownership guard, so a business user posts `{business_id:null, script:...}` and injects a global form script that runs in every tenant's form lifecycle (compounds S3). (b) `add.menu.dto.ts:13`/`add.module.dto.ts:13` require a client `business_id` that `process.menu.creation.ts:15` writes verbatim, with only `validateAccess()` (no cross-check against the session business) — cross-tenant menu/module write, and updates aren't business-scoped either. Buckets: B1, B3. Fix: derive `business_id` from the resolved session business; reject `null`/foreign ids from the business API; scope update lookups.

**S8 — Plaintext OAuth tokens & webhook secrets serialized by default — HIGH.** `slack.integration.entity.ts:19`, `business.email.entity.ts:37-40`, `business.app.integration.credential.entity.ts:18-21`, `business.webhook.entity.ts:22`, `fcm.token.entity.ts` hold `access_token`/`refresh_token`/`secret`/`token` with no `@Exclude({toPlainOnly:true})` or `select:false`, so `CommonEntity.toJSON()` emits them. `GET /api/b/business-email/:id` returns a live Gmail refresh token to any business member; they also ride inside job payloads. Bucket: B3. Fix: `@Exclude` the columns (base `UserEntity.password` already does this).

**S9 — Predictable API credentials from `Math.random()` — HIGH.** `create.user.business.secret.ts:107,162` builds `client_id`/`client_secret` via utility `generateRandomAlpha` → `Math.random()` (not CSPRNG, seed-recoverable) and force-lowercases, shrinking a 32-char secret to 26^32. These authenticate `v1/*` and personal-token access. Bucket: B2. Fix: `crypto.randomBytes(...).toString('base64url')`; stop lowercasing. (Contrast: `oauth.pkce.ts` correctly uses `crypto`.)

**S10 — Rotated user-business secret stored in plaintext, then auth breaks — HIGH (bug+security).** Hashing lives only in the subscriber's `beforeInsert`; the MCP-key rotation path `create.user.business.secret.ts:69-71` reassigns `client_secret` and calls `existing.save()` (an UPDATE → no `beforeUpdate` hook), so the raw secret is persisted and later `Hash.compare(plaintext, plaintext)` fails. Re-authorizing an existing client silently bricks the key while leaking it at rest. Bucket: B3. Fix: add a `beforeUpdate` re-hash guarded by `Hash.needsRehash`.

**S11 — `v1/*` REST surface ignores `read_only`/`access_scopes` — HIGH.** The MCP transport enforces scope (`McpScopePolicy`), but the equivalent REST transport sharing the same credentials does not: `listing.controller.ts` is dual-mounted on `api/b/listing-page` AND `v1/b/listing-page` and exposes `@Post(':slug')`, `@Delete(':slug/:id')`, `@Post(':slug/hard-reset-columns')`. A key minted `reporting:read` (→ `read_only`) can `DELETE /v1/b/listing-page/:slug/:id`. Bucket: B6. Fix: enforce `read_only`/scope in a `v1/*` guard, or stop dual-mounting mutating routes under `v1`.

**S12 — Unauthenticated `POST /webhook/html-to-pdf` → SSRF + cross-tenant overwrite — HIGH.** The `webhook/*` prefix has only parse-only global auth. `common.webhook.controller.ts:93` → `pdf.document.service.ts:34` trusts caller `document_id` (sequential, enumerable) and `pdf_url`, then server-side-fetches `pdf_url` (`uploadFromUrl`) and overwrites any tenant's `pdf_url`. Enables SSRF to cloud metadata and IDOR document replacement. Buckets: B4, B6 (B6 rated medium; reconciled up to HIGH for the SSRF+IDOR combination). Fix: authenticate via signed callback token/HMAC (constant-time), allowlist the fetch host, verify `document_id` awaits this callback.

**S13 — Hardcoded long-lived JWT bearer token committed in source — HIGH.** `accessUtility/services/file.upload.service.ts:134` hardcodes an `exp:2036` bearer for the unzip Lambda (replacing a `propertyService.get(...)` lookup); it ships in the published `dist/`. Bucket: B4. Fix: restore the property/env lookup; revoke and rotate the token; add a CI grep guard for `eyJ…` literals.

**S14 — MCP scope fails OPEN; API accounts can't be read-only — MEDIUM.** `mcp.execution.context.ts:44-50` maps an absent/empty scope to `MCP_FULL_SCOPE`, and `credential.resolve.service.ts:106` hardcodes `['mcp']` for every `api`-kind credential — so a user-secret minted without `access_scopes`, or any business API account, silently gets write tools. Bucket: B7. Fix: default absent scope to `reporting:read`/deny; honor a per-account scope column.

**S15 — Forgeable Slack OAuth `state` (CSRF/account-linking) — MEDIUM.** `process.slack.integration.ts:80-90` treats `state` as plaintext `"<businessId>,<userId>"` (sequential ids), unsigned and not session-bound, then binds the Slack token to those ids. Bucket: B2. Fix: HMAC-signed/random single-use nonce persisted against the initiating session.

**S16 — SSRF via user-configured outbound URLs — MEDIUM.** Business-supplied webhook/notification/bulk-upload URLs are fetched server-side with no host allowlist or private-range block: `send.webhook.request.ts:64`, `send.slack.webhook.notification..ts:47`, `send.gchat.webhook.notification.ts:44`, `bulk.upload.service.ts:25` (client `document_url`) → `read.xls.file.ts` fetch, `data.access.service.ts:28` (also raw-SQL interpolation). Buckets: B2, B4. Fix: scheme+host allowlist, block loopback/link-local, parameterize the `data.access` query.

**S17 — Idempotency & ORDER-BY / listing mass-assignment gaps — MEDIUM.** (a) `IdempotencyMiddleware` has no in-flight lock — two concurrent identical mutating requests both execute (B6). (b) `listing.controller.ts:124-172` `setDefaultColumn`/`hardResetColumns` take `@Body() body: any` (unwhitelistable) and rewrite global (non-tenant-scoped) `ListingColumnEntity`, with a floating unawaited `save()` (B1). Buckets: B1, B6. Fix: typed DTOs + admin gate + await; add an idempotency in-flight lock.

**S18 — Secrets/PII to stdout; `credential` hash & unencoded paths — LOW.** `login.service.ts:16,23,26` and `auth.controller.ts:23` `console.log` the full user/token; `api.account.controller.ts:81` returns the `credential` column (reconciled: it's bcrypt-hashed per B3, so leak is of a hash — LOW, still gate it); `read.xls.file.ts:36,80` debug logs; 3 MCP admin catalog tools interpolate `table_name` into a loopback path without `encodeURIComponent`. `PG_DB_LOGGING=true` writes SQL+params (PII) to a file. Buckets: B1, B4, B6, B7, B8.

### Architecture / release

**A1 — Repo cannot reproduce its own published package; next publish ships a broken `main`/`types` — CRITICAL.** Verified: no `src/index.ts` (nor any sub-barrel) exists, yet `package.json` `main:dist/index.js`, and `nest build` runs `deleteOutDir:true`. Barrels are produced by an uninstalled, undocumented external generator (`slnu syncClassess`); `src/index.ts` was deleted as collateral in commit `dfef6c3`. Published 1.0.73 is intact (built from untracked local files, now gone), but a fresh `npm run build && npm publish` emits no `index.js` → every downstream `require()` fails MODULE_NOT_FOUND and TS builds break. Bucket: B9. Fix: commit the generated barrels (or add a `prebuild`/`prepublishOnly` generation step) and add a CI gate `npm run build && node -e "require('./dist/index.js')"`.

**A2 — Runtime dependencies live in devDependencies or are absent — HIGH.** Shipped `dist/**` requires `typeorm` (339×), `@nestjs/common` (334×), `class-transformer`/`class-validator`, `@nestjs/core` — all in devDependencies — plus `jsonwebtoken`, `helmet`, `express-rate-limit`, `pg`, `typeorm-naming-strategies`, `dotenv`, `handlebars`, `rxjs`, `reflect-metadata`, `@nestjs/bullmq`, `@nestjs/config` that are **absent** entirely. Works today only via transitive hoisting from `@servicelabsco/nestjs-utility-services`; pnpm/npm-dedupe/dep-cleanup breaks consumers at require-time, and split `typeorm` copies would break decorator/`instanceof` registries. Bucket: B9. Fix: move single-instance-critical packages to `peerDependencies`, the rest to `dependencies`; add `publint`/require-walk in CI.

**A3 — Dead & divergent module `SomethingModule` on the public surface — MEDIUM.** `accessUtility/accessUtility.module.ts` exports `class SomethingModule` (wires only `TypeOrmModule.forFeature`, missing Auth/Platform/forwardRef), is imported nowhere, but the published barrel re-exports it — a consumer mounting it gets accessUtility controllers with no auth. Buckets: B4, B9. Fix: delete the file.

**A4 — `ReportingCatalogGate` never registered as a provider — MEDIUM.** `development.module.ts:14` spreads services/registries/commands but omits `es6Classes.gates`, so the gate's `onModuleInit` (demotes invalid agent-visible catalog views at boot) never runs — the documented safety net is dead code. Bucket: B8. Fix: add `...es6Classes.gates` to `providers`.

**A5 — `engines: node>=24.11.0` stricter than the core sibling — MEDIUM.** utility-services allows `^22 || ^24.11`; downstream on Node 22 hits EBADENGINE / hard failure under engine-strict. Bucket: B9. Fix: align the range.

### Bugs / correctness

**B1f — `numeric`→`parseFloat` precision loss on money — MEDIUM.** `typeorm.config.ts:12` (and read/orm configs) parse pg `numeric` (arbitrary precision) as IEEE-754 double process-wide — silent rounding on invoice/expense totals and SUM drift. `bigint`→`parseInt` (`:7`) is a latent >2^53 overflow. Bucket: B8. Fix: parse money `numeric` as string + decimal math; return string for out-of-safe-range bigint.

**B2f — Authority-delegation IDOR + force-activate — HIGH/MEDIUM.** `process.authority.delegation.data.ts:45-58`: `update()` loads by `{id, business_id}` only (no `user_id`), so any business user edits/reactivates another user's delegation window (approval-authority integrity); `record.active = payload.active || true` (`:55,75`) always forces active. `authority.delegation.controller.ts:42` `search` also lacks the manager-role gate its own contract requires (enumerates all delegators/delegates). Bucket: B5. Fix: scope update by `user_id`, use `?? true`, gate `search` by manager role.

**B3f — Delegation window uses a hardcoded IST offset — LOW.** `process.authority.delegation.data.ts:135-138` subtracts fixed 329/331 minutes (start vs end differ by 2 for no reason) despite the "business offset" name — wrong windows for non-IST tenants. Bucket: B5. Fix: derive from business timezone.

**B4f — Silent error swallowing masks job failures; missing idempotency — MEDIUM.** `process.whatsapp.message.ts:49,119` and `process.slack.integration.ts:151` catch→return undefined; `SendBusinessReportNotificationJob` catches→logs so a failed email marks the BullMQ job successful (no retry). Only `EvaluateEmailRuleJob` sets `noDuplicate`; script/trigger/report jobs re-run side effects (duplicate emails, double execution) on retry. `email.message.subscriber.ts:21` `.toLowerCase()` with no null guard throws on missing `from_email`. Buckets: B2, B4. Fix: rethrow to let BullMQ retry; add idempotency keys; null-guard.

**B5f — Excel/CSV formula injection on export; cross-tenant trigger fan-out — LOW.** `generate.bulk.upload.sheet.ts:363,370` writes stored values without neutralizing leading `= + - @` (import path strips `=`, export doesn't — asymmetric). `capture.event.trigger.job` runs all businesses' triggers when the source row lacks `business_id`. Buckets: B2, B4.

### Efficiency / quality

**E1 — Test gaps on the security-critical paths — MEDIUM.** 458 tests pass with no infra, but zero specs for the tenant-boundary middlewares (`business.middleware`, `external.access.middleware`, `user.business.access.middleware`), ~14/15 workflow approval libraries, and the injection-prone `process.db.find`/`process.report.data`; e2e is the untouched Nest scaffold needing live PG+Redis; `collectCoverageFrom` sweeps 266 migrations; `isolatedModules:true` disables type-checking in tests; `test:db` points at a missing file. Bucket: B9.

**E2 — Duplication & dead code — LOW.** The `ProcessDbFind`/find-search config is copy-pasted across 22+ controllers (one parameterization fix lands everywhere once S1 is fixed); `listing.service.ts:146-160` cache path fully commented; several workflow DB-event jobs are no-ops; `tslint.json` is dead (TSLint EOL); pg-parser boilerplate triplicated across the three orm configs; jest worker-leak warning; `strict:false` in tsconfig ships non-strict `.d.ts`. Buckets: B1, B5, B8, B9.

### Coverage gaps / unverified (carry into remediation)

- **Self-approval / approver-identity check lives outside the workflow bucket** (whatever sets `wf_workflow_users.activity_id`). B5 could not confirm a requester can't approve their own document, nor that the approver id comes from `Auth.user()`. Must be verified in the access-module approval controller before sign-off.
- **`Auth.user()`/`Auth.login()` request-scoping** (utility-services AsyncLocalStorage) is assumed by every MCP/middleware enforcement decision; if it's a mutable singleton, concurrent requests cross principals — verify upstream.
- B4 F8 (path traversal via crafted `.xlsx` sheet name → temp path) and Handlebars custom-helper injection are unverified.
- Synthetic **user id 2** is shared by `internal/*` and all `v1` api-account keys, collapsing idempotency namespaces (B7 M3) and identity — confirm it isn't over-privileged.

## Remediation order

Severity × blast-radius, dependencies noted. Lead-owned items (public surface, auth architecture, shared base) are marked ⚑.

1. **A1 ⚑** — restore/commit barrel generation + CI build-require gate. Blocks any release; do first so fixes can ship.
2. **S1 + S2** — parameterize the shared listing/find/report SQL processors and remove the raw-SQL passthrough DTO fields. One coordinated change closes the largest attack surface across 4 modules; touches shared libraries ⚑.
3. **S5 ⚑** — guard/relocate the root `AppController` ops endpoints.
4. **S4 ⚑** — replace raw-XFF IP reads with `req.ip` across all five middlewares/services.
5. **S6, S7, S11, S12** — tenant/authorization gaps (device-auth membership, global form/menu writes, `v1` scope enforcement, webhook auth). Independent files, parallelizable.
6. **S8, S9, S10, S13, S15** — credential handling (serialize-exclude, CSPRNG secrets, rotation re-hash, remove hardcoded token, sign OAuth state). S9/S10 touch the same secret library — sequence together.
7. **S3** — sandbox the `eval` sites (larger design change; gate authorship as an interim mitigation once S6/S7 land).
8. **A2 ⚑, A3, A4, A5** — package/dependency/module hygiene.
9. **S14, S16, S17, B1f, B2f, B4f** — medium security + correctness.
10. **B3f, B5f, S18, E1, E2** — low-severity + quality; E1 (tests) should accompany each fix above as its regression proof.