fix: standardize credential ID naming and improve error handling acro…#68
fix: standardize credential ID naming and improve error handling acro…#68TejaBudumuru3 merged 1 commit intomainfrom
Conversation
…ss routes and services
📝 WalkthroughWalkthroughThis PR standardizes HTTP status codes using constants across route files, renames credential-related fields from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/http-backend/src/routes/google_callback.ts (2)
202-237:⚠️ Potential issue | 🟠 MajorDebug endpoint exposes sensitive credential metadata.
The
/debug/credentialsendpoint returns credential details including token presence, refresh token length, expiry dates, and scopes. While useful for development, this endpoint should be:
- Disabled or protected in production environments
- Restricted to admin users only
Consider adding environment checks or authentication guards.
133-135:⚠️ Potential issue | 🟡 MinorHardcoded localhost URLs will break in production.
The redirect URLs are hardcoded to
http://localhost:3000. These should be configurable via environment variables.🔧 Proposed fix
+ const frontendUrl = process.env.FRONTEND_URL || "http://localhost:3000"; // Redirect to workflow page if workflowId is provided, otherwise to general workflow page const redirectUrl = workflowId - ? `http://localhost:3000/workflows/${workflowId}` - : "http://localhost:3000/workflow"; + ? `${frontendUrl}/workflows/${workflowId}` + : `${frontendUrl}/workflow`;apps/http-backend/src/routes/userRoutes/userRoutes.ts (2)
362-366:⚠️ Potential issue | 🟡 MinorInconsistent status code: use
UNAUTHORIZEDinstead ofBAD_GATEWAY.This route still uses
BAD_GATEWAYfor missing user authentication, while all other routes in this PR have been updated to useUNAUTHORIZED. This should be consistent.Proposed fix
if (!req.user) return res - .status(statusCodes.BAD_GATEWAY) + .status(statusCodes.UNAUTHORIZED) .json({ message: "User isnot logged in /not authorized" });
708-714:⚠️ Potential issue | 🟡 MinorInconsistent user identifier: use
req.user?.subinstead ofreq.user?.id.All other routes in this PR have been updated to use
req.user.sub, but this endpoint still usesreq.user?.id. This may returnundefinedif the JWT payload doesn't have anidfield.Proposed fix
router.get("/protected", userMiddleware, (req: AuthRequest, res) => { return res.json({ ok: true, - userId: req.user?.id, + userId: req.user?.sub, email: req.user?.email, }); });
🤖 Fix all issues with AI agents
In `@packages/nodes/src/common/google-oauth-service.ts`:
- Around line 92-97: The findFirst query using this.prisma.credential.findFirst
currently omits the user filter (credentialId is checked but userId is commented
out), allowing credential access by ID alone; restore and use the passed userId
in the where clause (e.g., include userId: userId alongside id: credentialId and
type: "google_oauth") so the query only returns credentials owned by the
requesting user, and ensure any callers still pass a validated userId parameter
when invoking the method that performs this lookup.
🧹 Nitpick comments (6)
apps/worker/src/tests/test.ts (2)
2-3: Remove commented-out import.Since the local
registory.tsfile has been removed in this PR, the commented-out import is dead code and should be deleted.🧹 Proposed fix
import { executeWorkflow } from "../engine/executor.js"; -// import { register } from "../engine/registory.js"; import { ExecutionRegister as register } from "@repo/nodes";
42-42: Add error handling todebug()call for consistency.
testDirect()has.catch(console.error)butdebug()does not. An unhandled promise rejection fromdebug()will cause inconsistent behavior.🧹 Proposed fix
-debug(); +debug().catch(console.error);apps/http-backend/src/routes/userRoutes/executionRoutes.ts (1)
34-38: Minor formatting inconsistency: extra space before comma.Line 36 has a trailing space before the comma (
config: config ,) which is inconsistent with the rest of the codebase.Also note that
credentialsIdis commented out. If credential support is needed for node execution, this should be addressed in a follow-up.🧹 Proposed fix
const context = { userId: req.user.sub, - config: config , + config: config, // credentialsId: nodeData.CredentialsID || "" }packages/nodes/src/google-sheets/google-sheets.executor.ts (1)
5-10: Local interface duplicates shared type with different nullability.
NodeExecutionContextis defined locally withcredentialIdas required, butExecutionContextinpackages/nodes/src/registry/Execution.config.types.tshascredentialId?: string(optional). This inconsistency could cause issues if the executor is called without acredentialId.Consider either:
- Importing and extending the shared
ExecutionContexttype, or- Ensuring the local type matches the shared definition with proper guards
packages/nodes/src/gmail/gmail.executor.ts (1)
4-9: Local interface duplicates shared type.Similar to
google-sheets.executor.ts, this file defines a localNodeExecutionContextwithcredentialIdas required. Consider importing from the shared types inpackages/nodes/src/registry/Execution.config.types.tsto maintain consistency.apps/http-backend/src/routes/sheet.routes.ts (1)
10-10: Consider aligning route parameter names with variable names.The route paths use
:credwhile the code now usescredentialId. For consistency with the PR's naming standardization goal, consider renaming the route parameters:- "/getDocuments/:cred", + "/getDocuments/:credentialId",- "/getSheets/:cred/:sheetId", + "/getSheets/:credentialId/:sheetId",Note: This is a breaking change if clients depend on the current URL structure.
Also applies to: 56-57
| const credentials = await this.prisma.credential.findFirst({ | ||
| where: { | ||
| id: credId, | ||
| id: credentialId, | ||
| // userId: userId, | ||
| type: "google_oauth", | ||
| }, |
There was a problem hiding this comment.
Security concern: userId filter is commented out.
The userId is passed as a parameter but not used in the query filter (Line 95 is commented out). This means any user who knows a credentialId could potentially access credentials belonging to other users.
This is an authorization bypass risk. The query should verify that the credential belongs to the requesting user.
🔒 Proposed fix
const credentials = await this.prisma.credential.findFirst({
where: {
id: credentialId,
- // userId: userId,
+ userId: userId,
type: "google_oauth",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const credentials = await this.prisma.credential.findFirst({ | |
| where: { | |
| id: credId, | |
| id: credentialId, | |
| // userId: userId, | |
| type: "google_oauth", | |
| }, | |
| const credentials = await this.prisma.credential.findFirst({ | |
| where: { | |
| id: credentialId, | |
| userId: userId, | |
| type: "google_oauth", | |
| }, |
🤖 Prompt for AI Agents
In `@packages/nodes/src/common/google-oauth-service.ts` around lines 92 - 97, The
findFirst query using this.prisma.credential.findFirst currently omits the user
filter (credentialId is checked but userId is commented out), allowing
credential access by ID alone; restore and use the passed userId in the where
clause (e.g., include userId: userId alongside id: credentialId and type:
"google_oauth") so the query only returns credentials owned by the requesting
user, and ensure any callers still pass a validated userId parameter when
invoking the method that performs this lookup.
2 participants
…ss routes and services
Summary by CodeRabbit
Bug Fixes
Improvements