Fix security issues: path traversal, typed errors, file validation

- Fix path traversal in file serve endpoint (validate company prefix, block ..) - Add typed error classes: ValidationError, NotFoundError, ForbiddenError, ConflictError, StorageError - Global error handler catches AppError subclasses with correct status codes - 4xx logged as warn, 5xx as error with request ID - File upload validates entityType whitelist, UUID format, category pattern - Remove fragile string-matching error handling from routes - Services throw typed errors instead of plain Error - Health endpoint documented as intentionally public
2026-03-28 16:03:45 -05:00
parent 5aadd68128
commit e65175ef19
9 changed files with 150 additions and 86 deletions
--- a/packages/backend/src/routes/v1/files.ts
+++ b/packages/backend/src/routes/v1/files.ts
@@ -1,11 +1,12 @@
 import type { FastifyPluginAsync } from 'fastify'
 import multipart from '@fastify/multipart'
 import { FileService } from '../../services/file.service.js'
+import { ValidationError } from '../../lib/errors.js'

 export const fileRoutes: FastifyPluginAsync = async (app) => {
  await app.register(multipart, {
    limits: {
-      fileSize: 25 * 1024 * 1024, // 25 MB max
+      fileSize: 25 * 1024 * 1024,
      files: 1,
    },
  })
@@ -14,11 +15,10 @@ export const fileRoutes: FastifyPluginAsync = async (app) => {
  app.get('/files', { preHandler: [app.authenticate] }, async (request, reply) => {
    const { entityType, entityId } = request.query as { entityType?: string; entityId?: string }
    if (!entityType || !entityId) {
-      return reply.status(400).send({
-        error: { message: 'entityType and entityId query params required', statusCode: 400 },
-      })
+      throw new ValidationError('entityType and entityId query params required')
    }

+    // Files are company-scoped in the service — companyId from JWT ensures access control
    const fileRecords = await FileService.listByEntity(app.db, request.companyId, entityType, entityId)
    const data = await Promise.all(
      fileRecords.map(async (f) => ({ ...f, url: await app.storage.getUrl(f.path) })),
@@ -30,7 +30,7 @@ export const fileRoutes: FastifyPluginAsync = async (app) => {
  app.post('/files', { preHandler: [app.authenticate] }, async (request, reply) => {
    const data = await request.file()
    if (!data) {
-      return reply.status(400).send({ error: { message: 'No file provided', statusCode: 400 } })
+      throw new ValidationError('No file provided')
    }

    const entityType = (data.fields.entityType as { value?: string })?.value
@@ -38,38 +38,54 @@ export const fileRoutes: FastifyPluginAsync = async (app) => {
    const category = (data.fields.category as { value?: string })?.value

    if (!entityType || !entityId || !category) {
-      return reply.status(400).send({
-        error: { message: 'entityType, entityId, and category are required', statusCode: 400 },
-      })
+      throw new ValidationError('entityType, entityId, and category are required')
+    }
+
+    // Validate entityType is a known type
+    const allowedEntityTypes = ['member', 'member_identifier', 'product', 'rental_agreement', 'repair_ticket']
+    if (!allowedEntityTypes.includes(entityType)) {
+      throw new ValidationError(`Invalid entityType: ${entityType}`)
+    }
+
+    // Validate entityId format (UUID)
+    if (!/^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(entityId)) {
+      throw new ValidationError('entityId must be a valid UUID')
+    }
+
+    // Validate category (alphanumeric + underscore only)
+    if (!/^[a-z0-9_]+$/.test(category)) {
+      throw new ValidationError('category must be lowercase alphanumeric with underscores')
    }

    const buffer = await data.toBuffer()

-    try {
-      const file = await FileService.upload(app.db, app.storage, request.companyId, {
-        data: buffer,
-        filename: data.filename,
-        contentType: data.mimetype,
-        entityType,
-        entityId,
-        category,
-        uploadedBy: request.user.id,
-      })
-      const url = await app.storage.getUrl(file.path)
-      return reply.status(201).send({ ...file, url })
-    } catch (err) {
-      if (err instanceof Error && (err.message.includes('not allowed') || err.message.includes('too large') || err.message.includes('Maximum'))) {
-        return reply.status(400).send({ error: { message: err.message, statusCode: 400 } })
-      }
-      throw err
-    }
+    const file = await FileService.upload(app.db, app.storage, request.companyId, {
+      data: buffer,
+      filename: data.filename,
+      contentType: data.mimetype,
+      entityType,
+      entityId,
+      category,
+      uploadedBy: request.user.id,
+    })
+
+    request.log.info({ fileId: file.id, entityType, entityId, category, sizeBytes: file.sizeBytes }, 'File uploaded')
+
+    const url = await app.storage.getUrl(file.path)
+    return reply.status(201).send({ ...file, url })
  })

  // Serve file content (for local provider)
+  // Path traversal protection: validate the path starts with the requesting company's ID
  app.get('/files/serve/*', { preHandler: [app.authenticate] }, async (request, reply) => {
    const filePath = (request.params as { '*': string })['*']
    if (!filePath) {
-      return reply.status(400).send({ error: { message: 'Path required', statusCode: 400 } })
+      throw new ValidationError('Path required')
+    }
+
+    // Path traversal protection: must start with company ID, no '..' allowed
+    if (filePath.includes('..') || !filePath.startsWith(request.companyId)) {
+      return reply.status(403).send({ error: { message: 'Access denied', statusCode: 403 } })
    }

    try {
@@ -101,6 +117,7 @@ export const fileRoutes: FastifyPluginAsync = async (app) => {
    const { id } = request.params as { id: string }
    const file = await FileService.delete(app.db, app.storage, request.companyId, id)
    if (!file) return reply.status(404).send({ error: { message: 'File not found', statusCode: 404 } })
+    request.log.info({ fileId: id, path: file.path }, 'File deleted')
    return reply.send(file)
  })
 }
--- a/packages/backend/src/routes/v1/health.ts
+++ b/packages/backend/src/routes/v1/health.ts
@@ -2,6 +2,7 @@ import type { FastifyPluginAsync } from 'fastify'
 import { sql } from 'drizzle-orm'

 export const healthRoutes: FastifyPluginAsync = async (app) => {
+  // Intentionally public — no auth. Load balancers, Docker health checks, and monitoring need this.
  app.get('/health', async (request, reply) => {
    let dbStatus = 'disconnected'
    let redisStatus = 'disconnected'
--- a/packages/backend/src/routes/v1/lookups.ts
+++ b/packages/backend/src/routes/v1/lookups.ts
@@ -1,6 +1,7 @@
 import type { FastifyPluginAsync } from 'fastify'
 import { LookupCreateSchema, LookupUpdateSchema } from '@forte/shared/schemas'
 import { UnitStatusService, ItemConditionService } from '../../services/lookup.service.js'
+import { ConflictError, ValidationError } from '../../lib/errors.js'

 function createLookupRoutes(prefix: string, service: typeof UnitStatusService) {
  const routes: FastifyPluginAsync = async (app) => {
@@ -12,13 +13,12 @@ function createLookupRoutes(prefix: string, service: typeof UnitStatusService) {
    app.post(`/${prefix}`, { preHandler: [app.authenticate] }, async (request, reply) => {
      const parsed = LookupCreateSchema.safeParse(request.body)
      if (!parsed.success) {
-        return reply.status(400).send({ error: { message: 'Validation failed', details: parsed.error.flatten(), statusCode: 400 } })
+        throw new ValidationError('Validation failed', parsed.error.flatten())
      }

-      // Check slug uniqueness
      const existing = await service.getBySlug(app.db, request.companyId, parsed.data.slug)
      if (existing) {
-        return reply.status(409).send({ error: { message: `Slug "${parsed.data.slug}" already exists`, statusCode: 409 } })
+        throw new ConflictError(`Slug "${parsed.data.slug}" already exists`)
      }

      const row = await service.create(app.db, request.companyId, parsed.data)
@@ -29,32 +29,18 @@ function createLookupRoutes(prefix: string, service: typeof UnitStatusService) {
      const { id } = request.params as { id: string }
      const parsed = LookupUpdateSchema.safeParse(request.body)
      if (!parsed.success) {
-        return reply.status(400).send({ error: { message: 'Validation failed', details: parsed.error.flatten(), statusCode: 400 } })
-      }
-      try {
-        const row = await service.update(app.db, request.companyId, id, parsed.data)
-        if (!row) return reply.status(404).send({ error: { message: 'Not found', statusCode: 404 } })
-        return reply.send(row)
-      } catch (err) {
-        if (err instanceof Error && err.message.includes('system')) {
-          return reply.status(403).send({ error: { message: err.message, statusCode: 403 } })
-        }
-        throw err
+        throw new ValidationError('Validation failed', parsed.error.flatten())
      }
+      const row = await service.update(app.db, request.companyId, id, parsed.data)
+      if (!row) return reply.status(404).send({ error: { message: 'Not found', statusCode: 404 } })
+      return reply.send(row)
    })

    app.delete(`/${prefix}/:id`, { preHandler: [app.authenticate] }, async (request, reply) => {
      const { id } = request.params as { id: string }
-      try {
-        const row = await service.delete(app.db, request.companyId, id)
-        if (!row) return reply.status(404).send({ error: { message: 'Not found', statusCode: 404 } })
-        return reply.send(row)
-      } catch (err) {
-        if (err instanceof Error && err.message.includes('system')) {
-          return reply.status(403).send({ error: { message: err.message, statusCode: 403 } })
-        }
-        throw err
-      }
+      const row = await service.delete(app.db, request.companyId, id)
+      if (!row) return reply.status(404).send({ error: { message: 'Not found', statusCode: 404 } })
+      return reply.send(row)
    })
  }
  return routes
--- a/packages/backend/src/routes/v1/products.ts
+++ b/packages/backend/src/routes/v1/products.ts
@@ -59,15 +59,8 @@ export const productRoutes: FastifyPluginAsync = async (app) => {
    if (!parsed.success) {
      return reply.status(400).send({ error: { message: 'Validation failed', details: parsed.error.flatten(), statusCode: 400 } })
    }
-    try {
-      const unit = await InventoryUnitService.create(app.db, request.companyId, parsed.data)
-      return reply.status(201).send(unit)
-    } catch (err) {
-      if (err instanceof Error && (err.message.includes('Invalid condition') || err.message.includes('Invalid status'))) {
-        return reply.status(400).send({ error: { message: err.message, statusCode: 400 } })
-      }
-      throw err
-    }
+    const unit = await InventoryUnitService.create(app.db, request.companyId, parsed.data)
+    return reply.status(201).send(unit)
  })

  app.get('/products/:productId/units', { preHandler: [app.authenticate] }, async (request, reply) => {
@@ -90,15 +83,8 @@ export const productRoutes: FastifyPluginAsync = async (app) => {
    if (!parsed.success) {
      return reply.status(400).send({ error: { message: 'Validation failed', details: parsed.error.flatten(), statusCode: 400 } })
    }
-    try {
-      const unit = await InventoryUnitService.update(app.db, request.companyId, id, parsed.data)
-      if (!unit) return reply.status(404).send({ error: { message: 'Unit not found', statusCode: 404 } })
-      return reply.send(unit)
-    } catch (err) {
-      if (err instanceof Error && (err.message.includes('Invalid condition') || err.message.includes('Invalid status'))) {
-        return reply.status(400).send({ error: { message: err.message, statusCode: 400 } })
-      }
-      throw err
-    }
+    const unit = await InventoryUnitService.update(app.db, request.companyId, id, parsed.data)
+    if (!unit) return reply.status(404).send({ error: { message: 'Unit not found', statusCode: 404 } })
+    return reply.send(unit)
  })
 }