Conversation
📝 WalkthroughWalkthroughChanges refactor request property access patterns across controllers using lodash Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api-service/src/controllers/Alerts/Metric.ts (1)
22-22:⚠️ Potential issue | 🟠 MajorPre-existing bug: comma expression discards the status code in
errorResponse.This isn't introduced by your PR, but worth flagging since it exists in every handler in this file. The double parentheses in
errorResponse((httpStatus.INTERNAL_SERVER_ERROR, { message: errorMessage }))create a comma expression — the status code500is evaluated and discarded, and only{ message: errorMessage }is passed toerrorResponse. This means all error responses likely use the default status rather than 500 explicitly.The same pattern appears on lines 33, 56, 69, and 81.
Fix for all occurrences
- next(errorResponse((httpStatus.INTERNAL_SERVER_ERROR, { message: errorMessage }))) + next(errorResponse(httpStatus.INTERNAL_SERVER_ERROR, errorMessage))api-service/src/controllers/NotificationChannel/Notification.ts (1)
101-101:⚠️ Potential issue | 🟠 MajorPre-existing bug:
Notification.update()is not awaited.The
Notification.update(...)call on Line 101 is missingawait. This means the response is sent before the database update completes. If the update fails, the error is unhandled (becomes an unhandled promise rejection), and the client already received a success response.Suggested fix
- Notification.update({ status: "live", updated_by: userID }, { where: { id } }); + await Notification.update({ status: "live", updated_by: userID }, { where: { id } });
🤖 Fix all issues with AI agents
In `@api-service/src/configs/ConnectionsConfig.ts`:
- Around line 5-10: The postgres config currently exposes a single concatenated
credentials string (postgres.credentials) which relies on a fragile "::"
separator and mismatched base64 assumptions; change ConnectionsConfig to expose
separate fields postgres.username and postgres.password (avoid concatenation),
set defaults as plain strings (e.g., "postgres"), and update the consumer
(databaseConnection.ts which currently does Buffer.from(credentials[1],
'base64')) to read connectionConfig.postgres.password and perform base64
decoding there only if the environment contract requires it; ensure you remove
any split("::") usage and handle decoding explicitly (Buffer.from(password,
'base64').toString('utf-8')) or skip decoding if the password is plaintext.
In `@api-service/src/connections/databaseConnection.ts`:
- Around line 4-7: Guard the fragile credentials parsing in databaseConnection
by validating the split result before decoding: check
connectionConfig.postgres.credentials contains "::" and that credentials.length
>= 2 and credentials[1] is a non-empty string, then attempt Buffer.from(...)
inside a try/catch to surface a clear, actionable error (or rethrow) if base64
decoding fails; update the variables referenced (credentials and
decodedCredentials) so they aren’t computed from undefined and add a descriptive
error message that mentions connectionConfig.postgres.credentials format
expectations and suggests using separate username/password fields.
In `@api-service/src/controllers/DataMetrics/DataMetricsController.ts`:
- Line 29: Standardize the Strict-Transport-Security header name to the
canonical casing: update the header string usages (e.g., the res.setHeader call
in DataMetricsController.ts) so all files use 'Strict-Transport-Security' (not
'STRICT-TRANSPORT-SECURITY'); locate occurrences in ResponseHandler.ts and
prometheus/index.ts (search for setHeader or the header key constant) and
replace the uppercase variant with the canonical 'Strict-Transport-Security'
string so every module uses the same header name.
In `@api-service/src/controllers/NotificationChannel/Notification.ts`:
- Line 50: The handler in Notification.ts uses _.get(request.body, 'request',
{}) which inconsistently accesses request.body directly; change it to use safe
lodash access consistent with other handlers—e.g., retrieve the body via
_.get(request, 'body', {}) and then extract the nested request object (or use
_.get(request, 'body.request', {})) so the destructuring of limit, filters,
offset uses the same safe access pattern as other handlers.
In `@api-service/src/metrics/prometheus/index.ts`:
- Line 28: Standardize the HSTS header name and centralize its setting: replace
all occurrences of the all-caps header string with the canonical
"Strict-Transport-Security" and remove the duplicated res.setHeader calls inside
ResponseHandler.successResponse, ResponseHandler.errorResponse,
ResponseHandler.obsrvErrorResponse, ResponseHandler.flatResponse,
ResponseHandler.goneResponse, metricsScrapeHandler and dataMetrics; instead add
a single Express middleware mounted early (e.g., via app.use) that calls
res.setHeader('Strict-Transport-Security', 'max-age=31536000;
includeSubDomains') and calls next() so every response automatically gets the
header.
🧹 Nitpick comments (3)
api-service/src/controllers/DatasetRead/DatasetRead.ts (1)
22-24: Duplicated_.getextraction invalidateRequestanddatasetRead.
dataset_id,fields, andmodeare extracted identically in both functions (lines 22-24 and 36-38). Consider extracting once indatasetReadand passing tovalidateRequest, or havingvalidateRequestreturn the parsed values.Also applies to: 36-38
api-service/src/controllers/Alerts/Metric.ts (1)
13-14: Redundantcomponentoverride on line 14.
componentis extracted fromreq.body.componenton line 13, then the entirereq.body(which already containscomponent) is spread, andcomponentis overridden with the same value. The override is a no-op.This is pre-existing, but since you're touching these lines, consider simplifying:
Suggested simplification
- const component = _.get(req, 'body.component'); - const metricsBody = await Metrics.create({ ...(_.get(req, 'body')), component: component }); + const metricsBody = await Metrics.create(_.get(req, 'body'));api-service/src/controllers/Alerts/Alerts.ts (1)
15-15: Inconsistent application of_.get()pattern across the file.Several handlers still use direct destructuring on
req.paramsandreq.query(Lines 71, 89, 90), while the changed handlers now use_.get(req, 'params.alertId'). If the goal is safer property access, the pattern should be applied uniformly. Otherwise, some handlers are protected and others are not, making the security posture inconsistent.That said,
req.body,req.params, andreq.queryare always present on ExpressRequestobjects after middleware parsing, so_.get()doesn't add meaningful safety here — it only adds verbosity. Consider whether this change is truly necessary.Also applies to: 33-33, 60-60, 108-109, 120-120, 135-135
| postgres: Object.freeze({ | ||
| host: env['postgres_host'] || "localhost", | ||
| port: env['postgres_port'] || 5432, | ||
| database: env['postgres_database'] || "obsrv", | ||
| credentials: `${env['postgres_username'] || "postgres"}::${env['postgres_password'] || "postgres"}` | ||
| }), |
There was a problem hiding this comment.
Critical: Credential string format creates multiple failure modes.
-
Base64 assumption mismatch: The consumer (
databaseConnection.tsLine 7) callsBuffer.from(credentials[1], 'base64')to decode the password portion. But the default value"postgres"on Line 9 is plaintext, not base64-encoded. This will silently produce garbled credentials and the DB connection will fail at startup. -
Fragile separator:
"::"can appear in passwords. If a password contains::,split("::")in the consumer will yield more than two elements, andcredentials[1]will be only a partial password — a silent data corruption bug. -
No security benefit: Concatenating credentials into one string only to split them in the consumer adds indirection without any hardening. The values still reside in plaintext in process memory. If the intent is to accept a pre-encoded credential blob from the environment, the config should accept a single opaque env var rather than concatenating two separate env vars.
Consider keeping username and password as separate config fields. If base64-encoded passwords are required, decode at the point of use with clear documentation of the expectation.
Suggested approach
postgres: Object.freeze({
host: env['postgres_host'] || "localhost",
port: env['postgres_port'] || 5432,
database: env['postgres_database'] || "obsrv",
- credentials: `${env['postgres_username'] || "postgres"}::${env['postgres_password'] || "postgres"}`
+ username: env['postgres_username'] || "postgres",
+ password: env['postgres_password'] || "postgres",
}),Then in databaseConnection.ts, if the password env var is expected to be base64-encoded:
const password = Buffer.from(connectionConfig.postgres.password, 'base64').toString('utf-8');🤖 Prompt for AI Agents
In `@api-service/src/configs/ConnectionsConfig.ts` around lines 5 - 10, The
postgres config currently exposes a single concatenated credentials string
(postgres.credentials) which relies on a fragile "::" separator and mismatched
base64 assumptions; change ConnectionsConfig to expose separate fields
postgres.username and postgres.password (avoid concatenation), set defaults as
plain strings (e.g., "postgres"), and update the consumer (databaseConnection.ts
which currently does Buffer.from(credentials[1], 'base64')) to read
connectionConfig.postgres.password and perform base64 decoding there only if the
environment contract requires it; ensure you remove any split("::") usage and
handle decoding explicitly (Buffer.from(password, 'base64').toString('utf-8'))
or skip decoding if the password is plaintext.
| const { database, host, port } = connectionConfig.postgres; | ||
| const credentials = connectionConfig.postgres.credentials.split("::"); | ||
|
|
||
| export const sequelize = new Sequelize({ | ||
| database, password, username: username, dialect: "postgres", host, port: +port, pool: { | ||
| max: 2, | ||
| min: 1, | ||
| acquire: 30000, | ||
| idle: 10000 | ||
| } | ||
| }) | ||
| const decodedCredentials = Buffer.from(credentials[1], 'base64').toString('utf-8'); |
There was a problem hiding this comment.
No validation on credential parsing — app will crash with an unhelpful error on misconfiguration.
If the credentials string from config doesn't contain "::", credentials[1] will be undefined, and Buffer.from(undefined, 'base64') will throw a TypeError at module load time, crashing the application with no indication that the credential format is wrong.
At minimum, add a guard:
Suggested fix
const credentials = connectionConfig.postgres.credentials.split("::");
+if (credentials.length < 2 || !credentials[1]) {
+ throw new Error("Invalid postgres credentials format. Expected 'username::base64password'.");
+}
const decodedCredentials = Buffer.from(credentials[1], 'base64').toString('utf-8');However, as noted in the ConnectionsConfig.ts review, the entire credential-string approach is fragile. Separate username and password fields would eliminate this class of errors.
📝 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 { database, host, port } = connectionConfig.postgres; | |
| const credentials = connectionConfig.postgres.credentials.split("::"); | |
| export const sequelize = new Sequelize({ | |
| database, password, username: username, dialect: "postgres", host, port: +port, pool: { | |
| max: 2, | |
| min: 1, | |
| acquire: 30000, | |
| idle: 10000 | |
| } | |
| }) | |
| const decodedCredentials = Buffer.from(credentials[1], 'base64').toString('utf-8'); | |
| const { database, host, port } = connectionConfig.postgres; | |
| const credentials = connectionConfig.postgres.credentials.split("::"); | |
| if (credentials.length < 2 || !credentials[1]) { | |
| throw new Error("Invalid postgres credentials format. Expected 'username::base64password'."); | |
| } | |
| const decodedCredentials = Buffer.from(credentials[1], 'base64').toString('utf-8'); |
🤖 Prompt for AI Agents
In `@api-service/src/connections/databaseConnection.ts` around lines 4 - 7, Guard
the fragile credentials parsing in databaseConnection by validating the split
result before decoding: check connectionConfig.postgres.credentials contains
"::" and that credentials.length >= 2 and credentials[1] is a non-empty string,
then attempt Buffer.from(...) inside a try/catch to surface a clear, actionable
error (or rethrow) if base64 decoding fails; update the variables referenced
(credentials and decodedCredentials) so they aren’t computed from undefined and
add a descriptive error message that mentions
connectionConfig.postgres.credentials format expectations and suggests using
separate username/password fields.
| const { url, method, headers = {}, body = {}, params = {}, ...rest } = query; | ||
| const apiResponse = await axios.request({ url, method, headers, params, data: body, ...rest }) | ||
| const data = _.get(apiResponse, "data"); | ||
| res.setHeader('Strict-Transport-Security', 'max-age=31536000; includeSubDomains'); |
There was a problem hiding this comment.
Header casing differs from other files in this PR.
This file uses 'Strict-Transport-Security' (canonical casing) while ResponseHandler.ts and prometheus/index.ts use 'STRICT-TRANSPORT-SECURITY'. Standardize to the canonical form across all files.
🤖 Prompt for AI Agents
In `@api-service/src/controllers/DataMetrics/DataMetricsController.ts` at line 29,
Standardize the Strict-Transport-Security header name to the canonical casing:
update the header string usages (e.g., the res.setHeader call in
DataMetricsController.ts) so all files use 'Strict-Transport-Security' (not
'STRICT-TRANSPORT-SECURITY'); locate occurrences in ResponseHandler.ts and
prometheus/index.ts (search for setHeader or the header key constant) and
replace the uppercase variant with the canonical 'Strict-Transport-Security'
string so every module uses the same header name.
| const listHandler = async (request: Request, response: Response, next: NextFunction) => { | ||
| try { | ||
| const { limit, filters, offset } = request.body?.request || {}; | ||
| const { limit, filters, offset } = _.get(request.body, 'request', {}); |
There was a problem hiding this comment.
Inconsistent: request.body is accessed directly here while all other handlers use _.get(request, "body").
If the goal is safe property access, this line should follow the same pattern:
- const { limit, filters, offset } = _.get(request.body, 'request', {});
+ const { limit, filters, offset } = _.get(request, 'body.request', {});📝 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 { limit, filters, offset } = _.get(request.body, 'request', {}); | |
| const { limit, filters, offset } = _.get(request, 'body.request', {}); |
🤖 Prompt for AI Agents
In `@api-service/src/controllers/NotificationChannel/Notification.ts` at line 50,
The handler in Notification.ts uses _.get(request.body, 'request', {}) which
inconsistently accesses request.body directly; change it to use safe lodash
access consistent with other handlers—e.g., retrieve the body via _.get(request,
'body', {}) and then extract the nested request object (or use _.get(request,
'body.request', {})) so the destructuring of limit, filters, offset uses the
same safe access pattern as other handlers.
| try { | ||
| res.set("Content-Type", register.contentType); | ||
| const metrics = await register.metrics() | ||
| res.setHeader('STRICT-TRANSPORT-SECURITY', 'max-age=31536000; includeSubDomains'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent header casing and duplicated HSTS logic across handlers.
Two issues:
-
Casing: This file and
ResponseHandler.tsuse'STRICT-TRANSPORT-SECURITY'(all-caps), whileDataMetricsController.ts(line 29) uses'Strict-Transport-Security'. HTTP headers are case-insensitive per RFC 7230, but the canonical form isStrict-Transport-Security. Standardize across the codebase. -
Duplication: The same
res.setHeader(...)call is copy-pasted into every response path (ResponseHandler.successResponse,errorResponse,obsrvErrorResponse,flatResponse,goneResponse,metricsScrapeHandler,dataMetrics). This is fragile — any new response path will silently miss the header. A single Express middleware would be simpler and more reliable:
app.use((req, res, next) => {
res.setHeader('Strict-Transport-Security', 'max-age=31536000; includeSubDomains');
next();
});🤖 Prompt for AI Agents
In `@api-service/src/metrics/prometheus/index.ts` at line 28, Standardize the HSTS
header name and centralize its setting: replace all occurrences of the all-caps
header string with the canonical "Strict-Transport-Security" and remove the
duplicated res.setHeader calls inside ResponseHandler.successResponse,
ResponseHandler.errorResponse, ResponseHandler.obsrvErrorResponse,
ResponseHandler.flatResponse, ResponseHandler.goneResponse, metricsScrapeHandler
and dataMetrics; instead add a single Express middleware mounted early (e.g.,
via app.use) that calls res.setHeader('Strict-Transport-Security',
'max-age=31536000; includeSubDomains') and calls next() so every response
automatically gets the header.
1 participant
Summary by CodeRabbit
Security Enhancements
Backend Improvements