Cursor new oma req review - #58
Conversation
📝 WalkthroughWalkthroughTwo new demonstration endpoints are added: one in ChangesSmoke Test Endpoints
🎯 1 (Trivial) | ⏱️ ~3 minutes
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: 3
🤖 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 38-40: Remove the hardcoded credential CR_SMOKE_FAKE_TOKEN from
the source and load it from the environment via the existing config/env.js
pattern: replace the constant usage with a reference to the env-config value
(e.g., use process.env or the project's config export) and ensure
CR_SMOKE_FAKE_TOKEN is defined by reading the appropriate env var in
config/env.js (add a fallback/validation if missing) so no secret literals
remain in backend/src/app.js.
- Around line 41-46: The demo auth route app.get('/cr-smoke-auth-demo', ...)
exposes a weak query-string token and must be hardened: only register this route
when not in production (check process.env.NODE_ENV !== 'production' or similar)
and stop accepting tokens via req.query; change the route to require a POST (or
at least require an Authorization header) and validate the token from
req.headers.authorization (or req.body.token) against the configured
CR_SMOKE_FAKE_TOKEN from env, returning 401 otherwise; ensure the route is only
present in non-production builds and do not log or leak the raw token.
In `@backend/src/routes/index.js`:
- Around line 15-19: The route handler for router.get('/cr-smoke-sum') reads a
and b from req.query as strings so a + b concatenates; update the handler to
parse a and b into numbers (e.g., using Number(...) or parseFloat(...)) before
summing, validate that parsing produced finite numbers (check for NaN or
!isFinite), and return a 400 response with an error message when inputs are
invalid; otherwise compute numericSum = parsedA + parsedB and respond with
res.json({ sum: numericSum }).
🪄 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: c0d6b7f0-d3e8-4a3f-83cc-4f31d247ec55
⛔ Files ignored due to path filters (1)
.github/workflows/coderabbit-auto-fix.ymlis excluded by!**/*.yml
📒 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
| // Smoke-only wrong patterns for CodeRabbit review (remove after automation check). | ||
| const CR_SMOKE_FAKE_TOKEN = 'smoke-hardcoded-not-a-real-secret'; | ||
|
|
There was a problem hiding this comment.
Remove hardcoded token from backend source.
At Line 39, a credential-like token is hardcoded. This must be sourced from config/env.js + environment variables.
Proposed fix
-const CR_SMOKE_FAKE_TOKEN = 'smoke-hardcoded-not-a-real-secret';
+const CR_SMOKE_DEMO_TOKEN = config.crSmoke?.demoToken;As per coding guidelines, "Backend source code must not contain hardcoded credentials, Shopify webhook secrets, or database passwords (must use env variables and config/env.js patterns)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/app.js` around lines 38 - 40, Remove the hardcoded credential
CR_SMOKE_FAKE_TOKEN from the source and load it from the environment via the
existing config/env.js pattern: replace the constant usage with a reference to
the env-config value (e.g., use process.env or the project's config export) and
ensure CR_SMOKE_FAKE_TOKEN is defined by reading the appropriate env var in
config/env.js (add a fallback/validation if missing) so no secret literals
remain in backend/src/app.js.
| 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.
Do not ship this public demo-auth route as-is.
This adds a weak alternate auth surface. At minimum, gate it to non-production and avoid query-string token transport.
Proposed hardening
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 });
+ if (config.nodeEnv === 'production') {
+ return res.status(404).json({ ok: false });
+ }
+
+ const token = req.get('x-smoke-token');
+ if (!token || token !== CR_SMOKE_DEMO_TOKEN) {
+ return res.status(401).json({ ok: false });
+ }
+
+ return res.json({ ok: true, data: req.query.payload });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/app.js` around lines 41 - 46, The demo auth route
app.get('/cr-smoke-auth-demo', ...) exposes a weak query-string token and must
be hardened: only register this route when not in production (check
process.env.NODE_ENV !== 'production' or similar) and stop accepting tokens via
req.query; change the route to require a POST (or at least require an
Authorization header) and validate the token from req.headers.authorization (or
req.body.token) against the configured CR_SMOKE_FAKE_TOKEN from env, returning
401 otherwise; ensure the route is only present in non-production builds and do
not log or leak the raw token.
| 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 query params as numbers before computing the sum.
At Line 18, a and b are strings from req.query, so a + b concatenates ("1" + "2" => "12"). Parse and validate inputs first.
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') reads a and b from req.query as strings so a + b
concatenates; update the handler to parse a and b into numbers (e.g., using
Number(...) or parseFloat(...)) before summing, validate that parsing produced
finite numbers (check for NaN or !isFinite), and return a 400 response with an
error message when inputs are invalid; otherwise compute numericSum = parsedA +
parsedB and respond with res.json({ sum: numericSum }).
2 participants
Summary by CodeRabbit