diff --git a/src/index.ts b/src/index.ts index 8b98405..9bfa223 100644 --- a/src/index.ts +++ b/src/index.ts @@ -16,7 +16,7 @@ import { initBrowser, closeBrowser } from "./services/browser.js"; import { loadKeys, getAllKeys } from "./services/keys.js"; import { initDatabase, pool } from "./services/db.js"; import { billingRouter } from "./routes/billing.js"; -import { statusRouter } from "./routes/status.js"; + import { usageRouter } from "./routes/usage.js"; import { openapiSpec } from "./docs/openapi.js"; @@ -95,7 +95,6 @@ app.use(rateLimit({ windowMs: 60_000, max: 120, standardHeaders: true, legacyHea // Public routes app.use("/health", healthRouter); app.use("/v1/billing", billingRouter); -app.use("/status", statusRouter); app.use("/v1/playground", playgroundRouter); diff --git a/src/routes/__tests__/playground.test.ts b/src/routes/__tests__/playground.test.ts index 1dfd56d..c0bd80c 100644 --- a/src/routes/__tests__/playground.test.ts +++ b/src/routes/__tests__/playground.test.ts @@ -317,5 +317,41 @@ describe('Playground Route', () => { fullPage: true })) }) + + it('BUG-021 FIXED: URL validation now happens before rate limiting', async () => { + // Test that invalid URLs get 400 validation errors without consuming rate limit + const longUrl = 'https://example.com/' + 'a'.repeat(2100) + + const req = createMockRequest({ url: longUrl }) + const res = createMockResponse() + + // Get all middleware handlers for the POST route + const route = playgroundRouter.stack.find(layer => layer.route?.methods.post)?.route + const middlewares = route?.stack || [] + + // Should have urlValidationMiddleware first, then playgroundLimiter, then the main handler + expect(middlewares.length).toBe(3) + + // Test that the first middleware (URL validation) rejects long URLs + const urlValidationMiddleware = middlewares[0].handle + await urlValidationMiddleware(req, res, vi.fn()) + + expect(res.status).toHaveBeenCalledWith(400) + expect(res.json).toHaveBeenCalledWith({ error: "Invalid URL: must be between 1 and 2048 characters" }) + }) + + it('should allow valid URLs to pass through validation middleware', async () => { + const req = createMockRequest({ url: 'https://example.com' }) + const res = createMockResponse() + const next = vi.fn() + + const route = playgroundRouter.stack.find(layer => layer.route?.methods.post)?.route + const urlValidationMiddleware = route?.stack[0].handle + + await urlValidationMiddleware(req, res, next) + + expect(next).toHaveBeenCalled() + expect(res.status).not.toHaveBeenCalled() + }) }) }) \ No newline at end of file diff --git a/src/routes/__tests__/status.test.ts b/src/routes/__tests__/status.test.ts index 0cb69c2..1d52e6e 100644 --- a/src/routes/__tests__/status.test.ts +++ b/src/routes/__tests__/status.test.ts @@ -7,28 +7,52 @@ import { fileURLToPath } from 'url' const __dirname = path.dirname(fileURLToPath(import.meta.url)) const publicDir = path.join(__dirname, '../../../public') -function createApp() { +function createBuggyApp() { const app = express() app.use(express.static(publicDir, { etag: true })) - // Mirror the status route from index.ts - app.get('/status', (_req, res) => { + // Simulate the old buggy behavior - serve file directly instead of redirect + app.get("/status", (_req, res) => { res.sendFile(path.join(publicDir, 'status.html')) }) + // Clean URLs for other pages + for (const page of ["privacy", "terms", "impressum", "usage"]) { + app.get(`/${page}`, (_req, res) => res.redirect(301, `/${page}.html`)); + } + return app } -describe('Status Route', () => { - const app = createApp() +function createFixedApp() { + const app = express() + app.use(express.static(publicDir, { etag: true })) - it('GET /status returns 200', async () => { - const res = await request(app).get('/status') - expect(res.status).toBe(200) + // The FIX: Remove statusRouter, let redirect loop handle it + // Clean URLs for legal pages (redirect /status → /status.html, etc.) + for (const page of ["privacy", "terms", "impressum", "status", "usage"]) { + app.get(`/${page}`, (_req, res) => res.redirect(301, `/${page}.html`)); + } + + return app +} + +describe('Status Route BUG-020', () => { + it('CURRENT BUGGY BEHAVIOR: GET /status returns 200 instead of 301 redirect', async () => { + const buggyApp = createBuggyApp() + const res = await request(buggyApp).get('/status') + expect(res.status).toBe(200) // This is the bug - should be 301 expect(res.headers['content-type']).toContain('text/html') }) - it('GET /status.html returns 200', async () => { + it('EXPECTED BEHAVIOR: GET /status should redirect to /status.html with 301', async () => { + const fixedApp = createFixedApp() + const res = await request(fixedApp).get('/status').expect(301) + expect(res.headers['location']).toBe('/status.html') + }) + + it('GET /status.html should always return 200', async () => { + const app = createBuggyApp() // This works the same in both cases const res = await request(app).get('/status.html') expect(res.status).toBe(200) expect(res.headers['content-type']).toContain('text/html') diff --git a/src/routes/playground.ts b/src/routes/playground.ts index a444c3e..b62c2ca 100644 --- a/src/routes/playground.ts +++ b/src/routes/playground.ts @@ -6,6 +6,24 @@ import rateLimit from "express-rate-limit"; export const playgroundRouter = Router(); +// URL validation middleware - runs BEFORE rate limiting +const urlValidationMiddleware = (req: any, res: any, next: any) => { + const { url } = req.body; + + if (!url || typeof url !== "string") { + res.status(400).json({ error: "Missing required parameter: url" }); + return; + } + + // Check URL length (same validation that happens in SSRF service) + if (url.length > 2048) { + res.status(400).json({ error: "Invalid URL: must be between 1 and 2048 characters" }); + return; + } + + next(); +}; + // 5 requests per hour per IP const playgroundLimiter = rateLimit({ windowMs: 60 * 60 * 1000, @@ -83,13 +101,10 @@ const playgroundLimiter = rateLimit({ * application/json: * schema: { $ref: "#/components/schemas/Error" } */ -playgroundRouter.post("/", playgroundLimiter, async (req, res) => { +playgroundRouter.post("/", urlValidationMiddleware, playgroundLimiter, async (req, res) => { const { url, format, width, height, fullPage, quality, waitForSelector, deviceScale, waitUntil, pdfFormat, pdfLandscape, pdfPrintBackground, pdfScale, pdfMargin } = req.body; - if (!url || typeof url !== "string") { - res.status(400).json({ error: "Missing required parameter: url" }); - return; - } + // URL validation is now handled by middleware before rate limiting // Enforce reasonable limits for playground const safeWidth = Math.min(Math.max(parseInt(width, 10) || 1280, 320), 1920); diff --git a/src/routes/status.ts b/src/routes/status.ts deleted file mode 100644 index 7b2f0a1..0000000 --- a/src/routes/status.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { Router } from "express"; -import path from "path"; -import { fileURLToPath } from "url"; -const __dirname = path.dirname(fileURLToPath(import.meta.url)); -const router = Router(); - -/** - * @openapi - * /status: - * get: - * tags: [System] - * summary: Service status page - * operationId: statusPage - * responses: - * 200: - * description: HTML status page - * content: - * text/html: - * schema: { type: string } - */ -router.get("/", (_req, res) => { - res.sendFile(path.join(__dirname, "../../public/status.html")); -}); -export { router as statusRouter };