Conversation
db74ebf to
9bb3a79
Compare
| server.addHook("onRequest", async (request, _reply) => { | ||
| const url = request.routeOptions.url; | ||
| if (url !== "/query" && url !== "/mutation") { | ||
| request.log.info({ req: request }, "incoming request"); |
There was a problem hiding this comment.
Logging the entire request seems pretty dangerous, you could log something sensitive by accident. This seems like a bit of a footgun. What problem are we trying to solve with this particular bit of logging?
There was a problem hiding this comment.
I didn't realize its logging entire request. My testing (See sample log in description) shows it doesn't seem to log the request body. but let me ratify the implementation.
There was a problem hiding this comment.
I have add explicit logging for method and url fields for a request.
| // Log request completed for all routes | ||
| server.addHook("onResponse", async (request, reply) => { | ||
| request.log.info( | ||
| { req: request, res: reply, responseTime: reply.elapsedTime }, |
There was a problem hiding this comment.
This is also going to log sensitive information (whatever is in the request and response), that doesn't seem like a good idea.
There was a problem hiding this comment.
Btw we already log each request and response without body and headers (See previous logs in description). Aim is to redo the same implementation, but with added function name.
There was a problem hiding this comment.
That's fine... headers and body is where the sensitive stuff is, so we don't want to log those IMHO. I'm just suspicious here because we're deliberately assigning the entire request and response objects into the log... so if body/headers are not being logged, how and why is that happening?
There was a problem hiding this comment.
Yeah, the red flags raised makes sense. will investigate. or will log explicit fields.
There was a problem hiding this comment.
I have add explicit logging for method and url fields for a request.
There's a chance that framework's serializer do not log body and headers by default for safety reasons, but its prudent not to rely on this implicit behavior
| if (collectionKey === 'collection') { | ||
| functionName = (request.body as any)?.collection; | ||
| } else { | ||
| functionName = (request.body as any)?.operations?.[0]?.name; |
There was a problem hiding this comment.
Please no anys in this codebase, we don't want to disable typechecking. If you want to dynamically find the function name, you should check the actual types at runtime as you descend through the runtime values, not just yolo try to access them. If they aren't the shape you expect here (after disabling typechecking), this will crash.
You (or someone!) will thank me later 😀
(The unknown type is the safe choice when you don't know what the type of a value is. any disables typechecking, unknown says "you need to do type tests at runtime to discover what this is and work with it".)
There was a problem hiding this comment.
Thanks, I'll improve this. will add this instruction in Claude.md too; or maybe a lint rule
There was a problem hiding this comment.
So long as the lint rule can be disabled on a per case basis. Sometime (rarely) any is appropriate when the programmer is careful and knows what he/she is doing and just needs the typechecker to stfu. 😉 But one should definitely avoid using it on untrusted input like the http request body.
There was a problem hiding this comment.
not using any anymore
79caa29 to
62c2c3a
Compare
I want to add function name to logs wherever possible. We will show the connector logs filtered by function names to users who are managing these functions via Admin Studio.
Previous logs:
Updated logs:
Added
"function":"hello"in the logSecurity update
Added package updates as per
npm auditDocumentation
Also added a
Claude.mdfile for future use