Cursor new oma req review - #57
Conversation
📝 WalkthroughWalkthroughTwo new smoke-test routes are added to the backend: ChangesSmoke Test Endpoints
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ 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: The route defines a hardcoded credential
CR_SMOKE_FAKE_TOKEN and authenticates using req.query.token (in the handler for
'/cr-smoke-auth-demo'), which must be removed; instead load the secret from your
env config (use the existing backend/src/config/env.js pattern) and validate a
header (e.g., Authorization or a custom X- header) on the '/cr-smoke-auth-demo'
handler, or remove/non-prod-gate the endpoint entirely; update the handler logic
that currently checks req.query.token to pull the secret from the config and
compare against req.get('Your-Header-Name') (and ensure the config key name is
added to env.js and .env as appropriate).
In `@backend/src/routes/index.js`:
- Around line 15-19: The handler for router.get('/cr-smoke-sum') currently
concatenates strings; coerce req.query.a and req.query.b to numbers (e.g., via
Number(...) or parseFloat), validate both are finite numbers (reject
NaN/Infinity) and return res.status(400).json({ error: 'invalid operands' }) for
bad inputs; otherwise compute numericSum = aNum + bNum and return res.json({
sum: numericSum }); update the anonymous route callback in index.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 Plus
Run ID: c539a103-0da7-49d9-91e5-e36ccae31dee
⛔ 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
| 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 and stop authenticating via req.query.
This adds a credential to source control and accepts it from the URL, which is easy to leak through logs, browser history, and proxies. If this route must exist, load the token through backend/src/config/env.js and read it from a header instead; otherwise remove or non-prod-gate the endpoint.
As per coding guidelines, backend/src/**/*.{js,ts}: 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 39 - 46, The route defines a hardcoded
credential CR_SMOKE_FAKE_TOKEN and authenticates using req.query.token (in the
handler for '/cr-smoke-auth-demo'), which must be removed; instead load the
secret from your env config (use the existing backend/src/config/env.js pattern)
and validate a header (e.g., Authorization or a custom X- header) on the
'/cr-smoke-auth-demo' handler, or remove/non-prod-gate the endpoint entirely;
update the handler logic that currently checks req.query.token to pull the
secret from the config and compare against req.get('Your-Header-Name') (and
ensure the config key name is added to env.js and .env as appropriate).
| 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 the operands before returning sum.
This endpoint currently returns the wrong result for numeric inputs: ?a=1&b=2 yields "12", and missing params can produce junk like "undefined2". Coerce both inputs to numbers and reject non-numeric values with 400 before adding them.
Suggested 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({
+ success: false,
+ message: 'Query params a and b must be numbers',
+ });
+ }
+
+ res.json({ sum: a + b });
});📝 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.
| router.get('/cr-smoke-sum', (req, res) => { | |
| const a = req.query.a; | |
| const b = req.query.b; | |
| res.json({ sum: a + b }); | |
| }); | |
| router.get('/cr-smoke-sum', (req, res) => { | |
| const a = Number(req.query.a); | |
| const b = Number(req.query.b); | |
| if (!Number.isFinite(a) || !Number.isFinite(b)) { | |
| return res.status(400).json({ | |
| success: false, | |
| message: 'Query params a and b must be 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 handler for
router.get('/cr-smoke-sum') currently concatenates strings; coerce req.query.a
and req.query.b to numbers (e.g., via Number(...) or parseFloat), validate both
are finite numbers (reject NaN/Infinity) and return res.status(400).json({
error: 'invalid operands' }) for bad inputs; otherwise compute numericSum = aNum
+ bNum and return res.json({ sum: numericSum }); update the anonymous route
callback in index.js accordingly.
2 participants
Summary by CodeRabbit