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.
|