parity(api,apps): security hotfix stage 1 - close unauthenticated write paths
Authorised by rulings D7 and T1-T3 in docs/parity/DECISIONS.md. This is the CLAUDE.md section 3 carve-out: matching the reference here would keep a money write path open to anonymous callers. Closed, each behind the rid the legacy vocabulary already had: GET accounting/customers/:id/balance -> wallet.list POST accounting/customers/:id/reconcile -> wallet.charge POST accounting/repayments/gold -> document.create POST accounting/repayments/rial -> document.create POST accounting/entries/:id/confirm -> document.list.confirm PATCH orders/:id/status -> order.list.sts.submit Four of the accounting routes WRITE to the ledger. The order-status route was worse in kind: its DTO takes a client-supplied actorAdminUserId, so an anonymous caller could move any order and pin it on any admin. It had no caller - the admin panel posts to admin/orders/status. GET /customers kept its public status (both apps' login still matches the phone number in the browser) but now selects only id/mobile/isActive/ isArchived. It previously returned every active customer's full name, national code, both debts, credit limit and the URLs of their identity-document scans. CustomerSummary narrowed in BOTH user apps in the same commit, per the v1/v2 parity rule. Not changed: /uploads stays public per ruling T2. The filename check requested alongside it passed unchanged - md5(time + name + uuid), no directory listing. Tests: apps/api/scripts/auth-smoke.sh asserts 401 without a token, 403 with a low-privilege token (ut 88, 5 unrelated rids) and pass-through for a super admin on every closed route, plus that the public routes still answer 200 and that GET /customers exposes only the four login fields. 23 passed, 0 failed against a live server. Typecheck clean on api, user-app and user-app-v2. [ref: henza_server/routes/admin.php:12,272; ApiMobileController.php:39-78] Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
s.esmaeili
co-authored by
Claude Opus 5
Claude Opus 5
parent
004a3945c2
commit
fdf833565f
@@ -0,0 +1,96 @@
|
||||
#!/usr/bin/env bash
|
||||
#
|
||||
# Auth smoke test for the routes closed in the security hotfix.
|
||||
#
|
||||
# For every route that was made non-public, asserts:
|
||||
# - no token -> 401
|
||||
# - low-privilege token -> 403
|
||||
# - super-admin token -> anything but 401/403 (the guard let it through)
|
||||
#
|
||||
# And asserts the genuinely-public routes still answer 200, plus that
|
||||
# GET /customers no longer leaks anything beyond the four login-lookup fields.
|
||||
#
|
||||
# Usage: API=http://localhost:3000/api bash scripts/auth-smoke.sh
|
||||
# Needs the API running and the dev seed applied (npx prisma db seed).
|
||||
|
||||
set -uo pipefail
|
||||
API="${API:-http://localhost:3000/api}"
|
||||
|
||||
SUPER_PHONE=09128081392; SUPER_PASS=09128081392 # ut 85, all rids
|
||||
LOW_PHONE=09123334444; LOW_PASS=admin1234 # ut 88, 5 unrelated rids
|
||||
|
||||
pass=0; fail=0
|
||||
ok() { printf ' \033[32mPASS\033[0m %s\n' "$1"; pass=$((pass+1)); }
|
||||
bad() { printf ' \033[31mFAIL\033[0m %s\n' "$1"; fail=$((fail+1)); }
|
||||
|
||||
login() {
|
||||
curl -s -X POST "$API/admin/auth/login" -H 'Content-Type: application/json' \
|
||||
-d "{\"phone\":\"$1\",\"password\":\"$2\"}" |
|
||||
sed -n 's/.*"api_token":"\([^"]*\)".*/\1/p'
|
||||
}
|
||||
|
||||
code() { # method path [token]
|
||||
local m=$1 p=$2 t=${3:-}
|
||||
if [ -n "$t" ]; then
|
||||
curl -s -o /dev/null -w '%{http_code}' -X "$m" "$API/$p" \
|
||||
-H "Authorization: Bearer $t" -H 'Content-Type: application/json' -d '{}'
|
||||
else
|
||||
curl -s -o /dev/null -w '%{http_code}' -X "$m" "$API/$p" \
|
||||
-H 'Content-Type: application/json' -d '{}'
|
||||
fi
|
||||
}
|
||||
|
||||
SUPER=$(login "$SUPER_PHONE" "$SUPER_PASS")
|
||||
LOW=$(login "$LOW_PHONE" "$LOW_PASS")
|
||||
[ -z "$SUPER" ] && { echo "super-admin login failed — is the seed applied?"; exit 1; }
|
||||
[ -z "$LOW" ] && { echo "low-priv login failed — is the seed applied?"; exit 1; }
|
||||
|
||||
# route | required rid
|
||||
CLOSED=(
|
||||
"GET|accounting/customers/x/balance|wallet.list"
|
||||
"POST|accounting/customers/x/reconcile|wallet.charge"
|
||||
"POST|accounting/repayments/gold|document.create"
|
||||
"POST|accounting/repayments/rial|document.create"
|
||||
"POST|accounting/entries/x/confirm|document.list.confirm"
|
||||
"PATCH|orders/x/status|order.list.sts.submit"
|
||||
)
|
||||
|
||||
echo "Closed routes — no token must be 401:"
|
||||
for row in "${CLOSED[@]}"; do
|
||||
IFS='|' read -r m p _ <<<"$row"
|
||||
c=$(code "$m" "$p")
|
||||
[ "$c" = 401 ] && ok "$m $p -> 401" || bad "$m $p -> $c (want 401)"
|
||||
done
|
||||
|
||||
echo "Closed routes — low-privilege token must be 403:"
|
||||
for row in "${CLOSED[@]}"; do
|
||||
IFS='|' read -r m p rid <<<"$row"
|
||||
c=$(code "$m" "$p" "$LOW")
|
||||
[ "$c" = 403 ] && ok "$m $p [$rid] -> 403" || bad "$m $p [$rid] -> $c (want 403)"
|
||||
done
|
||||
|
||||
echo "Closed routes — super admin must get past the guard:"
|
||||
for row in "${CLOSED[@]}"; do
|
||||
IFS='|' read -r m p _ <<<"$row"
|
||||
c=$(code "$m" "$p" "$SUPER")
|
||||
case "$c" in
|
||||
401|403) bad "$m $p -> $c (guard rejected a super admin)" ;;
|
||||
*) ok "$m $p -> $c (through the guard)" ;;
|
||||
esac
|
||||
done
|
||||
|
||||
echo "Public routes must stay reachable:"
|
||||
for p in "catalog/products" "pricing/latest?karatPerMille=750" "home/slider" "customers"; do
|
||||
c=$(curl -s -o /dev/null -w '%{http_code}' "$API/$p")
|
||||
[ "$c" = 200 ] && ok "GET $p -> 200" || bad "GET $p -> $c (want 200)"
|
||||
done
|
||||
|
||||
echo "GET /customers must expose only the login-lookup fields:"
|
||||
keys=$(curl -s "$API/customers" | sed -n 's/.*\({[^}]*}\).*/\1/p' | head -1 |
|
||||
grep -oE '"[a-zA-Z]+":' | tr -d '":' | sort -u | tr '\n' ' ')
|
||||
want="id isActive isArchived mobile "
|
||||
[ "$keys" = "$want" ] && ok "fields = $keys" || bad "fields = [$keys] want [$want]"
|
||||
|
||||
echo
|
||||
echo "passed: $pass failed: $fail"
|
||||
[ "$fail" -eq 0 ]
|
||||
@@ -2,34 +2,53 @@ import { Body, Controller, Get, Param, Post } from '@nestjs/common';
|
||||
import { LedgerService } from './ledger.service';
|
||||
import { PostGoldRepaymentDto } from './dto/post-gold-repayment.dto';
|
||||
import { PostRialRepaymentDto } from './dto/post-rial-repayment.dto';
|
||||
import { Public } from '../auth/guards';
|
||||
import { RequirePermission } from '../auth/guards';
|
||||
|
||||
@Public() // pre-existing unauthenticated endpoint; see Phase 1 notes
|
||||
/**
|
||||
* Ledger operations — admin only.
|
||||
*
|
||||
* These were `@Public()` until the security hotfix. That was faithful to the
|
||||
* reference, where the whole admin route file is unauthenticated (the
|
||||
* `auth:api` group wrapper is commented out at henza_server/routes/admin.php:12
|
||||
* and :272), but it contradicted decision D1 in docs/parity/DECISIONS.md, which
|
||||
* confirms the target keeps its stricter auth. Four of these five routes WRITE
|
||||
* to the ledger, so an anonymous caller could reconcile a balance or post and
|
||||
* confirm a repayment.
|
||||
*
|
||||
* Rids follow the legacy vocabulary: reading a balance is a wallet read,
|
||||
* repayments are financial documents, and confirming an entry is the existing
|
||||
* document-confirm permission.
|
||||
*/
|
||||
@Controller('accounting')
|
||||
export class AccountingController {
|
||||
constructor(private readonly ledger: LedgerService) {}
|
||||
|
||||
@Get('customers/:customerId/balance')
|
||||
@RequirePermission('wallet.list')
|
||||
getBalance(@Param('customerId') customerId: string) {
|
||||
return this.ledger.getBalance(customerId);
|
||||
}
|
||||
|
||||
@Post('customers/:customerId/reconcile')
|
||||
@RequirePermission('wallet.charge')
|
||||
reconcile(@Param('customerId') customerId: string) {
|
||||
return this.ledger.reconcileBalance(customerId);
|
||||
}
|
||||
|
||||
@Post('repayments/gold')
|
||||
@RequirePermission('document.create')
|
||||
postGoldRepayment(@Body() dto: PostGoldRepaymentDto) {
|
||||
return this.ledger.postGoldRepayment(dto);
|
||||
}
|
||||
|
||||
@Post('repayments/rial')
|
||||
@RequirePermission('document.create')
|
||||
postRialRepayment(@Body() dto: PostRialRepaymentDto) {
|
||||
return this.ledger.postRialRepayment(dto);
|
||||
}
|
||||
|
||||
@Post('entries/:entryId/confirm')
|
||||
@RequirePermission('document.list.confirm')
|
||||
confirmEntry(@Param('entryId') entryId: string) {
|
||||
return this.ledger.confirmEntry(entryId);
|
||||
}
|
||||
|
||||
@@ -2,11 +2,31 @@ import { Controller, Get, Param } from '@nestjs/common';
|
||||
import { CustomersService } from './customers.service';
|
||||
import { Public } from '../auth/guards';
|
||||
|
||||
@Public() // pre-existing unauthenticated endpoint; see Phase 1 notes
|
||||
/**
|
||||
* Customer-facing reads.
|
||||
*
|
||||
* Still `@Public()` because the target has no customer session yet — every one
|
||||
* of these is called with `session.customerId` straight from the browser
|
||||
* (DashboardView / ProfileView / OrdersView / TransactionsView), so any caller
|
||||
* who guesses or harvests an id can read that customer's data. That is a
|
||||
* textbook IDOR and it is what stage 2 of the security hotfix closes, by
|
||||
* deriving the customer from a `customer-jwt` token instead of the URL
|
||||
* (decision T1 in docs/parity/DECISIONS.md). The reference did exactly that
|
||||
* via its `api-customer` guard.
|
||||
*
|
||||
* What stage 1 already fixed: `list()` no longer returns the full customer
|
||||
* record — see LOGIN_LOOKUP_SELECT in customers.service.ts (SEC-08).
|
||||
*/
|
||||
@Public() // pre-existing unauthenticated endpoint; closed in hotfix stage 2
|
||||
@Controller('customers')
|
||||
export class CustomersController {
|
||||
constructor(private readonly customers: CustomersService) {}
|
||||
|
||||
/**
|
||||
* Login lookup only. Returns id/mobile/isActive/isArchived and nothing else;
|
||||
* both apps' LoginView downloads this and matches the phone in the browser.
|
||||
* Removed entirely in stage 2 once OTP verify returns the one customer.
|
||||
*/
|
||||
@Get()
|
||||
list() {
|
||||
return this.customers.list();
|
||||
|
||||
@@ -23,6 +23,31 @@ const PUBLIC_CUSTOMER_SELECT = {
|
||||
updatedAt: true,
|
||||
} satisfies Prisma.CustomerSelect;
|
||||
|
||||
/**
|
||||
* The only fields `GET /customers` may expose.
|
||||
*
|
||||
* That route is unauthenticated because both user apps' login screens download
|
||||
* the list and match the phone number in the browser. With the full select
|
||||
* above, an anonymous caller got the entire customer book: every national ID
|
||||
* number, every debt and credit limit, and the URLs of every identity-document
|
||||
* scan. Logged as SEC-08 in docs/parity/ISSUES.md.
|
||||
*
|
||||
* The reference never allowed this — its login was
|
||||
* POST userAuth (SMS) -> POST userVerfiy, and the server returned exactly ONE
|
||||
* customer after the code matched (henza_server ApiMobileController.php:39-78).
|
||||
*
|
||||
* Stage 2 of the security hotfix restores that flow and removes this route from
|
||||
* the public surface entirely. Until then these four fields are what the login
|
||||
* screens actually read (LoginView.vue:131-133 matches on mobile/isActive/
|
||||
* isArchived, then calls session.login(id, mobile)) — nothing more.
|
||||
*/
|
||||
const LOGIN_LOOKUP_SELECT = {
|
||||
id: true,
|
||||
mobile: true,
|
||||
isActive: true,
|
||||
isArchived: true,
|
||||
} satisfies Prisma.CustomerSelect;
|
||||
|
||||
@Injectable()
|
||||
export class CustomersService {
|
||||
constructor(private readonly prisma: PrismaService) {}
|
||||
@@ -46,7 +71,7 @@ export class CustomersService {
|
||||
list() {
|
||||
return this.prisma.customer.findMany({
|
||||
where: { isActive: true, isArchived: false },
|
||||
select: PUBLIC_CUSTOMER_SELECT,
|
||||
select: LOGIN_LOOKUP_SELECT,
|
||||
orderBy: { createdAt: 'desc' },
|
||||
});
|
||||
}
|
||||
|
||||
@@ -2,24 +2,42 @@ import { Body, Controller, Get, Param, Patch, Post } from '@nestjs/common';
|
||||
import { OrdersService } from './orders.service';
|
||||
import { CreateOrderDto } from './dto/create-order.dto';
|
||||
import { UpdateOrderStatusDto } from './dto/update-order-status.dto';
|
||||
import { Public } from '../auth/guards';
|
||||
import { Public, RequirePermission } from '../auth/guards';
|
||||
|
||||
@Public() // pre-existing unauthenticated endpoint; see Phase 1 notes
|
||||
/**
|
||||
* Customer-facing order routes.
|
||||
*
|
||||
* POST / and GET /:id stay `@Public()` for now: the target has no customer
|
||||
* session yet, so the customer id travels in the request body. The reference
|
||||
* derived it from the `api-customer` guard instead
|
||||
* (henza_server ApiMobileController.php:533 reads `auth()->id()`), which is what
|
||||
* stage 2 of the security hotfix restores. Tracked as SEC-02 / B1-B2 in
|
||||
* docs/parity/ISSUES.md.
|
||||
*
|
||||
* PATCH /:id/status was public too, which was worse in kind: it let an
|
||||
* anonymous caller move any order's status AND attribute the change to any
|
||||
* admin via the DTO's actorAdminUserId. It is an admin operation in the
|
||||
* reference, it had no caller in this repo (the admin panel posts to
|
||||
* admin/orders/status instead), so it is now behind the order-status rid.
|
||||
*/
|
||||
@Controller('orders')
|
||||
export class OrdersController {
|
||||
constructor(private readonly orders: OrdersService) {}
|
||||
|
||||
@Public()
|
||||
@Post()
|
||||
create(@Body() dto: CreateOrderDto) {
|
||||
return this.orders.createOrder(dto);
|
||||
}
|
||||
|
||||
@Public()
|
||||
@Get(':id')
|
||||
get(@Param('id') id: string) {
|
||||
return this.orders.getById(id);
|
||||
}
|
||||
|
||||
@Patch(':id/status')
|
||||
@RequirePermission('order.list.sts.submit')
|
||||
updateStatus(@Param('id') id: string, @Body() dto: UpdateOrderStatusDto) {
|
||||
return this.orders.updateStatus(id, dto.status, dto.actorAdminUserId, dto.note);
|
||||
}
|
||||
|
||||
@@ -70,13 +70,20 @@ export interface LedgerEntrySummary {
|
||||
note: string | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* What `GET /customers` returns — the login lookup, and nothing more.
|
||||
*
|
||||
* It used to carry fullName, national ID, both debts and the credit limit. The
|
||||
* route is unauthenticated (the login screen matches the phone number in the
|
||||
* browser), so that shipped the whole customer book to any visitor; the API now
|
||||
* selects only these four fields. See SEC-08 in docs/parity/ISSUES.md.
|
||||
*
|
||||
* The route disappears altogether once OTP verify returns the one authenticated
|
||||
* customer, the way the reference did it.
|
||||
*/
|
||||
export interface CustomerSummary {
|
||||
id: string;
|
||||
fullName: string;
|
||||
mobile: string;
|
||||
goldDebtGrams: string;
|
||||
fiatDebtRial: string;
|
||||
goldCreditLimitGrams: string;
|
||||
isActive: boolean;
|
||||
isArchived: boolean;
|
||||
}
|
||||
|
||||
@@ -85,13 +85,20 @@ export interface LedgerEntrySummary {
|
||||
note: string | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* What `GET /customers` returns — the login lookup, and nothing more.
|
||||
*
|
||||
* It used to carry fullName, national ID, both debts and the credit limit. The
|
||||
* route is unauthenticated (the login screen matches the phone number in the
|
||||
* browser), so that shipped the whole customer book to any visitor; the API now
|
||||
* selects only these four fields. See SEC-08 in docs/parity/ISSUES.md.
|
||||
*
|
||||
* The route disappears altogether once OTP verify returns the one authenticated
|
||||
* customer, the way the reference did it.
|
||||
*/
|
||||
export interface CustomerSummary {
|
||||
id: string;
|
||||
fullName: string;
|
||||
mobile: string;
|
||||
goldDebtGrams: string;
|
||||
fiatDebtRial: string;
|
||||
goldCreditLimitGrams: string;
|
||||
isActive: boolean;
|
||||
isArchived: boolean;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user