Dhananjaya99
150 lines
15 KiB
Markdown
150 lines
15 KiB
Markdown
# 02 · SECURITY — Phase 1 Review Aid (Accepted Risks + Per-Feature Checklist)
|
|
|
|
> **What this is:** a review aid, not a policy manual. Tick items during development and review. Before marking any feature `[x]` in `Backend/PROGRESS.md`, run its checklist here (Part C) plus the foundational controls (Part B).
|
|
> **Navigation:** you arrived from `00-CORE.md`. Business rules are in `10-BACKEND-PHASE1.md`, the API contract in `11-BACKEND-PHASE1.md`, the frontend validation posture in `20-FRONTEND.md`.
|
|
> **Context:** Phase 1 is single-tenant with **RBAC deferred** and **approvals auto-on**. Several exposures below are *deliberate, accepted* decisions — they are recorded in Part A so they are explicit, not implicit.
|
|
|
|
---
|
|
|
|
## Part A — Accepted Risks Register
|
|
|
|
These are known, deliberately-accepted Phase-1 exposures. Each has a compensating control and a trigger to revisit. Do not "fix" them ad-hoc during Phase 1 — they are tracked here and closed in Part D order.
|
|
|
|
| ID | Risk | Why accepted | Compensating control | Revisit trigger |
|
|
|---|---|---|---|---|
|
|
| **AR-01** | **No authorization** — any authenticated user can call any endpoint (post adjustments, confirm GRN, cancel PO, write off stock). | RBAC deferred (FR-X-01), Phase-1 scope. | Authentication required; **immutable audit trail** attributes every action. | First post-Phase-1 hardening → enable RBAC. |
|
|
| **AR-02** | **Approvals auto-on** — no value gate on PO or adjustment. | Phase-1 decision (FR-PROC-04, FR-STK-07). | Audit + mandatory reason codes. | Enable adjustment approval first, then PO approval. |
|
|
| **AR-03** | **IDOR** — sequential IDs + no RBAC → a user can access another's document by guessing the ID. | Consequence of AR-01. | Audit trail. | Closed with RBAC. |
|
|
| **AR-04** | **GRN-without-PO** — receive (and value) goods with no order; a fraud vector. | Emergency/direct receipt is useful (FR-GRN-02). | Permission-gated (when RBAC on); flagged for review; audit; cost entered here gets extra scrutiny (see C.3). | With RBAC / policy on direct receipts. |
|
|
| **AR-05** | **In-transit loss window** — dispatched-but-not-received stock is untracked shrinkage risk. | Inherent to in-transit transfers (FR-STK-05). | In-transit aging monitoring report. | Add stuck-transfer alert (Part D). |
|
|
| **AR-06** | **Localhost dev secrets** in `appsettings.Development.json`. | Local-dev convenience, current phase. | `.gitignore` + localhost only. | Before any shared/staging/prod → User Secrets / env vars; rotate. |
|
|
| **AR-07** | **`getUserDetails` / `LogoutUser` callable without a bearer token** — `GET /api/v1/auth/users/{userId}` and `POST /api/v1/auth/logout` resolve the target user from the URL/payload, not the caller's session, so any anonymous caller can fetch a profile or log out an arbitrary user's sessions by GUID. | Carried over verbatim from AuthHex's own dispatcher contract (API_REFERENCE.md §3) — ERPCore's `AuthController` proxies it as-is rather than silently tightening a contract it doesn't own. | GUIDs are not enumerable; every call is written to `AuthEventLogs` upstream in AuthHex. | Revisit once AuthHex exposes a token-scoped variant, or add ERPCore-side rate limiting / auth requirement ahead of AuthHex. |
|
|
| **AR-08** | **No rate limiting on `AuthController`'s anonymous endpoints** (login, register, refresh, recovery, OTP send/verify) — brute-force and account-enumeration exposure. | Not built in this pass (docs/11 §2.0, added 2026-07-16); AuthHex may rate-limit server-side but ERPCore does not add its own layer yet. | AuthHex's own lockout/backoff (per docs/10 NFR-03), immutable audit trail. | Add ASP.NET Core rate limiting middleware to `AuthController` before any non-local deployment. |
|
|
| **AR-09** | **HRM inherits AR-01 for salary/PII data** — any door-admitted authenticated user can currently view/download any employee's salary figures, payslip, or uploaded documents (NIC scans, contracts). This is a **explicit, flagged decision, not a silent inheritance**: salary/PII is categorically more sensitive than Phase-1 inventory data, and this was called out to the business owner before HRM build started (see `12-BACKEND-HRM.md` A.1). | RBAC still deferred repo-wide; a coarse HR-role door-gate was not made a blocking prerequisite for HRM go-live. | Immutable audit trail (as AR-01); sidebar-visibility hiding of Employees/Attendance/Payroll sections for non-HR roles via the existing `NavItem`/`RolePermission` mechanism (UI-level only, not a server-enforced gate). | **HRM should be the forcing function that enables per-endpoint RBAC ahead of the rest of the system** (see Part D) — salary-data exposure is a materially worse blast radius than inventory data. |
|
|
| **AR-10** | **`AuditLogsController` exposes salary figures** — a `PayrollLine`/`EmployeeSalaryStructure` mutation's `ChangeSet` contains salary amounts; `AuditLogsController` is not RBAC-gated, so any authenticated user can read another employee's salary history via `GET /audit-logs?entityType=PayrollLine&entityId=X`. | Consequence of AR-01/AR-09, specific enough to name on its own rather than leaving implicit. | None beyond authentication today. | Closed with RBAC (Part D), or an interim HRM-specific audit-log access filter. |
|
|
|
|
---
|
|
|
|
## Part B — Foundational Controls (cross-cutting — apply everywhere)
|
|
|
|
### B.1 Authentication & JWT
|
|
- [ ] Strong signing key (≥ 256-bit) from env/secrets; the dev `CHANGE_ME` key never reaches non-local environments
|
|
- [ ] Algorithm pinned (HS256 or RS256); reject `alg: none` and algorithm-confusion
|
|
- [ ] Validate issuer, audience, lifetime, and signature on every request
|
|
- [ ] Short access-token lifetime; define refresh strategy if needed
|
|
- [ ] Passwords hashed with a strong KDF (ASP.NET Identity / PBKDF2 / bcrypt / argon2) — never plaintext/reversible
|
|
- [ ] `/auth/login` rate-limited + backoff/lockout (brute-force)
|
|
- [ ] Generic auth-failure messages (no account-enumeration signal)
|
|
|
|
### B.2 Token storage & CSRF *(httpOnly-cookie decision)*
|
|
- [x] Token in an **httpOnly, Secure** cookie (never localStorage) — removes XSS token theft (`AuthCookieWriter.WriteSession`, `erp_at`/`erp_rt`, 2026-07-16)
|
|
- [x] `SameSite=Strict` on the auth cookie (`AuthCookieWriter`; assumes frontend + ERPCore share a registrable domain — revisit if deployed cross-domain)
|
|
- [x] **CSRF protection on every state-changing request** (double-submit `XSRF-TOKEN` cookie + `X-XSRF-TOKEN` header, `ValidateCsrfAttribute`, applied to every mutating `AuthController` action; Bearer-header callers exempt since they aren't cookie-driven)
|
|
- [ ] CORS locked to the known frontend origin(s); credentials mode aligned with the cookie — **not yet configured**; required before any browser frontend can call these endpoints cross-origin (tracked with the frontend-wiring follow-up)
|
|
- [x] Cookie scoped minimally (path/domain), Secure flag on (`erp_rt` scoped to `/api/v1/auth/refresh-token`; all three cookies `Secure=true`)
|
|
|
|
### B.3 Audit integrity *(this is the compensating control for AR-01 — it must hold)*
|
|
- [ ] Audit log **and** stock ledger are append-only **at the DB level** (the app's DB role has no `UPDATE`/`DELETE` on those tables)
|
|
- [ ] Every mutation records who / when / old→new / reason (FR-X-02)
|
|
- [ ] No API path edits or deletes a posted transaction (FR-X-05); corrections are reversing entries
|
|
- [ ] Actor derived from token `sub`, never from the request body
|
|
|
|
### B.4 Transport & headers
|
|
- [ ] HTTPS only; HSTS in production
|
|
- [ ] Security headers: CSP, `X-Content-Type-Options: nosniff`, frame-ancestors/`X-Frame-Options`, `Referrer-Policy`
|
|
|
|
### B.5 Error & logging hygiene
|
|
- [ ] `ProblemDetails.detail` leaks no stack traces / connection strings / internals in production
|
|
- [ ] Serilog never logs tokens, passwords, cookies, or secret-bearing request bodies
|
|
- [ ] Correlation/trace id present for support without exposing sensitive data
|
|
|
|
### B.6 Input & injection
|
|
- [ ] No raw or string-concatenated SQL; EF Core parameterizes — keep it that way
|
|
- [ ] **DTOs are narrow** — no mass-assignment/over-posting; client cannot set `status`, ids, `createdBy`, timestamps, or computed fields
|
|
- [ ] Server-side validation is authoritative (mirrors `20-FRONTEND §3`); client validation is UX only
|
|
- [ ] Pagination bounds enforced (`pageSize` max) to prevent resource exhaustion
|
|
|
|
### B.7 Concurrency as integrity
|
|
- [ ] FIFO layer consumption is **row-locked** inside the UoW transaction (no double-spend)
|
|
- [ ] `ETag`/`If-Match` on mutable resources; `412` on stale writes
|
|
|
|
---
|
|
|
|
## Part C — Per-Feature Checklists
|
|
|
|
### C.1 Master Data (Item / UOM / Category / Vendor / Warehouse / Bin)
|
|
- [ ] Create/update DTOs exclude server-controlled fields (`status`, ids, timestamps)
|
|
- [ ] Deactivate — not delete — referenced masters (FR-MD-08); hard delete blocked → `MASTER_IN_USE`
|
|
- [ ] Nested/reference writes validate the target exists and is active
|
|
- [ ] `Item.salePrice` is a **legitimately client-supplied** field (a deliberate exception to B.6's over-posting list) — validated `>= 0` server-side, nullable. It is **Sales-only** (never enters GRN/FIFO/ledger), so unlike GRN `unitCost` it has **no** inventory-value or costing impact; the fixed/stock-value choice is frontend UX (`docs/11 §2.1`).
|
|
|
|
### C.2 Procurement (Requisition / RFQ / PO / Purchase Return)
|
|
- [ ] PO totals computed **server-side** from lines (never trust client totals)
|
|
- [ ] PO **edit-while-open**: every change audit-logged old→new (this is the tampering surface)
|
|
- [ ] PO cancel blocked if any receipt exists
|
|
- [ ] Purchase-return quantity validated against received/available (no over-return)
|
|
- [ ] Note in review: **AR-01/AR-02/AR-03** apply to these endpoints
|
|
|
|
### C.3 GRN
|
|
- [ ] `unitCost` **defaults to the PO line price**; a per-line override **is now permitted** *(decision revised 2026-07-20 — was "locked, client cost ignored")*. When an override is entered it is used, and the PO price is snapshotted (`poUnitPrice`) so a **`priceVariance` is recorded** against it for review. Rationale: one PO legitimately spans batches received at different prices; the variance trail (plus the audit log) is the compensating control that replaces the old hard block.
|
|
- [ ] **Derived figures stay server-computed** — `netUnitCost`/`receivedValue`/`vatAmount`/`lineTotal` are never accepted from the client, so the client cannot inflate stock value except by an *auditable* unit-cost override. Discount reduces inventory cost; **VAT is recoverable and never enters stock value**.
|
|
- [ ] Direct GRN (no PO) remains the higher-scrutiny path where cost is entered with no PO to compare against → review flag + audit (**AR-04**)
|
|
- [ ] **Off-PO lines on a PO-based GRN** (`poLineId: null`, 2026-07-22) are the **same exposure class as AR-04** — cost is entered with no PO line to compare against, and `OVER_RECEIPT_TOLERANCE` does not apply to them. Treat them with the direct-receipt scrutiny (review flag + audit); they do not touch PO balances.
|
|
- [ ] Over-receipt tolerance enforced server-side → `OVER_RECEIPT_TOLERANCE` (PO-linked lines only; off-PO lines have no PO qty to check)
|
|
- [ ] On-hold stock is not issuable (FR-WH-07); expired batch blocked
|
|
|
|
### C.4 Stock Core (FIFO / Ledger)
|
|
- [ ] FIFO consumption row-locked (B.7) — integrity + no double-spend
|
|
- [ ] Negative-stock block enforced server-side → `STOCK_NEGATIVE_BLOCKED`
|
|
- [ ] Ledger append-only (B.3)
|
|
- [ ] Valuation computed from layers server-side; never from client input
|
|
|
|
### C.5 Adjustments — **HIGHEST-RISK FEATURE IN THE PHASE**
|
|
Auto-post + no approval + direct write-off = the primary theft/fraud surface. Reason code + user stamp are the *only* live controls.
|
|
- [ ] Reason code mandatory, server-enforced → `REASON_CODE_REQUIRED`
|
|
- [ ] User stamp mandatory (from token)
|
|
- [ ] Decrease cannot drive available negative → `STOCK_NEGATIVE_BLOCKED`
|
|
- [ ] `qtyDelta` sanity bounds; large write-offs surface on a review report
|
|
- [ ] Review note: **AR-02** — re-enabling adjustment approval is the **first** control to turn on post-Phase-1 (Part D)
|
|
|
|
### C.6 Transfers
|
|
- [ ] Dispatch validates available ≥ qty → block otherwise
|
|
- [ ] Cost-preserving (dest inherits source cost) — no revaluation via transfer
|
|
- [ ] `destWarehouseId != srcWarehouseId`
|
|
- [ ] In-transit aging monitored (**AR-05**: stuck/never-received transfers)
|
|
|
|
### C.7 Counts
|
|
- [ ] Variance posting is an adjustment in disguise → apply C.5 controls
|
|
- [ ] System-qty snapshot immutable once the count is opened
|
|
- [ ] Large variances flagged for review
|
|
|
|
### C.8 HRM (Employee / Documents / Attendance / Leave / Payroll) — **salary/PII data, see AR-09/AR-10**
|
|
- [ ] Create/update DTOs exclude server-controlled fields (`status`, ids, `createdBy`, timestamps, computed payroll amounts)
|
|
- [ ] Employee is **never hard-deleted** (deactivate via `EmployeeStatus` only), matching FR-MD-08's deactivate-not-delete pattern
|
|
- [ ] `Employee.UserId` uniqueness (one User per Employee) enforced at the DB level (filtered unique index), not just service-level
|
|
- [ ] File uploads (`EmployeeDocument`, attendance spreadsheets): extension allowlist + content-type cross-check + size cap enforced **server-side** (client checks are UX only); magic-byte sniffing / antivirus scanning explicitly **deferred**, not silently skipped — treat as an extension of AR-08's "not built in this pass" posture
|
|
- [ ] `EmployeeDocument` download is never served via a static/guessable URL — authenticated controller action streaming through `IFileStorageService` only
|
|
- [ ] Attendance batch lock (`Confirmed`/`UsedInPayroll`) genuinely blocks record edits server-side (`ATTENDANCE_BATCH_LOCKED`), not just hidden in the UI
|
|
- [ ] Payroll figures (Gross/Net/Tax/EPF/ETF) are computed **server-side only**; the client never supplies or overrides them
|
|
- [ ] **Payroll Unlock is the highest-risk action in this module** (parallel to C.5's framing of Adjustments) — mandatory reason, heavy audit; treat as the first HRM candidate for real per-endpoint RBAC
|
|
- [ ] Untrusted spreadsheet parsing (`ClosedXML`/`CsvHelper`): packages pinned to current versions, no macro/external-entity execution path enabled
|
|
- [ ] Review note: **AR-01/AR-09/AR-10** apply to every HRM endpoint until RBAC lands
|
|
|
|
---
|
|
|
|
## Part D — Post-Phase-1 controls to enable (in order)
|
|
1. **Adjustment approval** (config flag already reserved) — closes the top fraud surface (AR-02, C.5).
|
|
2. **RBAC enforcement** (role→permission) — closes AR-01, AR-03, AR-04, and **AR-09/AR-10 (HRM salary/PII)**. Given HRM's materially worse blast radius, consider bringing this forward ahead of item 3 once HRM ships (see AR-09's revisit trigger).
|
|
3. **PO approval** (value thresholds) — closes remaining AR-02.
|
|
4. **Monitoring reports** — stuck-transfer aging (AR-05) and large-variance/write-off review.
|
|
|
|
---
|
|
|
|
## Part E — Locked security-relevant decisions
|
|
- **GRN `unitCost` from the PO line** (server-derived; not user-entered). Direct GRN is the audited exception.
|
|
- **Frontend token in an httpOnly Secure cookie + CSRF protection** (not localStorage).
|
|
|
|
---
|
|
|
|
*End of 02-SECURITY.md. Run the relevant checklist before ticking a feature in `Backend/PROGRESS.md`.*
|