diff --git a/.changelog/next/fixed-issue-4137-followup.md b/.changelog/next/fixed-issue-4137-followup.md new file mode 100644 index 0000000000..50337779bf --- /dev/null +++ b/.changelog/next/fixed-issue-4137-followup.md @@ -0,0 +1 @@ +- App detection no longer treats an index.html beside a client-side app.js/main.js as a served app, and PM2 standardization launched by app id now always targets that app's own repo diff --git a/server/routes/standardize.js b/server/routes/standardize.js index 8a17cbbd3b..30c45acc30 100644 --- a/server/routes/standardize.js +++ b/server/routes/standardize.js @@ -11,9 +11,16 @@ const router = Router(); * Whenever an `appId` names a real record, it gets the precondition check — * even alongside an explicit `repoPath`, so passing both can't smuggle a * refused app past the gate. Standardization rewrites the target repo, so an - * app PortOS never runs under PM2 (and PortOS itself) is refused here rather - * than relying on the button being hidden in the UI. A bare `repoPath` has no - * app record to type-check, so it is taken at face value. + * app PortOS never runs under PM2 (or a non-Node repo, or PortOS itself) is + * refused here rather than relying on the button being hidden in the UI. A bare + * `repoPath` has no app record to type-check, so it is taken at face value. + * + * The checked record's OWN `repoPath` is what gets returned — a companion + * `repoPath` is ignored rather than preferred. Typing an app record and then + * rewriting a different directory would make the gate decorative: a permitted + * Node `appId` alongside some other repo's path would carry a Python or Docker + * project straight past the refusal. No caller sends both (the client wrappers + * in `apiSystem.js` send one or the other), so this only closes the hole. */ async function resolveStandardizeTarget({ repoPath, appId }) { if (appId) { @@ -25,7 +32,7 @@ async function resolveStandardizeTarget({ repoPath, appId }) { if (refusal) { throw new ServerError(refusal, { status: 400, code: 'NOT_STANDARDIZABLE' }); } - return repoPath || app.repoPath; + return app.repoPath; } if (repoPath) return repoPath; diff --git a/server/routes/standardize.test.js b/server/routes/standardize.test.js index 8e77d4bcc9..eefdcf7d67 100644 --- a/server/routes/standardize.test.js +++ b/server/routes/standardize.test.js @@ -77,6 +77,20 @@ describe('standardize routes — target resolution and the refusal gate', () => expectNoStandardizerWork(); }); + it('standardizes the checked app\'s own repo, ignoring a companion repoPath', async () => { + // Typing one record and then rewriting a different directory would make the + // gate decorative — a permitted Node appId could carry a Python or Docker + // repo straight past the refusal. + vi.mocked(getAppById).mockResolvedValue({ id: 'app-1', type: 'vite+express', repoPath: '/srv/example-app' }); + + const res = await request(makeApp()) + .post('/api/standardize/analyze') + .send({ appId: 'app-1', repoPath: '/srv/some-python-service' }); + + expect(res.status).toBe(200); + expect(analyzeApp).toHaveBeenCalledWith('/srv/example-app', undefined); + }); + it('404s an appId with no app record', async () => { vi.mocked(getAppById).mockResolvedValue(null); diff --git a/server/services/streamingDetect.js b/server/services/streamingDetect.js index 324e0db08c..36397ec827 100644 --- a/server/services/streamingDetect.js +++ b/server/services/streamingDetect.js @@ -28,11 +28,16 @@ const DOCKER_MARKERS = ['docker-compose.yml', 'docker-compose.yaml', 'compose.ym * package-less repo shipping `server.mjs` is a Node app someone containerized * (or that also serves a page), and classifying it `docker`/`static` would * withdraw standardization from an app that genuinely wants it — the exact - * false negative this predicate exists to avoid. `index` is deliberately absent - * from the basenames: next to an `index.html` it is far more often a - * client-side script than a server entry point. + * false negative this predicate exists to avoid. + * + * The basename list is deliberately just `server`, the one name that can only + * mean a server. `index`, `app`, and `main` are all at least as common as + * browser scripts loaded by an `index.html`, and treating those as evidence + * would leave real static sites unclassified (and still offered the Node + * standardizer) — trading this rule's false negative for the very false + * positive the issue was filed about. */ -const SERVER_ENTRY_BASENAMES = ['server', 'app', 'main']; +const SERVER_ENTRY_BASENAMES = ['server']; const SERVER_ENTRY_EXTENSIONS = ['.js', '.mjs', '.cjs', '.ts']; const hasNodeServerEntry = (present) => diff --git a/server/services/streamingDetect.test.js b/server/services/streamingDetect.test.js index 108f61d17e..f1da9beb4e 100644 --- a/server/services/streamingDetect.test.js +++ b/server/services/streamingDetect.test.js @@ -1142,7 +1142,6 @@ describe('classifyNonNodeType', () => { // — `server.mjs` is as much an entry point as `server.js`. for (const server of [ 'server.js', 'server.mjs', 'server.cjs', 'server.ts', - 'app.js', 'app.mjs', 'main.js', 'main.ts', 'ecosystem.config.js', 'ecosystem.config.cjs' ]) { expect(classifyNonNodeType(['index.html', server])).toBeNull(); @@ -1153,19 +1152,22 @@ describe('classifyNonNodeType', () => { // A containerized Node service is still a Node service — the entry point // outranks the packaging, the same way a language marker does. expect(classifyNonNodeType(['server.js', 'Dockerfile'])).toBeNull(); - expect(classifyNonNodeType(['app.mjs', 'docker-compose.yml'])).toBeNull(); + expect(classifyNonNodeType(['server.mjs', 'docker-compose.yml'])).toBeNull(); }); - it('still treats a client-side index.js as static, not a server', () => { - // In a static site an `index.js` is a browser script far more often than a - // server entry point, so it must not suppress the classification. - expect(classifyNonNodeType(['index.html', 'index.js', 'style.css'])).toBe('static'); + it('treats ambiguous browser-script names as static, not as servers', () => { + // `index.js`, `app.js`, and `main.js` are at least as common as scripts a + // static page loads. Counting them as server evidence would leave real + // static sites unclassified and still offered the Node standardizer. + for (const script of ['index.js', 'app.js', 'main.js']) { + expect(classifyNonNodeType(['index.html', script, 'style.css'])).toBe('static'); + } }); it('keeps the language markers ahead of a Node entry point', () => { - // A Python repo with a stray `app.py`-adjacent `main.js` build script is - // still python — the language check runs first. - expect(classifyNonNodeType(['requirements.txt', 'main.js'])).toBe('python'); + // A Python repo that also ships a `server.js` helper is still python — + // the language check runs first. + expect(classifyNonNodeType(['requirements.txt', 'server.js'])).toBe('python'); }); it('prefers the language over the packaging when both markers are present', () => {