Repository navigation
AI Based Code Review - #65
morgangraphics wants to merge 1 commit into
Conversation
…n execution ordering, security issues
There was a problem hiding this comment.
Pull request overview
This PR focuses on tightening default configuration/security behaviors, cleaning up API/docs, and improving test reliability around CORS and data filtering/sorting.
Changes:
- Harden startup configuration handling (fail fast on missing production config; improve env-derived defaults).
- Fix data-processing behaviors in
FileUtils(random limiting correctness, sorting comparator refactor, apply column filtering post-processing) and extend unit tests accordingly. - Update API route behavior/docs/tests (CORS Cypress tests made promise-aware; API validation/docs tweaks; remove unused cache entry and related documentation).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
server.js |
Safer config fallback behavior; production refuses to start without config. |
README.md |
Documentation cleanup (tests script name, option notes, typos, removed format mentions in places). |
cypress/e2e/cors.cy.js |
Make axios calls returned Promises; strengthen failure expectations. |
config/default.example.yaml |
Add Cypress CORS test environments (cors-valid, cors-invalid). |
components/files.js |
Refactor filtering/sorting; fix random indexing; move header filtering to output stage; adjust data loading/cache behavior. |
components/files.test.js |
Add regression test for random limiting; reset shared cache between suites; update expectations for sort order and renamed comparator. |
components/api.js |
Remove format/help from handleConfig; tighten nulls validation; doc/comment fixes. |
components/api.test.js |
Update expectations to reflect removed format/help fields from handleConfig. |
api/marvel.js |
Change error handling messaging/logging; adjust {characters} param validation call. |
api/dc.js |
Remove per-route cors: true; change error handling messaging/logging; adjust {characters} param validation call. |
api/_common.js |
Remove unused cache.search. |
api/_common.test.js |
Update tests to match cache structure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tags: ['api'], | ||
| validate: { | ||
| params: api.validateParams(['characters'], 'post'), | ||
| params: api.validateParams(['characters'], 'get'), | ||
| query: api.validateParams(common.validCharQParams, 'get'), | ||
| }, |
There was a problem hiding this comment.
The {characters} path param is now validated with validateParams(['characters'], 'get'), which uses a restrictive regex intended for query strings. This will reject common character names containing hyphens/spaces (e.g. "spider-man"). Path params should allow the full character string; use the more permissive schema (or a dedicated params validator) for route params.
| tags: ['api'], | ||
| validate: { | ||
| params: api.validateParams(['characters'], 'post'), | ||
| params: api.validateParams(['characters'], 'get'), | ||
| query: api.validateParams(common.validCharQParams, 'get'), | ||
| }, |
There was a problem hiding this comment.
Same issue as Marvel: validating {characters} with validateParams(['characters'], 'get') applies the query-string regex and will reject hyphens/spaces in character names. Use a permissive params validator for this route param so /dc/{characters} works for real-world names.
| PORT: <PRODUCTION_SERVICE_PORT> | ||
| SWAGGER_ENABLED: false |
There was a problem hiding this comment.
In the production: section, PORT and SWAGGER_ENABLED are indented differently than the other keys. This makes the YAML invalid or changes the parsed structure. Align indentation so all keys under production: use the same indentation level.
| PORT: <PRODUCTION_SERVICE_PORT> | |
| SWAGGER_ENABLED: false | |
| PORT: <PRODUCTION_SERVICE_PORT> | |
| SWAGGER_ENABLED: false |
| if (common.cache[this.universe].length > 0) { | ||
| return Promise.resolve(this.filterData()); | ||
| } | ||
|
|
||
| if (['name', 'urlslug'].includes(header) && ['"', '/'].some(str => value.includes(str))) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| value = value.replace(/\\/g, ''); | ||
| } | ||
| // Will convert empty values to null | ||
| if (!value) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| value = null; | ||
| } | ||
| return new Promise((resolve, reject) => { | ||
| const filePath = path.join(__dirname, '..', 'files', this.characters[this.universe]); | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename | ||
| fs.createReadStream(filePath) |
There was a problem hiding this comment.
getData() relies on common.cache[this.universe].length > 0 as a “cache warm” signal, but the cache is never cleared before starting a CSV read. If a previous read partially populated the array (e.g., after a stream error), subsequent calls will incorrectly return incomplete data. Clear the cache at the start of the read and/or only treat it as valid after a successful end.
| handleConfig(options) { | ||
| const config = {}; | ||
| // These are the only params we're concerned about - Object Destructuring | ||
| const { | ||
| characters, format, h, help, limit, nulls, pretty, prune, random, s, seed, universe, | ||
| characters, h, limit, nulls, pretty, prune, random, s, seed, universe, | ||
| } = options; | ||
| if (characters) { | ||
| config.characters = this.searchObj(characters); | ||
| } | ||
| config.format = format; | ||
| if (h) { |
There was a problem hiding this comment.
handleConfig() no longer consumes format, but validateParams() still always validates format and help text still documents it. This is an API contract mismatch: either fully remove format support (valid param lists, Joi schema, docs/help) or keep it in handleConfig() and enforce behavior.
Some code cleanup, documentation, dead code, security issues, function execution ordering, security issues