Skip to content

Commit 6b300e4

Browse files
melvincarvalhomelvincarvalho
committed
review: validate mountApp secondary prefix; guard handler after hijack
- a provided-but-invalid opts.prefix now fails activate() (same rule as entry.prefix) instead of silently mounting — and WAC-exempting — the app at the entry prefix - the wrapped handler is guarded like ws.route: after hijack() Fastify sends nothing, so a sync throw hung the client and an unawaited async rejection could take the process down; both now log and answer 500 when nothing has gone out, else drop the one affected socket Three regression tests; full suite 1023/1023.
1 parent 83401a5 commit 6b300e4

2 files changed

Lines changed: 73 additions & 2 deletions

File tree

‎src/plugins.js‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,14 @@ export async function loadPlugins(fastify, entries, ctx) {
165165
if (typeof handler !== 'function') {
166166
throw new Error(`plugin ${id}: mountApp(handler) needs a (req, res) function`);
167167
}
168-
const mountPrefix = normalizePrefix(opts.prefix) || prefix;
168+
// Any provided prefix must validate — same rule as entry.prefix: a
169+
// falsy normalization silently falling back to the entry prefix
170+
// would mount the app (and WAC-exempt it) somewhere unexpected.
171+
const secondary = normalizePrefix(opts.prefix);
172+
if (opts.prefix !== undefined && !secondary) {
173+
throw new Error(`plugin ${id}: mountApp invalid prefix ${JSON.stringify(opts.prefix)} (must start with '/'; omit to use the entry prefix)`);
174+
}
175+
const mountPrefix = secondary || prefix;
169176
if (!mountPrefix) {
170177
throw new Error(`plugin ${id}: mountApp needs a prefix (entry.prefix or opts.prefix)`);
171178
}
@@ -175,9 +182,27 @@ export async function loadPlugins(fastify, entries, ctx) {
175182
await fastify.register(async (scope) => {
176183
scope.removeAllContentTypeParsers();
177184
scope.addContentTypeParser('*', (req, payload, done) => done(null, payload));
185+
// After hijack() Fastify sends nothing, so a handler bug must not
186+
// hang the client or become an unhandled rejection (same contract
187+
// as ws.route below): log, answer 500 if nothing went out yet,
188+
// else drop the one affected socket.
189+
const fail = (res, err) => {
190+
log.error(`plugin ${id}: mounted app handler failed: ${err.message}`);
191+
if (!res.headersSent && !res.writableEnded) {
192+
res.statusCode = 500;
193+
res.end();
194+
} else {
195+
res.destroy();
196+
}
197+
};
178198
const wrapped = (request, reply) => {
179199
reply.hijack();
180-
handler(request.raw, reply.raw);
200+
try {
201+
Promise.resolve(handler(request.raw, reply.raw))
202+
.catch((err) => fail(reply.raw, err));
203+
} catch (err) {
204+
fail(reply.raw, err);
205+
}
181206
};
182207
scope.all(mountPrefix, wrapped);
183208
scope.all(mountPrefix + '/*', wrapped);

‎test/plugin-mountapp.test.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,17 @@ export async function activate(api) {
3232
res.writeHead(200, { 'content-type': 'text/plain' });
3333
res.end('secondary');
3434
}, { prefix: '/wrapped2' });
35+
// Handlers that fail — sync throw and async rejection — to prove a
36+
// handler bug after hijack() answers 500 instead of hanging the client.
37+
await api.mountApp(() => { throw new Error('sync boom'); }, { prefix: '/boom' });
38+
await api.mountApp(async () => { throw new Error('async boom'); }, { prefix: '/boom-async' });
39+
}
40+
`;
41+
42+
// A plugin whose secondary mount prefix is invalid — must fail the boot.
43+
const BAD_PREFIX_FIXTURE = `
44+
export async function activate(api) {
45+
await api.mountApp((req, res) => res.end('x'), { prefix: 'chat' });
3546
}
3647
`;
3748

@@ -103,4 +114,39 @@ describe('api.mountApp (#583)', () => {
103114
const res = await fetch(`${baseUrl}/somepod/private/x`, { method: 'PUT', body: 'data' });
104115
assert.ok([401, 403].includes(res.status), `expected WAC rejection, got ${res.status}`);
105116
});
117+
118+
it('a handler that throws sync answers 500 and the server survives', async () => {
119+
await start();
120+
const res = await fetch(`${baseUrl}/boom`);
121+
assert.strictEqual(res.status, 500);
122+
// Process and server still alive: a healthy mount keeps answering.
123+
const ok = await fetch(`${baseUrl}/wrapped2/still-up`);
124+
assert.strictEqual(ok.status, 200);
125+
});
126+
127+
it('a handler that rejects async answers 500 instead of leaking the rejection', async () => {
128+
await start();
129+
const res = await fetch(`${baseUrl}/boom-async`);
130+
assert.strictEqual(res.status, 500);
131+
const ok = await fetch(`${baseUrl}/wrapped2/still-up`);
132+
assert.strictEqual(ok.status, 200);
133+
});
134+
135+
it('a provided-but-invalid secondary prefix fails the boot instead of mounting at the entry prefix', async () => {
136+
await fs.writeFile(path.join(FIXTURE_DIR, 'bad-prefix.js'), BAD_PREFIX_FIXTURE);
137+
await fs.emptyDir(TEST_DATA_DIR);
138+
const { createServer } = await import('../src/server.js');
139+
server = createServer({
140+
logger: false,
141+
forceCloseConnections: true,
142+
root: TEST_DATA_DIR,
143+
plugins: [
144+
{ id: 'bad', module: path.join(FIXTURE_DIR, 'bad-prefix.js'), prefix: '/ok' },
145+
],
146+
});
147+
await assert.rejects(
148+
server.listen({ port: 0, host: '127.0.0.1' }),
149+
/invalid prefix "chat"/,
150+
);
151+
});
106152
});

0 commit comments

Comments
 (0)