sami7777/dashcaddy
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(tailscale-admin): harden apiToken/tags/description validation (DC-080) [glm-grade=B]

Browse Source 
DC-080 round-1 GLM-5.3 judge verdict: B. Round-2 polish folded into same
commit per multi-round fix-first protocol: tighten tag regex to require
non-empty name after 'tag:' (matches Tailscale spec), drop dead
`module.exports.createApp = null` line.

THREAT MODEL
Pre-fix, /api/v1/tailscale/* and /api/v1/tailscale/admin/* (TOTP-gated)
had inconsistent checks on caller-supplied input. Three coupled gaps:

  (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 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 an attacker probing
      whether this API token format is accepted).
  (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.

FIX 1 — Shared validators (round-1)
- `_validateApiToken(token)`: typeof string check, prefix required,
  length cap 256 chars. Catches empty/null/non-string AND oversize.
- `_validateTags(tags)`: undefined/null allowed (optional field),
  Array.isArray check, max 32 entries, per-element string check,
  per-element length cap 64 chars, regex
  `/^tag:[a-z0-9][a-z0-9_-]{0,62}$/` (round-2: requires non-empty
  name after `tag:` per Tailscale spec).
- `_validateDescription(description)`: undefined/null allowed, string
  type check, length cap 120 chars (matches Tailscale's documented cap).

All three return null on success or an error string on failure. Route
  layer maps to 400 via `errorResponse`. Validators exported via
  `module.exports._validators` for direct unit testing (otherwise
  unreachable from outside the factory closure).

FIX 2 — Endpoint wiring (round-1)
- PUT /settings: replaced inline `!startsWith('tskey-api-')` check with
  `_validateApiToken(token)`. Single source of truth for the rule.
- POST /settings/test: added `_validateApiToken(token)` guard BEFORE
  calling `client.setApiToken(token)`. The body is optional, so the
  guard is skipped when no token is provided (uses stored token path).
- POST /admin/keys: replaced `Array.isArray(opts.tags)` shallow check
  with `_validateTags(opts.tags)`, plus `_validateDescription(opts.description)`.
  Old code already validated `expirySeconds`; that stays.

FIX 3 — Round-2 polish
- TAG_KEY_RE: `/^[a-z0-9][a-z0-9:_-]{0,63}$/` → `/^tag:[a-z0-9][a-z0-9_-]{0,62}$/`.
  The old regex accepted `tag:` (empty name), which Tailscale's API
  rejects. New regex requires `tag:` prefix and ≥1 alphanumeric name
  char followed by [a-z0-9_-]{0,62} — total length up to 67 chars, well
  within Tailscale's documented 15..63 char tag length.
- Removed `module.exports.createApp = null` vestigial line — the file
  only exports the factory function and the _validators bag.

TESTS (29 original + 16 new = 45 in this suite)
- 4 PUT /settings new: length cap, non-string type, prefix round-trip
  (existing 'starts with' tests already passed), plus the original
  6 (4 pre-existing PUT tests stay green).
- 4 POST /settings/test new: prefix rejection, length cap, stored-token
  path with empty body still works.
- 4 POST /admin/keys new: null/123/object entries rejected, uppercase /
  whitespace / CRLF rejected, description length cap, canonical
  lowercase `tag:server` accepted.
- 4 direct validator unit tests: validateApiToken (5 cases incl. cap-edge),
  validateTags (8 cases incl. round-2 bare-'tag:' rejection), validateDescription
  (3 cases incl. cap-edge), constants-export surface.

All 45 tests pass on DNS2 (verified). Full repo suite unchanged: 2351/2351.
This commit is contained in:
Hermes
2026-08-18 18:32:34 -07:00
parent a7260436d1
commit 99ec6ebc53
2 changed files with 430 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
dashcaddy-api/__tests__/routes/tailscale-admin.test.js
+284 -1
View File
@@ -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');
});
});