Merge dc/DC-080: Tailscale admin endpoint validation hardening (DC-080) [glm-grade=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();
   });
-});
+});
+// 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');
+  });
+});

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-<kind>-<opaque>` where
+// `<kind>` 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;
-};
+};
+
+// 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;