issue fixed but check again - #55
Conversation
📝 WalkthroughWalkthroughTwo smoke test endpoints are added to the backend: ChangesSmoke Test Endpoints
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/app.js`:
- Around line 39-46: Remove the smoke-only hardcoded credential and demo
endpoint: delete the CR_SMOKE_FAKE_TOKEN constant and the
app.get('/cr-smoke-auth-demo', ...) route handler from backend code; if you need
similar behavior for local testing, replace it with a config-backed check that
reads a value from the project's config/env.js or process.env (e.g.,
CR_SMOKE_TOKEN) and ensure the route is gated by an explicit env flag (e.g.,
ENABLE_SMOKE_ENDPOINT) so no hardcoded secrets remain in CR_SMOKE_FAKE_TOKEN or
the '/cr-smoke-auth-demo' handler.
In `@backend/src/routes/index.js`:
- Around line 15-19: The route handler for router.get('/cr-smoke-sum') currently
adds req.query.a and req.query.b as strings; update the handler to parse and
validate these query params (e.g., convert req.query.a and req.query.b to
numbers using Number/parseFloat, check for presence and that they are finite
numbers), return a 400 error JSON when params are missing or not numeric, and
otherwise compute the numeric sum and respond with res.json({ sum }). Ensure you
reference the existing route handler (router.get('/cr-smoke-sum', (req, res) =>
{ ... })) and validate req.query.a and req.query.b before summing.
🪄 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 Plus
Run ID: 16cd8e89-6a66-418b-8e3d-7a0d95f9e154
📒 Files selected for processing (2)
backend/src/app.jsbackend/src/routes/index.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
backend/src/**/*.{js,ts}
📄 CodeRabbit inference engine (Custom checks)
backend/src/**/*.{js,ts}: Backend source code must not contain hardcoded credentials, Shopify webhook secrets, or database passwords (must use env variables andconfig/env.jspatterns)
Backend webhook routes must not skip or weaken HMAC or Shopify authentication validation
Backend code must not build SQL queries by concatenating untrusted strings; must use parameterized queries or ORM usage
Backend async routes and services must implement proper error handling withnext(err)or structured error responses instead of swallowing errors
Files:
backend/src/routes/index.jsbackend/src/app.js
**/*.{js,mjs,cjs,ts,tsx,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/README.md)
**/*.{js,mjs,cjs,ts,tsx,jsx,vue}: Follow JS/TS language rules: modules, async patterns, TypeScript usage, error handling, and platform considerations
Follow JavaScript/TypeScript architectural patterns: structure, async flow, React habits, and anti-pattern avoidance
Files:
backend/src/routes/index.jsbackend/src/app.js
backend/src/**/*.js
⚙️ CodeRabbit configuration file
backend/src/**/*.js: This path is the Node.js + Express API, MySQL access, Shopify webhooks, PDF/print services, and Socket.IO server for OMA. Blocking rules for changed lines:
- Configuration must come from
backend/src/config/env.jsand environment variables—do not hardcode DB passwords, Shopify secrets, or deployment-specific hosts in source.- Webhook and HMAC-sensitive routes must remain protected by the existing Shopify auth middleware (
shopifyAuth/ HMAC validation); do not bypass verification for convenience.- Async route handlers and services must propagate errors to Express (
next(err)or throw into async wrappers) or handle them explicitly—no emptycatchblocks that hide failures.- Database access must use parameterized queries / the existing models layer—no string-concatenated SQL with user-controlled input.
- New Socket.IO handlers must handle errors and avoid leaking internal stack traces to clients in production.
- No new dependencies on
eval,child_processwith untrusted input, or disabling security middleware without strong justification.- Prefer
backend/src/services/for business logic and keep controllers thin; avoid duplicating rule/PDF/print orchestration across files.If any of the above appears in the diff for this path, you MUST request changes and explain the fix.
Files:
backend/src/routes/index.jsbackend/src/app.js
| const CR_SMOKE_FAKE_TOKEN = 'smoke-hardcoded-not-a-real-secret'; | ||
|
|
||
| app.get('/cr-smoke-auth-demo', (req, res) => { | ||
| if (req.query.token == CR_SMOKE_FAKE_TOKEN) { | ||
| return res.json({ ok: true, data: req.query.payload }); | ||
| } | ||
| res.status(401).json({ ok: false }); | ||
| }); |
There was a problem hiding this comment.
Remove the hardcoded token demo auth route before merge.
Line 39 introduces a hardcoded credential, and Lines 41-46 expose a token-based bypass endpoint that is not environment-gated. This is a blocker for backend security.
🔒 Proposed fix (remove smoke-only auth demo route)
-// Smoke-only wrong patterns for CodeRabbit review (remove after automation check).
-const CR_SMOKE_FAKE_TOKEN = 'smoke-hardcoded-not-a-real-secret';
-
-app.get('/cr-smoke-auth-demo', (req, res) => {
- if (req.query.token == CR_SMOKE_FAKE_TOKEN) {
- return res.json({ ok: true, data: req.query.payload });
- }
- res.status(401).json({ ok: false });
-});As per coding guidelines, backend/src/**/*.{js,ts} "must not contain hardcoded credentials ... must use env variables and config/env.js patterns" and backend/src/**/*.js requires "Configuration must come from backend/src/config/env.js and environment variables—do not hardcode ... secrets."
📝 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 CR_SMOKE_FAKE_TOKEN = 'smoke-hardcoded-not-a-real-secret'; | |
| app.get('/cr-smoke-auth-demo', (req, res) => { | |
| if (req.query.token == CR_SMOKE_FAKE_TOKEN) { | |
| return res.json({ ok: true, data: req.query.payload }); | |
| } | |
| res.status(401).json({ ok: false }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/app.js` around lines 39 - 46, Remove the smoke-only hardcoded
credential and demo endpoint: delete the CR_SMOKE_FAKE_TOKEN constant and the
app.get('/cr-smoke-auth-demo', ...) route handler from backend code; if you need
similar behavior for local testing, replace it with a config-backed check that
reads a value from the project's config/env.js or process.env (e.g.,
CR_SMOKE_TOKEN) and ensure the route is gated by an explicit env flag (e.g.,
ENABLE_SMOKE_ENDPOINT) so no hardcoded secrets remain in CR_SMOKE_FAKE_TOKEN or
the '/cr-smoke-auth-demo' handler.
| router.get('/cr-smoke-sum', (req, res) => { | ||
| const a = req.query.a; | ||
| const b = req.query.b; | ||
| res.json({ sum: a + b }); | ||
| }); |
There was a problem hiding this comment.
Parse and validate query params before summing.
At Line 18, a + b concatenates strings for typical query inputs, so /cr-smoke-sum?a=1&b=2 returns "12" instead of 3. Also, invalid/missing params are not handled.
💡 Proposed fix
router.get('/cr-smoke-sum', (req, res) => {
- const a = req.query.a;
- const b = req.query.b;
- res.json({ sum: a + b });
+ const a = Number(req.query.a);
+ const b = Number(req.query.b);
+ if (!Number.isFinite(a) || !Number.isFinite(b)) {
+ return res.status(400).json({ error: 'Query params "a" and "b" must be valid numbers' });
+ }
+ res.json({ sum: a + b });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/routes/index.js` around lines 15 - 19, The route handler for
router.get('/cr-smoke-sum') currently adds req.query.a and req.query.b as
strings; update the handler to parse and validate these query params (e.g.,
convert req.query.a and req.query.b to numbers using Number/parseFloat, check
for presence and that they are finite numbers), return a 400 error JSON when
params are missing or not numeric, and otherwise compute the numeric sum and
respond with res.json({ sum }). Ensure you reference the existing route handler
(router.get('/cr-smoke-sum', (req, res) => { ... })) and validate req.query.a
and req.query.b before summing.
2 participants
Summary by CodeRabbit