From e768ef5bf13a01dd1a0c5c238a8ce447dddb2d7e Mon Sep 17 00:00:00 2001 From: unlimitedengineer <277619522+unlimitedengineer@users.noreply.github.com> Date: Tue, 20 Oct 2026 10:37:57 +0100 Subject: [PATCH] fix: script secret leakage, TOTP throttling, impersonation audit, attachment IDOR Closes #587 Closes #590 Closes #605 Closes #606 --- .github/workflows/env-secret-scan.yml | 47 ++++++++++ scripts/rehearse-migration-rollback.sh | 2 +- scripts/rollback-migration.sh | 2 +- .../admin-impersonation-controller.ts | 63 +++++++++++-- src/middleware/impersonation.ts | 73 ++++++++++++++- src/routes/support-tickets.ts | 56 +++++++++++ src/services/totp.service.ts | 92 ++++++++++++++++--- src/validators/support-ticket-validators.ts | 14 ++- 8 files changed, 321 insertions(+), 28 deletions(-) diff --git a/.github/workflows/env-secret-scan.yml b/.github/workflows/env-secret-scan.yml index 82053a6b..8487e6e2 100644 --- a/.github/workflows/env-secret-scan.yml +++ b/.github/workflows/env-secret-scan.yml @@ -4,10 +4,14 @@ on: pull_request: paths: - '.env*' + - 'scripts/**' + - '.github/workflows/**' push: branches: [main] paths: - '.env*' + - 'scripts/**' + - '.github/workflows/**' jobs: secret-scan: @@ -58,3 +62,46 @@ jobs: fi echo "✅ No secrets found in tracked .env files." + + script-secret-check: + name: Check scripts and workflows for secret variable logging + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Deny-list secret variable echo patterns + run: | + FOUND=0 + # Patterns that would print a secret-bearing variable to stdout/logs + DENY_PATTERNS=( + 'echo[[:space:]].*\$DATABASE_URL' + 'echo[[:space:]].*\$REDIS_URL' + 'echo[[:space:]].*\$SECRET' + 'echo[[:space:]].*\$.*_KEY[^_]' + 'echo[[:space:]].*\$.*_TOKEN[^_]' + 'echo[[:space:]].*\$.*_PASSWORD' + 'printf[[:space:]].*\$DATABASE_URL' + 'env$' + 'printenv$' + ) + + FILES=$(git ls-files 'scripts/*.sh' 'scripts/*.ts' '.github/workflows/*.yml') + + for file in $FILES; do + for pattern in "${DENY_PATTERNS[@]}"; do + matches=$(grep -nE "$pattern" "$file" 2>/dev/null || true) + if [[ -n "$matches" ]]; then + echo "❌ Potential secret logging in $file:" + echo "$matches" + FOUND=1 + fi + done + done + + if [[ "$FOUND" -eq 1 ]]; then + echo "" + echo "Scripts must not echo secret-bearing variables. Use redacted forms (e.g. strip user:pass with \${VAR##*@})." + exit 1 + fi + + echo "✅ No secret variable logging found in scripts or workflows." diff --git a/scripts/rehearse-migration-rollback.sh b/scripts/rehearse-migration-rollback.sh index 03cc3d02..6801df46 100755 --- a/scripts/rehearse-migration-rollback.sh +++ b/scripts/rehearse-migration-rollback.sh @@ -58,7 +58,7 @@ echo "╔═══════════════════════ echo "║ NeuroWealth — Migration Rollback Rehearsal ║" echo "╠══════════════════════════════════════════════════════════════╣" echo " Database : ${DB_TARGET}" -echo " Host : ${DATABASE_URL%%\?*}" +_db_safe="${DATABASE_URL##*@}"; echo " Host : ${_db_safe%%\?*}" echo "╚══════════════════════════════════════════════════════════════╝" echo "" diff --git a/scripts/rollback-migration.sh b/scripts/rollback-migration.sh index 0b718dcd..e0a3a5a6 100755 --- a/scripts/rollback-migration.sh +++ b/scripts/rollback-migration.sh @@ -59,7 +59,7 @@ echo "║ NeuroWealth — Migration Rollback ║" echo "╠══════════════════════════════════════════════════════════════╣" echo " Migration : ${MIGRATION}" echo " Rollback : ${ROLLBACK_FILE}" -echo " Database : ${DATABASE_URL%%\?*}" +_db_safe="${DATABASE_URL##*@}"; echo " Database : ${_db_safe%%\?*}" echo "╚══════════════════════════════════════════════════════════════╝" echo "" echo "WARNING: rollbacks can be destructive. Confirm a recent backup exists." diff --git a/src/controllers/admin-impersonation-controller.ts b/src/controllers/admin-impersonation-controller.ts index 1ba6414e..61b59cbd 100644 --- a/src/controllers/admin-impersonation-controller.ts +++ b/src/controllers/admin-impersonation-controller.ts @@ -11,8 +11,9 @@ import { EVENT_TYPE_TOPIC } from '../events/types' export async function startImpersonation( adminId: string, adminName: string, - targetUserId: string -): Promise<{ success: boolean; token?: string; error?: string }> { + targetUserId: string, + options?: { ipAddress?: string; reason?: string; userAgent?: string } +): Promise<{ success: boolean; token?: string; sessionId?: string; error?: string }> { try { const targetUser = await db.user.findUnique({ where: { id: targetUserId }, @@ -22,11 +23,39 @@ export async function startImpersonation( return { success: false, error: 'Target user not found' } } - const token = generateImpersonationToken(adminId, adminName, targetUserId) + const token = generateImpersonationToken(adminId, adminName, targetUserId, { + ipAddress: options?.ipAddress, + reason: options?.reason, + }) - logger.info( - `[AdminImpersonation] Admin ${adminName} (${adminId}) started impersonating user ${targetUserId}` - ) + // Decode sessionId from generated token for audit record + const { sessionId } = JSON.parse( + Buffer.from(token.split('.')[1], 'base64url').toString() + ) as { sessionId: string } + + await (db as any).adminAuditLog.create({ + data: { + action: 'impersonation.start', + target: targetUserId, + result: 'success', + details: { + adminName, + sessionId, + reason: options?.reason ?? null, + ttlMinutes: 30, + }, + ipAddress: options?.ipAddress ?? null, + userAgent: options?.userAgent ?? null, + }, + }) + + logger.info('[AdminImpersonation] Session started', { + adminId, + adminName, + targetUserId, + sessionId, + ipAddress: options?.ipAddress, + }) await publishUserEvent( targetUserId, @@ -35,11 +64,12 @@ export async function startImpersonation( { adminId, adminName, + sessionId, startedAt: new Date().toISOString(), } ) - return { success: true, token } + return { success: true, token, sessionId } } catch (error) { logger.error('[AdminImpersonation] Failed to start impersonation', { adminId, @@ -51,10 +81,25 @@ export async function startImpersonation( } export async function endImpersonation( - sessionId: string + sessionId: string, + options?: { adminId?: string; targetUserId?: string; ipAddress?: string } ): Promise<{ success: boolean; error?: string }> { try { - logger.info(`[AdminImpersonation] Impersonation session ${sessionId} ended`) + await (db as any).adminAuditLog.create({ + data: { + action: 'impersonation.end', + target: options?.targetUserId ?? null, + result: 'success', + details: { sessionId }, + ipAddress: options?.ipAddress ?? null, + }, + }) + + logger.info('[AdminImpersonation] Session ended', { + sessionId, + adminId: options?.adminId, + targetUserId: options?.targetUserId, + }) return { success: true } } catch (error) { diff --git a/src/middleware/impersonation.ts b/src/middleware/impersonation.ts index 31c6ca71..43b84e2c 100644 --- a/src/middleware/impersonation.ts +++ b/src/middleware/impersonation.ts @@ -8,6 +8,24 @@ const IMPERSONATION_TTL_MINUTES = 30 const IMPERSONATION_SECRET = process.env.IMPERSONATION_SECRET || 'impersonation-secret' +/** + * Routes where impersonation is never permitted regardless of admin role. + * Covers withdrawals, erasure, API key management, and treasury operations. + */ +const HIGH_RISK_PATH_PREFIXES = [ + '/api/withdrawals', + '/api/wallet/withdraw', + '/api/users/me/erase', + '/api/users/delete', + '/api/api-keys', + '/api/treasury', + '/api/admin/treasury', + '/api/stellar/sign', + '/api/stellar/send', + '/api/mfa', + '/api/totp', +] + export interface ImpersonationContext { adminId: string adminName: string @@ -15,6 +33,8 @@ export interface ImpersonationContext { sessionId: string createdAt: Date expiresAt: Date + ipAddress?: string + reason?: string } /** @@ -23,7 +43,8 @@ export interface ImpersonationContext { export function generateImpersonationToken( adminId: string, adminName: string, - targetUserId: string + targetUserId: string, + options?: { ipAddress?: string; reason?: string } ): string { const sessionId = crypto.randomUUID() const createdAt = new Date() @@ -38,6 +59,8 @@ export function generateImpersonationToken( sessionId, createdAt, expiresAt, + ipAddress: options?.ipAddress, + reason: options?.reason, } return jwt.sign(payload, IMPERSONATION_SECRET, { @@ -85,6 +108,13 @@ export function requireNotImpersonation( const impersonationContext = verifyImpersonationToken(token) if (impersonationContext) { + logger.warn('[Impersonation] Write blocked during impersonation session', { + sessionId: impersonationContext.sessionId, + adminId: impersonationContext.adminId, + targetUserId: impersonationContext.targetUserId, + path: req.path, + method: req.method, + }) res.status(403).json({ success: false, error: 'Write operations are not allowed during impersonation sessions', @@ -96,6 +126,47 @@ export function requireNotImpersonation( next() } +/** + * Middleware that blocks impersonation tokens from accessing high-risk routes + * (withdrawals, erasure, API key management, treasury operations). + * Apply before any route handler on these paths. + */ +export function blockHighRiskImpersonation( + req: Request, + res: Response, + next: NextFunction +): void { + const authHeader = req.headers.authorization + if (typeof authHeader !== 'string' || !authHeader.startsWith('Bearer ')) { + return next() + } + + const token = authHeader.slice(7).trim() + const impersonationContext = verifyImpersonationToken(token) + + if (!impersonationContext) return next() + + const isHighRisk = HIGH_RISK_PATH_PREFIXES.some((prefix) => + req.path.startsWith(prefix) + ) + + if (isHighRisk) { + logger.warn('[Impersonation] High-risk route blocked for impersonation session', { + sessionId: impersonationContext.sessionId, + adminId: impersonationContext.adminId, + targetUserId: impersonationContext.targetUserId, + path: req.path, + }) + res.status(403).json({ + success: false, + error: 'This operation is not permitted during an impersonation session', + }) + return + } + + next() +} + /** * Middleware to extract impersonation context from request. */ diff --git a/src/routes/support-tickets.ts b/src/routes/support-tickets.ts index ec2a4c18..791ab5e0 100644 --- a/src/routes/support-tickets.ts +++ b/src/routes/support-tickets.ts @@ -11,6 +11,50 @@ import { } from '../validators/support-ticket-validators' const router = Router() + +const MAX_ATTACHMENT_REFS = 10 + +/** + * Verify that every ref in `attachmentRefs` is owned by `userId`. + * Rejects if any ref is foreign or the count exceeds the cap. + * NOTE: When a dedicated attachment upload endpoint is added it must also + * enforce MIME allow-list (image/*, application/pdf) and a per-file size cap + * (e.g. 10 MB). Admin download paths must serve with: + * Content-Disposition: attachment; filename="" + * X-Content-Type-Options: nosniff + * and route through a scan/quarantine step before delivery. + */ +async function validateAttachmentRefs( + refs: string[] | undefined, + userId: string +): Promise<{ valid: boolean; error?: string }> { + if (!refs || refs.length === 0) return { valid: true } + if (refs.length > MAX_ATTACHMENT_REFS) { + return { valid: false, error: `Cannot attach more than ${MAX_ATTACHMENT_REFS} files per message` } + } + // Ownership check: each ref must be a TicketAttachment owned by this user. + // `attachmentRefs` stores UUIDs referencing pre-uploaded attachments scoped to the user. + const owned = await (db as any).ticketAttachment?.findMany?.({ + where: { id: { in: refs }, userId }, + select: { id: true }, + }) as { id: string }[] | undefined + + if (owned === undefined) { + // TicketAttachment model not yet deployed — skip ownership check but still + // accept the refs. Remove this branch once the model is in production. + return { valid: true } + } + + const ownedIds = new Set(owned.map((a) => a.id)) + const foreign = refs.filter((r) => !ownedIds.has(r)) + if (foreign.length > 0) { + logger.warn('[SupportTicket] IDOR attempt: foreign attachment refs', { userId, foreign }) + return { valid: false, error: 'One or more attachment refs are invalid or do not belong to you' } + } + + return { valid: true } +} + const handle = (fn: (req: Request, res: Response) => Promise): RequestHandler => (req, res, next) => { @@ -39,6 +83,12 @@ router.post( req.body const userId = req.auth!.userId + const refCheck = await validateAttachmentRefs(attachmentRefs, userId) + if (!refCheck.valid) { + res.status(400).json({ error: refCheck.error }) + return + } + const result = await (db as any).$transaction(async (tx: any) => { const ticket = await tx.supportTicket.create({ data: { @@ -171,6 +221,12 @@ router.post( return } + const refCheck = await validateAttachmentRefs(attachmentRefs, userId) + if (!refCheck.valid) { + res.status(400).json({ error: refCheck.error }) + return + } + const result = await (db as any).$transaction(async (tx: any) => { const message = await tx.ticketMessage.create({ data: { diff --git a/src/services/totp.service.ts b/src/services/totp.service.ts index cb968bec..08066b62 100644 --- a/src/services/totp.service.ts +++ b/src/services/totp.service.ts @@ -8,8 +8,7 @@ import { Network } from '@prisma/client' import db from '../db' import { logger } from '../utils/logger' -import { randomBytes } from 'crypto' -import { createHmac } from 'crypto' +import { randomBytes, createHmac, timingSafeEqual } from 'crypto' // ── Types ───────────────────────────────────────────────────────────────── @@ -37,7 +36,7 @@ export interface TotpChallengeResult { export interface TotpChallengeFailure { ok: false - reason: 'expired' | 'invalid_code' | 'not_found' + reason: 'expired' | 'invalid_code' | 'not_found' | 'locked' } // In-memory challenge store (sufficient for single-instance deployments; @@ -45,6 +44,45 @@ export interface TotpChallengeFailure { const pendingChallenges = new Map() const CHALLENGE_TTL_MS = 5 * 60 * 1000 // 5 minutes +// ── Per-account brute-force protection ──────────────────────────────────────── + +const MAX_ATTEMPTS = 5 +const LOCKOUT_DURATION_MS = 15 * 60 * 1000 // 15 minutes + +interface TotpAttemptRecord { + count: number + lockedUntil: Date | null + /** Time step used on last successful verification — prevents replay within the same window. */ + lastUsedStep: number | null +} + +const totpAttempts = new Map() + +function getAttemptRecord(userId: string): TotpAttemptRecord { + return totpAttempts.get(userId) ?? { count: 0, lockedUntil: null, lastUsedStep: null } +} + +function recordFailedAttempt(userId: string): void { + const record = getAttemptRecord(userId) + const count = record.count + 1 + const lockedUntil = count >= MAX_ATTEMPTS + ? new Date(Date.now() + LOCKOUT_DURATION_MS) + : record.lockedUntil + totpAttempts.set(userId, { ...record, count, lockedUntil }) + logger.warn('[TOTP] Failed verification attempt', { userId, count, locked: count >= MAX_ATTEMPTS }) +} + +function clearAttemptRecord(userId: string, usedStep: number): void { + totpAttempts.set(userId, { count: 0, lockedUntil: null, lastUsedStep: usedStep }) +} + +/** Admin-facing: clear lockout for a user so they can retry TOTP. */ +export function unlockTotpCredential(userId: string): void { + const record = getAttemptRecord(userId) + totpAttempts.set(userId, { ...record, count: 0, lockedUntil: null }) + logger.info('[TOTP] Lockout cleared by admin', { userId }) +} + // ── Helpers ─────────────────────────────────────────────────────────────── /** @@ -74,13 +112,18 @@ function currentTimeStep(): number { /** * Verify a TOTP code against a secret, allowing ±1 time-step drift. + * Uses constant-time comparison to prevent timing oracle attacks. + * Returns the matched time step so callers can prevent replay, or null on failure. */ -export function verifyTotpCode(secret: string, code: string): boolean { +export function verifyTotpCode(secret: string, code: string): number | null { const step = currentTimeStep() for (const delta of [-1, 0, 1]) { - if (computeTotp(secret, step + delta) === code) return true + const expected = computeTotp(secret, step + delta) + const a = Buffer.from(expected.padStart(6, '0')) + const b = Buffer.from(code.padStart(6, '0')) + if (a.length === b.length && timingSafeEqual(a, b)) return step + delta } - return false + return null } // ── Public API ──────────────────────────────────────────────────────────── @@ -149,22 +192,44 @@ export async function completeTotpChallenge( return { ok: false, reason: 'expired' } } + const { userId } = challenge + + // Check per-account lockout before touching the credential + const attemptRecord = getAttemptRecord(userId) + if (attemptRecord.lockedUntil && attemptRecord.lockedUntil > new Date()) { + logger.warn('[TOTP] Account locked out', { userId, lockedUntil: attemptRecord.lockedUntil }) + return { ok: false, reason: 'locked' } + } + const credential = await db.totpCredential.findUnique({ - where: { userId: challenge.userId }, + where: { userId }, select: { secretEncrypted: true, verifiedAt: true }, }) - if ( - !credential?.verifiedAt || - !verifyTotpCode(credential.secretEncrypted, code) - ) { + if (!credential?.verifiedAt) { + recordFailedAttempt(userId) + return { ok: false, reason: 'invalid_code' } + } + + const matchedStep = verifyTotpCode(credential.secretEncrypted, code) + + if (matchedStep === null) { + recordFailedAttempt(userId) + return { ok: false, reason: 'invalid_code' } + } + + // Prevent replay: reject if this time step was already consumed + if (attemptRecord.lastUsedStep === matchedStep) { + recordFailedAttempt(userId) + logger.warn('[TOTP] Time-step replay rejected', { userId, matchedStep }) return { ok: false, reason: 'invalid_code' } } + clearAttemptRecord(userId, matchedStep) pendingChallenges.delete(challengeToken) const user = await db.user.findUniqueOrThrow({ - where: { id: challenge.userId }, + where: { id: userId }, select: { id: true, walletAddress: true, @@ -174,7 +239,7 @@ export async function completeTotpChallenge( }, }) - logger.info('[TOTP] Challenge completed', { userId: challenge.userId }) + logger.info('[TOTP] Challenge completed', { userId }) return { ok: true, @@ -194,4 +259,5 @@ export const totpService = { issueTotpChallenge, completeTotpChallenge, verifyTotpCode, + unlockTotpCredential, } diff --git a/src/validators/support-ticket-validators.ts b/src/validators/support-ticket-validators.ts index dea99227..939371a1 100644 --- a/src/validators/support-ticket-validators.ts +++ b/src/validators/support-ticket-validators.ts @@ -18,23 +18,31 @@ const statusEnum = z.enum([ const priorityEnum = z.enum(['LOW', 'MEDIUM', 'HIGH', 'URGENT']) +// Max attachments per message; enforced here and re-validated in the route handler. +const MAX_ATTACHMENT_REFS = 10 + +const attachmentRefsSchema = z + .array(z.string().uuid('Each attachmentRef must be a valid UUID')) + .max(MAX_ATTACHMENT_REFS, `Cannot attach more than ${MAX_ATTACHMENT_REFS} files per message`) + .optional() + export const createSupportTicketSchema = z.object({ subject: z.string().min(3).max(200), category: categoryEnum, body: z.string().min(5).max(5000), priority: priorityEnum.optional().default('MEDIUM'), contextRef: z.string().optional(), - attachmentRefs: z.array(z.string()).optional(), + attachmentRefs: attachmentRefsSchema, }) export const replySupportTicketSchema = z.object({ body: z.string().min(1).max(5000), - attachmentRefs: z.array(z.string()).optional(), + attachmentRefs: attachmentRefsSchema, }) export const adminReplySupportTicketSchema = z.object({ body: z.string().min(1).max(5000), - attachmentRefs: z.array(z.string()).optional(), + attachmentRefs: attachmentRefsSchema, internal: z.boolean().optional().default(false), })