mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-08-08 01:43:55 +00:00
fix(stacks): refuse file-explorer delete/rename/chmod on protected stack files (#1202)
* fix(stacks): refuse file-explorer delete/rename/chmod on protected stack files Previously the per-stack file explorer treated PROTECTED_STACK_FILES (compose.yaml, compose.yml, docker-compose.yaml/.yml, .env) as a UI hint only. A direct API call from any user with stack:edit could delete or rename compose.yaml and break the stack irrecoverably because the next deploy would fail to find a compose file and the write was unrecoverable without a DB backup. The frontend DeleteFileConfirm enforced a type-to-confirm gate but a stale UI or a scripted client bypassed it. FileSystemService now refuses the destructive ops at the service layer with a new PROTECTED_FILE error code that the route layer surfaces as 409. The compose-editor save path (PUT /files/content) and the upload-overwrite path (POST /files/upload writing a same-named file) remain unblocked because both are legitimate ways to update compose.yaml. Tests pin the allowed paths so a future tightening can not silently regress them. The protection is scoped to entries at the stack root; subdirectory files happen to share a protected name (e.g., a snapshot under backups/compose.yaml) are not blocked because compose CLI only reads the root file. A trailing-slash bypass is closed by stripping trailing separators before the basename check. Frontend DeleteFileConfirm needs no change. Its existing toast.error surface renders the friendly server message. * fix(stack-files): drop polynomial regex from protected-file helpers CodeQL js/polynomial-redos flagged the /\/+$/ pattern used to strip trailing slashes from relPath in isProtectedRelPath and protectedFileError. The regex is bounded in practice (the upstream validator rejects '//' anywhere in the path) but the static analyzer cannot follow that dataflow guarantee and would have flagged any future caller that skips the validator. Replace the two callsites with a small stripTrailingSlash helper that uses endsWith + slice. Bounded O(1), no regex, no analyzer alert. The inline comment documents the upstream invariant so a future reader does not reintroduce the /+ quantifier.
This commit is contained in:
@@ -631,3 +631,109 @@ describe('permission gating', () => {
|
||||
expect(typeof res.body.octal).toBe('string');
|
||||
});
|
||||
});
|
||||
|
||||
// ── protected stack files (compose.yaml, .env) ────────────────────────────────
|
||||
|
||||
describe('protected stack files', () => {
|
||||
it('DELETE /files refuses compose.yaml with 409 PROTECTED_FILE', async () => {
|
||||
const res = await request(app)
|
||||
.delete(`/api/stacks/${STACK}/files`)
|
||||
.query({ path: 'compose.yaml' })
|
||||
.set('Cookie', adminCookie);
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.code).toBe('PROTECTED_FILE');
|
||||
});
|
||||
|
||||
it('DELETE /files refuses .env with 409 PROTECTED_FILE', async () => {
|
||||
const res = await request(app)
|
||||
.delete(`/api/stacks/${STACK}/files`)
|
||||
.query({ path: '.env' })
|
||||
.set('Cookie', adminCookie);
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.code).toBe('PROTECTED_FILE');
|
||||
});
|
||||
|
||||
it('PATCH /files/rename refuses compose.yaml as source with 409 PROTECTED_FILE', async () => {
|
||||
const res = await request(app)
|
||||
.patch(`/api/stacks/${STACK}/files/rename`)
|
||||
.set('Cookie', adminCookie)
|
||||
.send({ from: 'compose.yaml', to: 'renamed-compose.yaml' });
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.code).toBe('PROTECTED_FILE');
|
||||
});
|
||||
|
||||
it('PATCH /files/rename refuses compose.yaml as destination with 409 PROTECTED_FILE', async () => {
|
||||
await fs.writeFile(path.join(stacksDir, STACK, 'pretend.yaml'), 'services: {}\n');
|
||||
const res = await request(app)
|
||||
.patch(`/api/stacks/${STACK}/files/rename`)
|
||||
.set('Cookie', adminCookie)
|
||||
.send({ from: 'pretend.yaml', to: 'compose.yaml' });
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.code).toBe('PROTECTED_FILE');
|
||||
await fs.unlink(path.join(stacksDir, STACK, 'pretend.yaml'));
|
||||
});
|
||||
|
||||
it('PUT /files/permissions refuses .env with 409 PROTECTED_FILE', async () => {
|
||||
const res = await request(app)
|
||||
.put(`/api/stacks/${STACK}/files/permissions`)
|
||||
.query({ path: '.env' })
|
||||
.set('Cookie', adminCookie)
|
||||
.send({ mode: 0o644 });
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.code).toBe('PROTECTED_FILE');
|
||||
});
|
||||
|
||||
it('DELETE /files still succeeds on a non-protected file', async () => {
|
||||
const target = path.join(stacksDir, STACK, 'disposable.txt');
|
||||
await fs.writeFile(target, 'temporary');
|
||||
const res = await request(app)
|
||||
.delete(`/api/stacks/${STACK}/files`)
|
||||
.query({ path: 'disposable.txt' })
|
||||
.set('Cookie', adminCookie);
|
||||
expect(res.status).toBe(204);
|
||||
});
|
||||
|
||||
it('PUT /files/content still succeeds on compose.yaml (compose editor path)', async () => {
|
||||
const res = await request(app)
|
||||
.put(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'compose.yaml' })
|
||||
.set('Cookie', adminCookie)
|
||||
.send({ content: 'services:\n echo:\n image: busybox\n' });
|
||||
expect(res.status).toBe(204);
|
||||
});
|
||||
|
||||
it('POST /files/upload still succeeds when overwriting compose.yaml (legitimate replace)', async () => {
|
||||
const replacement = 'services:\n uploaded:\n image: busybox\n';
|
||||
const res = await request(app)
|
||||
.post(`/api/stacks/${STACK}/files/upload`)
|
||||
.set('Cookie', adminCookie)
|
||||
.attach('file', Buffer.from(replacement), 'compose.yaml');
|
||||
expect(res.status).toBe(204);
|
||||
const written = await fs.readFile(path.join(stacksDir, STACK, 'compose.yaml'), 'utf-8');
|
||||
expect(written).toContain('uploaded');
|
||||
});
|
||||
|
||||
it('DELETE /files succeeds on a subdirectory file named compose.yaml (not the active compose file)', async () => {
|
||||
const subdir = path.join(stacksDir, STACK, 'backups');
|
||||
await fs.mkdir(subdir, { recursive: true });
|
||||
await fs.writeFile(path.join(subdir, 'compose.yaml'), 'services: {}\n');
|
||||
const res = await request(app)
|
||||
.delete(`/api/stacks/${STACK}/files`)
|
||||
.query({ path: 'backups/compose.yaml' })
|
||||
.set('Cookie', adminCookie);
|
||||
expect(res.status).toBe(204);
|
||||
await fs.rm(subdir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('DELETE /files refuses compose.yaml even with a trailing slash', async () => {
|
||||
const res = await request(app)
|
||||
.delete(`/api/stacks/${STACK}/files`)
|
||||
.query({ path: 'compose.yaml/' })
|
||||
.set('Cookie', adminCookie);
|
||||
// Either the validator rejects the trailing slash (400) or the protected-file
|
||||
// guard catches the normalized basename (409). Both are acceptable; what is NOT
|
||||
// acceptable is a 204 success that silently deletes the protected file.
|
||||
expect([400, 409]).toContain(res.status);
|
||||
if (res.status === 409) expect(res.body.code).toBe('PROTECTED_FILE');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1309,7 +1309,8 @@ type FsErrorCode =
|
||||
| 'NOT_EMPTY'
|
||||
| 'NOT_FOUND'
|
||||
| 'TOO_LARGE'
|
||||
| 'ALREADY_EXISTS';
|
||||
| 'ALREADY_EXISTS'
|
||||
| 'PROTECTED_FILE';
|
||||
|
||||
function sendFsError(
|
||||
res: Response,
|
||||
@@ -1327,6 +1328,9 @@ function sendFsError(
|
||||
if (e.code === 'NOT_EMPTY') {
|
||||
return res.status(409).json({ error: e.message, code: e.code as FsErrorCode });
|
||||
}
|
||||
if (e.code === 'PROTECTED_FILE') {
|
||||
return res.status(409).json({ error: e.message, code: 'PROTECTED_FILE' satisfies FsErrorCode });
|
||||
}
|
||||
if (e.code === 'EEXIST') {
|
||||
return res.status(409).json({ error: e.message, code: 'ALREADY_EXISTS' satisfies FsErrorCode });
|
||||
}
|
||||
|
||||
@@ -35,6 +35,33 @@ const PROTECTED_STACK_FILES = new Set([
|
||||
'.env',
|
||||
]);
|
||||
|
||||
// Strips at most one trailing slash. The upstream validator
|
||||
// (isValidRelativeStackPath) rejects any '//' sequence, so a string reaching
|
||||
// this helper can carry at most one trailing slash, and a single slice is
|
||||
// sufficient. Avoids the polynomial regex /\/+$/ that CodeQL would flag for
|
||||
// callers without the upstream length guarantee.
|
||||
function stripTrailingSlash(s: string): string {
|
||||
return s.endsWith('/') ? s.slice(0, -1) : s;
|
||||
}
|
||||
|
||||
function isProtectedRelPath(relPath: string): boolean {
|
||||
if (!relPath) return false;
|
||||
const normalized = stripTrailingSlash(relPath);
|
||||
// Only files at the stack root are protected; compose CLI reads compose.yaml from
|
||||
// the stack directory itself, so a subdirectory entry named compose.yaml is just
|
||||
// an arbitrary file and the user may want to delete it.
|
||||
if (normalized.includes('/')) return false;
|
||||
return PROTECTED_STACK_FILES.has(normalized);
|
||||
}
|
||||
|
||||
function protectedFileError(relPath: string): Error & { code: string } {
|
||||
const basename = stripTrailingSlash(relPath).split('/').pop() ?? relPath;
|
||||
return Object.assign(
|
||||
new Error(`${basename} is a protected stack file. Delete the stack itself via Stack Actions instead.`),
|
||||
{ code: 'PROTECTED_FILE' as const },
|
||||
);
|
||||
}
|
||||
|
||||
const MIME_MAP: Record<string, string> = {
|
||||
'.yaml': 'text/yaml',
|
||||
'.yml': 'text/yaml',
|
||||
@@ -685,6 +712,7 @@ export class FileSystemService {
|
||||
}
|
||||
|
||||
async deleteStackPath(stackName: string, relPath: string, recursive: boolean = false): Promise<void> {
|
||||
if (isProtectedRelPath(relPath)) throw protectedFileError(relPath);
|
||||
const safePath = await this.resolveSafeStackPath(stackName, relPath);
|
||||
|
||||
if (recursive) {
|
||||
@@ -717,6 +745,8 @@ export class FileSystemService {
|
||||
}
|
||||
|
||||
async renameStackPath(stackName: string, fromRel: string, toRel: string): Promise<void> {
|
||||
if (isProtectedRelPath(fromRel)) throw protectedFileError(fromRel);
|
||||
if (isProtectedRelPath(toRel)) throw protectedFileError(toRel);
|
||||
const fromPath = await this.resolveSafeStackPath(stackName, fromRel);
|
||||
// toRel must resolve to the same parent directory (rename only, no cross-dir move).
|
||||
const toPath = await this.resolveSafeStackPath(stackName, toRel);
|
||||
@@ -749,6 +779,7 @@ export class FileSystemService {
|
||||
if (!Number.isInteger(mode) || mode < 0 || mode > 0o777) {
|
||||
throw Object.assign(new Error('Invalid permission bits'), { code: 'INVALID_PATH' });
|
||||
}
|
||||
if (isProtectedRelPath(relPath)) throw protectedFileError(relPath);
|
||||
const safePath = await this.resolveSafeStackPath(stackName, relPath);
|
||||
await fsPromises.chmod(safePath, mode);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user