Files
sencho/backend/src/__tests__/stack-compose-mtime.test.ts
T
Anso fbd13accda feat(stacks): optimistic concurrency on compose and env file writes (#1183)
* feat(stacks): optimistic concurrency on compose and env file writes

Two browser tabs (or one tab + an out-of-band edit) could silently
overwrite each other's compose.yaml or .env edits. GET /api/stacks/:name
and GET /api/stacks/:name/env now emit a W/"<mtime>" ETag header. PUT
on the same endpoints reads If-Match and returns 412 with
{code: 'stack_file_changed', currentMtimeMs, currentContent} on a stale
write, so the editor can recover without losing the user's text.

The 412 path in the editor surfaces a confirm dialog: "Overwrite
their changes?" Cancel loads the latest content into the editor and
exits edit mode. OK retries the PUT with no If-Match header.

If-Match is optional. A client that doesn't send it falls through to
the previous unconditional-write behavior so partial deploys (file
explorer uploads, git source sync) don't gain a surprise 412 surface.

* fix(stacks): consistent stat+read for compose mtime via held file handle

Promise.all([readFile, stat]) lets a concurrent write interleave between
the two calls: the read can return new content while the stat returns
the old mtime (or vice versa). The next If-Match check would then either
spuriously trigger 412 or silently allow an overwrite.

Hold the file descriptor open across stat and read so both ops observe
the same inode state. A rename-replace by another writer would not
affect the held fd's view.

Adds a near-boundary mtime test (1-second bump) to confirm the
Math.floor comparison detects whole-second changes on filesystems that
round to second-level precision.

* fix(stacks): defense-in-depth path validation in new compose mtime methods

CodeQL's taint engine on PR #1183 flagged 10 js/path-injection errors
across the four new FileSystemService methods (getStackContentWithMtime,
saveStackContentIfUnchanged, writeFileIfUnchanged, statMtime). The
engine does not follow the existing assertWithinBase guard across the
resolveStackDir / getComposeFilePath helper boundary; from its view the
stackName flows straight from req.params into a filesystem sink.

Eight alerts (getStackContentWithMtime + saveStackContentIfUnchanged)
were CodeQL blind-spot: the path WAS validated inside resolveStackDir.
Two alerts (writeFileIfUnchanged + statMtime) were genuinely missing
service-level guards because those methods accept a raw targetPath
from the caller and trusted the route to have validated upstream.

Add an explicit this.assertWithinBase(filePath) at the top of each
new method. The check is redundant for the two methods that already
went through resolveStackDir but makes the safety boundary visible
both to readers and to CodeQL's taint follower.

While here, port the held-file-descriptor pattern (already applied to
getStackContentWithMtime in dd7545eb) to the stat-then-read sequence
in saveStackContentIfUnchanged and writeFileIfUnchanged. This closes
the two js/file-system-race warnings on those branches so a concurrent
rename-replace cannot interleave between the mismatch detection and
the currentContent capture returned in the 412 payload.

The two remaining js/http-to-file-access warnings ("write to file
system depends on untrusted data") are semantic and intentional: this
is the save endpoint by design. They stay as warnings (not errors),
do not block CI, and are not suppressed because the codebase does not
do CodeQL suppression annotations.

* fix(stacks): inline path.resolve+startsWith barrier per CodeQL recommendation

The previous fix added this.assertWithinBase() calls at the top of each
new method. CodeQL's taint-flow analysis does not follow that helper
call across the function boundary, so it still saw the path as
user-tainted at every fs sink (10 -> 8 alerts after the first attempt).

CodeQL's documented js/path-injection sanitizer recognizes the
following inline pattern:

  filePath = path.resolve(ROOT, filePath);
  if (!filePath.startsWith(ROOT)) { throw / return; }
  // use the reassigned filePath below

The key elements are (1) path.resolve as the normalizer, (2) startsWith
check, (3) reject inline, (4) downstream use of the reassigned
variable. None of these can hide behind a helper call or the taint
tracker re-flags every sink.

Inlines the pattern in each of the four new methods (getStackContentWith-
Mtime, saveStackContentIfUnchanged, writeFileIfUnchanged, statMtime).
The check stays runtime-correct (it's the same logic isPathWithinBase
implements) but is now visible to static analysis.

writeFileIfUnchanged and statMtime had no service-level guard at all
before this commit (they trusted the caller); the inline barrier closes
that real gap as well as the CodeQL-recognition gap.

* fix(stacks): canonical CodeQL js/path-injection barrier shape

The prior inline check used path.resolve(filePath) with a single
argument and a compound condition (a !== b && !c.startsWith(d)).
CodeQL's path-injection sanitizer recognizer is shape-sensitive: it
matches path.resolve(SAFE_ROOT, untrusted) with the safe root as the
first argument, followed by a single unary startsWith check on the
resolved variable. The compound form and the single-arg resolve fell
outside the recognized pattern, leaving 8 alerts unchanged across the
four new methods.

Rewrite the barrier in each method to match the documented shape
verbatim:

  const baseResolved = path.resolve(this.baseDir);
  const safePath = path.resolve(baseResolved, untrustedInput);
  if (!safePath.startsWith(baseResolved + path.sep)) {
    throw ...;
  }
  // sinks consume safePath

baseResolved is a local variable (anchors the resolve call against a
known-safe root). safePath is the reassigned, sanitized variable that
every downstream fs.* call consumes. The single startsWith check with
path.sep appended prevents the prefix-match edge case (/foo matches
/foobar without the separator). All four affected methods get the
same form.

This is the third attempt at the CodeQL fix. The first added an
assertWithinBase helper (function call, not followed across the
boundary). The second inlined path.resolve(x) with a compound check
(non-canonical shape). This commit uses the literal recommended
sanitizer.
2026-05-24 15:48:17 -04:00

237 lines
8.3 KiB
TypeScript

/**
* Integration tests for optimistic concurrency on PUT /api/stacks/:name and
* PUT /api/stacks/:name/env.
*
* GET returns the file content with an `ETag` header carrying the mtimeMs.
* The frontend echoes that as `If-Match` on save. When the file on disk has
* mutated in the interim, the server returns 412 with the current content
* and mtime so the caller can show a "file changed" recovery sheet.
*
* These tests use real fs ops against a temp COMPOSE_DIR rather than mocks
* because the contract is specifically about mtime semantics.
*/
import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest';
import request from 'supertest';
import fs from 'fs';
import path from 'path';
import { setupTestDb, cleanupTestDb, loginAsTestAdmin } from './helpers/setupTestDb';
let tmpDir: string;
let composeDir: string;
let app: import('express').Express;
let authCookie: string;
const STACK = 'web';
function seedStack(stackName: string, content: string): string {
const stackDir = path.join(composeDir, stackName);
fs.mkdirSync(stackDir, { recursive: true });
const filePath = path.join(stackDir, 'compose.yaml');
fs.writeFileSync(filePath, content, 'utf-8');
return filePath;
}
function seedEnv(stackName: string, content: string): string {
const stackDir = path.join(composeDir, stackName);
fs.mkdirSync(stackDir, { recursive: true });
const envPath = path.join(stackDir, '.env');
fs.writeFileSync(envPath, content, 'utf-8');
return envPath;
}
function parseEtag(etag: string | undefined): number | null {
if (!etag) return null;
const m = etag.match(/(?:W\/)?"(\d+)"/);
return m ? Number(m[1]) : null;
}
beforeAll(async () => {
tmpDir = await setupTestDb();
composeDir = process.env.COMPOSE_DIR as string;
({ app } = await import('../index'));
authCookie = await loginAsTestAdmin(app);
});
afterAll(() => {
cleanupTestDb(tmpDir);
});
beforeEach(() => {
const stackDir = path.join(composeDir, STACK);
if (fs.existsSync(stackDir)) {
fs.rmSync(stackDir, { recursive: true, force: true });
}
});
describe('GET /api/stacks/:stackName emits ETag with mtime', () => {
it('responds 200 with content body and a W/"<mtime>" ETag header', async () => {
seedStack(STACK, 'services:\n web:\n image: nginx\n');
const res = await request(app)
.get(`/api/stacks/${STACK}`)
.set('Cookie', authCookie);
expect(res.status).toBe(200);
expect(res.text).toContain('image: nginx');
const etag = res.headers.etag;
expect(etag).toMatch(/^W\/"\d+"$/);
expect(parseEtag(etag)).toBeGreaterThan(0);
});
});
describe('PUT /api/stacks/:stackName optimistic concurrency', () => {
it('writes successfully when If-Match matches current mtime', async () => {
seedStack(STACK, 'original');
const getRes = await request(app).get(`/api/stacks/${STACK}`).set('Cookie', authCookie);
const etag = getRes.headers.etag as string;
const putRes = await request(app)
.put(`/api/stacks/${STACK}`)
.set('Cookie', authCookie)
.set('If-Match', etag)
.send({ content: 'updated' });
expect(putRes.status).toBe(200);
expect(putRes.headers.etag).toMatch(/^W\/"\d+"$/);
expect(typeof putRes.body.mtimeMs).toBe('number');
const filePath = path.join(composeDir, STACK, 'compose.yaml');
expect(fs.readFileSync(filePath, 'utf-8')).toBe('updated');
});
it('returns 412 with stack_file_changed and the current content on If-Match mismatch', async () => {
seedStack(STACK, 'original');
const getRes = await request(app).get(`/api/stacks/${STACK}`).set('Cookie', authCookie);
const etag = getRes.headers.etag as string;
const filePath = path.join(composeDir, STACK, 'compose.yaml');
fs.writeFileSync(filePath, 'changed-by-other-tab', 'utf-8');
const future = Date.now() + 5_000;
fs.utimesSync(filePath, future / 1000, future / 1000);
const putRes = await request(app)
.put(`/api/stacks/${STACK}`)
.set('Cookie', authCookie)
.set('If-Match', etag)
.send({ content: 'my-overwrite' });
expect(putRes.status).toBe(412);
expect(putRes.body).toMatchObject({
code: 'stack_file_changed',
currentContent: 'changed-by-other-tab',
});
expect(typeof putRes.body.currentMtimeMs).toBe('number');
expect(putRes.headers.etag).toMatch(/^W\/"\d+"$/);
expect(fs.readFileSync(filePath, 'utf-8')).toBe('changed-by-other-tab');
});
it('writes through when If-Match is absent', async () => {
seedStack(STACK, 'original');
const putRes = await request(app)
.put(`/api/stacks/${STACK}`)
.set('Cookie', authCookie)
.send({ content: 'forced' });
expect(putRes.status).toBe(200);
const filePath = path.join(composeDir, STACK, 'compose.yaml');
expect(fs.readFileSync(filePath, 'utf-8')).toBe('forced');
});
it('writes through when the file does not exist yet (first save creates compose.yaml)', async () => {
const stackDir = path.join(composeDir, STACK);
fs.mkdirSync(stackDir, { recursive: true });
fs.writeFileSync(path.join(stackDir, 'docker-compose.yaml'), 'legacy', 'utf-8');
const putRes = await request(app)
.put(`/api/stacks/${STACK}`)
.set('Cookie', authCookie)
.set('If-Match', 'W/"1234"')
.send({ content: 'fresh' });
expect(putRes.status).toBe(200);
expect(fs.readFileSync(path.join(stackDir, 'compose.yaml'), 'utf-8')).toBe('fresh');
});
it('detects a near-boundary mtime change (Math.floor precision)', async () => {
seedStack(STACK, 'original');
const getRes = await request(app).get(`/api/stacks/${STACK}`).set('Cookie', authCookie);
const etag = getRes.headers.etag as string;
const filePath = path.join(composeDir, STACK, 'compose.yaml');
const originalStat = fs.statSync(filePath);
// Bump the mtime by exactly one full second so Math.floor(mtimeMs) is
// guaranteed to differ even on filesystems that round to whole seconds.
fs.writeFileSync(filePath, 'changed-just-after', 'utf-8');
const bumped = (originalStat.mtimeMs + 1000) / 1000;
fs.utimesSync(filePath, bumped, bumped);
const putRes = await request(app)
.put(`/api/stacks/${STACK}`)
.set('Cookie', authCookie)
.set('If-Match', etag)
.send({ content: 'my-edit' });
expect(putRes.status).toBe(412);
expect(putRes.body.code).toBe('stack_file_changed');
});
it('ignores malformed If-Match headers and writes through', async () => {
seedStack(STACK, 'original');
const putRes = await request(app)
.put(`/api/stacks/${STACK}`)
.set('Cookie', authCookie)
.set('If-Match', 'not-a-valid-etag')
.send({ content: 'forced' });
expect(putRes.status).toBe(200);
});
});
describe('PUT /api/stacks/:stackName/env optimistic concurrency', () => {
it('writes successfully when If-Match matches', async () => {
seedStack(STACK, 'services: {}');
const envPath = seedEnv(STACK, 'FOO=1');
const getRes = await request(app)
.get(`/api/stacks/${STACK}/env?file=${encodeURIComponent(envPath)}`)
.set('Cookie', authCookie);
const etag = getRes.headers.etag as string;
expect(etag).toMatch(/^W\/"\d+"$/);
const putRes = await request(app)
.put(`/api/stacks/${STACK}/env?file=${encodeURIComponent(envPath)}`)
.set('Cookie', authCookie)
.set('If-Match', etag)
.send({ content: 'FOO=2' });
expect(putRes.status).toBe(200);
expect(fs.readFileSync(envPath, 'utf-8')).toBe('FOO=2');
});
it('returns 412 on env-file mtime mismatch', async () => {
seedStack(STACK, 'services: {}');
const envPath = seedEnv(STACK, 'FOO=1');
const getRes = await request(app)
.get(`/api/stacks/${STACK}/env?file=${encodeURIComponent(envPath)}`)
.set('Cookie', authCookie);
const etag = getRes.headers.etag as string;
fs.writeFileSync(envPath, 'FOO=bumped', 'utf-8');
const future = Date.now() + 5_000;
fs.utimesSync(envPath, future / 1000, future / 1000);
const putRes = await request(app)
.put(`/api/stacks/${STACK}/env?file=${encodeURIComponent(envPath)}`)
.set('Cookie', authCookie)
.set('If-Match', etag)
.send({ content: 'FOO=my-overwrite' });
expect(putRes.status).toBe(412);
expect(putRes.body.code).toBe('stack_file_changed');
expect(putRes.body.currentContent).toBe('FOO=bumped');
expect(fs.readFileSync(envPath, 'utf-8')).toBe('FOO=bumped');
});
});