Conversation
📝 WalkthroughWalkthroughAdds global HSTS via Helmet and injects Strict-Transport-Security headers across many request handlers and error paths; introduces safer input extraction/validation (lodash.get), reflected-XSS sanitization (he.encode), minor refactors, and removal of commented legacy code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 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)
No actionable comments were generated in the recent review. 🎉 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/controllers/user_update.ts (1)
15-25:⚠️ Potential issue | 🔴 CriticalBug: missing
returnafter sending the 403 response — request processing continues.After the
res.status(403).json(...)call on line 16, execution falls through to line 26 and beyond, performing the DB update and attempting to send a second200response. This will cause a runtime error ("Cannot set headers after they are sent to the client") and, worse, allows unauthorized users to modify data.🐛 Proposed fix
if (user_name !== sessionUserName) { - res.status(403).json( + return res.status(403).json( transform({ responseCode: 'FORBIDDEN', params: { err: 'FORBIDDEN', errmsg: 'Access denied', }, }), ); }
🤖 Fix all issues with AI agents
In `@src/app.ts`:
- Around line 14-24: The Strict-Transport-Security header is being set multiple
times because Helmet is configured here via app.use(helmet({ hsts: ... })) and a
manual middleware (the function using response.setHeader), and Helmet is also
applied again where app is imported; remove the manual HSTS middleware (the
anonymous middleware that calls response.setHeader('Strict-Transport-Security',
...)) and ensure Helmet is only configured in one place (either keep the
helmet(...) call in this module or remove the duplicate helmet(...) call where
app is imported), leaving a single app.use(helmet({ hsts: ... })) configuration.
In `@src/index.ts`:
- Around line 21-27: The Helmet middleware is being registered twice on the same
Express app instance — remove the duplicate registration by deleting the
app.use(helmet({ hsts: { maxAge: 31536000, includeSubDomains: true, preload:
true } })) call from index.ts (since app is imported from ./app which already
applies Helmet), or alternatively remove the Helmet setup in the app module so
only one module calls helmet; ensure only one app.use(helmet(...)) remains
referencing the app variable to avoid double header handling.
In `@src/main/controllers/dataset_aggregator.ts`:
- Around line 224-228: The variable status is never reassigned so change its
declaration to a const to satisfy ESLint: replace the current let status =
_.get(request, 'query.status') with const status = _.get(request,
'query.status'), keeping datasetId as let (since datasetId is mutated later);
reference: request, params.datasetId, query.status, datasetId, status in the
dataset_aggregator.ts snippet.
In `@src/main/controllers/dataset_exists.ts`:
- Around line 21-23: The code in dataset_exists that sets the response status
uses reason.status (which is undefined for Axios errors) causing Express to send
200; change the status extraction to use reason.response.status (e.g.,
_.get(draftDataset[0], ['reason','response','status'])) and provide a numeric
fallback (like 500) to ensure a valid HTTP status; update the
response.status(...) call accordingly where draftDataset[0].status ===
'rejected' and errorData is computed.
In `@src/main/controllers/metrics_scrap.ts`:
- Around line 10-11: Add the missing semicolon after the await expression so the
declaration using register.metrics() is properly terminated; update the const
metrics = await register.metrics() statement in the metrics_scrap.ts controller
(the line just before response.setHeader(...)) to end with a semicolon to
satisfy ESLint and prevent potential ASI issues.
In `@src/main/controllers/test_connection.ts`:
- Line 3: Replace double-quoted string literals with single-quoted ones across
this file (e.g., change import * as _ from "lodash" to import * as _ from
'lodash') and update any other occurrences (the string literals referenced
around the other comment locations) so all string delimiters use single quotes
to comply with the project's Prettier config.
- Around line 20-27: The handler currently throws "Topic does not exist" which
gets caught and turned into a 500; change the flow so that when topicExists is
false you do not throw but instead return a 200 with { connectionEstablished:
true, topicExists: false } (i.e., remove the throw new Error("Topic does not
exist") and let the existing result send path handle it), or alternately detect
that specific error in the catch (error?.message === "Topic does not exist") and
respond with 200 and topicExists: false; update the logic around topicExists,
result, and the catch block to ensure only real connection failures produce the
500 error response.
In `@src/main/controllers/user_manage_status.ts`:
- Line 17: Remove the redundant per-handler HSTS header by deleting the explicit
res.setHeader('Strict-Transport-Security', 'max-age=31536000; includeSubDomains;
preload'); call inside this controller (and any other controllers/middlewares
where it appears) and rely on the centralized helmet configuration already
applied application-wide; simply remove the res.setHeader invocation from
user_manage_status handler(s) so responses use the global HSTS provided by
helmet.
In `@src/shared/utils/transformResponse.ts`:
- Around line 6-21: The current transformResponse uses he.encode (he.encode(id),
he.encode(err), he.encode(errmsg)) which HTML-entity-encodes JSON values and
causes double-encoding on clients; remove the direct calls to he.encode and
instead implement and call a lightweight server-side sanitizer (e.g.,
sanitizeString) that strips or escapes only dangerous characters like angle
brackets and control characters (not full HTML entities), and apply it to id,
err, errmsg and recursively to any string values inside result (handle
objects/arrays), keeping response JSON raw for clients to handle safe DOM
insertion; update the code paths around variables id, err, errmsg, and result to
use sanitizeString instead of he.encode.
🧹 Nitpick comments (12)
src/shared/utils/transformResponse.ts (1)
6-6: Split destructuring to satisfyconstcorrectness.Only
idneedslet; the rest are never reassigned. Same applies to line 13 where onlyerranderrmsgneedlet.Proposed fix
- let { id, ver = 'v1', ets = Date.now(), params = {}, responseCode = 'OK', result = {} } = payload; + let { id } = payload; + const { ver = 'v1', ets = Date.now(), params = {}, responseCode = 'OK', result = {} } = payload;- let { resmsgid = `${uuidv4()}`, err = '', status = responseCode === 'OK' ? 'SUCCESSFUL' : 'FAILED', errmsg = '' } = params; + const { resmsgid = `${uuidv4()}`, status = responseCode === 'OK' ? 'SUCCESSFUL' : 'FAILED' } = params; + let { err = '', errmsg = '' } = params;src/main/controllers/dataset_diff.ts (1)
10-10: Remove redundant HSTS header — helmet already sets this globally.Helmet is configured in
src/app.ts(lines 14-20) withhsts: { maxAge: 31536000, includeSubDomains: true, preload: true }, which sets the Strict-Transport-Security header on every response. The per-controller headers at line 10 and across 15+ other controllers duplicate this work without adding security value.Additionally,
src/app.tsitself redundantly sets this header twice: once via helmet's middleware and again in an explicit middleware at line 22 and route handler at line 66.Remove the per-controller headers and rely solely on helmet's global configuration, or extract the header value to a shared constant if defense-in-depth layering is intentional.
src/app.ts (1)
65-68: Redundant per-route HSTS header.With Helmet configured globally, this
setHeadercall is unnecessary. The same applies to every other controller that manually sets this header in the PR. Relying on the global middleware is the standard pattern — it's easier to maintain and less error-prone than scattering identical header calls across dozens of handlers.src/index.ts (1)
34-37: Redundant manual HSTS header in the catch-all route.Same as in
app.ts— Helmet already covers this. Remove thesetHeadercall.src/main/controllers/config.ts (1)
7-7: Redundant — covered by global Helmet middleware.Once the global Helmet HSTS configuration is consolidated (see comments on
app.ts/index.ts), this per-handlersetHeadercall can be removed.src/main/controllers/auth_user_info.ts (1)
14-14: Redundant HSTS header — same as other controllers.Remove once global Helmet middleware is consolidated.
src/main/controllers/auth_logout.ts (1)
9-9: Redundant HSTS header — same as other controllers.src/main/controllers/user_read.ts (2)
19-20: Safer token parsing — looks good, but consider guarding against malformed JSON.
JSON.parsewill throw on a corrupted/truncated session string. The caller'stry/catch(line 34) will forward this to the error handler, but the resulting 500 with aSyntaxErrormessage may confuse consumers. A targeted catch here could return a clearer "invalid session" error orundefined.
50-50: Redundant HSTS header — same as other controllers.src/main/controllers/dataset_exists.ts (2)
11-18:Promise.allSettledwith a single promise is unnecessary complexity.Wrapping a single promise in
Promise.allSettled([…])and then indexing[0]adds indirection for no benefit. A plaintry/catchonawait fetchDataset(…)is simpler and more readable. The same applies tofetchDraftDataseton line 16.Simplified approach
try { - const liveDataset = await Promise.allSettled([fetchDataset({ datasetId })]) - if (liveDataset[0].status === 'fulfilled') { - return response.status(200).json(liveDataset[0].value) - } - - const draftDataset = await Promise.allSettled([fetchDraftDataset({ datasetId })]) - if (draftDataset[0].status === 'fulfilled') { - return response.status(200).json(draftDataset[0].value) - } - - if (draftDataset[0].status === 'rejected') { - const errorData = _.get(draftDataset[0], ['reason', 'response', 'data']); - return response.status(_.get(draftDataset[0], ['reason', 'response', 'status'], 500)).json(errorData) - } + try { + const liveResult = await fetchDataset({ datasetId }); + return response.status(200).json(liveResult); + } catch (_) { /* live not found, try draft */ } + + try { + const draftResult = await fetchDraftDataset({ datasetId }); + return response.status(200).json(draftResult); + } catch (draftError: any) { + const statusCode = _.get(draftError, ['response', 'status'], 500); + const errorData = _.get(draftError, ['response', 'data']); + return response.status(statusCode).json(errorData); + } } catch (error) { next(error); }
9-9: Redundant HSTS header — same as other controllers.src/main/controllers/test_connection.ts (1)
33-40: Pre-existing: Kafka admin client is never disconnected.
kafka.admin()opens a connection butadmin.disconnect()is never called afterlistTopics(). This leaks connections on every test request. Not introduced by this PR, but worth noting since you're already improving this handler.Suggested fix
- getTopics(bootstrap: any) { + async getTopics(bootstrap: any) { const kafka = new Kafka({ clientId: 'test-kafka-connection', brokers: bootstrap.split(","), }); const admin = kafka.admin(); - return admin.listTopics(); + try { + await admin.connect(); + return await admin.listTopics(); + } finally { + await admin.disconnect(); + } }
| app.use(helmet({ | ||
| hsts: { | ||
| maxAge: 31536000, // 1 year in seconds | ||
| includeSubDomains: true, | ||
| preload: true | ||
| } | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'helmet' --type=ts -C3Repository: Sanketika-Obsrv/obsrv-web-console
Length of output: 1078
🏁 Script executed:
head -30 src/index.tsRepository: Sanketika-Obsrv/obsrv-web-console
Length of output: 959
🏁 Script executed:
head -30 src/app.tsRepository: Sanketika-Obsrv/obsrv-web-console
Length of output: 1022
Duplicate Helmet registration — app imported from ./app already applies Helmet middleware.
app is imported from ./app (line 8), which already calls app.use(helmet({ hsts: … })) in app.ts line 14. Applying Helmet again here in index.ts creates a duplicate middleware registration on the same app instance, causing Helmet header handlers to run twice per request. Remove the Helmet call from either app.ts or index.ts.
🧰 Tools
🪛 ESLint
[error] 21-21: Insert ⏎····
(prettier/prettier)
[error] 22-22: Insert ······
(prettier/prettier)
[error] 23-23: Insert ········
(prettier/prettier)
[error] 24-24: Insert ········
(prettier/prettier)
[error] 25-25: Replace ····preload:·true with ············preload:·true,
(prettier/prettier)
[error] 26-26: Replace ··} with ········},
(prettier/prettier)
[error] 27-27: Replace }) with ····}),⏎
(prettier/prettier)
🤖 Prompt for AI Agents
In `@src/index.ts` around lines 21 - 27, The Helmet middleware is being registered
twice on the same Express app instance — remove the duplicate registration by
deleting the app.use(helmet({ hsts: { maxAge: 31536000, includeSubDomains: true,
preload: true } })) call from index.ts (since app is imported from ./app which
already applies Helmet), or alternatively remove the Helmet setup in the app
module so only one module calls helmet; ensure only one app.use(helmet(...))
remains referencing the app variable to avoid double header handling.
| if (draftDataset[0].status === 'rejected') { | ||
| const errorData = _.get(draftDataset[0], ['reason', 'response', 'data']); | ||
| return response.status(_.get(draftDataset[0], ['reason', 'status'])).json(errorData) |
There was a problem hiding this comment.
Bug: reason.status is not the HTTP status code — use reason.response.status.
For Axios errors, the HTTP status code lives at error.response.status, not error.status. The error data on line 22 is correctly extracted from reason.response.data, but the status on line 23 reads from reason.status (which is typically undefined). When response.status(undefined) is called, Express coerces it to 200, so the client receives error data with a 200 OK — silently masking the failure.
Proposed fix
if (draftDataset[0].status === 'rejected') {
const errorData = _.get(draftDataset[0], ['reason', 'response', 'data']);
- return response.status(_.get(draftDataset[0], ['reason', 'status'])).json(errorData)
+ const statusCode = _.get(draftDataset[0], ['reason', 'response', 'status'], 500);
+ return response.status(statusCode).json(errorData)
}📝 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.
| if (draftDataset[0].status === 'rejected') { | |
| const errorData = _.get(draftDataset[0], ['reason', 'response', 'data']); | |
| return response.status(_.get(draftDataset[0], ['reason', 'status'])).json(errorData) | |
| if (draftDataset[0].status === 'rejected') { | |
| const errorData = _.get(draftDataset[0], ['reason', 'response', 'data']); | |
| const statusCode = _.get(draftDataset[0], ['reason', 'response', 'status'], 500); | |
| return response.status(statusCode).json(errorData); | |
| } |
🧰 Tools
🪛 ESLint
[error] 23-23: Insert ;
(prettier/prettier)
🤖 Prompt for AI Agents
In `@src/main/controllers/dataset_exists.ts` around lines 21 - 23, The code in
dataset_exists that sets the response status uses reason.status (which is
undefined for Axios errors) causing Express to send 200; change the status
extraction to use reason.response.status (e.g., _.get(draftDataset[0],
['reason','response','status'])) and provide a numeric fallback (like 500) to
ensure a valid HTTP status; update the response.status(...) call accordingly
where draftDataset[0].status === 'rejected' and errorData is computed.
| if (!topicExists) throw new Error("Topic does not exist"); | ||
| const result = { connectionEstablished: true, topicExists: topicExists }; | ||
| response.setHeader('Content-Type', 'application/json'); | ||
| response.status(200).send(result); | ||
| } catch (error: any) { | ||
| console.log(error?.message); | ||
| next("Failed to establish connection to the client") | ||
| response.setHeader('Content-Type', 'application/json'); | ||
| response.status(500).send({ error: "Failed to establish connection to the client" }); |
There was a problem hiding this comment.
Topic-not-found is misreported as a connection failure (500).
Line 20 throws when the topic doesn't exist, but the catch block on line 27 returns a generic 500 "Failed to establish connection". A missing topic on a reachable broker is not a connection failure — it's a valid result. Previously the handler could distinguish these cases; now all errors are flattened into the same 500.
Consider returning the topic-not-found case as a successful connection with topicExists: false (which is arguably the point of a "test connection" endpoint), or at least return a distinct status/message.
Proposed fix: return topicExists: false instead of throwing
- if (!topicExists) throw new Error("Topic does not exist");
- const result = { connectionEstablished: true, topicExists: topicExists };
+ const result = { connectionEstablished: true, topicExists };📝 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.
| if (!topicExists) throw new Error("Topic does not exist"); | |
| const result = { connectionEstablished: true, topicExists: topicExists }; | |
| response.setHeader('Content-Type', 'application/json'); | |
| response.status(200).send(result); | |
| } catch (error: any) { | |
| console.log(error?.message); | |
| next("Failed to establish connection to the client") | |
| response.setHeader('Content-Type', 'application/json'); | |
| response.status(500).send({ error: "Failed to establish connection to the client" }); | |
| const result = { connectionEstablished: true, topicExists }; | |
| response.setHeader('Content-Type', 'application/json'); | |
| response.status(200).send(result); | |
| } catch (error: any) { | |
| console.log(error?.message); | |
| response.setHeader('Content-Type', 'application/json'); | |
| response.status(500).send({ error: "Failed to establish connection to the client" }); |
🧰 Tools
🪛 ESLint
[error] 20-20: Replace "Topic·does·not·exist" with 'Topic·does·not·exist'
(prettier/prettier)
[error] 25-25: Unexpected console statement.
(no-console)
[error] 27-27: Replace "Failed·to·establish·connection·to·the·client" with 'Failed·to·establish·connection·to·the·client'
(prettier/prettier)
🤖 Prompt for AI Agents
In `@src/main/controllers/test_connection.ts` around lines 20 - 27, The handler
currently throws "Topic does not exist" which gets caught and turned into a 500;
change the flow so that when topicExists is false you do not throw but instead
return a 200 with { connectionEstablished: true, topicExists: false } (i.e.,
remove the throw new Error("Topic does not exist") and let the existing result
send path handle it), or alternately detect that specific error in the catch
(error?.message === "Topic does not exist") and respond with 200 and
topicExists: false; update the logic around topicExists, result, and the catch
block to ensure only real connection failures produce the 500 error response.
| let { id, ver = 'v1', ets = Date.now(), params = {}, responseCode = 'OK', result = {} } = payload; | ||
|
|
||
| const { resmsgid = `${uuidv4()}`, err = '', status = responseCode === 'OK' ? 'SUCCESSFUL' : 'FAILED', errmsg = '' } = params; | ||
| // Sanitize ID to prevent Reflected XSS | ||
| if (typeof id === 'string') { | ||
| id = he.encode(id); | ||
| } | ||
|
|
||
| let { resmsgid = `${uuidv4()}`, err = '', status = responseCode === 'OK' ? 'SUCCESSFUL' : 'FAILED', errmsg = '' } = params; | ||
|
|
||
| // Sanitize error messages to prevent Reflected XSS | ||
| if (typeof err === 'string') { | ||
| err = he.encode(err); | ||
| } | ||
| if (typeof errmsg === 'string') { | ||
| errmsg = he.encode(errmsg); | ||
| } |
There was a problem hiding this comment.
HTML-encoding JSON response values may cause double-encoding on the client side.
he.encode produces HTML entities (e.g., &, '). Since these values are served in application/json responses, browsers won't interpret them as HTML. If the consuming client also applies HTML escaping (as most modern frameworks like React do by default), users will see garbled text like &.
Consider whether the actual XSS vector is on the client side (DOM insertion via innerHTML) — if so, the fix belongs there. If you want server-side defense-in-depth, ensure the client doesn't double-encode, or use a simpler approach like stripping angle brackets and quotes instead of full HTML entity encoding.
Also note that result is not sanitized but could contain user-controlled data.
🧰 Tools
🪛 ESLint
[error] 6-6: Insert ··
(prettier/prettier)
[error] 6-6: 'ver' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 6-6: 'ets' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 6-6: 'params' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 6-6: 'responseCode' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 6-6: 'result' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 8-8: Insert ··
(prettier/prettier)
[error] 9-9: Insert ··
(prettier/prettier)
[error] 10-10: Replace ···· with ········
(prettier/prettier)
[error] 11-11: Insert ··
(prettier/prettier)
[error] 13-13: Insert ··
(prettier/prettier)
[error] 13-13: 'resmsgid' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 13-13: 'status' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 15-15: Replace ·· with ····
(prettier/prettier)
[error] 16-16: Insert ··
(prettier/prettier)
[error] 17-17: Replace ···· with ········
(prettier/prettier)
[error] 18-18: Replace ·· with ····
(prettier/prettier)
[error] 19-19: Insert ··
(prettier/prettier)
[error] 20-20: Insert ····
(prettier/prettier)
[error] 21-21: Insert ··
(prettier/prettier)
🤖 Prompt for AI Agents
In `@src/shared/utils/transformResponse.ts` around lines 6 - 21, The current
transformResponse uses he.encode (he.encode(id), he.encode(err),
he.encode(errmsg)) which HTML-entity-encodes JSON values and causes
double-encoding on clients; remove the direct calls to he.encode and instead
implement and call a lightweight server-side sanitizer (e.g., sanitizeString)
that strips or escapes only dangerous characters like angle brackets and control
characters (not full HTML entities), and apply it to id, err, errmsg and
recursively to any string values inside result (handle objects/arrays), keeping
response JSON raw for clients to handle safe DOM insertion; update the code
paths around variables id, err, errmsg, and result to use sanitizeString instead
of he.encode.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/controllers/user_update.ts (1)
30-30:⚠️ Potential issue | 🟠 MajorUnused database query result.
useris fetched from the database but never referenced. This is a wasted I/O call. If the intent was to verify the user exists before updating, the result should be checked (and a 404 returned if not found). Otherwise, remove the call.Proposed fix (if existence check is intended)
const user = await userService.find({ user_name }); + if (!user) { + return res.status(404).json( + transform({ + responseCode: 'NOT_FOUND', + params: { + err: 'NOT_FOUND', + errmsg: 'User not found', + }, + }), + ); + }
🤖 Fix all issues with AI agents
In `@src/main/controllers/dataset_aggregator.ts`:
- Around line 226-227: Remove the trailing blank whitespace and fix the
semicolon and quote style on the datasetId reassignment inside the dataset
aggregation logic: change the line "datasetId = status === "Live" ?
_.split(datasetId, '.', 1)[0] : datasetId" to use single quotes for 'Live' and
'.' and terminate the statement with a semicolon; ensure the preceding blank
line has no trailing spaces. This affects the reassignment expression for
datasetId (the _.split usage) in the dataset aggregation function in
dataset_aggregator.ts.
In `@src/main/controllers/test_connection.ts`:
- Around line 10-11: The current use of _.get for topic and kafkaBrokers can
return null (not undefined), causing .toString() to throw; update the assignment
of topic and kafkaBrokers to safely coerce null/undefined to an empty string
before calling toString()/trim() (e.g., use nullish coalescing or explicit
String() on _.get(request.body, 'topic', undefined) and _.get(request.body,
'kafkaBrokers', undefined) so that topic and kafkaBrokers are always a string),
and keep the rest of the validation/error path in the same handler (ensure
references to topic and kafkaBrokers use the new safe values).
🧹 Nitpick comments (5)
src/main/controllers/user_update.ts (1)
14-14: Redundant manual HSTS header — Helmet already handles this globally.Helmet is configured with
hstsin bothsrc/app.tsandsrc/index.ts, which already sets this header on every response. The manualsetHeadercall here is unnecessary and duplicates that work. This applies to all controllers in this PR.src/index.ts (1)
35-38: Redundant manual HSTS header in catch-all route.With Helmet configured globally, this manual
setHeaderis unnecessary. If you remove the duplicate Helmet call above and keep the one inapp.ts, this header is already set by middleware before this handler runs.src/app.ts (1)
61-64: Redundant manual HSTS header — Helmet already covers this route.The Helmet middleware on Line 14 already sets HSTS on every response, including this
/consoleroute. The manualsetHeaderon Line 62 is unnecessary.Proposed fix
app.get('/console', authProvider.authenticate(), (req, res) => { - res.setHeader('Strict-Transport-Security', 'max-age=31536000; includeSubDomains; preload'); res.sendFile(path.join(__dirname, 'build', 'index.html')); });src/main/controllers/test_connection.ts (2)
25-25:console.logviolates the project'sno-consoleESLint rule.Replace with the project's logger or remove it.
Proposed fix (if a logger is available)
- console.log(error?.message); + // Use the project's structured logger, e.g.: + // logger.error({ err: error }, 'Test connection failed');
32-40: Resource leak:admin.disconnect()is never called afterlistTopics().Every invocation of this handler creates a Kafka admin connection that is never closed. Under load this will exhaust connections or file descriptors. While this is pre-existing code, the handler refactor is a good opportunity to fix it.
Proposed fix
const service = { - getTopics(bootstrap: any) { + async getTopics(bootstrap: string) { const kafka = new Kafka({ clientId: 'test-kafka-connection', brokers: bootstrap.split(","), }); const admin = kafka.admin(); - return admin.listTopics(); + try { + await admin.connect(); + return await admin.listTopics(); + } finally { + await admin.disconnect(); + } } };
|
|
||
| datasetId = status === "Live" ? _.split(datasetId, '.', 1)[0] : datasetId |
There was a problem hiding this comment.
Fix lint errors: trailing whitespace and missing semicolon.
ESLint reports a blank line with whitespace on Line 226 and a missing semicolon on Line 227. Also, double quotes should be single quotes per prettier config.
Proposed fix
const status = _.get(request, 'query.status');
-
- datasetId = status === "Live" ? _.split(datasetId, '.', 1)[0] : datasetId
+ datasetId = status === 'Live' ? _.split(datasetId, '.', 1)[0] : datasetId;📝 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.
| datasetId = status === "Live" ? _.split(datasetId, '.', 1)[0] : datasetId | |
| datasetId = status === 'Live' ? _.split(datasetId, '.', 1)[0] : datasetId; |
🧰 Tools
🪛 ESLint
[error] 226-226: Delete ········
(prettier/prettier)
[error] 227-227: Replace "Live"·?·_.split(datasetId,·'.',·1)[0]·:·datasetId with 'Live'·?·_.split(datasetId,·'.',·1)[0]·:·datasetId;
(prettier/prettier)
🤖 Prompt for AI Agents
In `@src/main/controllers/dataset_aggregator.ts` around lines 226 - 227, Remove
the trailing blank whitespace and fix the semicolon and quote style on the
datasetId reassignment inside the dataset aggregation logic: change the line
"datasetId = status === "Live" ? _.split(datasetId, '.', 1)[0] : datasetId" to
use single quotes for 'Live' and '.' and terminate the statement with a
semicolon; ensure the preceding blank line has no trailing spaces. This affects
the reassignment expression for datasetId (the _.split usage) in the dataset
aggregation function in dataset_aggregator.ts.
| const topic = _.get(request.body, 'topic', "").toString().trim(); | ||
| const kafkaBrokers = _.get(request.body, 'kafkaBrokers', "").toString().trim(); |
There was a problem hiding this comment.
_.get returns null (not the default) when the property exists but is null, causing .toString() to throw.
_.get only falls back to the default value when the resolved value is undefined. A request body like { "topic": null } will bypass the default, and null.toString() throws a TypeError caught by line 24, returning a misleading 500 instead of 400.
Use nullish coalescing or an explicit fallback:
Proposed fix
- const topic = _.get(request.body, 'topic', "").toString().trim();
- const kafkaBrokers = _.get(request.body, 'kafkaBrokers', "").toString().trim();
+ const topic = String(_.get(request.body, 'topic') ?? '').trim();
+ const kafkaBrokers = String(_.get(request.body, 'kafkaBrokers') ?? '').trim();📝 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 topic = _.get(request.body, 'topic', "").toString().trim(); | |
| const kafkaBrokers = _.get(request.body, 'kafkaBrokers', "").toString().trim(); | |
| const topic = String(_.get(request.body, 'topic') ?? '').trim(); | |
| const kafkaBrokers = String(_.get(request.body, 'kafkaBrokers') ?? '').trim(); |
🧰 Tools
🪛 ESLint
[error] 10-10: Replace "" with ''
(prettier/prettier)
[error] 11-11: Replace "" with ''
(prettier/prettier)
🤖 Prompt for AI Agents
In `@src/main/controllers/test_connection.ts` around lines 10 - 11, The current
use of _.get for topic and kafkaBrokers can return null (not undefined), causing
.toString() to throw; update the assignment of topic and kafkaBrokers to safely
coerce null/undefined to an empty string before calling toString()/trim() (e.g.,
use nullish coalescing or explicit String() on _.get(request.body, 'topic',
undefined) and _.get(request.body, 'kafkaBrokers', undefined) so that topic and
kafkaBrokers are always a string), and keep the rest of the validation/error
path in the same handler (ensure references to topic and kafkaBrokers use the
new safe values).
1 participant
This pull request adds comprehensive HTTP security headers, specifically the
Strict-Transport-Security(HSTS) header, across the application to enforce HTTPS and improve security. The header is now set globally via middleware and explicitly in all major API endpoints and controller responses. Additionally, there are minor improvements in request parameter handling and error responses in several controllers.Security enhancements:
helmetmiddleware with HSTS configuration insrc/app.tsandsrc/index.tsto enforce HTTPS for one year, including subdomains and preload. [1] [2]Strict-Transport-Securityheader in all controller responses and error handlers, ensuring consistent security across API endpoints. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22]Controller improvements:
_.getfor safer access to request parameters and session tokens. [1] [2] [3] [4] [5]test_connection.tsto return JSON responses with proper status codes and error messages.User management improvements:
Dataset and metrics endpoints:
Authentication and authorization:
These changes collectively ensure that all responses from the application are secured with HSTS, reducing the risk of protocol downgrade attacks and cookie hijacking.
Summary by CodeRabbit
Security
Bug Fixes