fix(gateway): resolve device.proto path in Docker container#16
fix(gateway): resolve device.proto path in Docker container#16pahuldeepp merged 2 commits intomasterfrom
Conversation
The compiled gateway runs from /app/dist/services/. The proto file is copied to /app/libs/proto/ in the Docker image. The previous path (../../../../libs/proto/device.proto) resolved to /libs/proto which doesn't exist. Use /app/libs/proto directly with a local fallback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThree changes: device service proto path resolution now prefers an absolute Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Gateway as Gateway (Express)
participant Auth as authMiddleware
participant RateLimiter as apiRateLimiter
participant Handler as Billing Handler
participant Stripe as Stripe/DB
Client->>Gateway: HTTP request (e.g., POST /billing/checkout)
Gateway->>Auth: run authMiddleware
Auth-->>Gateway: auth result
Gateway->>RateLimiter: run apiRateLimiter
RateLimiter-->>Gateway: allowed/blocked
Gateway->>Handler: invoke route handler
Handler->>Stripe: process checkout / query subscription
Stripe-->>Handler: response
Handler-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 docstrings
🧪 Generate unit tests (beta)
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 (1)
apps/gateway/src/services/device.ts (1)
38-44: 🧹 Nitpick | 🔵 TrivialmTLS falls back to insecure credentials silently.
When
/certs/ca.crtis missing, the code falls back togrpc.credentials.createInsecure()without any log or warning. This is intentional for local dev, but in production if certs are accidentally missing, traffic would be unencrypted without any visible alert.Consider logging when insecure mode is used:
📝 Suggested improvement
const credentials = !fs.existsSync("/certs/ca.crt") - ? grpc.credentials.createInsecure() + ? (console.warn("[device-service] mTLS certs not found, using insecure credentials"), grpc.credentials.createInsecure()) : grpc.credentials.createSsl(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/services/device.ts` around lines 38 - 44, The code silently falls back to grpc.credentials.createInsecure() when fs.existsSync("/certs/ca.crt") is false; modify the branch that sets the credentials variable (the ternary using fs.existsSync, grpc.credentials.createInsecure, and grpc.credentials.createSsl) to emit a clear warning/log (e.g., processLogger.warn or console.warn) whenever insecure credentials are used, include the path checked and a brief note that this is falling back to insecure mode, and keep the existing behavior for local dev while ensuring production accidental-misconfiguration is visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/services/device.ts`:
- Around line 11-13: The fallback path for protoPath is wrong for local dev:
update the fallback in the protoPath expression (the ternary using fs.existsSync
and __dirname) to point to the repo-root location (e.g., use
path.resolve(__dirname, "../../../../libs/proto/device.proto") or
path.resolve(__dirname, "../../../..", "libs/proto/device.proto")) so when
running apps/gateway/dist/services/device.js it resolves to
libs/proto/device.proto; alternatively, if local development support is
intentionally not required, add a clarifying comment above the protoPath
declaration noting the code only supports the container absolute path.
---
Outside diff comments:
In `@apps/gateway/src/services/device.ts`:
- Around line 38-44: The code silently falls back to
grpc.credentials.createInsecure() when fs.existsSync("/certs/ca.crt") is false;
modify the branch that sets the credentials variable (the ternary using
fs.existsSync, grpc.credentials.createInsecure, and grpc.credentials.createSsl)
to emit a clear warning/log (e.g., processLogger.warn or console.warn) whenever
insecure credentials are used, include the path checked and a brief note that
this is falling back to insecure mode, and keep the existing behavior for local
dev while ensuring production accidental-misconfiguration is visible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d85746ce-de9c-4dca-8032-2b50ca985dcd
📒 Files selected for processing (1)
apps/gateway/src/services/device.ts
| const protoPath = fs.existsSync("/app/libs/proto/device.proto") | ||
| ? "/app/libs/proto/device.proto" | ||
| : path.resolve(__dirname, "../../libs/proto/device.proto"); |
There was a problem hiding this comment.
Fallback path incorrect for local development.
The primary absolute path /app/libs/proto/device.proto correctly resolves inside the Docker container. However, the fallback ../../libs/proto/device.proto assumes a flat structure that doesn't match local development.
When running locally from apps/gateway/dist/services/device.js, ../../libs/ resolves to apps/gateway/libs/, but the proto file lives at the repo root (libs/proto/device.proto), requiring ../../../../libs/proto/device.proto.
If local dev support isn't needed, consider adding a comment. Otherwise:
🔧 Suggested fix to support both environments
-const protoPath = fs.existsSync("/app/libs/proto/device.proto")
- ? "/app/libs/proto/device.proto"
- : path.resolve(__dirname, "../../libs/proto/device.proto");
+// Docker: proto at /app/libs/proto/device.proto
+// Local: proto at repo root, 4 levels up from apps/gateway/dist/services/
+const protoPath = fs.existsSync("/app/libs/proto/device.proto")
+ ? "/app/libs/proto/device.proto"
+ : path.resolve(__dirname, "../../../../libs/proto/device.proto");📝 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 protoPath = fs.existsSync("/app/libs/proto/device.proto") | |
| ? "/app/libs/proto/device.proto" | |
| : path.resolve(__dirname, "../../libs/proto/device.proto"); | |
| // Docker: proto at /app/libs/proto/device.proto | |
| // Local: proto at repo root, 4 levels up from apps/gateway/dist/services/ | |
| const protoPath = fs.existsSync("/app/libs/proto/device.proto") | |
| ? "/app/libs/proto/device.proto" | |
| : path.resolve(__dirname, "../../../../libs/proto/device.proto"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/services/device.ts` around lines 11 - 13, The fallback path
for protoPath is wrong for local dev: update the fallback in the protoPath
expression (the ternary using fs.existsSync and __dirname) to point to the
repo-root location (e.g., use path.resolve(__dirname,
"../../../../libs/proto/device.proto") or path.resolve(__dirname, "../../../..",
"libs/proto/device.proto")) so when running apps/gateway/dist/services/device.js
it resolves to libs/proto/device.proto; alternatively, if local development
support is intentionally not required, add a clarifying comment above the
protoPath declaration noting the code only supports the container absolute path.
- performance-budget.js: change `catch {}` to `catch (error) {}` for k6/Babel compatibility
- billing.ts: add apiRateLimiter to all three billing routes (CodeQL js/missing-rate-limiting)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| billingRouter.get( | ||
| "/billing/subscription", | ||
| authMiddleware, | ||
| apiRateLimiter, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Copilot Autofix
AI about 1 month ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| billingRouter.post( | ||
| "/billing/checkout", | ||
| authMiddleware, | ||
| apiRateLimiter, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Copilot Autofix
AI about 1 month ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| billingRouter.post( | ||
| "/billing/portal", | ||
| authMiddleware, | ||
| apiRateLimiter, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix missing rate limiting on an expensive route, introduce a rate-limiting middleware (or reuse an existing one) and apply it to that route so that the number of accepted requests per client (or globally) is capped over a time window. This protects backing services (database, queues, external APIs) from overload due to bursts of requests.
In this file, the best fix with minimal functional change is to apply the existing apiRateLimiter middleware (already imported and used on /billing/checkout and /billing/portal) to the stripeWebhookHandler route. Since stripeWebhookHandler is currently exported as a bare function, the most consistent change—while staying within the snippet—is to wrap it in an Express router post definition that includes apiRateLimiter. However, the surrounding router wiring isn’t shown, so the smallest safe change is to define and export a new Express router that mounts the webhook handler with apiRateLimiter, and adjust the handler to be an Express-style middleware in that router. Within this snippet, we can convert stripeWebhookHandler from a standalone exported function to a billingRouter.post("/billing/webhook", authMiddleware?, apiRateLimiter, handler) style definition, mirroring the pattern used for the other routes. Because we must not assume changes in other files, we keep the handler logic intact and just add apiRateLimiter into the middleware chain for the webhook endpoint.
Concretely:
- Locate the
export async function stripeWebhookHandlerdefinition near the end ofapps/gateway/src/routes/billing.ts. - Replace that exported function with a
billingRouter.post("/billing/webhook", apiRateLimiter, async (req, res) => { ... }), reusing the existing handler body unchanged. - If
billingRouteris already defined earlier in the file (as implied bybillingRouter.post("/billing/checkout", ...)andbillingRouter.post("/billing/portal", ...)), no new definitions or imports are needed. - This ensures that every incoming webhook request passes through
apiRateLimiterbefore executing the expensive RabbitMQ publishing logic.
| @@ -220,31 +220,37 @@ | ||
| } | ||
| ); | ||
|
|
||
| export async function stripeWebhookHandler(req: Request, res: Response) { | ||
| const sig = req.headers["stripe-signature"] as string; | ||
| billingRouter.post( | ||
| "/billing/webhook", | ||
| apiRateLimiter, | ||
| async (req: Request, res: Response) => { | ||
| const sig = req.headers["stripe-signature"] as string; | ||
|
|
||
| let event: Stripe.Event; | ||
| try { | ||
| event = stripe.webhooks.constructEvent( | ||
| req.body, | ||
| sig, | ||
| process.env.STRIPE_WEBHOOK_SECRET! | ||
| ); | ||
| } catch (err: any) { | ||
| console.error("[billing] webhook signature failed:", err.message); | ||
| return res.status(400).send(`Webhook Error: ${err.message}`); | ||
| } | ||
| let event: Stripe.Event; | ||
| try { | ||
| event = stripe.webhooks.constructEvent( | ||
| req.body, | ||
| sig, | ||
| process.env.STRIPE_WEBHOOK_SECRET! | ||
| ); | ||
| } catch (err: any) { | ||
| console.error("[billing] webhook signature failed:", err.message); | ||
| return res.status(400).send(`Webhook Error: ${err.message}`); | ||
| } | ||
|
|
||
| try { | ||
| await publishToStripeQueue({ | ||
| stripeEventId: event.id, | ||
| stripeEventType: event.type, | ||
| payload: event.data.object, | ||
| }); | ||
| console.log(`[billing] queued stripe event ${event.type} id=${event.id}`); | ||
| return res.json({ received: true }); | ||
| } catch (err) { | ||
| console.error("[billing] failed to queue stripe event:", err); | ||
| return res.status(500).json({ error: "queue_publish_failed" }); | ||
| try { | ||
| await publishToStripeQueue({ | ||
| stripeEventId: event.id, | ||
| stripeEventType: event.type, | ||
| payload: event.data.object, | ||
| }); | ||
| console.log( | ||
| `[billing] queued stripe event ${event.type} id=${event.id}` | ||
| ); | ||
| return res.json({ received: true }); | ||
| } catch (err) { | ||
| console.error("[billing] failed to queue stripe event:", err); | ||
| return res.status(500).json({ error: "queue_publish_failed" }); | ||
| } | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/load-tests/performance-budget.js`:
- Around line 85-87: The catch block currently declares catch (error) but never
uses it; either remove the unused binding by changing it to the optional catch
binding (catch { return true; }) or log the thrown error for debugging by
keeping catch (error) and adding a logging statement before returning (e.g.,
console.error or the existing logger) so the code becomes catch (error) {
console.error('Parse failed in performance budget check:', error); return true;
} — update the catch around the failing parse/validation in
performance-budget.js accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b05f96a9-7ae6-4e5f-b112-35c69e6fbeb5
📒 Files selected for processing (2)
apps/gateway/src/routes/billing.tsscripts/load-tests/performance-budget.js
| } catch (error) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused error parameter in catch block.
The error parameter is captured but never used. Consider either:
- Logging the error for debugging parse failures during load tests, or
- Keeping the ES2019+ optional catch binding syntax (
catch {) if the error is not needed
Option 1: Log the error for debugging
- } catch (error) {
+ } catch (e) {
+ console.warn('GraphQL response parse failed:', e.message);
return true;
}Option 2: Use optional catch binding (cleaner if error is not needed)
- } catch (error) {
+ } catch {
return true;
}📝 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.
| } catch (error) { | |
| return true; | |
| } | |
| } catch { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/load-tests/performance-budget.js` around lines 85 - 87, The catch
block currently declares catch (error) but never uses it; either remove the
unused binding by changing it to the optional catch binding (catch { return
true; }) or log the thrown error for debugging by keeping catch (error) and
adding a logging statement before returning (e.g., console.error or the existing
logger) so the code becomes catch (error) { console.error('Parse failed in
performance budget check:', error); return true; } — update the catch around the
failing parse/validation in performance-budget.js accordingly.
2 participants
Summary
ENOENT /libs/proto/device.proto/app/dist/services/; proto is at/app/libs/proto/device.proto../../../../(4 levels up →/); correct is 2 levels up or hardcoded/app/libs/protoTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores