-
Notifications
You must be signed in to change notification settings - Fork 121
Feat: fastify support with nitro #386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
@eersnington is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| if (!workflowFile) { | ||
| return reply.code(400).send('No workflowFile query parameter provided'); | ||
| } | ||
| const workflows = allWorkflows[workflowFile as keyof typeof allWorkflows]; | ||
| if (!workflows) { | ||
| return reply.code(400).send(`Workflow file "${workflowFile}" not found`); | ||
| } | ||
|
|
||
| const workflowFn = (req.query.workflowFn as string) || 'simple'; | ||
| if (!workflowFn) { | ||
| return reply.code(400).send('No workflow query parameter provided'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (!workflowFile) { | |
| return reply.code(400).send('No workflowFile query parameter provided'); | |
| } | |
| const workflows = allWorkflows[workflowFile as keyof typeof allWorkflows]; | |
| if (!workflows) { | |
| return reply.code(400).send(`Workflow file "${workflowFile}" not found`); | |
| } | |
| const workflowFn = (req.query.workflowFn as string) || 'simple'; | |
| if (!workflowFn) { | |
| return reply.code(400).send('No workflow query parameter provided'); | |
| } |
The parameter validation checks on lines 67-68 and 76-77 are dead code and will never execute, because the variables are always assigned non-empty default values using the OR operator above them.
View Details
Analysis
Dead code: unreachable validation checks in POST /api/trigger endpoint
What fails: Lines 67-68 and 76-77 in workbench/fastify/src/index.ts contain unreachable validation checks that will never execute.
How to reproduce: The following code pattern causes dead code because the OR operator guarantees non-falsy values:
const workflowFile = (req.query.workflowFile as string) || 'workflows/99_e2e.ts';
if (!workflowFile) { // This condition will never be true
return reply.code(400).send('No workflowFile query parameter provided');
}When req.query.workflowFile is undefined, the expression (undefined || 'workflows/99_e2e.ts') evaluates to the default string 'workflows/99_e2e.ts', which is truthy. The subsequent if (!workflowFile) check will never execute.
Result: Dead code remains in the codebase, creating confusion about intended behavior.
Expected: Since default values are intentionally provided via the OR operator, the validation checks should be removed. The defaults indicate the developer intended these parameters to be optional, making the checks that assume they might be missing logically inconsistent.
Fix: Removed the unreachable validation checks on lines 67-68 and 76-77, keeping only the default value assignments and the subsequent existence checks on the workflow lookup (lines 68-70 and 74-77), which properly validate that the assigned workflow file and function actually exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically correct nit, but this pattern is consistent across all the api/trigger (POST) routes in all benches. Is this change necessary?
|
I'll wait for a review before updating test matrix and local-build.test.ts CI? |
| export default defineNitroConfig({ | ||
| modules: ["workflow/nitro"], | ||
| vercel: { entryFormat: "node" }, | ||
| preset: "vercel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi i think nitro auto-detects presets depending on the environment variables set, so i don't think this is needed
| ); | ||
|
|
||
| server.post('/api/hook', async (req: any, reply) => { | ||
| const body = typeof req.body === 'string' ? JSON.parse(req.body) : req.body; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unguarded JSON.parse() call without error handling - if req.body is a string with invalid JSON, it will crash the server.
View Details
📝 Patch Details
diff --git a/workbench/fastify/src/index.ts b/workbench/fastify/src/index.ts
index f4c96e9..88fe09f 100644
--- a/workbench/fastify/src/index.ts
+++ b/workbench/fastify/src/index.ts
@@ -40,7 +40,12 @@ server.addContentTypeParser(
);
server.post('/api/hook', async (req: any, reply) => {
- const body = typeof req.body === 'string' ? JSON.parse(req.body) : req.body;
+ let body;
+ try {
+ body = typeof req.body === 'string' ? JSON.parse(req.body) : req.body;
+ } catch (error) {
+ return reply.code(400).send('Invalid JSON in request body');
+ }
const { token, data } = body;
let hook: Awaited<ReturnType<typeof getHookByToken>>;
@@ -94,7 +99,11 @@ server.post('/api/trigger', async (req: any, reply) => {
// Args from body
const body = req.body;
if (body && typeof body === 'string') {
- args = hydrateWorkflowArguments(JSON.parse(body), globalThis);
+ try {
+ args = hydrateWorkflowArguments(JSON.parse(body), globalThis);
+ } catch (error) {
+ return reply.code(400).send('Invalid JSON in request body');
+ }
} else if (body && typeof body === 'object') {
args = hydrateWorkflowArguments(body, globalThis);
} else {
Analysis
Unguarded JSON.parse() calls cause server crash when malformed JSON is sent via text/* content-types
What fails: Routes /api/hook (line 43) and /api/trigger (line 97) in workbench/fastify/src/index.ts crash with unhandled SyntaxError when requests with Content-Type: text/plain containing malformed JSON are sent.
How to reproduce:
# Test /api/hook route with malformed JSON
curl -X POST http://localhost:3000/api/hook \
-H "Content-Type: text/plain" \
-d '{invalid json}'
# Returns: 500 Internal Server Error
# Error: "Expected property name or '}' in JSON at position 1"Result: Server returns 500 error. The error originates from line 43 (in /api/hook) and line 97 (in /api/trigger) where JSON.parse(req.body) is called without try-catch protection. When requests with Content-Type: text/plain containing invalid JSON are sent, the text/* content-type parser uses getDefaultJsonParser('ignore', 'ignore') which returns the malformed string as-is to the route handler, causing an unhandled exception.
Expected: Should return 400 Bad Request with descriptive error message, as the JSON is malformed and cannot be parsed.
Root cause: The text/* parser (lines 24-28) does not parse JSON, just returns raw strings. Routes that may receive strings need to handle JSON.parse() errors. The application/json parser (lines 31-40) has proper error handling via the parseJson() function and the done(error) callback, but this doesn't protect against text/* requests.
Fix applied: Wrapped both JSON.parse() calls in try-catch blocks that return 400 Bad Request status with error message when parsing fails.
|
ty @eersnington for the help! left some comments as nitpicks for the docs. we should definitely use components soon Wanna wire up the local dev and local prod tests? i can hop on later to wire up the vercel prod tests then we should be good :) |
alright bet, I can do that right now too |
Signed-off-by: Sree Narayanan <[email protected]> Co-authored-by: Adrian <[email protected]>
Signed-off-by: Sree Narayanan <[email protected]> Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
Signed-off-by: Sree Narayanan <[email protected]>
Signed-off-by: Sree Narayanan <[email protected]> Co-authored-by: Adrian <[email protected]>
Signed-off-by: Sree Narayanan <[email protected]> Co-authored-by: Adrian <[email protected]>
Signed-off-by: Sree Narayanan <[email protected]>
DCO Remediation Commit for Sree Narayanan <[email protected]> I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 54c3e8a I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 423f722 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: f760616 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: a269b8a I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: b3cedf7 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 596cf21 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 0ea1aa7 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: dbc8638 Signed-off-by: Sree Narayanan <[email protected]>
Signed-off-by: Sree Narayanan <[email protected]>
Signed-off-by: Sree Narayanan <[email protected]>
Signed-off-by: Sree Narayanan <[email protected]>
Signed-off-by: Sree Narayanan <[email protected]>
DCO Remediation Commit for Sree Narayanan <[email protected]> I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: b78bf22 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: e7a4565 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 37ee9dd I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 72912dc I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 4ebda5b I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: ccb690f I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 6b80913 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: c9fb3eb Signed-off-by: Sree Narayanan <[email protected]>
This reverts commit 081d29f.
ca4c617 to
e60e0e8
Compare
Signed-off-by: Sree Narayanan <[email protected]>
DCO Remediation Commit for Sree Narayanan <[email protected]> I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: df5dfc9 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 809bacb I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: b270d02 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: cb73ae5 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 2ee3af6 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: cab1381 I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 9c7471b I, Sree Narayanan <[email protected]>, hereby add my Signed-off-by to this commit: 4583452 Signed-off-by: Sree Narayanan <[email protected]>
|
/runci |
i don't think that command works |
running ci on #424 |






Cooked up a working demo of using workflow devkit with fastify, using nitro as the bundler. I will add a docs page and e2e tests as well (following adrian's example for express).
Really quick guide
1. Configure Nitro
Configure
nitro.config.tsto load the workflow module and direct all routes to your entry file.2. Server Entry Adapter
In your entry file, bridge Nitro to Fastify by manually emitting the request event to Fastify's underlying server instance.
3. Create Workflows
Import your workflow and call
start()within any standard Fastify route handler.