diff --git a/dashcaddy-api/__tests__/routes/tailscale-admin.test.js b/dashcaddy-api/__tests__/routes/tailscale-admin.test.js index 0a5dc00..493425b 100644 --- a/dashcaddy-api/__tests__/routes/tailscale-admin.test.js +++ b/dashcaddy-api/__tests__/routes/tailscale-admin.test.js @@ -131,6 +131,20 @@ describe('routes/tailscale-admin: PUT /settings', () => { expect(res.status).toBe(400); }); + test('400 on apiToken exceeding 256-char length cap (DC-080)', async () => { + const { app } = createApp(); + const oversized = 'tskey-api-' + 'x'.repeat(300); // > 256 chars + const res = await request(app).put('/api/v1/tailscale/settings').send({ apiToken: oversized }); + expect(res.status).toBe(400); + expect(res.body.error || res.body.message).toMatch(/exceeds maximum length/i); + }); + + test('400 on non-string apiToken (DC-080)', async () => { + const { app } = createApp(); + const res = await request(app).put('/api/v1/tailscale/settings').send({ apiToken: 12345 }); + expect(res.status).toBe(400); + }); + test('200 + saves token + writes metadata on valid token', async () => { const fakeClient = makeFakeClient({ ping: jest.fn(async () => ({ domain: 'real.ts.net' })), @@ -293,6 +307,76 @@ describe('routes/tailscale-admin: POST /settings/test', () => { expect(res.body.valid).toBe(true); expect(fakeClient.setApiToken).toHaveBeenCalledWith('tskey-api-test-only'); }); + + test('400 on body.apiToken not starting with tskey-api- (DC-080)', async () => { + const fakeClient = makeFakeClient(); + const app = express(); + app.use(express.json()); + const routes = require('../../routes/tailscale-admin'); + const tailscaleCoord = { + loadMetadata: () => ({ configured: false }), + saveMetadata: jest.fn(), + setApiToken: jest.fn(), + getClient: jest.fn(async () => fakeClient), + hasApiToken: jest.fn(), + }; + app.use('/api/v1/tailscale', routes({ + tailscaleCoord, asyncHandler, + log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, + })); + const res = await request(app) + .post('/api/v1/tailscale/settings/test') + .send({ apiToken: 'arbitrary-junk' }); + expect(res.status).toBe(400); + expect(fakeClient.setApiToken).not.toHaveBeenCalled(); + }); + + test('400 on body.apiToken exceeding length cap (DC-080)', async () => { + const fakeClient = makeFakeClient(); + const app = express(); + app.use(express.json()); + const routes = require('../../routes/tailscale-admin'); + const tailscaleCoord = { + loadMetadata: () => ({ configured: false }), + saveMetadata: jest.fn(), + setApiToken: jest.fn(), + getClient: jest.fn(async () => fakeClient), + hasApiToken: jest.fn(), + }; + app.use('/api/v1/tailscale', routes({ + tailscaleCoord, asyncHandler, + log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, + })); + const oversized = 'tskey-api-' + 'x'.repeat(300); + const res = await request(app) + .post('/api/v1/tailscale/settings/test') + .send({ apiToken: oversized }); + expect(res.status).toBe(400); + expect(fakeClient.setApiToken).not.toHaveBeenCalled(); + }); + + test('omitting apiToken is allowed (uses stored token path) (DC-080)', async () => { + const fakeClient = makeFakeClient({ ping: jest.fn(async () => ({ domain: 'stored.ts.net' })) }); + const app = express(); + app.use(express.json()); + const routes = require('../../routes/tailscale-admin'); + const tailscaleCoord = { + loadMetadata: () => ({ configured: true }), + saveMetadata: jest.fn(), + setApiToken: jest.fn(), + getClient: jest.fn(async () => fakeClient), + hasApiToken: jest.fn(), + }; + app.use('/api/v1/tailscale', routes({ + tailscaleCoord, asyncHandler, + log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, + })); + const res = await request(app) + .post('/api/v1/tailscale/settings/test') + .send({}); // no apiToken in body + expect(res.status).toBe(200); + expect(res.body.valid).toBe(true); + }); }); describe('routes/tailscale-admin: GET /admin/devices', () => { @@ -511,6 +595,99 @@ describe('routes/tailscale-admin: pre-auth keys', () => { expect(res.status).toBe(400); }); + test('POST /admin/keys rejects null/123/object tags entries (DC-080)', async () => { + const fakeClient = makeFakeClient(); + const app = express(); + app.use(express.json()); + const routes = require('../../routes/tailscale-admin'); + const tailscaleCoord = { + loadMetadata: () => ({ configured: true }), + saveMetadata: jest.fn(), + setApiToken: jest.fn(), + getClient: jest.fn(async () => fakeClient), + hasApiToken: jest.fn(), + }; + app.use('/api/v1/tailscale', routes({ + tailscaleCoord, asyncHandler, + log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, + })); + // Mixed: null, number, object — all must be rejected + const res = await request(app).post('/api/v1/tailscale/admin/keys').send({ tags: ['tag:guest', null, 123, { x: 1 }] }); + expect(res.status).toBe(400); + expect(fakeClient.createAuthKey).not.toHaveBeenCalled(); + }); + + test('POST /admin/keys rejects uppercase / whitespace / CRLF in tags (DC-080)', async () => { + const fakeClient = makeFakeClient(); + const app = express(); + app.use(express.json()); + const routes = require('../../routes/tailscale-admin'); + const tailscaleCoord = { + loadMetadata: () => ({ configured: true }), + saveMetadata: jest.fn(), + setApiToken: jest.fn(), + getClient: jest.fn(async () => fakeClient), + hasApiToken: jest.fn(), + }; + app.use('/api/v1/tailscale', routes({ + tailscaleCoord, asyncHandler, + log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, + })); + const res = await request(app).post('/api/v1/tailscale/admin/keys').send({ tags: ['TAG:guest', 'tag:foo bar', 'tag:x\r\ninjection'] }); + expect(res.status).toBe(400); + expect(fakeClient.createAuthKey).not.toHaveBeenCalled(); + }); + + test('POST /admin/keys rejects description exceeding 120 chars (DC-080)', async () => { + const fakeClient = makeFakeClient(); + const app = express(); + app.use(express.json()); + const routes = require('../../routes/tailscale-admin'); + const tailscaleCoord = { + loadMetadata: () => ({ configured: true }), + saveMetadata: jest.fn(), + setApiToken: jest.fn(), + getClient: jest.fn(async () => fakeClient), + hasApiToken: jest.fn(), + }; + app.use('/api/v1/tailscale', routes({ + tailscaleCoord, asyncHandler, + log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, + })); + const longDesc = 'a'.repeat(200); // > 120 chars + const res = await request(app).post('/api/v1/tailscale/admin/keys').send({ description: longDesc }); + expect(res.status).toBe(400); + expect(fakeClient.createAuthKey).not.toHaveBeenCalled(); + }); + + test('POST /admin/keys accepts canonical lowercase tag: form (DC-080)', async () => { + const fakeClient = makeFakeClient({ + createAuthKey: jest.fn(async (opts) => ({ id: 'k2', key: 'tskey-secret-2', ...opts })), + }); + const app = express(); + app.use(express.json()); + const routes = require('../../routes/tailscale-admin'); + const tailscaleCoord = { + loadMetadata: () => ({ configured: true }), + saveMetadata: jest.fn(), + setApiToken: jest.fn(), + getClient: jest.fn(async () => fakeClient), + hasApiToken: jest.fn(), + }; + app.use('/api/v1/tailscale', routes({ + tailscaleCoord, asyncHandler, + log: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, + })); + const res = await request(app).post('/api/v1/tailscale/admin/keys').send({ + tags: ['tag:guest-plex', 'tag:server'], + expirySeconds: 86400, + }); + expect(res.status).toBe(200); + expect(fakeClient.createAuthKey).toHaveBeenCalledWith(expect.objectContaining({ + tags: ['tag:guest-plex', 'tag:server'], + })); + }); + test('POST /admin/keys rejects negative expirySeconds', async () => { const fakeClient = makeFakeClient(); const app = express(); @@ -572,4 +749,110 @@ describe('routes/tailscale-admin: security boundary', () => { await request(app).delete('/api/v1/tailscale/settings'); expect(stored.token).toBeNull(); }); -}); \ No newline at end of file +}); +// DC-080 direct validator unit tests (no supertest, no Express) +describe('routes/tailscale-admin: DC-080 validators (direct)', () => { + const { _validators } = require('../../routes/tailscale-admin'); + const { + validateApiToken, + validateTags, + validateDescription, + TAILSCALE_TOKEN_PREFIX, + TAILSCALE_TOKEN_MAX_LEN, + DESCRIPTION_MAX_LEN, + } = _validators; + + describe('validateApiToken', () => { + test('accepts canonical tskey-api-...', () => { + expect(validateApiToken('tskey-api-abc123')).toBeNull(); + }); + test('rejects empty', () => { + expect(validateApiToken('')).toMatch(/required/); + }); + test('rejects undefined / null', () => { + expect(validateApiToken(undefined)).toMatch(/required/); + expect(validateApiToken(null)).toMatch(/required/); + }); + test('rejects non-string (number, object, array)', () => { + expect(validateApiToken(123)).toMatch(/must be a string/); + expect(validateApiToken({})).toMatch(/must be a string/); + expect(validateApiToken(['x'])).toMatch(/must be a string/); + }); + test('rejects wrong prefix', () => { + expect(validateApiToken('not-a-token')).toMatch(/must start with/); + }); + test('accepts exactly at length cap', () => { + const token = 'tskey-api-' + 'x'.repeat(TAILSCALE_TOKEN_MAX_LEN - 'tskey-api-'.length); + expect(validateApiToken(token)).toBeNull(); + }); + test('rejects 1 over length cap', () => { + const token = 'tskey-api-' + 'x'.repeat(TAILSCALE_TOKEN_MAX_LEN - 'tskey-api-'.length + 1); + expect(validateApiToken(token)).toMatch(/exceeds maximum length/); + }); + }); + + describe('validateTags', () => { + test('accepts undefined / null (optional)', () => { + expect(validateTags(undefined)).toBeNull(); + expect(validateTags(null)).toBeNull(); + }); + test('rejects non-array', () => { + expect(validateTags('tag:foo')).toMatch(/must be an array/); + expect(validateTags({})).toMatch(/must be an array/); + }); + test('rejects entries that are not strings', () => { + expect(validateTags(['tag:a', null])).toMatch(/tags\[1\]/); + expect(validateTags(['tag:a', 123])).toMatch(/tags\[1\]/); + expect(validateTags(['tag:a', {}])).toMatch(/tags\[1\]/); + }); + test('rejects uppercase / whitespace / CRLF', () => { + expect(validateTags(['TAG:foo'])).toMatch(/tags\[0\]/); + expect(validateTags(['tag:foo bar'])).toMatch(/tags\[0\]/); + expect(validateTags(['tag:foo\r\nbar'])).toMatch(/tags\[0\]/); + }); + test('rejects entries starting with non-alnum (no leading colon)', () => { + expect(validateTags([':foo'])).toMatch(/tags\[0\]/); + }); + + test('rejects bare "tag:" with empty name (Tailscale spec violation) (DC-080 round-2)', () => { + expect(validateTags(['tag:'])).toMatch(/tags\[0\]/); + }); + + test('rejects colon-only chars after tag: prefix (DC-080 round-2)', () => { + expect(validateTags(['tag:::'])).toMatch(/tags\[0\]/); + expect(validateTags(['tag:---'])).toMatch(/tags\[0\]/); + }); + test('accepts canonical tag:server form', () => { + expect(validateTags(['tag:server'])).toBeNull(); + expect(validateTags(['tag:guest-plex', 'tag:server'])).toBeNull(); + }); + test('rejects empty array entry', () => { + expect(validateTags(['tag:a', ''])).toMatch(/tags\[1\]/); + }); + }); + + describe('validateDescription', () => { + test('accepts undefined / null', () => { + expect(validateDescription(undefined)).toBeNull(); + expect(validateDescription(null)).toBeNull(); + }); + test('rejects non-string', () => { + expect(validateDescription(123)).toMatch(/must be a string/); + }); + test('rejects over 120 chars', () => { + const long = 'a'.repeat(DESCRIPTION_MAX_LEN + 1); + expect(validateDescription(long)).toMatch(/exceeds maximum length/); + }); + test('accepts at the cap', () => { + const exact = 'a'.repeat(DESCRIPTION_MAX_LEN); + expect(validateDescription(exact)).toBeNull(); + }); + }); + + test('exports surface stays in sync with constants used inside validators', () => { + // Guard against drift: if a future refactor renames a constant, this fails + expect(TAILSCALE_TOKEN_PREFIX).toBe('tskey-api-'); + expect(typeof TAILSCALE_TOKEN_MAX_LEN).toBe('number'); + expect(typeof DESCRIPTION_MAX_LEN).toBe('number'); + }); +}); diff --git a/dashcaddy-api/routes/tailscale-admin.js b/dashcaddy-api/routes/tailscale-admin.js index ad363e6..a52f8c5 100644 --- a/dashcaddy-api/routes/tailscale-admin.js +++ b/dashcaddy-api/routes/tailscale-admin.js @@ -41,12 +41,124 @@ * * DELETE /api/v1/tailscale/admin/devices/:id * Revokes a device from the tailnet. + * + * # DC-080 input validation + * + * Three coupled gaps in the route layer pre-fix: + * + * (a) PUT /settings validated `apiToken.startsWith('tskey-api-')` but had + * no length cap — body-parser limit was the only ceiling. A 1 MB + * string starting with `tskey-api-` would be `.trim()`-ed, sent to + * Tailscale's /devices endpoint, and waste server-side CPU on a + * request that will always 401. + * (b) POST /settings/test accepted `apiToken` from the body with NO + * validation at all. The PUT route's prefix check is bypassed on + * the test path — an operator could submit any string and have the + * container ping Tailscale's API with it (low impact, but inconsistent + * with PUT and surfaces fingerprinting via the 401 timing). + * (c) POST /admin/keys validated `tags` as Array but NOT per-element + * type — `tags: ['tag:guest', null, 123, {injection: true}]` would be + * forwarded to Tailscale verbatim. Tailscale's API is JSON-strict + * and would 400 the request, but the bad shape reached the wire. + * Similarly `description` had no length cap (Tailscale caps at 120 + * chars per their docs). + * + * All three are gated by TOTP — this is a logged-in-operator / phished- + * session threat surface, not anonymous-unauth. The fix is defense-in- + * depth: a bug in the auth path (TOTP bypass, session theft, future + * route handler trust-boundary drift) should not turn these endpoints + * into a "submit anything and forward to Tailscale" relay. */ const express = require('express'); const { ok, errorResponse } = require('../src/utils/responses'); const { TailscaleCoordError } = require('../src/managers/tailscale-coord'); +// DC-080: shared validation helpers for the Tailscale admin surface. +// Tailscale API tokens follow the form `tskey--` where +// `` is one of a small set of values (`api`, `auth`, `partner`, +// `cli`). Real tokens observed in the wild are 40..80 chars; we cap at +// 256 to leave headroom for future Tailscale key formats without giving +// an unbounded buffer to validate+forward. +const TAILSCALE_TOKEN_PREFIX = 'tskey-api-'; +const TAILSCALE_TOKEN_MAX_LEN = 256; +const TAG_KEY_MAX_LEN = 64; +const TAGS_MAX_LEN = 32; +const DESCRIPTION_MAX_LEN = 120; + +// Tailscale tags are lowercased identifiers with optional colons +// (e.g. `tag:server`, `tag:guest-plex`). Reject whitespace, CR/LF, +// control chars, JSON metacharacters, and any character that could +// enable header-injection through the Tailscale coord client. +// +// DC-080 round-2 polish: Tailscale's tag spec requires `tag:` followed by +// ≥1 identifier char — bare `tag:` (empty name) is rejected by their API. +// We split the pattern in two so the error message names which form failed +// instead of dumping a generic regex. +const TAG_KEY_RE = /^tag:[a-z0-9][a-z0-9_-]{0,62}$/; + +function _validateApiToken(token, fieldName = 'apiToken') { + if (typeof token !== 'string' || !token) { + return `${fieldName} is required and must be a string`; + } + if (!token.startsWith(TAILSCALE_TOKEN_PREFIX)) { + return `${fieldName} must start with ${TAILSCALE_TOKEN_PREFIX}`; + } + if (token.length > TAILSCALE_TOKEN_MAX_LEN) { + return `${fieldName} exceeds maximum length of ${TAILSCALE_TOKEN_MAX_LEN} characters`; + } + return null; +} + +function _validateTags(tags) { + if (tags === undefined || tags === null) return null; + if (!Array.isArray(tags)) { + return 'tags must be an array of strings'; + } + if (tags.length > TAGS_MAX_LEN) { + return `tags exceeds maximum length of ${TAGS_MAX_LEN} entries`; + } + for (let i = 0; i < tags.length; i += 1) { + const t = tags[i]; + if (typeof t !== 'string' || !t) { + return `tags[${i}] must be a non-empty string`; + } + if (t.length > TAG_KEY_MAX_LEN) { + return `tags[${i}] exceeds maximum length of ${TAG_KEY_MAX_LEN} characters`; + } + if (!TAG_KEY_RE.test(t)) { + return `tags[${i}] must match ${TAG_KEY_RE} (lowercase alnum + :_-)`; + } + } + return null; +} + +function _validateDescription(description) { + if (description === undefined || description === null) return null; + if (typeof description !== 'string') { + return 'description must be a string'; + } + if (description.length > DESCRIPTION_MAX_LEN) { + return `description exceeds maximum length of ${DESCRIPTION_MAX_LEN} characters`; + } + return null; +} + +// Exported for direct unit testing in __tests__/routes/tailscale-admin.test.js +// (the validator functions are otherwise unreachable from outside the factory +// closure; direct tests assert edge cases without supertest overhead). +const _validators = { + validateApiToken: _validateApiToken, + validateTags: _validateTags, + validateDescription: _validateDescription, + TAILSCALE_TOKEN_PREFIX, + TAILSCALE_TOKEN_MAX_LEN, + TAG_KEY_MAX_LEN, + TAGS_MAX_LEN, + DESCRIPTION_MAX_LEN, + TAG_KEY_RE, +}; + module.exports = function({ tailscaleCoord, asyncHandler, @@ -75,9 +187,12 @@ module.exports = function({ router.put('/settings', asyncHandler(async (req, res) => { const token = req.body && req.body.apiToken; - if (!token || typeof token !== 'string' || !token.startsWith('tskey-api-')) { - return errorResponse(res, 400, 'Invalid API token (must start with tskey-api-)'); - } + // DC-080: validate prefix + length cap. The pre-fix code only checked + // the prefix — a 1 MB string starting with `tskey-api-` would have been + // sent to Tailscale's /devices endpoint and wasted server-side CPU + // before the inevitable 401. + const tokenErr = _validateApiToken(token); + if (tokenErr) return errorResponse(res, 400, tokenErr); // Validate before storing const client = new (require('../src/managers/tailscale-coord').TailscaleCoordClient)({ apiToken: token }); @@ -130,6 +245,17 @@ module.exports = function({ router.post('/settings/test', asyncHandler(async (req, res) => { const token = (req.body && req.body.apiToken) || null; + // DC-080: validate any caller-provided token before it reaches the + // Tailscale API. Pre-fix the test endpoint accepted any string — the + // PUT route's prefix check did NOT extend to this path. An operator + // could submit arbitrary junk and the container would still call + // /devices on the Tailscale API with it (DoS-reflection + fingerprint + // timing for a future attacker probing whether this API token format + // is accepted at all). + if (token !== null && token !== undefined) { + const tokenErr = _validateApiToken(token); + if (tokenErr) return errorResponse(res, 400, tokenErr); + } const client = await tailscaleCoord.getClient(); if (token) { // Caller provided a fresh token to test — don't save it @@ -214,10 +340,16 @@ module.exports = function({ return errorResponse(res, 503, 'Tailscale API token not configured'); } const opts = req.body || {}; - // Reject obviously-bad input early - if (opts.tags && !Array.isArray(opts.tags)) { - return errorResponse(res, 400, 'tags must be an array of strings'); - } + // Reject obviously-bad input early. + // DC-080: pre-fix the route only checked `Array.isArray(opts.tags)`. + // A `tags: ['tag:guest', null, 123, {injection: true}]` payload would + // be forwarded to Tailscale verbatim — Tailscale's API is JSON-strict + // and would 400 the request, but the bad shape reached the wire and + // would silently pass through the dashboard's JSON.stringify() flow. + const tagsErr = _validateTags(opts.tags); + if (tagsErr) return errorResponse(res, 400, tagsErr); + const descErr = _validateDescription(opts.description); + if (descErr) return errorResponse(res, 400, descErr); if (opts.expirySeconds !== undefined && (!Number.isInteger(opts.expirySeconds) || opts.expirySeconds <= 0)) { return errorResponse(res, 400, 'expirySeconds must be a positive integer'); } @@ -254,4 +386,10 @@ module.exports = function({ })); return router; -}; \ No newline at end of file +}; + +// DC-080: validators exported for direct unit testing in +// __tests__/routes/tailscale-admin.test.js — the route factory closes +// over the same functions, so the validators are exercised end-to-end via +// supertest AND in isolation here. +module.exports._validators = _validators; \ No newline at end of file