From b0caacd90ab1efe65d6425073e3de0fe983c3fb5 Mon Sep 17 00:00:00 2001 From: Suriya Date: Mon, 3 Aug 2026 20:19:04 +0530 Subject: [PATCH] pos --- SECURITY_HANDOFF.md | 95 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 SECURITY_HANDOFF.md diff --git a/SECURITY_HANDOFF.md b/SECURITY_HANDOFF.md new file mode 100644 index 0000000..687c49c --- /dev/null +++ b/SECURITY_HANDOFF.md @@ -0,0 +1,95 @@ +# Handoff: Broken Access Control (IDOR) audit & fixes — Fiesta backend + +Repo: `backend_fiesta` (Go + Fiber + GORM), consumed by `nearledaily/daily_merchant_web` (React/TS) and a mobile app (not in this repo). + +## 1. The root problem (still not fully fixed — read this first) + +**There is no authentication system in this backend.** Grep confirms: +- No JWT/session token is ever issued. `Login`, `TenantLogin`, `TenantWebLogin`, `AppLogin` (in `controllers/userController.go`) just look up the user/tenant and return their info in the JSON body — no token. +- No auth middleware exists anywhere. `routes/routes.go` / `main.go` only wire up CORS middleware. Every route is wide open — anyone who can reach the API can call any endpoint with any query params. + +Because of that, every endpoint trusts client-supplied query params (`tenantid`, `customerid`, `partnerid`, etc.) as the sole source of "who is asking." There is currently **nothing stopping a logged-in store admin for tenant 1135 from just requesting `?tenantid=1136`** and getting another tenant's data — the frontend happens to always send the logged-in user's own tenantid, but the backend never checks it. + +**This session's fixes only close one specific hole**, described below. The real fix — deriving identity server-side from a verified token instead of trusting query params — has not been started. Whoever picks this up should treat that as the actual next milestone. + +## 2. The specific bug that was found and fixed this session + +Pattern found repeatedly across the codebase: repository functions build SQL dynamically, e.g. + +```go +query := "SELECT ... FROM orders WHERE 1=1" +if tenantID != 0 { + query += " AND tenantid = ?" + params = append(params, tenantID) +} +// ...similar optional blocks for partnerid, customerid, etc. +``` + +**If none of the scoping params were supplied (0 / empty), the query silently fell through to "no WHERE clause" and returned every row in the table across every tenant.** This was directly reachable — e.g. `orders/getorders` with no `tenantid` returned all ~300 orders in the DB rather than 400ing, which is how the user first noticed this (logged in as a store admin, expected only their store's orders, saw everyone's). + +### Fix pattern applied + +Rather than rewriting every repository query (large surface area, higher regression risk), a **controller-level guard** was added to each affected endpoint: if none of the valid scoping ids are present in the query string, return `400` immediately instead of calling the service/repo at all. + +Standard error shape used everywhere: +```json +{ "status": false, "code": 400, "message": "" } +``` + +## 3. Endpoints fixed (8 total) + +| # | Endpoint | File / function | Guard added | +|---|---|---|---| +| 1 | `GET /v1/web/orders/getorders` (+ mob) | `controllers/orderController.go` `GetOrders` (line 24) | requires one of `tenantid`, `partnerid`, `customerid`, `applocationid`, `appuserid` — else 400 (line ~102-110). Previously the `else` branch called `GetAllOrders` (unscoped). | +| 2 | `GET /v1/web/orders/getordersummary` | `controllers/orderController.go` `GetOrderSummary` (line 129) | requires one of `tenantid`, `partnerid`, `customerid`, `locationid` (line 137-143) | +| 3 | `GET /v1/web/orders/getlocationsummary` | `controllers/orderController.go` `GetlocationOrderSummary` (line 163) | requires `tenantid` (line 167-173) | +| 4 | `GET /v1/web/users/getallusers` | `controllers/userController.go` `GetAllUsers` (line 22) | requires `tenantid` (line 29-35). Note: this endpoint's query selects `a.pin` (login PIN) — this was a high-severity leak (PINs across all tenants) before the fix. | +| 5 | `GET /v1/web/deliveries/getdeliveries` (+ mob) | `controllers/deliveriesController.go` `GetDeliveries` (line 194) | requires one of `tenantid`, `partnerid`, `customerid`, `applocationid`, `userid`, `appuserid` (line 212-218) | +| 6 | `GET /v1/web/partners/getriders` (+ mob) | `controllers/partnerController.go` `GetActiveRiders` (line 19) | requires one of `tenantid`, `partnerid`, `applocationid`, `userid` (line 25-31). Lower severity — underlying repo query defaults to `userid = 0` rather than a full dump, but fixed for consistency. | +| 7 | `GET /v1/web/partners/getriderlogs` (+ mob) | `controllers/partnerController.go` `GetRiderLogs` (line 121) | requires one of `partnerid`, `applocationid` (line 127-133). **Also fixed an unrelated bug in the same function**: `tdate` was reading `c.Query("fromdate")` (copy-paste error) so the end of any date range was always silently overwritten with the start date. Now correctly reads `c.Query("todate")` (line 125). | +| 8 | `POST /v1/mob/orders/getcustomerorders` | `controllers/orderController.go` `GetCustomerOrders` (line 374) | requires `customerid` (line 394-400) | + +### Also fixed alongside #2: SQL injection in `GetOrderSummary` + +`repositories/orderRepository.go` `GetOrderSummary` previously built the date filter by **string-concatenating** `fdate`/`tdate` directly into raw SQL. Rewritten to use parameterized `?` placeholders passed through `r.db.Raw(query, params...)`. The `strconv` import was removed from that file since it became unused after the rewrite (verified via grep no other usage remained). + +## 4. Reviewed and explicitly NOT changed (don't re-flag these) + +Same `WHERE 1=1` pattern exists elsewhere but was judged not to be a bug, or already safe: + +- **`repositories/tenantRepository.go` `GetAllTenants`** — intentionally lists all tenants for a platform/super-admin console. The gap here is "no RBAC to restrict who can call this," which is the same root-cause auth gap from section 1, not a scoping bug to patch individually. +- **`repositories/productRepository.go` `GetProductSubCategory`** — has an explanatory comment: subcategories are intentionally shared/global master data plus tenant-owned overrides. Not a bug. +- **`repositories/productRepository.go` `GetProductCount`** — returns aggregate counts only (no PII), low severity, left as-is. +- **`repositories/utilsRepository.go` `GetSubcategories`** — global taxonomy/reference data; the model has no tenant field at all. +- **`repositories/orderRepository.go` `GetAdminOrders`** — has `WHERE 1=1` internally but is safe because its only caller (`GetOrders` controller) only invokes it when `applocationid != 0`. +- **`repositories/orderRepository.go` `GetAllOrders`** — now dead code (unreachable) after fix #1 above; confirmed via grep it's no longer called anywhere. Could be deleted as cleanup but left in place. +- **`repositories/tenantRepository.go` `GetTenantLocations`** — already always filters `WHERE tenantid = ?`. Safe, unchanged. + +## 5. Known pre-existing bug found during this audit, NOT yet fixed anywhere + +**Frontend/backend path mismatch on rider logs.** In `nearledaily/daily_merchant_web/src/services/fiestaApi.ts`, `getRiderLogs()` (~line 1132) calls: +```ts +fiestaGet('riders/getriderlogs', {...}) +``` +`FIESTA_BASE` is `https://fiesta.nearle.app/live/api/v1/web`, so this resolves to `.../v1/web/riders/getriderlogs`. But the backend only registers this route under the `partners` group (`routes/partnerroutes.go`): `partner.Get("/getriderlogs", ...)` on `api.Group("/v1/web/partners")`, i.e. the real path is `.../v1/web/partners/getriderlogs`. Confirmed via grep there is no `/v1/web/riders` route group anywhere in the backend. + +**This means `getRiderLogs()` in the web console has likely been 404ing already, independent of anything fixed this session.** Fix is a one-line FE change: `'riders/getriderlogs'` → `'partners/getriderlogs'`. Not fixed yet because it's a frontend-repo change and wasn't the scope of this backend security pass — flagging it here so it isn't lost. + +## 6. Frontend compatibility check (already done, no FE changes needed for the 8 fixes above) + +Checked `daily_merchant_web/src/services/fiestaApi.ts` against every fix — the web console already sends the now-required params in all cases: +- `getOrders`, `getAllUsers`, `getDeliveries`, `getOrderSummary`, `getLocationSummary` — all declare `tenantid: number` as a **required** (non-optional) TS field already. +- `getRiders` — always sends `applocationid: opts.applocationid ?? FIESTA_APPLOCATION_ID` (never zero/undefined) plus required `tenantid`. +- `getRiderLogs` — sends `tenantid`/`applocationid` when available, but see the path bug in section 5 — worth re-verifying once that's fixed. +- **`mob/orders/getcustomerorders`** (fix #8) is called from the **mobile app**, which is not in this repo — whoever owns that codebase needs to verify every call site always sends `customerid`. Not verified in this session. + +## 7. Environment note + +**No Go toolchain is available in the sandbox this session ran in** (`command not found: go`). All edits above were manually reviewed (imports, syntax, call sites checked via Read/grep) but **never compiled**. Run `go build ./...` and the existing test suite (if any) before deploying any of this. + +## 8. Suggested next steps for whoever picks this up + +1. `go build ./...` and smoke-test all 8 changed endpoints (call with and without the required param, confirm 200 vs 400). +2. Fix the `riders/getriderlogs` → `partners/getriderlogs` path bug in `fiestaApi.ts` (section 5). +3. Verify the mobile app always sends `customerid` to `mob/orders/getcustomerorders` before this ships, since that's the one fixed endpoint not verified from a frontend contract. +4. Scope and plan the real fix: JWT/session auth issuance + middleware, so `tenantid`/`customerid`/etc. are derived from a verified server-side identity instead of trusted from query params. Until that lands, the 8 fixes in this doc only prevent the "forgot to pass an id → get everything" failure mode — they do **not** prevent a malicious or buggy client from passing a *different* tenant's/customer's/partner's real id and getting their data.