371 lines
19 KiB
Markdown
371 lines
19 KiB
Markdown
# Phase 6 - Security Audit
|
|
|
|
## Executive Summary
|
|
|
|
The application has a solid baseline in two important areas: server-side authorization is used broadly across route handlers, and many sensitive workflow actions are audited. The main security risks are not widespread missing auth checks, but a smaller set of high-impact gaps in file delivery, request hardening, dependency posture, and abuse protection.
|
|
|
|
The most serious application-layer issue is direct public exposure of uploaded online-lesson assets. Uploaded video, attachment, and thumbnail files are stored under `public/uploads/...`, serialized back to clients as direct public URLs, and rendered directly in the UI. This bypasses the guarded download pattern that already exists in other parts of the system.
|
|
|
|
The second major risk is the import surface. Both spreadsheet import endpoints accept files based only on extension presence, read the entire file into memory, and rely on `xlsx`, which `npm audit` currently reports with high-severity advisories. Combined with the absence of rate limiting and explicit file-size controls on import endpoints, this increases the risk of denial-of-service and unsafe spreadsheet parsing.
|
|
|
|
## Threat Surface Inventory
|
|
|
|
Primary security-sensitive surfaces reviewed:
|
|
|
|
- Auth.js credential login flow
|
|
- Session and active-organization switching
|
|
- Server-side authorization helpers
|
|
- Permission-management APIs
|
|
- Announcement, online-lesson, and certificate file handling
|
|
- Training-record and employee spreadsheet imports
|
|
- Report export endpoints
|
|
- Notification and workflow action endpoints
|
|
- Next.js runtime/configuration and proxy perimeter
|
|
|
|
## Authentication Assessment
|
|
|
|
### Strengths
|
|
|
|
- `src/auth.ts:25-33` validates login input and uses bcrypt password verification.
|
|
- `src/auth.ts:57,81,103,137` records login success and failure events in audit logs.
|
|
- Self-service registration is disabled in `src/app/api/auth/register/route.ts`.
|
|
- Session enforcement is centralized through `requireSession()`.
|
|
|
|
### Findings
|
|
|
|
- `src/auth.ts:33` falls back to `dev-only-auth-secret-change-me` outside production. This is acceptable for local development but risky if a non-production environment is internet-exposed with weak secret hygiene.
|
|
- `src/auth.ts:36` sets `trustHost: true`. This is common in proxied deployments, but it raises deployment sensitivity if host/header forwarding is not tightly controlled upstream.
|
|
- No brute-force controls, lockout logic, or request throttling were found around credential login.
|
|
- Password policy is minimal: sign-in and auth forms enforce only `min(8)`, and `src/features/users/schemas/user.ts` does not enforce password strength itself.
|
|
|
|
### Assessment
|
|
|
|
Authentication is functionally correct, but it lacks abuse resistance and stronger operational guardrails.
|
|
|
|
## Authorization Assessment
|
|
|
|
### Strengths
|
|
|
|
- Broad route coverage uses server-side guards such as `requireOrganizationAccess`, `requirePermission`, `requireHRD`, `requireAllPermissions`, and `requireSuperAdminOrganizationAccess`.
|
|
- Permission-management endpoints are protected at the route-handler level even though they are not included in the proxy perimeter list.
|
|
- Download routes for announcement attachments and training-record certificates enforce organization-aware access checks before reading files from disk.
|
|
|
|
### Findings
|
|
|
|
- `src/proxy.ts:4-18,41-56` only protects a subset of API prefixes at the perimeter. Notably, online-lesson and permission-management APIs are not listed there, even though their handlers do perform server-side checks. This is a defense-in-depth gap rather than a direct bypass.
|
|
- `src/lib/auth/session.ts` and `src/app/api/organizations/active/route.ts` can auto-create admin memberships for super-admin/HRD organization access. This may be intended, but it is a privileged side effect inside an access path and should be treated carefully.
|
|
- Permission-management routes use `requireSuperAdminOrganizationAccess()`, which currently depends on the same organization-access helper that can create fallback memberships.
|
|
|
|
### Assessment
|
|
|
|
Authorization is stronger than average for an admin dashboard. The main risk is not broken route protection by default, but privileged side effects embedded inside access-resolution flows.
|
|
|
|
## Input Validation Assessment
|
|
|
|
### Strengths
|
|
|
|
- Core workflow routes use Zod or equivalent server-side validation.
|
|
- Online-lesson URLs are constrained to `http/https` via schema validation.
|
|
- Route params are commonly parsed and rejected when numeric IDs are invalid.
|
|
|
|
### Findings
|
|
|
|
- `src/app/api/training-records/import/route.ts:228-236` and `src/app/api/import-employees/route.ts:460-474` only verify that the upload is a `File` and that the name ends with `.xlsx`; no explicit file-size or MIME validation is enforced before parsing.
|
|
- Import endpoints load the full spreadsheet with `await file.arrayBuffer()` before further validation.
|
|
- Several workflow routes perform large amounts of procedural validation inline inside route handlers, which makes consistency harder to maintain.
|
|
|
|
### Assessment
|
|
|
|
Input validation is decent for routine CRUD, but weaker on large-file and batch-import surfaces.
|
|
|
|
## Injection Assessment
|
|
|
|
### Strengths
|
|
|
|
- Most database access uses Drizzle query builders and parameterized expressions.
|
|
- No command-execution patterns or child-process usage were found in application runtime paths.
|
|
- Storage helpers sanitize path segments and verify resolved paths stay under upload roots.
|
|
|
|
### Findings
|
|
|
|
- `npm audit` reports a high-severity advisory for `drizzle-orm` regarding SQL injection via improperly escaped SQL identifiers in versions `<0.45.2`.
|
|
- `src/features/reports/server/report-data.ts:446-555` uses raw SQL composition for reporting. The visible usage appears parameterized, but this area should be considered high-scrutiny because it is the most SQL-heavy path in the app.
|
|
- `npm audit` reports high-severity advisories for `xlsx`, which is used to parse untrusted spreadsheet uploads.
|
|
- CSV export exists in `src/app/api/audit-logs/export/route.ts`; this phase did not fully verify whether cells beginning with spreadsheet formula prefixes are neutralized.
|
|
|
|
### Assessment
|
|
|
|
There is no clear app-authored SQL injection bug in the reviewed code, but dependency-level injection risk is real and currently unresolved.
|
|
|
|
## XSS Assessment
|
|
|
|
### Strengths
|
|
|
|
- Announcement content, review comments, and online-lesson descriptions are rendered as plain text with `whitespace-pre-wrap`, not injected HTML.
|
|
- No user-content Markdown or rich-text renderer was found in the reviewed content workflows.
|
|
- The single `dangerouslySetInnerHTML` usage in `src/components/ui/chart.tsx` is used to emit generated CSS variables, not user-provided rich text.
|
|
|
|
### Findings
|
|
|
|
- Online-lesson external video URLs are rendered into outbound links and embed decisions. The code restricts embeddable iframes to recognized YouTube hosts, which is good, but general outbound URLs are still allowed for non-embeddable links.
|
|
- No repo-level Content Security Policy was found in `next.config.ts` or related runtime config.
|
|
|
|
### Assessment
|
|
|
|
Direct stored-XSS exposure appears low in the reviewed flows. The larger XSS concern is missing browser-side hardening such as CSP.
|
|
|
|
## CSRF & Request Protection Assessment
|
|
|
|
### Strengths
|
|
|
|
- State-changing operations generally use non-GET methods.
|
|
- Auth is session-based and routed through Auth.js rather than custom ad hoc cookies.
|
|
|
|
### Findings
|
|
|
|
- No explicit CSRF token validation, origin checks, or referer checks were found in state-changing route handlers.
|
|
- No centralized request-hardening middleware was found for mutation endpoints.
|
|
- Because cookie behavior is library-managed, SameSite/secure cookie posture was not explicitly verified from local code.
|
|
|
|
### Assessment
|
|
|
|
This is primarily a hardening gap, not a confirmed exploit in the reviewed code. Still, the app currently relies heavily on framework defaults and browser cookie behavior rather than explicit anti-CSRF controls.
|
|
|
|
## File Upload Assessment
|
|
|
|
### High-risk finding
|
|
|
|
- `src/lib/online-lesson-storage.ts:36-86` writes uploaded lesson assets to `public/uploads/online-lessons/...` and returns direct public URLs.
|
|
- `src/features/online-lessons/server/online-lesson-data.ts:51,53,55` serializes those direct file URLs to clients unchanged.
|
|
- `src/features/online-lessons/components/online-lesson-detail-page.tsx:198-200,284-290,328-336` renders those URLs directly in `<img>`, `<video>`, and download/open links.
|
|
|
|
Impact:
|
|
|
|
- Any user or external party who obtains one of these URLs can fetch the file directly from the public assets path without passing through authorization checks.
|
|
- This creates a real broken-access-control and data-exposure risk for uploaded lesson media and attachments.
|
|
|
|
### Additional findings
|
|
|
|
- `src/lib/announcement-storage.ts` and `src/lib/certificate-storage.ts` also store files under `public/uploads/...` and create direct public paths, but the primary application serializers now prefer guarded API download URLs instead of returning the raw storage path to clients.
|
|
- No virus scanning, content scanning, or magic-byte verification was found for uploaded files.
|
|
- Online-lesson attachments permit several document/archive formats, including ZIP, but are still served from public paths.
|
|
|
|
### Assessment
|
|
|
|
File upload handling is the most important application-layer security weakness in the current codebase, especially for online-lesson assets.
|
|
|
|
## Sensitive Data Assessment
|
|
|
|
### Strengths
|
|
|
|
- Many protected download responses use `Cache-Control: private, no-store`.
|
|
- Permission and organization access is checked server-side before reading many sensitive records.
|
|
|
|
### Findings
|
|
|
|
- Uploaded lesson assets are publicly reachable by URL, which can expose internal training content and attachments.
|
|
- Login failure audit logs in `src/auth.ts` persist identifiers and, in one case, user email inside audit payloads. This may be acceptable operationally, but it increases PII footprint inside logs.
|
|
- User and employee tables render email, employee code, organization, and department information broadly across admin UIs; this is expected for the app, but it increases the need for stronger export and audit controls.
|
|
|
|
### Assessment
|
|
|
|
The biggest sensitive-data problem is file exposure, not routine JSON overexposure in the reviewed API routes.
|
|
|
|
## Security Headers Assessment
|
|
|
|
### Findings
|
|
|
|
- No `headers()` configuration was found in `next.config.ts`.
|
|
- No explicit Content Security Policy, `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy`, `Permissions-Policy`, or HSTS configuration was found in reviewed app config.
|
|
- Download routes set `Cache-Control`, but broader site-level browser hardening appears absent.
|
|
|
|
### Assessment
|
|
|
|
Browser-side security headers are currently under-configured.
|
|
|
|
## Rate Limiting Assessment
|
|
|
|
### Findings
|
|
|
|
- No rate-limiting or throttling implementation was found for login, imports, exports, uploads, or workflow actions.
|
|
- This directly affects:
|
|
- Credential login
|
|
- Spreadsheet import endpoints
|
|
- Report export endpoint
|
|
- File upload endpoints
|
|
- Notification mutation endpoints
|
|
|
|
### Assessment
|
|
|
|
Rate limiting is a notable gap and materially increases brute-force and denial-of-service risk.
|
|
|
|
## Audit Log Assessment
|
|
|
|
### Strengths
|
|
|
|
- Authentication success/failure is logged.
|
|
- Permission changes have dedicated audit infrastructure.
|
|
- Import operations log audit entries.
|
|
- Announcement and workflow transitions show extensive audit logging.
|
|
|
|
### Findings
|
|
|
|
- `src/app/api/reports/export/route.ts` does not log report exports, even though exports can contain organization-wide training and employee data.
|
|
- This creates an audit gap for sensitive data extraction.
|
|
- This phase did not verify logout logging or sensitive-record read auditing beyond the reviewed routes.
|
|
|
|
### Assessment
|
|
|
|
Audit coverage is good for mutations, but weaker for data-export visibility.
|
|
|
|
## Dependency & Configuration Assessment
|
|
|
|
### `npm audit` results
|
|
|
|
- Total vulnerabilities: `11`
|
|
- High: `3`
|
|
- Moderate: `8`
|
|
|
|
Key findings:
|
|
|
|
- `drizzle-orm`
|
|
- High severity advisory reported by `npm audit`.
|
|
- `xlsx`
|
|
- High severity advisories reported by `npm audit`.
|
|
- Especially relevant because the app parses untrusted spreadsheet uploads in two endpoints.
|
|
- `sort-by` / `object-path`
|
|
- High/moderate advisory chain reported by `npm audit`.
|
|
- `@opentelemetry/core` and related packages
|
|
- Moderate memory-allocation advisory reported by `npm audit`.
|
|
|
|
### Configuration findings
|
|
|
|
- `src/auth.ts:33` uses a development fallback secret.
|
|
- `src/auth.ts:36` sets `trustHost: true`.
|
|
- `next.config.ts` removes console logs in production, which is good, but does not add security headers.
|
|
|
|
### Assessment
|
|
|
|
Dependency security posture needs attention now, especially because the vulnerable packages are in active code paths rather than unused tooling only.
|
|
|
|
## Business Logic Security Assessment
|
|
|
|
### Strengths
|
|
|
|
- Review/publish/archive flows check content status and permissions before transition.
|
|
- Training-record certificate download checks ownership/scope through the parent training record.
|
|
|
|
### Findings
|
|
|
|
- Privileged organization switching can materialize new admin memberships as a side effect.
|
|
- Report exports are permission-checked, but not audited.
|
|
- This phase did not find a clear "publish without approve" bypass in the reviewed content workflows, but the workflow handlers are large enough that they deserve dedicated regression tests.
|
|
|
|
### Assessment
|
|
|
|
Business-logic security is generally thoughtful, but several privileged flows remain too implicit and too large for easy verification.
|
|
|
|
## Findings by Severity
|
|
|
|
### High
|
|
|
|
1. Online-lesson uploaded assets are publicly accessible by direct URL.
|
|
- Evidence: [online-lesson-storage.ts](/D:/WY-2569/HRD/training-system-minimal-refactor/src/lib/online-lesson-storage.ts:36), [online-lesson-data.ts](/D:/WY-2569/HRD/training-system-minimal-refactor/src/features/online-lessons/server/online-lesson-data.ts:51), [online-lesson-detail-page.tsx](/D:/WY-2569/HRD/training-system-minimal-refactor/src/features/online-lessons/components/online-lesson-detail-page.tsx:284)
|
|
|
|
2. Spreadsheet import endpoints accept unbounded `.xlsx` uploads and parse them with a dependency that currently has high-severity advisories.
|
|
- Evidence: [training-record import route](/D:/WY-2569/HRD/training-system-minimal-refactor/src/app/api/training-records/import/route.ts:228), [employee import route](/D:/WY-2569/HRD/training-system-minimal-refactor/src/app/api/import-employees/route.ts:460), `npm audit` finding for `xlsx`
|
|
|
|
3. Active dependency vulnerabilities include high-severity advisories in `drizzle-orm` and `xlsx`.
|
|
- Evidence: `npm audit --json`
|
|
|
|
### Medium
|
|
|
|
1. No brute-force or rate-limiting protections were found for login, upload, import, or export endpoints.
|
|
2. No repo-level security headers configuration was found.
|
|
3. No explicit CSRF/origin validation layer was found for mutation endpoints.
|
|
4. Report exports are not audit-logged.
|
|
5. Privileged organization access can create admin memberships as a side effect.
|
|
6. Login audit logs increase sensitive identifier/email footprint inside logs.
|
|
|
|
### Low
|
|
|
|
1. Proxy perimeter coverage is incomplete, though server-side route guards still protect the affected APIs.
|
|
2. Development fallback secret is acceptable locally, but risky if misused in a shared environment.
|
|
|
|
## Quick Wins
|
|
|
|
1. Stop returning direct public URLs for online-lesson files; serve them through guarded download/stream routes.
|
|
2. Add strict file-size limits and MIME checks to both spreadsheet import endpoints before `arrayBuffer()` parsing.
|
|
3. Add rate limiting for `/api/auth/[...nextauth]`, import, export, and upload routes.
|
|
4. Add export audit logging for reports.
|
|
5. Add baseline browser security headers in Next.js config or edge middleware.
|
|
|
|
## Structural Improvements
|
|
|
|
1. Move all sensitive file delivery to authenticated route handlers and eliminate `public/uploads` for protected assets.
|
|
2. Introduce a centralized request-hardening layer for origin checking, abuse controls, and common mutation protections.
|
|
3. Separate privileged organization-access resolution from membership-creation side effects.
|
|
4. Add dedicated security tests for workflow transitions, file access, and export scope enforcement.
|
|
5. Review and remediate vulnerable dependencies in a controlled upgrade track, especially `xlsx` and `drizzle-orm`.
|
|
|
|
## Recommended Remediation Order
|
|
|
|
1. Online-lesson file exposure
|
|
- Highest real-world access-control risk.
|
|
|
|
2. Import surface hardening
|
|
- Add file-size checks and reduce exposure from vulnerable spreadsheet parsing.
|
|
|
|
3. Dependency remediation
|
|
- Prioritize packages in active parsing/data-access paths.
|
|
|
|
4. Rate limiting and request-hardening
|
|
- Best leverage across login, import, export, and uploads.
|
|
|
|
5. Security headers and audit-gap cleanup
|
|
- Important defense-in-depth after the higher-risk access issues are contained.
|
|
|
|
## Commands Executed and Results
|
|
|
|
- `rg` and `Select-String` across auth, proxy, storage, API routes, and config
|
|
- Used to inspect auth guards, upload paths, security headers, and audit coverage.
|
|
- `npm audit --json`
|
|
- Reported `11` vulnerabilities total: `3` high and `8` moderate.
|
|
|
|
## Files Reviewed
|
|
|
|
- `src/auth.ts`
|
|
- `src/proxy.ts`
|
|
- `src/lib/auth/session.ts`
|
|
- `src/lib/auth/authorization.server.ts`
|
|
- `next.config.ts`
|
|
- `src/lib/announcement-storage.ts`
|
|
- `src/lib/online-lesson-storage.ts`
|
|
- `src/lib/certificate-storage.ts`
|
|
- `src/app/api/auth/register/route.ts`
|
|
- `src/app/api/organizations/active/route.ts`
|
|
- `src/app/api/reports/export/route.ts`
|
|
- `src/app/api/training-records/import/route.ts`
|
|
- `src/app/api/import-employees/route.ts`
|
|
- `src/app/api/announcements/[id]/attachment/route.ts`
|
|
- `src/app/api/training-records/[id]/certificates/route.ts`
|
|
- `src/app/api/training-records/[id]/certificates/[certificateId]/download/route.ts`
|
|
- `src/app/api/user-permissions/route.ts`
|
|
- `src/app/api/permission-templates/route.ts`
|
|
- `src/app/api/permissions/catalog/route.ts`
|
|
- `src/features/announcements/server/announcement-data.ts`
|
|
- `src/features/online-lessons/server/online-lesson-data.ts`
|
|
- `src/features/training-records/server/training-record-data.ts`
|
|
- `src/features/online-lessons/schemas/online-lesson.ts`
|
|
- `src/features/announcements/components/announcement-view-page.tsx`
|
|
- `src/features/online-lessons/components/online-lesson-detail-page.tsx`
|
|
- `src/components/ui/chart.tsx`
|
|
- `src/features/users/schemas/user.ts`
|
|
|
|
## Areas Not Verified
|
|
|
|
- This phase did not perform live penetration testing, browser interception, or runtime CSRF validation against a running deployment.
|
|
- Cookie flags were not verified from actual HTTP responses.
|
|
- This phase did not exhaustively review every route handler in the repository; it focused on the highest-risk auth, permission, upload, import, and export surfaces.
|
|
- Dependency advisories were taken from local `npm audit` output and were not independently triaged against actual reachable exploit paths in this specific deployment model.
|