c0a7fbc1b1
app_users is the only thing the two products have in common, and the code was treating it as though it were the whole relationship. Both directions leaked. Back-office roles were leaking into the till. PosRoleCanManageStaff returned true for roleid 1 to 6, on the reasoning that somebody who already administers a shop from a browser is not made less privileged by standing at the counter. That sounds fine and is wrong: measured against live data it handed till-supervisor powers to 68 accounts, 59 of them Nearle Daily Super admins, not one of whom is the administrator of anybody's POS. Meanwhile the actual shop accounts carry roleid 0 and were refused, so the mapping was backwards from intent in both halves at once. Till accounts were leaking into the application. GetStaffs is WHERE tenantid with no role filter, so a Counter Cashier appeared in the tenant staff list beside the delivery riders — a row every action on that page would fail against, since a cashier has no app login, no rider shift and no back-office screen. So: eligibility for a till is now granted explicitly by provisioning a Supervisor or a Cashier, never inherited from a back-office role, and roles 7 and 8 are excluded from every Nearle Daily lookup. The exclusion lives in the queries rather than in a check after them, because a check bolted on afterwards has to be repeated at six call sites and is one edit away from being forgotten at one of them — and that one would be the hole. A till account is not rejected by the app login; it is not found. Two things this surfaced that were not visible before. A Supervisor could not open a till. PIN sign-in needs a session that already exists, so once back-office roles were refused, an outlet whose only POS accounts were PIN-only had no way in at all. Supervisors are now provisioned with a username and password as well as a PIN; cashiers deliberately get neither, because they sign on at a counter somebody has already opened and a second password would be one more credential to leak for no capability gained. UpdatePosUser silently dropped authname. It wrote the password, reported success, and left the account unreachable by either lookup — the failure surfaced at a counter as "not recognised" rather than on the screen that caused it. Contactno had the same gap. Verified against live rows rather than asserted, by scratch/posseparation: a provisioned supervisor signs in and gets the supervisor shell; five real back-office accounts including Super admins are refused; the supervisor is invisible to applogin, tenant weblogin and the password-setup lookup; and no till account appears in getallusers, while asking for role 7 by name still returns them so the console can read its own people. All five outlets that stock products now have a Supervisor and a Cashier. Also moves the loose markdown into docs/, which was already staged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
96 lines
9.7 KiB
Markdown
96 lines
9.7 KiB
Markdown
# 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": "<specific 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.
|