328 lines
14 KiB
Markdown
328 lines
14 KiB
Markdown
# Frontend Audit Report — POS System
|
||
|
||
**Auditor**: Senior Frontend Microservice Engineer
|
||
**Date**: 2026-03-20
|
||
**Scope**: Blazor WASM apps (`web-client-tpos-net`, `web-client-base-net`), MudBlazor usage, component architecture, state management
|
||
**Codebase**: `/apps/web-client-tpos-net` (195 Razor files, 248 C# files)
|
||
|
||
---
|
||
|
||
## Executive Summary
|
||
|
||
The POS System frontend (`web-client-tpos-net`) has a solid architectural foundation with well-structured multi-layout design, multi-vertical POS support, and clean component separation. However, **3 critical security vulnerabilities** were identified (JWT in localStorage, hardcoded OAuth2 client secret, password grant flow), combined with near-zero test coverage (~1 test file, 36 lines) and ~20% of POS pages having incomplete backend integration. These issues must be resolved before production deployment.
|
||
|
||
---
|
||
|
||
## Critical Issues
|
||
|
||
### CRIT-01 — JWT Stored in localStorage (XSS Risk)
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Services/AuthService.cs` lines 147–148
|
||
**Severity**: CRITICAL
|
||
|
||
```csharp
|
||
await _js.InvokeVoidAsync("localStorage.setItem", TokenKey(role), token.AccessToken);
|
||
```
|
||
|
||
JWT tokens are stored in `localStorage`, making them accessible to JavaScript. Any XSS exploit will steal all active tokens and allow full account impersonation.
|
||
|
||
**Fix**: Use `httpOnly` cookies via the BFF (WebClientTpos.Server). Add a `/api/auth/token` endpoint on the BFF that sets the token as a cookie rather than returning it in the response body.
|
||
|
||
---
|
||
|
||
### CRIT-02 — Client Secret Hardcoded in Client-Side Code
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Services/AuthService.cs` lines 39–40
|
||
**Severity**: CRITICAL
|
||
|
||
```csharp
|
||
private const string ClientId = "password-client";
|
||
private const string ClientSecret = "password-client-secret";
|
||
```
|
||
|
||
Blazor WASM compiles to WebAssembly served to the browser. Any constant is extractable via browser developer tools or disassembly. If `password-client-secret` is a real secret used in Duende IdentityServer, it is already compromised.
|
||
|
||
**Fix**: Move all OAuth2 token exchange calls to the BFF. The BFF holds the client secret server-side and proxies to IAM on behalf of the browser.
|
||
|
||
---
|
||
|
||
### CRIT-03 — Deprecated Password Grant (OAuth2)
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Services/AuthService.cs` line 136
|
||
**Severity**: CRITICAL
|
||
|
||
```csharp
|
||
new KeyValuePair<string, string>("grant_type", "password"),
|
||
```
|
||
|
||
The Resource Owner Password Credentials grant is deprecated in OAuth 2.1. It bypasses consent screens and exposes credentials to the client application. It cannot support MFA flows properly.
|
||
|
||
**Fix**: Migrate to Authorization Code Flow with PKCE. Use Duende IdentityServer's built-in OIDC endpoints.
|
||
|
||
---
|
||
|
||
### CRIT-04 — No CDN Subresource Integrity (SRI)
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/wwwroot/index.html` line 19
|
||
**Severity**: HIGH
|
||
|
||
Lucide icons loaded from `unpkg.com` CDN without an `integrity` attribute. A CDN compromise would allow arbitrary JavaScript execution in all POS sessions.
|
||
|
||
**Fix**:
|
||
```html
|
||
<script src="https://unpkg.com/lucide@latest/dist/umd/lucide.min.js"
|
||
integrity="sha384-[HASH]"
|
||
crossorigin="anonymous"></script>
|
||
```
|
||
|
||
Use a pinned version and generate the SRI hash with `openssl dgst -sha384`.
|
||
|
||
---
|
||
|
||
## Warnings
|
||
|
||
### WARN-01 — Token Refresh Not Implemented
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Services/AuthStateService.cs`
|
||
**Severity**: HIGH
|
||
|
||
`AuthStateService` stores the access token but never checks expiry or calls the refresh token endpoint. When tokens expire, API calls silently fail with 401, and users are not redirected to login.
|
||
|
||
**Fix**: Add a `TokenExpiry` property and a background timer that refreshes the token 60 seconds before expiry using the `refresh_token` from `OAuthTokenResponse`.
|
||
|
||
---
|
||
|
||
### WARN-02 — Global HttpClient Header Mutation (Race Condition)
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Services/PosDataService.cs` lines 40–47
|
||
**Severity**: HIGH
|
||
|
||
```csharp
|
||
_http.DefaultRequestHeaders.Authorization =
|
||
new AuthenticationHeaderValue("Bearer", _authState.Token);
|
||
```
|
||
|
||
Mutating `DefaultRequestHeaders` on a shared `HttpClient` instance is not thread-safe. Under concurrent requests, one request may attach or overwrite the header mid-flight.
|
||
|
||
**Fix**: Use `HttpRequestMessage` with per-request headers, or register a `DelegatingHandler` that injects the Bearer token automatically without touching global headers.
|
||
|
||
---
|
||
|
||
### WARN-03 — Lucide Re-Init on Every Render
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Layout/AdminLayout.razor` line 196
|
||
**Severity**: MEDIUM
|
||
|
||
```csharp
|
||
protected override async Task OnAfterRenderAsync(bool firstRender)
|
||
{
|
||
try { await JS.InvokeVoidAsync("lucide.createIcons"); } catch { }
|
||
```
|
||
|
||
`createIcons()` traverses the entire DOM on every render cycle. `index.html` already sets up a `MutationObserver` for this purpose (lines 25–46). The explicit call is redundant and adds unnecessary overhead.
|
||
|
||
**Fix**: Remove the `lucide.createIcons()` call from `OnAfterRenderAsync` entirely. The `MutationObserver` handles dynamic icon insertion.
|
||
|
||
---
|
||
|
||
### WARN-04 — MudBlazor ThemeProvider Declared in Multiple Layouts
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Layout/AdminLayout.razor` lines 17–20
|
||
**Severity**: MEDIUM
|
||
|
||
`<MudThemeProvider>`, `<MudPopoverProvider>`, `<MudDialogProvider>`, and `<MudSnackbarProvider>` are declared in each layout independently. This causes multiple instances in the component tree and can create CSS specificity conflicts and duplicate dialog/snackbar stacks.
|
||
|
||
**Fix**: Move all four providers to `App.razor` once, outside the `<Router>`. Remove from individual layouts.
|
||
|
||
---
|
||
|
||
### WARN-05 — localStorage Logic Duplicated Across 5 Files
|
||
**Severity**: MEDIUM
|
||
|
||
Token read/write logic is repeated in:
|
||
1. `AuthService.cs` lines 147–148
|
||
2. `AuthService.cs` lines 230–236
|
||
3. `AdminLayout.razor` lines 215–220
|
||
4. `StaffLayout.razor` (similar pattern)
|
||
5. `LanguageSwitcher.razor`
|
||
|
||
**Fix**: Extract a `LocalStorageService` with typed `GetAsync<T>` / `SetAsync<T>` / `RemoveAsync` methods. Register as singleton.
|
||
|
||
---
|
||
|
||
### WARN-06 — No Route Guards for Authenticated Pages
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Layout/AdminLayout.razor`
|
||
**Severity**: HIGH
|
||
|
||
Session restore happens in `OnAfterRenderAsync`, meaning the admin layout renders briefly before auth is checked. No `<AuthorizeView>` or route guard redirects unauthenticated users before render.
|
||
|
||
**Fix**: Add a `RedirectToLogin` component or use Blazor's `[Authorize]` attribute with a proper `AuthenticationStateProvider` backed by `AuthStateService`.
|
||
|
||
---
|
||
|
||
### WARN-07 — shopId Not Validated Against User Permissions
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Layout/AdminLayout.razor` lines 246–286
|
||
**Severity**: HIGH
|
||
|
||
Shop context is detected by parsing the URL path (e.g., `/admin/shop/{shopId}/cafe/`). There is no check that the authenticated user has access to the given `shopId`. A user could navigate directly to another merchant's shop URL.
|
||
|
||
**Fix**: After detecting `shopId` from URL, call the backend to verify the current user owns/manages that shop. Redirect to `/admin` with an error if unauthorized.
|
||
|
||
---
|
||
|
||
### WARN-08 — Fragile Multi-Format API Response Deserialization
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Services/PosDataService.cs` lines 122–185
|
||
**Severity**: MEDIUM
|
||
|
||
The deserializer tries 5 different response shapes in order. At line 152–155, it falls back to selecting the **first array property** found in the JSON object. If a response contains multiple array properties (e.g., `{ "items": [...], "errors": [...] }`), it selects whichever comes first in the serialized JSON, which is non-deterministic.
|
||
|
||
**Fix**: Standardize all API responses to a single envelope: `{ "success": bool, "data": T, "error": { "code": "", "message": "" } }`. Remove the multi-format fallback logic.
|
||
|
||
---
|
||
|
||
### WARN-09 — Hardcoded String in AuthInput Component
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Components/Auth/AuthInput.razor` line 56
|
||
**Severity**: LOW
|
||
|
||
```csharp
|
||
public string ForgotPasswordText { get; set; } = "Quên mật khẩu?";
|
||
```
|
||
|
||
Default value hardcoded in Vietnamese. English-culture users will see Vietnamese text.
|
||
|
||
**Fix**: Use `@inject IStringLocalizer<AuthInput> L` and default to `L["ForgotPassword"]`.
|
||
|
||
---
|
||
|
||
### WARN-10 — ~20% of POS Pages Have Incomplete Backend Integration
|
||
**Severity**: HIGH
|
||
|
||
21 `TODO` comments indicate unimplemented API integration:
|
||
|
||
| File | Missing Integration |
|
||
|------|---------------------|
|
||
| `MemberCard.razor` | Member visit history API |
|
||
| `PartialPayment.razor` | Order Service API |
|
||
| `TipEntry.razor` | Order Service API |
|
||
| `GiftCardPayment.razor` | Order Service API |
|
||
| `PaymentPending.razor` | Order Service API |
|
||
| `CashDrawer.razor` | Merchant Service staff/shift |
|
||
| `QuickSale.razor` | Merchant Service staff/shift |
|
||
| `PendingOrders.razor` | Merchant Service staff/shift |
|
||
| `ClockInOut.razor` | Merchant Service staff/shift |
|
||
| `ShiftManagement.razor` | Merchant Service staff/shift |
|
||
| `OrderCancel.razor` | DDD Value Object mapping fix |
|
||
| `PriceCheck.razor` | DDD Value Object mapping fix |
|
||
| `OrderEdit.razor` | DDD Value Object mapping fix |
|
||
| `VoidRefund.razor` | DDD Value Object mapping fix |
|
||
| `StockIn.razor` | DDD Value Object mapping fix |
|
||
| `StockTransfer.razor` | DDD Value Object mapping fix |
|
||
| `StockOut.razor` | DDD Value Object mapping fix |
|
||
| `TreatmentPlan.razor` | Backend CRUD API |
|
||
| `ConsentForm.razor` | Backend persistence API |
|
||
|
||
---
|
||
|
||
### WARN-11 — Incomplete vi-VN Translations
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/wwwroot/locales/vi-VN.json`
|
||
**Severity**: LOW
|
||
|
||
Some keys present in `en-US.json` are missing in `vi-VN.json`. When a key is missing, `JsonStringLocalizer` returns the key name as the display text.
|
||
|
||
**Fix**: Run a diff between the two JSON files in CI. Fail the build on missing translation keys.
|
||
|
||
---
|
||
|
||
### WARN-12 — No IFormatProvider in JsonStringLocalizer
|
||
**File**: `apps/web-client-tpos-net/src/WebClientTpos.Client/Localization/JsonStringLocalizer.cs`
|
||
**Severity**: LOW
|
||
|
||
`string.Format(format, arguments)` is called without a culture-aware `IFormatProvider`. Numbers and dates will be formatted using the thread's current culture, not the user's selected locale.
|
||
|
||
**Fix**:
|
||
```csharp
|
||
var value = string.Format(CultureInfo.CurrentUICulture, format ?? name, arguments);
|
||
```
|
||
|
||
---
|
||
|
||
### WARN-13 — No IAsyncDisposable on Layout Components
|
||
**Severity**: LOW
|
||
|
||
`AdminLayout.razor` subscribes to `AuthStateService.OnChange` but does not implement `IAsyncDisposable` to unsubscribe. In long-lived SPA sessions with layout reloads (e.g., via forceLoad navigation), this will leak event handlers.
|
||
|
||
**Fix**:
|
||
```csharp
|
||
@implements IDisposable
|
||
// ...
|
||
public void Dispose() => _authState.OnChange -= StateHasChanged;
|
||
```
|
||
|
||
---
|
||
|
||
## Improvements
|
||
|
||
### IMP-01 — Implement Authorization Code Flow + PKCE
|
||
Replace the password grant with a proper OIDC flow. Duende IdentityServer supports this natively. This enables MFA, social login, and OAuth2 compliance.
|
||
|
||
### IMP-02 — Add Content Security Policy
|
||
Add CSP header in BFF or via Traefik middleware:
|
||
```
|
||
Content-Security-Policy: default-src 'self'; script-src 'self' unpkg.com; style-src 'self' 'unsafe-inline'
|
||
```
|
||
|
||
### IMP-03 — Add @key to List Renders
|
||
All `@foreach` that render lists of components should use `@key` to prevent unnecessary DOM diffing:
|
||
```razor
|
||
@foreach (var item in _items)
|
||
{
|
||
<ItemComponent @key="item.Id" Item="item" />
|
||
}
|
||
```
|
||
|
||
### IMP-04 — Lazy Load POS Vertical Pages
|
||
Each POS vertical (Karaoke, Restaurant, Spa, Cafe, Retail) should be lazy-loaded. Blazor WASM supports lazy assembly loading via `LazyAssemblyLoader`. This will reduce initial download size.
|
||
|
||
### IMP-05 — Extract AdminLayout Shop Context to Service
|
||
`AdminLayout.razor` is 328 lines with complex shop context detection inline. Extract shop context detection to a `ShopContextService` (scoped) that parses the URL and validates permissions.
|
||
|
||
### IMP-06 — Add `@rendermode InteractiveWebAssembly` Explicitly
|
||
With .NET 8+, pages should declare render mode explicitly to avoid confusion with static rendering.
|
||
|
||
### IMP-07 — Standardize DTO Naming Convention
|
||
DTOs use mixed `Dto` vs no-suffix naming:
|
||
- `MerchantRegisterDto` (has `Dto` suffix) vs `AuthTokenResponse` (no suffix)
|
||
|
||
Standardize to always use `Dto` suffix for data transfer objects.
|
||
|
||
### IMP-08 — Add BFF Health Check Routes
|
||
`WebClientTpos.Server/Program.cs` has no health check endpoints. The BFF should expose `/health/live` and `/health/ready` for K8s probes.
|
||
|
||
### IMP-09 — Pin Lucide Version
|
||
Currently using `lucide@latest`. Pin to a specific version (e.g., `lucide@0.363.0`) to prevent breaking changes from CDN updates.
|
||
|
||
---
|
||
|
||
## Action Items
|
||
|
||
Prioritized by severity:
|
||
|
||
| Priority | Item | File(s) | Effort |
|
||
|----------|------|---------|--------|
|
||
| P0 | CRIT-02: Verify ClientSecret not exposed in prod | `AuthService.cs:40` | 1h |
|
||
| P0 | CRIT-01: Migrate JWT to httpOnly cookie via BFF | `AuthService.cs`, BFF | 2d |
|
||
| P0 | CRIT-03: Replace password grant with PKCE flow | `AuthService.cs` | 3d |
|
||
| P0 | WARN-06: Add route guards / AuthorizeView | All layouts | 1d |
|
||
| P0 | WARN-07: Validate shopId against user permissions | `AdminLayout.razor:246` | 4h |
|
||
| P1 | CRIT-04: Add SRI hash to Lucide CDN | `index.html:19` | 30m |
|
||
| P1 | WARN-01: Implement token refresh | `AuthStateService.cs` | 1d |
|
||
| P1 | WARN-02: Fix global HttpClient header mutation | `PosDataService.cs:40` | 4h |
|
||
| P1 | WARN-03: Remove redundant lucide.createIcons call | `AdminLayout.razor:196` | 30m |
|
||
| P1 | WARN-04: Move MudBlazor providers to App.razor | All layouts | 1h |
|
||
| P1 | WARN-10: Implement missing API integrations (21 TODOs) | Multiple POS pages | 5d |
|
||
| P2 | WARN-05: Extract LocalStorageService | 5 files | 4h |
|
||
| P2 | WARN-08: Standardize API response envelope | `PosDataService.cs:122` | 2d |
|
||
| P2 | WARN-13: Add IDisposable to layouts | Layout files | 1h |
|
||
| P2 | IMP-01: Write 50+ unit tests for services | `/tests/` | 3d |
|
||
| P2 | IMP-04: Lazy load POS vertical assemblies | Program.cs | 1d |
|
||
| P3 | WARN-09: Localize ForgotPasswordText | `AuthInput.razor:56` | 30m |
|
||
| P3 | WARN-11: CI check for missing translation keys | CI pipeline | 2h |
|
||
| P3 | WARN-12: Add IFormatProvider to localizer | `JsonStringLocalizer.cs` | 1h |
|
||
| P3 | IMP-09: Pin Lucide version | `index.html:19` | 15m |
|
||
|
||
---
|
||
|
||
*Generated by Senior Frontend Microservice Engineer audit — TEC-229*
|