fix: replace koa-helmet #379
Conversation
WalkthroughMigrated from koa-helmet to native helmet package with a custom Koa adapter wrapper. Created koaHelmet factory function to adapt Express-style helmet middleware for Koa integration, updated module exports, and adjusted tests throughout. Removed explicit return statement in health router. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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: 0
🧹 Nitpick comments (1)
src/middleware/helmet.ts (1)
1-19: Helmet Express→Koa adapter looks correct; suggest naming + clearer control flowThe adapter correctly:
- Instantiates
helmet(options)once.- Invokes it with
ctx.req/ctx.res.- Propagates errors via Promise rejection and only calls
nextafter helmet completes (via Promise adoption).Two small improvements would make this more explicit and fix the Sonar warning about an unnamed function:
- Name the default-exported function (e.g.
koaHelmet) for clearer stack traces and to satisfy static analysis.- Separate the Express call from Koa’s
next()instead of relying onresolve(next()), which works but hides the sequencing in Promise internals.Suggested refactor:
-import helmet, { type HelmetOptions } from 'helmet' -import type { Middleware } from 'koa' +import helmet, { type HelmetOptions } from 'helmet' +import type { Middleware } from 'koa' @@ -export default function (options?: HelmetOptions): Middleware { - const expressHelmet = helmet(options) - return async (ctx, next) => { - return new Promise<void>((resolve, reject) => { - expressHelmet(ctx.req, ctx.res, (err) => { - if (err) return reject(err) - resolve(next()) - }) - }) - } -} +export default function koaHelmet(options?: HelmetOptions): Middleware { + const expressHelmet = helmet(options) + return async (ctx, next) => { + await new Promise<void>((resolve, reject) => { + expressHelmet(ctx.req, ctx.res, (err) => { + if (err) return reject(err) + resolve() + }) + }) + await next() + } +}This keeps behavior the same, makes the flow easier to read, and addresses the SonarCloud “function should be named” warning. As per static analysis hints.
Please re-run the SonarCloud check and your tests after this change to confirm the warning disappears and headers/error handling remain unchanged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(1 hunks)src/index.ts(1 hunks)src/middleware/helmet.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
src/middleware/helmet.ts
[warning] 9-9: The function should be named.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Release
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Unit test
- GitHub Check: Scorecard analysis
- GitHub Check: Lint Dockerfile
- GitHub Check: Build and publish
- GitHub Check: Integration test
🔇 Additional comments (2)
src/index.ts (1)
17-17: Usinghelmet()as a Koa middleware factory looks correctGiven the new wrapper in
src/middleware/helmet.tsreturns a KoaMiddlewarewhen invoked, switching from.use(helmet)to.use(helmet())is the right call pattern and keeps the middleware chain intact.Please run your existing integration/e2e tests (especially any that assert response headers) to confirm the new
helmetsetup produces the expected security headers across routes.package.json (1)
24-31: The dependency in question does not exist in the currentpackage.json. Thedependenciessection contains only legitimate runtime packages (@koa/router,dotenv,helmet,koa,koa-compress,pg,postgraphile, and others), with no"test"entry present. The codebase correctly uses Vitest for testing (found inoptionalDependencies), and no usage of a generictestpackage was found.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.test.ts (1)
30-33: Mocking and expectations forhelmetalign with the new factory contract, but can be tightenedThe updated mock and assertions correctly reflect
helmetas a factory whose result is passed tokoa.use, so behavior-wise this looks good.One minor improvement: in
expect.soft(helmet).toHaveBeenCalledWith() expect.soft(mockKoaInstance.use).toHaveBeenCalledWith(helmet())the second line calls
helmet()again inside the assertion. This:
- Increments the mock’s call count (which you don’t currently assert on), and
- Relies on
mockReturnValuereturning the same instance every time.You can make the test more explicit and stable by capturing the first returned middleware and asserting against that, without additional calls:
await import('./index.js') @@ - expect.soft(helmet).toHaveBeenCalledWith() - expect.soft(mockKoaInstance.use).toHaveBeenCalledWith(helmet()) + expect.soft(helmet).toHaveBeenCalledTimes(1) + expect.soft(helmet).toHaveBeenCalledWith() + + const helmetMiddleware = vi.mocked(helmet).mock.results[0]?.value + expect.soft(mockKoaInstance.use).toHaveBeenCalledWith(helmetMiddleware)This keeps the intent clear (“koa.use gets exactly what helmet() returned”) without side effects in the assertion itself.
Also applies to: 49-49, 59-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.test.ts(3 hunks)src/middleware/helmet.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build
src/middleware/helmet.test.ts
[error] 12-12: AssertionError: expected "vi.fn()" to be called once, but got 0 times
- Updated test descriptions for clarity. - Added tests to verify middleware creation and option handling. - Included error handling test for express helmet middleware.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #379 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 5 +1
Lines 23 29 +6
Branches 4 5 +1
=========================================
+ Hits 23 29 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Added beforeEach hook to clear mocks before each test. - Updated test descriptions for clarity. - Enhanced tests to verify middleware creation and option handling.
- Updated the import statement to rename `helmetMiddleware` to `helmetFactory`. - Adjusted the middleware instantiation accordingly for clarity and consistency.
- Updated the middleware import and instantiation from `helmet` to `koaHelmet` across the application. - Adjusted related tests to reflect the new naming convention for consistency and clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/index.test.ts(4 hunks)src/index.ts(2 hunks)src/middleware/helmet.test.ts(1 hunks)src/middleware/helmet.ts(1 hunks)src/middleware/index.test.ts(3 hunks)src/middleware/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- src/middleware/helmet.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/middleware/helmet.test.ts (1)
src/middleware/helmet.ts (1)
koaHelmet(9-20)
src/index.test.ts (1)
src/middleware/helmet.ts (1)
koaHelmet(9-20)
🪛 GitHub Actions: Build
src/middleware/index.test.ts
[error] 31-37: Formatter would have printed the following content; formatting mismatch detected during linting. Command: 'pnpm run lint'. The Biome/formatter reported a diff and exit code 1.
🔇 Additional comments (3)
src/middleware/index.ts (1)
3-3: LGTM! Wildcard export aligns with the new named export pattern.The change from exporting a default alias to a wildcard re-export correctly exposes the new
koaHelmetnamed export. This is a breaking API change (consumers must now importkoaHelmetinstead ofhelmet), but it aligns with the PR's objective to replacekoa-helmetwith the standardhelmetlibrary via a Koa-compatible wrapper.src/middleware/index.test.ts (1)
4-4: LGTM! Test correctly updated for the new named export.The import and mock have been properly updated from
helmettokoaHelmet, and the test expectations now correctly reference the new API surface.Also applies to: 14-14, 29-29
src/middleware/helmet.test.ts (1)
1-96: Excellent test coverage for the Koa adapter!The test suite comprehensively validates the
koaHelmetfactory pattern:
- Factory invocation and middleware return type
- Options forwarding to the underlying
helmetlibrary- Middleware execution flow (Express callback → Koa next)
- Error propagation (reject promise, skip next)
The dynamic import pattern after mocking and the
beforeEachcleanup ensure proper test isolation.
- Refactored test assertions in `index.test.ts` for better readability. - Adjusted the way keys are compared in the test to enhance clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/middleware/index.test.ts (1)
4-4: koaHelmet migration in test looks consistent; consider aligning mock specifierThe switch from default
helmetto namedkoaHelmetis wired through correctly here: the import, mock shape, andexpectedexport map all referencekoaHelmetconsistently, which matches the new middleware API.One small readability/robustness nit: the import uses
'./helmet.js'while the mock targets'./helmet'. Depending on your Vitest/TS module resolution, this might or might not refer to the same module. For clarity (and to avoid subtle mismatches later), consider updating the mock to also use'./helmet.js'so the specifiers are identical.Also applies to: 14-15, 29-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/middleware/index.test.ts(3 hunks)src/middleware/postgraphile.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build
src/middleware/postgraphile.ts
[error] 15-15: Type 'typeof import("/home/runner/work/bss-web-graphql-backend/bss-web-graphql-backend/node_modules/.pnpm/@graphile-contrib+pg-simplify-inflector@6.1.0/node_modules/@graphile-contrib/pg-simplify-inflector/index")' is not assignable to type 'Plugin'. TS2322. Build failed during 'pnpm run build' (tsc).
🪛 GitHub Check: Build
src/middleware/postgraphile.ts
[failure] 15-15:
Type 'typeof import("/home/runner/work/bss-web-graphql-backend/bss-web-graphql-backend/node_modules/.pnpm/@graphile-contrib+pg-simplify-inflector@6.1.0/node_modules/@graphile-contrib/pg-simplify-inflector/index")' is not assignable to type 'Plugin'.
🔇 Additional comments (2)
src/middleware/index.test.ts (1)
33-35: Additional key-order-agnostic assertion is soundThe second
expect.softthat compares sortedObject.keys(actual)andObject.keys(expected)is a good complement totoMatchObject, and the total of two expectations matchesexpect.assertions(2). No further changes needed here.src/middleware/postgraphile.ts (1)
15-15: Correct the import to remove.default— the plugin is already unwrapped by the default import.When using ESM
import ... fromsyntax with a default export, the exported value is automatically unwrapped. Accessing.defaultagain returns the module namespace rather than the plugin itself, causing the TypeScript error.The current code is incorrect:
import PgSimplifyInflectorPlugin from '@graphile-contrib/pg-simplify-inflector' appendPlugins: [PgSimplifyInflectorPlugin.default] // Wrong: accesses .default on unwrapped importThe correct usage is:
import PgSimplifyInflectorPlugin from '@graphile-contrib/pg-simplify-inflector' appendPlugins: [PgSimplifyInflectorPlugin] // Correct: plugin is already the default exportRemove
.defaultfrom line 15.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Replaced the Koa-specific helmet package in the project manifest with the standard helmet dependency. * **Refactor** * Helmet middleware now must be instantiated (called) with optional configuration and is re-exposed via the middleware barrel. * **Tests** * Updated tests and mocks to reflect the middleware factory pattern and adjusted assertions/counts accordingly. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
|
🎉 This PR is included in version 2.0.17 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.