feat: support cjs and esm both by tshy #101
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request represents a comprehensive modernization and refactoring of the Egg.js security plugin, transitioning from a CommonJS-based implementation to a TypeScript-first, ES module approach. The changes include renaming the package from Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Securities as Security Middleware
participant Context as Request Context
participant Response as Response
App->>Securities: Configure security middleware
Securities->>Context: Apply security checks
Context->>Context: Validate request
Context->>Response: Set security headers
Response-->>Context: Return secured response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
src/config/config.default.ts (1)
Line range hint
80-85: Add missing 'reportOnly' and 'supportIE' properties tocspconfigurationThe
cspconfiguration is missing the required propertiesreportOnlyandsupportIEas specified in theSecurityConfiginterface. This omission is causing a TypeScript type error in the pipeline.Apply this diff to fix the issue:
csp: { enable: false, policy: {}, + reportOnly: false, + supportIE: false, },src/app/extend/context.ts (1)
Line range hint
224-224: Fix TypeScript type error: Handle possible null in 'getFromUrl' resultAt line 224,
utils.getFromUrl(referer, 'host')may returnnull, leading to a type error whenhostis expected to be a string in subsequent usage.Apply this diff to handle the possible
nullvalue:- const host = utils.getFromUrl(referer, 'host'); + const host = utils.getFromUrl(referer, 'host') ?? '';This ensures
hostis always a string before it's used inutils.isSafeDomain.
🧹 Nitpick comments (16)
src/app.ts (2)
16-16: Simplify null checks using optional chainingThe condition
if (app.config.security.csrf && app.config.security.csrf.enable)can be simplified using optional chaining for better readability.Apply this diff to simplify the condition:
- if (app.config.security.csrf && app.config.security.csrf.enable) { + if (app.config.security.csrf?.enable) {🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
24-24: Correct typo in error message module nameIn the error message,
[@eggjs/security/ap]should be[@eggjs/security/app]to match the correct module name.Apply this diff to fix the typo:
- '[@eggjs/security/ap] `config.security.csrf.type` must be one of ' + legalTypes.join(', ')); + '[@eggjs/security/app] `config.security.csrf.type` must be one of ' + legalTypes.join(', '));src/lib/extend/safe_curl.ts (2)
20-20: Remove unnecessarythisaliasingAssigning
thistoappis unnecessary. You can usethisdirectly within the function for clarity.Apply this diff to remove the alias:
- // eslint-disable-next-line @typescript-eslint/no-this-alias - const app = this;Update references to
appwithin the function tothis.🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
22-38: Simplify redundant checks forssrfConfig.checkAddressThe function checks
ssrfConfig?.checkAddresstwice, leading to redundant code. Refactor the function to reduce redundancy and improve readability.Apply this diff to simplify the logic:
- if (ssrfConfig?.checkAddress) { - options.checkAddress = ssrfConfig.checkAddress; - } else { - app.logger.warn('[@eggjs/security] please configure `config.security.ssrf` first'); - } - - if (ssrfConfig?.checkAddress) { - let httpClient = app[SSRF_HTTPCLIENT] as ReturnType<EggCore['createHttpClient']>; - // use the new httpClient init with checkAddress - if (!httpClient) { - httpClient = app[SSRF_HTTPCLIENT] = app.createHttpClient({ - checkAddress: ssrfConfig.checkAddress, - }); - } - return await httpClient.request(url, options); - } - - return await app.curl(url, options); + if (ssrfConfig?.checkAddress) { + options.checkAddress = ssrfConfig.checkAddress; + let httpClient = this[SSRF_HTTPCLIENT] as ReturnType<EggCore['createHttpClient']>; + // Use the new httpClient initialized with checkAddress + if (!httpClient) { + httpClient = this[SSRF_HTTPCLIENT] = this.createHttpClient({ + checkAddress: ssrfConfig.checkAddress, + }); + } + return await httpClient.request(url, options); + } else { + this.logger.warn('[@eggjs/security] please configure `config.security.ssrf` first'); + return await this.curl(url, options); + }Also, ensure to update
appreferences tothisthroughout the function after removing the unnecessary aliasing.src/config/config.default.ts (1)
Line range hint
67-67: Verify the usage ofundefinedfor SSRF configuration propertiesSetting
ssrfconfiguration properties toundefinedmay not be necessary, as these properties are optional in theSecurityConfiginterface. You can omit them entirely if they are not used.Consider simplifying the
ssrfconfiguration:ssrf: { - ipBlackList: undefined, - ipExceptionList: undefined, - hostnameExceptionList: undefined, - checkAddress: undefined, + // No SSRF configuration provided },src/app/extend/response.ts (1)
Line range hint
38-80: Improve URL validation and error handling inredirectmethodThe current implementation of the
redirectmethod gracefully handles invalid URLs by redirecting to'/'. However, consider logging more detailed error information in development environments to aid debugging.You could enhance the error handling as follows:
} catch { url = '/'; + if (process.env.NODE_ENV !== 'production') { + this.app.coreLogger.warn('[@eggjs/security/response/redirect] Invalid URL provided for redirection.'); + } this.unsafeRedirect(url); return; }🧰 Tools
🪛 GitHub Actions: Publish Any Commit
[error] 67-67: Type error: Object is of type 'unknown'.
src/lib/utils.ts (4)
44-44: Typo in log message: 'dta' should be 'data'At line 44, the log message contains a typo where 'dta' should be 'data'. Correcting this will improve clarity in logging.
Apply this diff to correct the typo:
- ctx.coreLogger.warn('[@eggjs/security: dta global block] : decode file path %j failed.', path); + ctx.coreLogger.warn('[@eggjs/security: data global block] : decode file path %j failed.', path);
119-119: Suggestion: Simplify condition with optional chainingThe condition at line 119 can be simplified using optional chaining for better readability and modern syntax.
Apply this diff to refactor the condition:
- if (ssrf && ssrf.ipBlackList && !ssrf.checkAddress) { + if (ssrf?.ipBlackList && !ssrf.checkAddress) {🧰 Tools
🪛 Biome (1.9.4)
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
175-175: Suggestion: Use optional chaining to simplify conditionAt line 175, you can use optional chaining to make the condition more concise.
Apply this diff to refactor the condition:
- if (config.csrf && config.csrf.refererWhiteList) { + if (config.csrf?.refererWhiteList) {🧰 Tools
🪛 Biome (1.9.4)
[error] 175-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
143-146: Offer assistance to implement IPv6 supportThe code currently skips IPv6 addresses with a
FIXMEnote at lines 143-146. Implementing IPv6 support would enhance the functionality.Do you want me to provide a solution to add IPv6 support for the
ipAddresshandling in this function?src/app/extend/agent.ts (1)
2-2: Update the import path extension.The import statement includes a
.jsextension which might cause issues with TypeScript module resolution. Consider removing the.jsextension to maintain consistency with TypeScript import conventions.-import { safeCurlForApplication } from '../../lib/extend/safe_curl.js'; +import { safeCurlForApplication } from '../../lib/extend/safe_curl';src/agent.ts (1)
2-2: Update the import path extension.The import statement includes a
.jsextension which might cause issues with TypeScript module resolution. Consider removing the.jsextension to maintain consistency with TypeScript import conventions.-import { preprocessConfig } from './lib/utils.js'; +import { preprocessConfig } from './lib/utils';src/lib/helper/surl.js (2)
23-25: Use template literals for string concatenation.Replace string concatenation with template literals for better readability.
- this.ctx.coreLogger.warn('[@eggjs/security/surl] url: %j, protocol: %j, ' + - 'protocol is empty or not in white list, convert to empty string', val, protocol); + this.ctx.coreLogger.warn( + '[@eggjs/security/surl] url: %j, protocol: %j, protocol is empty or not in white list, convert to empty string', + val, + protocol + );
30-32: Simplify arrow function.The arrow function can be simplified by removing the explicit return.
- return val.replace(/["'<>]/g, ch => { - return escapeMap[ch]; - }); + return val.replace(/["'<>]/g, ch => escapeMap[ch]);src/lib/middlewares/csp.ts (1)
Line range hint
34-63: Extract CSP value generation logic into separate functions.The CSP header generation logic is complex and could benefit from being split into smaller, more focused functions for better maintainability.
Example refactor:
function generateSandboxValue(key: string, value: any[]): string | undefined { if (key === 'sandbox' && value[0] === true) { return key; } return undefined; } function generateScriptSrcValue(value: string[], ctx: Context): string[] { const hasNonce = value.some(val => val.includes('nonce-')); if (!hasNonce) { value.push(`'nonce-${ctx.nonce}'`); } return value; } function normalizeDirectiveValue(value: string): string { return value.startsWith('.') ? '*' + value : value; }test/lib/helper/surl.test.ts (1)
Line range hint
8-20: Merge duplicate before hooks for better maintainability.The two before hooks can be combined into a single hook to improve code organization.
- before(async () => { - app = mm.app({ - baseDir: 'apps/helper-app', - }); - await app.ready(); - }); - - before(async () => { - app2 = mm.app({ - baseDir: 'apps/helper-app-surlextend', - }); - await app2.ready(); - }); + before(async () => { + app = mm.app({ + baseDir: 'apps/helper-app', + }); + app2 = mm.app({ + baseDir: 'apps/helper-app-surlextend', + }); + await Promise.all([ + app.ready(), + app2.ready(), + ]); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.eslintignore(0 hunks).eslintrc(1 hunks).github/workflows/nodejs.yml(1 hunks).gitignore(1 hunks)agent.js(0 hunks)app.js(0 hunks)app/extend/agent.js(0 hunks)app/extend/application.js(0 hunks)config/config.local.js(0 hunks)index.js(0 hunks)lib/extend/safe_curl.js(0 hunks)package.json(1 hunks)src/agent.ts(1 hunks)src/app.ts(1 hunks)src/app/extend/agent.ts(1 hunks)src/app/extend/application.ts(1 hunks)src/app/extend/context.ts(7 hunks)src/app/extend/response.ts(4 hunks)src/config/config.default.ts(3 hunks)src/config/config.local.ts(1 hunks)src/index.ts(1 hunks)src/lib/extend/safe_curl.ts(1 hunks)src/lib/helper/surl.js(1 hunks)src/lib/middlewares/csp.ts(2 hunks)src/lib/utils.ts(5 hunks)src/types.ts(1 hunks)src/typings/index.d.ts(1 hunks)test/lib/helper/surl.test.ts(3 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (8)
- agent.js
- .eslintignore
- app/extend/agent.js
- config/config.local.js
- index.js
- lib/extend/safe_curl.js
- app.js
- app/extend/application.js
✅ Files skipped from review due to trivial changes (5)
- .eslintrc
- src/typings/index.d.ts
- tsconfig.json
- .gitignore
- src/index.ts
🧰 Additional context used
🪛 GitHub Actions: Publish Any Commit
src/config/config.default.ts
[error] 4-91: Type error: Conversion of type to 'SecurityConfig' may be incorrect. Missing required properties 'reportOnly' and 'supportIE' in csp configuration.
src/app/extend/response.ts
[error] 67-67: Type error: Object is of type 'unknown'.
src/app/extend/context.ts
[error] 224-224: Type error: Argument of type 'string | undefined' is not assignable to parameter of type 'string'. Type 'undefined' is not assignable to type 'string'.
🪛 Biome (1.9.4)
test/lib/helper/surl.test.ts
[error] 15-20: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
src/lib/extend/safe_curl.ts
[error] 20-20: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
src/app.ts
[error] 16-16: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/lib/utils.ts
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 175-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
🔇 Additional comments (14)
src/types.ts (1)
1-270: Well-structured and comprehensive security type definitionsThe security-related type definitions are thorough and will enhance type safety throughout the codebase. This contributes to better maintainability and reduces potential runtime errors.
src/config/config.local.ts (1)
1-9: Configuration change approvedThe
config.local.tsfile correctly disables HSTS for the local environment, aligning with best practices for development settings.src/app/extend/agent.ts (1)
4-6: LGTM! Clean and concise class implementation.The
SecurityAgentclass follows TypeScript best practices with proper type inheritance and property assignment.src/agent.ts (1)
4-14: LGTM! Well-structured class implementation.The
AgentBootclass follows TypeScript best practices:
- Proper interface implementation
- Private readonly member with type annotation
- Typed constructor parameter
- Async lifecycle method
src/lib/middlewares/csp.ts (2)
15-16: Consider removing IE-specific code.Since the minimum Node.js version is now 18.19.0 (PR objective), supporting Internet Explorer might be unnecessary. Consider removing IE-specific code unless there's a specific requirement to maintain it.
22-25: LGTM! Clean options merging.The use of object spread operator for merging options is clean and follows modern JavaScript practices.
test/lib/helper/surl.test.ts (2)
1-2: LGTM! Modern import syntax with proper type imports.The conversion to ES modules and TypeScript imports is done correctly.
4-6: LGTM! Proper type annotations added.The MockApplication type is correctly applied to the app variables.
.github/workflows/nodejs.yml (1)
14-14: LGTM! Node.js version requirements aligned with PR objectives.The Node.js versions (18.19.0, 20, 22) correctly reflect the new minimum version requirement.
package.json (5)
13-17: LGTM! Proper dual package configuration.The exports field correctly defines entry points for both ESM and CJS formats, along with TypeScript source.
92-104: LGTM! Comprehensive package exports configuration.The exports field properly handles:
- TypeScript type definitions for both ESM and CJS
- Separate entry points for import and require
- Package.json exports for tooling support
76-83: LGTM! Modern build and test workflow.The scripts section properly handles:
- TypeScript linting
- Clean builds
- Test execution
- Coverage reporting
- Publishing preparation
85-91: LGTM! Proper tshy configuration for dual package support.The tshy configuration correctly defines the TypeScript source entry points.
39-52: Verify dependency versions for potential security issues.Some dependencies might have known security vulnerabilities or compatibility issues with Node.js 18+.
commit: |
commit: |
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
src/lib/helper/spath.ts (1)
Line range hint
11-21: Add maximum iterations limit to prevent DoSThe while loop for decoding URLs could potentially run indefinitely if the input is crafted maliciously.
+ const MAX_ITERATIONS = 5; + let iterations = 0; while (path.indexOf('%') !== -1) { + if (iterations++ >= MAX_ITERATIONS) { + if (process.env.NODE_ENV !== 'production') { + this.ctx.coreLogger.warn('[@eggjs/security/lib/helper/spath] : max decoding iterations reached for path %j', path); + } + return null; + } try { path = decodeURIComponent(path); } catch (e) {
🧹 Nitpick comments (33)
src/app/extend/response.ts (4)
25-26: Remove commented code.These commented lines appear to be obsolete. If they're no longer needed, they should be removed to maintain code cleanliness.
- // app.response.unsafeRedirect = app.response.redirect; - // delegate(app.context, 'response').method('unsafeRedirect');
19-23: Enhance security warning in JSDoc.Consider adding more specific security implications and recommended alternatives in the documentation.
* This is an unsafe redirection, and we WON'T check if the * destination url is safe or not. * Please DO NOT use this method unless in some very special cases, * otherwise there may be security vulnerabilities. + * @security This method can lead to Open Redirect vulnerabilities if used with user-controlled input + * @see redirect For a safer alternative that validates the destination URL * * @function Response#unsafeRedirect
Line range hint
64-79: Enhance URL validation security.The URL validation could be strengthened by:
- Adding validation for URL encoding attacks
- Implementing length limits for URLs
- Checking for common bypass patterns
if (urlObject.protocol !== 'http:' && urlObject.protocol !== 'https:') { url = '/'; } else if (!urlObject.hostname) { url = '/'; } else { + // Check for URL encoding attacks + if (url !== decodeURIComponent(encodeURIComponent(url))) { + url = '/'; + return; + } + // Implement reasonable URL length limit + if (url.length > 2048) { + url = '/'; + return; + } if (domainWhiteList && domainWhiteList.length !== 0) {
54-61: Improve error handling for invalid URLs.The current error handling silently redirects to '/' when the URL is invalid. Consider logging the invalid URL attempt for security monitoring.
try { urlObject = new URL(url); } catch { + const message = `Invalid URL detected: ${url}`; + this.app.coreLogger.warn('[@eggjs/security/response/redirect] %s', message); url = '/'; this.unsafeRedirect(url); return; }src/types.ts (1)
281-282: Replace void with undefined in union type.The use of
voidin a union type can be confusing. Consider usingundefinedinstead.- tag: string, name: string, value: string, isWhiteAttr: boolean) => string | void; + tag: string, name: string, value: string, isWhiteAttr: boolean) => string | undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 282-282: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/app/middleware/securities.ts (2)
23-23: Use direct property access instead ofReflect.getfor clarityUsing
options[middlewareName]instead ofReflect.get(options, middlewareName)improves readability and avoids accessing inherited properties from the prototype chain.Apply this diff:
- const opt = Reflect.get(options, middlewareName); + const opt = options[middlewareName];
31-31: Enhance conditional readability by adding parenthesesThe condition
if (opt === false || opt && opt.enable === false)may be unclear due to operator precedence. Adding parentheses improves readability.Apply this diff:
- if (opt === false || opt && opt.enable === false) { + if (opt === false || (opt && opt.enable === false)) {test/fixtures/apps/method/app/router.js (1)
1-6: Avoid usinghttp.METHODSdue to potential inconsistenciesThe
METHODSarray fromnode:httpis undocumented and may vary between Node.js versions, leading to compatibility issues. It might include methods not suitable for routing.Consider using the well-maintained
methodspackage for consistency.Apply this diff:
-const { METHODS } = require('node:http'); +const methods = require('methods'); module.exports = function(app) { - METHODS.forEach(function(m) { + methods.forEach(function(m) { m = m.toLowerCase(); app.router[m] && app.router[m]('/', async function() { this.body = '123'; }); }); };test/fixtures/apps/helper-app/app.js (1)
3-12: LGTM! Consider adding JSDoc comments.The transition to a class-based approach with async/await is well-structured. Consider adding JSDoc comments to document the class purpose and method behavior.
+/** + * Test boot class for helper app + */ module.exports = class Boot { constructor(app) { this.app = app; } + /** + * Verify that surl helper rejects unsafe URLs + */ async willReady() {src/lib/middlewares/dta.ts (1)
Line range hint
1-11: LGTM! Consider enhancing error message.The transition to ES modules and TypeScript types is well-executed. Consider providing a more descriptive error message when throwing the 400 status.
return function dta(ctx: Context, next: Next) { const path = ctx.path; if (!isSafePath(path, ctx)) { - ctx.throw(400); + ctx.throw(400, 'Invalid path: Directory traversal attack detected'); } return next(); };src/lib/helper/escapeShellCmd.ts (1)
Line range hint
3-15: LGTM! Consider performance and security enhancements.The transition to ES modules and TypeScript is well-executed. Consider these improvements:
- Cache string length for better performance
- Add input validation
- Document the security implications
+/** + * Escapes shell command arguments to prevent command injection + * @param text - The text to escape + * @returns The escaped text + * @security This function helps prevent command injection but should not be relied upon as the sole defense + */ export default function escapeShellCmd(text: string) { + if (typeof text !== 'string') { + throw new TypeError('Input must be a string'); + } const str = '' + text; let res = ''; let ascii; + const len = str.length; - for (let index = 0; index < str.length; index++) { + for (let index = 0; index < len; index++) {src/lib/helper/index.ts (1)
11-21: Consider adding TypeScript type definitions.Add type definitions for the exported object to improve type safety and developer experience.
+interface SecurityHelpers { + cliFilter: (text: string) => string; + escapeHtml: (text: string) => string; + escapeShellArg: (text: string) => string; + escapeShellCmd: (text: string) => string; + shtml: (val: string) => string; + sjs: (text: string) => string; + sjson: (obj: any) => string; + spath: (path: string) => string; + surl: (val: string) => string; +} + -export default { +const helpers: SecurityHelpers = { cliFilter, - escape, + escapeHtml, escapeShellArg, escapeShellCmd, shtml, sjs, sjson, spath, surl, }; + +export default helpers;src/lib/middlewares/noopen.ts (1)
5-18: Improve documentation and header setting timing.The implementation could be improved by:
- Adding comprehensive JSDoc documentation
- Setting the header before
next()to prevent potential overwrites// @see http://blogs.msdn.com/b/ieinternals/archive/2009/06/30/internet-explorer-custom-http-headers.aspx +/** + * Noopen middleware. + * Sets X-Download-Options: noopen to prevent IE from executing downloads in the context of the site. + * @param {SecurityConfig['noopen']} options - Configuration options + */ export default (options: SecurityConfig['noopen']) => { return async function noopen(ctx: Context, next: Next) { - await next(); - const opts = { ...options, ...ctx.securityOptions.noopen, }; if (checkIfIgnore(opts, ctx)) return; ctx.set('x-download-options', 'noopen'); + + await next(); }; };src/lib/middlewares/methodnoallow.ts (2)
5-11: Optimize the HTTP methods map initialization.The
safeHttpMethodsMapcould be precomputed usingObject.fromEntriesfor better readability and performance.-const safeHttpMethodsMap: Record<string, boolean> = {}; - -for (const method of METHODS) { - if (!METHODS_NOT_ALLOWED.includes(method)) { - safeHttpMethodsMap[method.toUpperCase()] = true; - } -} +const safeHttpMethodsMap: Record<string, boolean> = Object.fromEntries( + METHODS + .filter(method => !METHODS_NOT_ALLOWED.includes(method)) + .map(method => [method.toUpperCase(), true]) +);
Line range hint
15-22: Add comprehensive documentation for the middleware.Add JSDoc documentation explaining the security implications and purpose of this middleware.
+/** + * Method Not Allowed middleware. + * Blocks HTTP TRACE/TRACK methods to prevent Cross-Site Tracing (XST) attacks. + * @see https://www.owasp.org/index.php/Cross_Site_Tracing + */ export default () => { return function notAllow(ctx: Context, next: Next) { // ctx.method is upper case if (!safeHttpMethodsMap[ctx.method]) { ctx.throw(405); } return next(); }; };src/lib/middlewares/index.ts (1)
13-24: Improve type safety of the middleware collection.Instead of type assertion, consider using explicit typing for better type safety and documentation.
+/** Type for security middleware factory function */ +type SecurityMiddlewareFactory = (options: unknown, app: EggCore) => MiddlewareFunc; + +/** Collection of security middleware factories */ +const securityMiddlewares: Record<string, SecurityMiddlewareFactory> = { -export default { csp, csrf, dta, hsts, methodnoallow, noopen, nosniff, referrerPolicy, xframe, xssProtection, -} as Record<string, (options: unknown, app: EggCore) => MiddlewareFunc>; +}; + +export default securityMiddlewares;src/lib/middlewares/xframe.ts (2)
9-13: Consider adding validation for merged options.The options merging could potentially override critical security settings. Consider adding validation to ensure that
ctx.securityOptions.xframecannot weaken the base security configuration.const opts = { ...options, ...ctx.securityOptions.xframe, }; +// Validate that the merged options maintain minimum security requirements +if (opts.value && !['DENY', 'SAMEORIGIN'].includes(opts.value)) { + ctx.logger.warn('[security] Potentially unsafe x-frame-options value:', opts.value); +}
17-18: Consider using uppercase for header value comparison.To ensure consistent header value handling, consider converting the value to uppercase before setting it.
-const value = opts.value || 'SAMEORIGIN'; -ctx.set('x-frame-options', value); +const value = (opts.value || 'SAMEORIGIN').toUpperCase(); +ctx.set('x-frame-options', value);src/lib/helper/sjson.ts (1)
28-34: Optimize JSON sanitization performance.The current implementation performs multiple string operations. Consider caching the sanitized values and using a Map for better performance with large objects.
export default function jsonEscape(obj: any) { + const cache = new Map(); return JSON.stringify(sanitizeKey(obj), (_k, v) => { if (typeof v === 'string') { + if (cache.has(v)) return cache.get(v); + const sanitized = sjs(v); + cache.set(v, sanitized); + return sanitized; - return sjs(v); } return v; }); }src/lib/middlewares/nosniff.ts (2)
7-15: Consider using an enum or constants for status codes.Using a Record type is good, but consider using TypeScript enums or named constants for better maintainability and documentation.
-const RedirectStatus: Record<number, boolean> = { - 300: true, - 301: true, - 302: true, - 303: true, - 305: true, - 307: true, - 308: true, -}; +enum RedirectStatus { + MULTIPLE_CHOICES = 300, + MOVED_PERMANENTLY = 301, + FOUND = 302, + SEE_OTHER = 303, + USE_PROXY = 305, + TEMPORARY_REDIRECT = 307, + PERMANENT_REDIRECT = 308, +} + +const isRedirectStatus = (status: number): boolean => + Object.values(RedirectStatus).includes(status);
21-22: Use the isRedirectStatus helper for better readability.The current status code check could be more expressive.
-if (RedirectStatus[ctx.status]) return; +if (isRedirectStatus(ctx.status)) return;src/lib/middlewares/referrerPolicy.ts (1)
22-25: Consider defensive merging of options.While using the spread operator is concise, consider adding null checks to prevent runtime errors if either
optionsorctx.securityOptions.referrerPolicyis undefined.const opts = { - ...options, - ...ctx.securityOptions.referrerPolicy, + ...(options ?? {}), + ...(ctx.securityOptions?.referrerPolicy ?? {}), };src/lib/helper/surl.ts (1)
32-34: Consider using a more comprehensive HTML escaping library.While the current escaping handles basic cases, consider using a well-tested library like
hefor more thorough HTML entity encoding.src/lib/middlewares/csrf.ts (1)
Line range hint
17-29: Optimize request method validation.Convert methods array to Set for O(1) lookup performance.
let isSupported = false; for (const eachRule of options.supportedRequests) { if (eachRule.path.test(ctx.path)) { - if (eachRule.methods.includes(method)) { + if (new Set(eachRule.methods).has(method)) { isSupported = true; break; } } }src/lib/helper/sjs.ts (1)
Line range hint
35-42: Consider using string builder for better performance.For large strings with many escapes, using an array and joining at the end could be more efficient.
- if (lastIndex !== index) { - res += str.substring(lastIndex, index); - } - - lastIndex = index + 1; - res += map[ascii]; + const parts = []; + // ... collect parts ... + return parts.join('');src/lib/helper/shtml.ts (1)
Line range hint
48-56: Consider removing duplicated domain whitelist checkThe code checks domainWhiteList twice and shows a deprecation warning. Consider simplifying this logic to reduce complexity.
- if (shtmlConfig.domainWhiteList && shtmlConfig.domainWhiteList.length > 0) { - app.deprecate('[@eggjs/security/lib/helper/shtml] `config.helper.shtml.domainWhiteList` has been deprecate. Please use `config.security.domainWhiteList` instead.'); - if (!isSafeDomain(hostname, shtmlConfig.domainWhiteList)) { - return ''; - } - } else { - return ''; - } + // Since this is a deprecated feature, simply return empty string + return '';src/lib/helper/cliFilter.ts (1)
7-8: Consider additional security measures for CLI input filteringWhile the basic alphabet filtering is good, consider adding:
- Maximum length check to prevent DoS
- Logging of filtered characters in debug mode
export default function cliFilter(text: string) { + const MAX_LENGTH = 1000; // Adjust based on your needs + if (text.length > MAX_LENGTH) { + throw new Error('Input text exceeds maximum allowed length'); + } const str = '' + text;src/app.ts (2)
16-20: Use optional chaining for safer configuration accessThe static analysis tool correctly suggests using optional chaining for safer configuration access.
- if (app.config.security.csrf && app.config.security.csrf.enable) { + if (app.config.security.csrf?.enable) {🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
22-24: Consider using const assertion for type safetyThe legal types array could benefit from const assertion to make it more type-safe.
- const legalTypes = [ 'all', 'referer', 'ctoken', 'any' ]; + const legalTypes = [ 'all', 'referer', 'ctoken', 'any' ] as const; + type LegalType = typeof legalTypes[number];src/app/extend/context.ts (1)
138-141: Enhance type safety for cookie options.The spread operator usage with cookieOptions could allow unexpected properties to be passed to the cookie setter.
Consider creating an interface for cookie options and validating the spread:
interface CsrfCookieOptions { domain?: string; httpOnly?: boolean; signed?: boolean; overwrite?: boolean; } const cookieOpts: CsrfCookieOptions = { domain: cookieDomain, ...cookieOptions, };src/lib/middlewares/csp.ts (1)
15-16: Consider using a more robust browser detection method.The current MSIE detection using a simple regex might be unreliable due to:
- User-Agent string spoofing
- Variations in IE User-Agent strings
- Modern browsers that include "MSIE" for compatibility
Consider using the
@eggjs/user-agentpackage or Browserslist for more reliable browser detection.src/config/config.default.ts (1)
86-89: Improve SSRF configuration documentation.The SSRF configuration uses
undefinedvalues, but it's not immediately clear what the fallback behavior is. Consider adding JSDoc comments to explain:
- The purpose of each configuration option
- The default behavior when undefined
- Example values for each option
src/lib/utils.ts (1)
181-191: Consider using a frozen Set for protocol whitelist.The protocol whitelist Set is mutable and could be modified at runtime. Since these are security-critical protocols, consider freezing the Set.
- Object.defineProperty(config, '__protocolWhiteListSet', { - value: protocolWhiteListSet, - enumerable: false, - }); + Object.defineProperty(config, '__protocolWhiteListSet', { + value: Object.freeze(protocolWhiteListSet), + enumerable: false, + configurable: false, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
README.md(1 hunks)README.zh-CN.md(1 hunks)package.json(1 hunks)src/agent.ts(1 hunks)src/app.ts(1 hunks)src/app/extend/context.ts(7 hunks)src/app/extend/helper.ts(1 hunks)src/app/extend/response.ts(4 hunks)src/app/middleware/securities.ts(1 hunks)src/config/config.default.ts(3 hunks)src/lib/helper/cliFilter.ts(1 hunks)src/lib/helper/escape.ts(1 hunks)src/lib/helper/escapeShellArg.ts(1 hunks)src/lib/helper/escapeShellCmd.ts(1 hunks)src/lib/helper/index.ts(1 hunks)src/lib/helper/shtml.ts(3 hunks)src/lib/helper/sjs.ts(1 hunks)src/lib/helper/sjson.ts(2 hunks)src/lib/helper/spath.ts(2 hunks)src/lib/helper/surl.ts(1 hunks)src/lib/middlewares/csp.ts(2 hunks)src/lib/middlewares/csrf.ts(2 hunks)src/lib/middlewares/dta.ts(1 hunks)src/lib/middlewares/hsts.ts(1 hunks)src/lib/middlewares/index.ts(1 hunks)src/lib/middlewares/methodnoallow.ts(1 hunks)src/lib/middlewares/noopen.ts(1 hunks)src/lib/middlewares/nosniff.ts(1 hunks)src/lib/middlewares/referrerPolicy.ts(2 hunks)src/lib/middlewares/xframe.ts(1 hunks)src/lib/middlewares/xssProtection.ts(1 hunks)src/lib/utils.ts(5 hunks)src/types.ts(1 hunks)test/fixtures/apps/helper-app/app.js(1 hunks)test/fixtures/apps/method/app/router.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/app/extend/helper.ts
- src/lib/helper/escape.ts
- README.zh-CN.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/agent.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/helper/index.ts
[error] 2-2: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/types.ts
[error] 282-282: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/lib/utils.ts
[error] 120-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 176-176: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/app.ts
[error] 16-16: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (20)
src/app/extend/response.ts (3)
1-7: LGTM! Clean class setup with proper type declarations.The imports and class setup follow TypeScript best practices with proper type declarations.
70-70: Fix TypeScript type error: Ensurethis.ctx.isSafeDomainis properly typedThe TypeScript type error for
this.ctx.isSafeDomainstill needs to be addressed.
86-92: LGTM! Well-structured type declarations.The type declarations properly extend the core Response interface with both redirect methods.
package.json (5)
2-5: LGTM! Package metadata changes look good.The package has been renamed to
@eggjs/securityand configured for public access, which aligns with the PR objectives.
13-17: LGTM! Dual package hazard handled correctly.The exports field is properly configured to support both ESM and CommonJS formats, following Node.js best practices for dual package hazard:
- ESM:
./dist/esm- CommonJS:
./dist/commonjs- TypeScript:
./srcAlso applies to: 92-103
39-40: LGTM! Dependencies updated for TypeScript and dual package support.Dependencies have been properly updated:
- Core dependencies updated to latest versions
- Added necessary dev dependencies for TypeScript and dual package support (tshy, typescript, etc.)
Also applies to: 53-73
76-83: LGTM! Build scripts properly configured.The scripts section is well organized with all necessary commands for TypeScript development:
- Linting with ESLint
- Clean builds with rimraf
- TypeScript builds with tshy
85-91: LGTM! tshy configuration looks good.The tshy configuration is properly set up to handle dual package builds from TypeScript source.
src/types.ts (3)
1-5: LGTM! Imports are properly structured.Using the new
node:protocol for Node.js built-in modules and properly importing types from @eggjs/core.
6-15: LGTM! Type definitions are clear and well-documented.The interfaces provide good type safety for CSRF and SSRF configurations.
302-308: LGTM! Module augmentation is properly implemented.Correctly extends EggAppConfig with security and helper configurations.
README.md (1)
1-26: LGTM! Documentation updates are comprehensive.The README has been properly updated to reflect:
- New package name (@eggjs/security)
- Updated badge URLs
- Added Node.js version and PR badges
src/lib/middlewares/referrerPolicy.ts (1)
Line range hint
28-31: LGTM! Robust policy validation.Good security practice: The code properly validates the referrer policy against a predefined whitelist before setting the header.
src/lib/helper/shtml.ts (3)
1-4: LGTM! Clean transition to ES modulesThe import statements and type definitions are well-structured, providing good type safety with the BaseContextClass and SecurityHelperOnTagAttrHandler types.
17-23:⚠️ Potential issuePotential undefined access in configuration merging
The buildInOnTagAttrHandler is used in the config object before being defined, which could lead to undefined behavior.
Move the handler definition before the config creation:
- let buildInOnTagAttrHandler: SecurityHelperOnTagAttrHandler | undefined; - const shtmlConfig = { - ...this.app.config.helper.shtml, - ...securityOptions.shtml, - [BUILD_IN_ON_TAG_ATTR]: buildInOnTagAttrHandler, - }; + const buildInOnTagAttrHandler: SecurityHelperOnTagAttrHandler | undefined = undefined; + const shtmlConfig = { + ...this.app.config.helper.shtml, + ...securityOptions.shtml, + [BUILD_IN_ON_TAG_ATTR]: buildInOnTagAttrHandler, + };Likely invalid or redundant comment.
62-70: Verify custom handler's this bindingThe custom handler is called with apply, but the this binding might not be what the user expects.
Consider documenting the this binding behavior or using bind/call instead of apply for clarity:
- const result = customOnTagAttrHandler.apply(this, [ tag, name, value, isWhiteAttr ]); + // Preserve the original context + const result = customOnTagAttrHandler.call(this, tag, name, value, isWhiteAttr);✅ Verification successful
The this binding implementation is correct and safe
The current implementation correctly preserves the same context for both custom and built-in handlers, which is essential for the XSS sanitizer's operation. The use of
apply()is functionally equivalent tocall()in this case, and the code ensures consistent behavior with the underlying js-xss library's expectations.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for tests and usage of onTagAttr handlers rg -l "onTagAttr" --type ts # Look for documentation about handler context fd README.md --exec cat {} # Find similar handler patterns ast-grep --pattern 'onTagAttr = function($$$) { $$$ }'Length of output: 17362
src/app/extend/context.ts (1)
261-263: Correct the return type of the 'safeCurl' methodThe
safeCurlmethod isasyncand thus returns aPromise, but the return type is declared asHttpClientRequestReturn.Apply this diff to correct the return type:
- async safeCurl(url: HttpClientRequestURL, options?: HttpClientOptions): HttpClientRequestReturn { + async safeCurl(url: HttpClientRequestURL, options?: HttpClientOptions): Promise<HttpClientRequestReturn> {Also update the module augmentation:
- safeCurl(url: HttpClientRequestURL, options?: HttpClientOptions): HttpClientRequestReturn; + safeCurl(url: HttpClientRequestURL, options?: HttpClientOptions): Promise<HttpClientRequestReturn>;src/lib/middlewares/csp.ts (2)
22-25: LGTM! Clean options merging using spread operator.The spread operator provides a more concise and readable way to merge options compared to the old
utils.mergeapproach.
34-34: Verify IE support requirement.Since Node.js < 18.19.0 support is being dropped, verify if IE-specific code paths are still necessary, as IE is no longer supported by Microsoft.
✅ Verification successful
Keep the IE-specific code paths for now
The IE support is optional (controlled by
supportIEflag) and isolated to security header handling. Removing it would be a breaking change. Consider deprecating it in the next major version.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other IE-specific code paths that might need cleanup rg -i "msie|internet.?explorer" --type tsLength of output: 414
Script:
#!/bin/bash # Check for supportIE configuration and tests rg -i "supportie" --type ts -A 2 # Check package.json for browser support cat package.json | jq -r '.browserslist // .engines // empty' # Check documentation fd -e md | xargs rg -i "internet.?explorer|browser.?support" -A 2Length of output: 509
src/lib/utils.ts (1)
194-201: LGTM! Safe URL parsing with proper error handling.The URL parsing implementation:
- Uses the built-in URL API instead of regex
- Safely handles invalid URLs
- Uses Reflect.get for safe property access
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
test/fixtures/apps/csp-reportonly/app/router.js (2)
2-4: Consider using async/await for consistency.While the current implementation works, consider using async/await for better consistency with modern Node.js practices:
- app.get('/testcsp', function(){ + app.get('/testcsp', async function() { this.body = this.nonce; });
5-7: Consider using async/await for consistency.Similarly, update this route handler to use async/await:
- app.get('/testcsp2', function(){ + app.get('/testcsp2', async function() { this.body = this.nonce; });test/csp.test.ts (1)
90-91: Consider adding null check assertion.The non-null assertion (
!) assumes the regex match will always succeed. Consider adding an explicit assertion:const m = re_nonce.exec(header); + assert(m !== null, 'CSP header should contain a valid nonce'); assert.equal(nonce, m![1], header);src/types.ts (3)
11-15: Add JSDoc documentation for SSRFCheckAddressFunction type.Consider adding documentation to explain the purpose and parameters of this function type, especially the meaning of the
familyparameter and the expected format ofaddresses.+/** + * Function type for checking if an address should be allowed in SSRF protection + * @param addresses - The IP addresses or hostname to check (can be string, LookupAddress, or array) + * @param family - The IP address family (e.g., 4 for IPv4, 6 for IPv6) + * @param hostname - The original hostname being checked + * @returns boolean indicating if the address should be allowed + */ export type SSRFCheckAddressFunction = ( addresses: string | LookupAddress | (string | LookupAddress)[], family: number | string, hostname: string, ) => boolean;
281-282: Replace void with undefined in union type.The static analysis tool correctly flags that void in a union type can be confusing. Consider using undefined instead as it's more appropriate for return types in this context.
export type SecurityHelperOnTagAttrHandler = ( - tag: string, name: string, value: string, isWhiteAttr: boolean) => string | void; + tag: string, name: string, value: string, isWhiteAttr: boolean) => string | undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 282-282: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
1-308: Consider splitting the configuration into smaller interfaces.The SecurityConfig interface is quite large and handles multiple concerns. Consider splitting it into smaller, focused interfaces for better maintainability:
interface CSRFConfig { /* csrf specific config */ } interface XFrameConfig { /* xframe specific config */ } interface HSTSConfig { /* hsts specific config */ } interface SecurityConfig { csrf: CSRFConfig; xframe: XFrameConfig; hsts: HSTSConfig; // ... other configs }This would:
- Improve code organization
- Make it easier to maintain individual security features
- Allow for better type reuse
Would you like me to help with this refactoring?🧰 Tools
🪛 Biome (1.9.4)
[error] 282-282: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
package.json (2)
47-47: Consider upgrading nanoid to v4.The current version (^3.3.8) of nanoid uses the older crypto API. Version 4 provides better support for newer Node.js versions and uses the Web Crypto API.
Since you're already requiring Node.js 18+, you could benefit from the improvements in nanoid v4.
82-82: Enhance the clean script coverage.The clean script only removes the
distdirectory. Consider adding other generated files/directories like.eslintcache,coverage, etc.- "clean": "rimraf dist", + "clean": "rimraf dist .eslintcache coverage",src/lib/middlewares/csp.ts (4)
1-4: Consider using a more modern alternative to the 'extend' package.While the imports are well-structured, the
extendpackage could be replaced with more modern alternatives likestructuredClone()or the spread operator for shallow copies.-import extend from 'extend'; +// For deep cloning +const extend = <T>(target: T, source: Partial<T>): T => { + return structuredClone({ ...target, ...source }); +};
6-16: Consider dropping IE support and improving documentation.
- Given that IE is no longer supported by Microsoft, consider removing IE-specific code unless explicitly required.
- The comment for IE detection could be more descriptive.
-// Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1) +/** + * Regular expression to detect Internet Explorer User-Agent strings. + * @deprecated IE is no longer supported by Microsoft as of June 15, 2022 + */ const MSIE_REGEXP = / MSIE /i;
18-26: Ensure immutability of the options object.The options merging could be more explicit about creating a new object to prevent potential side effects.
- const opts = { - ...options, - ...ctx.securityOptions.csp, - }; + const opts = Object.freeze({ + ...options, + ...ctx.securityOptions.csp, + });
28-37: Simplify header selection logic.The header selection logic could be more concise and avoid unnecessary object cloning.
- let finalHeader; - const matchedOption = extend(true, {}, opts.policy); - const bufArray = []; - - const headers = opts.reportOnly ? REPORT_ONLY_HEADER : HEADER; - if (opts.supportIE && MSIE_REGEXP.test(ctx.get('user-agent'))) { - finalHeader = headers[0]; - } else { - finalHeader = headers[1]; - } + const headers = opts.reportOnly ? REPORT_ONLY_HEADER : HEADER; + const finalHeader = (opts.supportIE && MSIE_REGEXP.test(ctx.get('user-agent'))) + ? headers[0] + : headers[1]; + const matchedOption = structuredClone(opts.policy); + const bufArray = [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package.json(1 hunks)src/lib/middlewares/csp.ts(1 hunks)src/types.ts(1 hunks)test/csp.test.ts(2 hunks)test/fixtures/apps/csp-reportonly/app/router.js(1 hunks)test/fixtures/apps/csp-supportie/config/config.js(0 hunks)
💤 Files with no reviewable changes (1)
- test/fixtures/apps/csp-supportie/config/config.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/types.ts
[error] 282-282: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
🔇 Additional comments (8)
test/csp.test.ts (3)
1-2: LGTM! Modern import statements align with ESM transition.The use of node: protocol and ESM imports aligns well with the PR objectives.
4-8: LGTM! Proper type annotations improve type safety.The addition of MockApplication type annotations enhances code reliability.
28-33: LGTM! Proper resource cleanup.Good practice to close all test applications in the after hook.
src/types.ts (3)
235-237: Address commented-out CSP configuration properties.These commented-out properties appear to be valid CSP configuration options. Consider either:
- Implementing them as optional properties
- Removing them if they're not planned for implementation
- Adding a TODO comment explaining the plan for these properties
Would you like me to help implement these properties with proper TypeScript types?
291-294: Plan removal of deprecated domainWhiteList.Since this property is deprecated in favor of
config.security.domainWhiteList, consider:
- Adding a warning if this property is used
- Planning for removal in the next major version
Would you like me to help create a migration guide for users still using this property?
302-308: LGTM! Module augmentation is well-structured.The module augmentation correctly extends EggAppConfig with the security types, following TypeScript best practices.
package.json (2)
92-104: Well-structured dual package configuration!The exports field correctly handles both ESM and CommonJS use cases with proper TypeScript declarations. This follows the Node.js best practices for dual packages.
35-37: Verify the Node.js version requirement impact.The upgrade to Node.js >= 18.19.0 is a significant breaking change. This aligns with the PR objectives but warrants careful consideration.
Let's analyze the impact:
✅ Verification successful
Node.js version requirement is appropriate
The upgrade to Node.js >= 18.19.0 is justified by:
- ESM-first approach with dual ESM/CommonJS support
- Core dependencies already requiring modern Node.js versions
- No breaking usage of Node.js 18+ specific features in the source code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any code relies on Node.js APIs that changed significantly between v14 and v18 rg -l 'require\(["\']node:|import ["\']node:' # Check for potentially incompatible syntax features ast-grep --pattern 'class { #private }' # Private class fields (Node.js 16+) ast-grep --pattern 'await import(' # Top-level await (Node.js 16+)Length of output: 116
Script:
#!/bin/bash # Check for node: protocol usage rg -l "require\('node:" || rg -l "import.*'node:" # Check for modern syntax features ast-grep --pattern 'class $_ { #$_ }' ast-grep --pattern 'await import($$$)' # Check package.json for dependencies cat package.jsonLength of output: 3037
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
test/fixtures/apps/csrf-cookieOptions/app/controller/home.js (1)
3-3: LGTM! Consider documenting the Node.js version requirement.The conversion to async/await is consistent across all test fixtures. Since this is part of a breaking change dropping support for Node.js < 18.19.0, consider:
- Adding a note in the test README about the Node.js version requirement
- Including an engine requirement in package.json
test/context.test.ts (1)
23-23: Consider adding more descriptive assertion messages.While the
assert.equalusage is good, the assertions could benefit from more specific error messages to aid debugging.- assert.equal(res.text, 'false'); + assert.equal(res.text, 'false', 'Expected unsafe domain check to return false'); - assert.equal(res.text, 'true'); + assert.equal(res.text, 'true', 'Expected safe domain check to return true');Also applies to: 31-31
test/csrf_cookieDomain.test.ts (2)
Line range hint
47-63: Consider expanding cookie options test coverage.While the current test validates the httpOnly flag, consider adding test cases for other important cookie options like:
- sameSite attribute
- secure flag
- expires/maxAge values
Line range hint
67-83: Consider enhancing signed cookie validation.While the test verifies the presence of both token and signature cookies, consider adding assertions to:
- Validate the signature format
- Verify signature validation on subsequent requests
- Test signature tampering scenarios
test/app/extends/cliFilter.test.ts (1)
12-14: Consolidate test cleanup hooks for consistency.Consider following the pattern used in other test files by:
- Using
afterEach(mm.restore)for mock cleanup after each test- Keeping
after(() => app.close())for application cleanupThis makes the cleanup strategy more consistent across the test suite.
after(() => app.close()); - after(mm.restore); + afterEach(mm.restore);🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/app/extends/sjs.test.ts (1)
1-1: Consider standardizing test cleanup patterns across all test files.The migration to TypeScript and ES modules is well-executed. To further improve maintainability, consider standardizing the test cleanup pattern across all test files:
- Use
after(() => app.close())for application cleanup- Use
afterEach(mm.restore)for mock restorationThis pattern, as implemented in
sjs.test.ts, provides the clearest separation of concerns and most reliable cleanup strategy.test/app/extends/helper.test.ts (1)
25-29: Consider adding error handling to the cleanup logic.While the async cleanup is good, consider adding try-catch blocks to handle potential cleanup failures gracefully.
after(async () => { - await app.close(); - await app2.close(); - await app3.close(); + try { + await app.close(); + await app2.close(); + await app3.close(); + } catch (error) { + console.error('Failed to clean up test apps:', error); + throw error; + } });test/app/extends/spath.test.ts (2)
12-14: Consider combining the after hooks.The two separate
afterhooks could be combined for better maintainability.- after(() => app.close()); - - after(mm.restore); + after(async () => { + await app.close(); + mm.restore(); + });🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
Line range hint
16-71: Enhance test descriptions for security context.While the test coverage is comprehensive, consider making the security implications more explicit in the test descriptions. This would help developers better understand the security risks being tested.
Example improvements:
- it('should pass when filepath is safe', () => { + it('should pass when filepath contains no directory traversal attempts', () => { - it('should return null when filepath is not safe(contains ..)', () => { + it('should return null to prevent directory traversal attacks using ".."', () => { - it('should decode first when filepath contains %', () => { + it('should decode URL-encoded characters to prevent encoded traversal attempts', () => {🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
test/app/extends/cliFilter.test.ts(1 hunks)test/app/extends/escapeShellArg.test.ts(1 hunks)test/app/extends/escapeShellCmd.test.ts(1 hunks)test/app/extends/helper.test.ts(1 hunks)test/app/extends/sjs.test.ts(1 hunks)test/app/extends/sjson.test.ts(1 hunks)test/app/extends/spath.test.ts(1 hunks)test/context.test.ts(1 hunks)test/csrf_cookieDomain.test.ts(4 hunks)test/fixtures/apps/csrf-cookieOptions/app/controller/home.js(1 hunks)test/fixtures/apps/csrf-string-cookiedomain/app/controller/home.js(1 hunks)test/fixtures/apps/ctoken/app/controller/home.js(1 hunks)
🔇 Additional comments (18)
test/fixtures/apps/ctoken/app/controller/home.js (1)
3-3: LGTM! Part of modernization to async/await syntax.The conversion from generator function to async function aligns with the PR's modernization goals and the minimum Node.js version requirement of 18.19.0.
test/fixtures/apps/csrf-string-cookiedomain/app/controller/home.js (1)
3-3: LGTM! Consistent with the modernization pattern.The change maintains consistency with other fixtures, converting from generator to async function.
test/context.test.ts (3)
1-2: Well-structured module imports!Good practices observed:
- Using the
node:protocol for built-in modules- Explicit type imports from
@eggjs/mock- Clean ESM syntax
4-4: Good type safety improvements!The explicit
MockApplicationtype annotation enhances code reliability and IDE support.Also applies to: 8-8
16-16: Good test cleanup practice!The explicit
app.close()in theafterhook ensures proper cleanup of resources between tests.test/csrf_cookieDomain.test.ts (3)
1-1: LGTM! Clean migration to ES modules and TypeScript.The changes follow modern JavaScript practices with proper ES module imports and accurate test file path updates.
Also applies to: 3-3
Line range hint
7-23: LGTM! Well-structured test with proper type safety.The MockApplication type annotation enhances type safety while maintaining the original test functionality for dynamic cookie domains.
Line range hint
27-43: LGTM! Consistent type safety implementation.The test suite maintains consistency in type annotations while properly testing static cookie domain configuration.
test/app/extends/escapeShellCmd.test.ts (1)
1-1: Well-structured test setup with proper cleanup!Good practices implemented:
- ES module imports with proper type annotations
- Proper cleanup with
app.close()in theafterhook- Mock restoration after each test with
afterEach(mm.restore)Also applies to: 4-4, 12-12, 14-14
test/app/extends/escapeShellArg.test.ts (1)
12-14: Consolidate test cleanup hooks for consistency.Consider following the pattern used in other test files by:
- Using
afterEach(mm.restore)for mock cleanup after each test- Keeping
after(() => app.close())for application cleanupafter(() => app.close()); - after(mm.restore); + afterEach(mm.restore);🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/app/extends/sjs.test.ts (1)
1-1: Excellent test setup implementation!This file demonstrates the ideal pattern for test setup and cleanup:
- Clear ES module imports with proper type annotations
- Proper application cleanup with
app.close()- Mock restoration after each test with
afterEach(mm.restore)Also applies to: 4-4, 12-12, 14-14
test/app/extends/sjson.test.ts (2)
1-1: LGTM! Clean migration to TypeScript.The changes properly implement TypeScript types and ES module imports while maintaining the test's original functionality.
Also applies to: 3-4
12-14: Improved test cleanup with proper resource management.The changes enhance test isolation by:
- Properly closing the app instance after all tests
- Restoring mocks after each test
test/app/extends/helper.test.ts (3)
1-2: LGTM! Clean migration to ES modules.Good use of the
node:protocol for built-in modules and proper TypeScript imports.
5-7: LGTM! Proper TypeScript type annotations.Consistent use of MockApplication type for all app instances.
31-31: LGTM! Proper mock cleanup.Good addition of mock cleanup after each test to ensure test isolation.
test/app/extends/spath.test.ts (2)
4-4: LGTM! Type safety improvement.The explicit type annotation for
appenhances code maintainability and type safety.
1-1: LGTM! Package migration looks good.The transition to ES modules and TypeScript types aligns well with the PR objectives.
Let's verify the new package name and its exports:
✅ Verification successful
Import statement is correct and package is properly maintained
The
@eggjs/mockpackage (v6.0.5) is available and theMockApplicationtype is consistently used across the test suite, confirming this is a valid import statement.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify @eggjs/mock package npm view @eggjs/mock version # Check if MockApplication type is exported ast-grep --pattern 'export type MockApplication = $_'Length of output: 92
Script:
#!/bin/bash # Search for MockApplication type declaration or usage rg "MockApplication" -A 2 # Look for any mock-related type exports ast-grep --pattern 'export $_Mock$_'Length of output: 4776
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/config/config.default.ts (1)
177-185:⚠️ Potential issueSecurity: Enable httpOnly for CSRF cookie.
The CSRF token is transmitted via forms, headers, and request parameters. No client-side JavaScript access to the cookie is required, so
httpOnly: trueshould be set for enhanced XSS protection.cookieOptions: z.object({ signed: z.boolean(), httpOnly: z.boolean(), overwrite: z.boolean(), }).default({ signed: false, - httpOnly: false, + httpOnly: true, overwrite: true, }),
🧹 Nitpick comments (11)
src/lib/extend/safe_curl.ts (2)
1-10: Well-structured type definitions!The type definitions effectively leverage TypeScript's utility types and follow best practices. Consider adding JSDoc comments for the exported types to improve documentation.
Add documentation for the exported types:
+/** Type for HTTP client request URL parameter */ export type HttpClientRequestURL = HttpClientParameters[0]; +/** Type for HTTP client options with optional SSRF check function */ export type HttpClientOptions = HttpClientParameters[1] & { checkAddress?: SSRFCheckAddressFunction }; +/** Type for HTTP client request return value */ export type HttpClientRequestReturn = ReturnType<HttpClient['prototype']['request']>;
22-38: Refactor duplicate SSRF configuration check.The SSRF configuration check is duplicated. Consider consolidating the logic to improve maintainability.
const ssrfConfig = this.config.security.ssrf; + let httpClient: ReturnType<EggCore['createHttpClient']> | undefined; if (ssrfConfig?.checkAddress) { options.checkAddress = ssrfConfig.checkAddress; + httpClient = this[SSRF_HTTPCLIENT]; + if (!httpClient) { + httpClient = this[SSRF_HTTPCLIENT] = this.createHttpClient({ + checkAddress: ssrfConfig.checkAddress, + }); + } } else { - app.logger.warn('[@eggjs/security] please configure `config.security.ssrf` first'); + this.logger.warn( + '[@eggjs/security] SSRF protection is disabled. Please configure `config.security.ssrf.checkAddress` to enable protection against Server-Side Request Forgery attacks.' + ); } - if (ssrfConfig?.checkAddress) { - let httpClient = app[SSRF_HTTPCLIENT] as ReturnType<EggCore['createHttpClient']>; - // use the new httpClient init with checkAddress - if (!httpClient) { - httpClient = app[SSRF_HTTPCLIENT] = app.createHttpClient({ - checkAddress: ssrfConfig.checkAddress, - }); - } - return await httpClient.request(url, options); - } + return httpClient + ? await httpClient.request(url, options) + : await this.curl(url, options);package.json (2)
47-47: Consider upgrading nanoid to v4.The current version (v3.3.8) is not the latest major version. Version 4 offers better security and performance improvements, and uses modern crypto APIs.
Apply this diff to upgrade:
- "nanoid": "^3.3.8", + "nanoid": "^4.0.0",
78-85: Add coverage reporting to test script.While coverage is checked in CI, it would be beneficial to have coverage reporting during local development to catch coverage gaps early.
Apply this diff to add coverage reporting:
- "test": "egg-bin test", + "test": "egg-bin test --coverage",__snapshots__/config.default.test.ts.js (1)
39-43: Consider enabling HSTS by default.HSTS is currently disabled by default. Consider enabling it to enhance transport security.
Apply this diff:
"hsts": { - "enable": false, + "enable": true, "maxAge": 31536000, - "includeSubdomains": false + "includeSubdomains": true },🧰 Tools
🪛 eslint
[error] 39-39: Unnecessarily quoted property 'refererWhiteList' found.
(quote-props)
[error] 39-39: Strings must use singlequote.
(quotes)
[error] 40-40: Strings must use singlequote.
(quotes)
[error] 41-41: Unnecessarily quoted property 'signed' found.
(quote-props)
[error] 41-41: Strings must use singlequote.
(quotes)
[error] 42-42: Unnecessarily quoted property 'httpOnly' found.
(quote-props)
[error] 42-42: Strings must use singlequote.
(quotes)
[error] 43-43: Unnecessarily quoted property 'overwrite' found.
(quote-props)
[error] 43-43: Strings must use singlequote.
(quotes)
src/app.ts (2)
24-25: Fix typo in error message.There's a typo in the error message: "ap" should be "app".
Apply this diff:
- '[@eggjs/security/ap] `config.security.csrf.type` must be one of ' + legalTypes.join(', ')); + '[@eggjs/security/app] `config.security.csrf.type` must be one of ' + legalTypes.join(', '));
17-21: Improve code readability and deprecation message.Two suggestions:
- Use optional chaining for better readability
- Include migration guidance in the deprecation message
Apply this diff:
- if (app.config.security.csrf && app.config.security.csrf.enable) { - const { ignoreJSON, type } = app.config.security.csrf; + if (app.config.security.csrf?.enable) { + const { ignoreJSON, type } = app.config.security.csrf; if (ignoreJSON) { - app.deprecate('[@eggjs/security/app] `app.config.security.csrf.ignoreJSON` is not safe now, please disable it.'); + app.deprecate('[@eggjs/security/app] `app.config.security.csrf.ignoreJSON` is not safe now. Please disable it and ensure proper CSRF protection for both JSON and non-JSON requests.'); }🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/app/middleware/securities.ts (2)
26-26: Avoid type assertion to 'any'.Use proper typing from the SecurityConfig type instead of type assertion to 'any'.
- const opt = Reflect.get(options, middlewareName) as any; + const opt = Reflect.get(options, middlewareName) as SecurityConfig[SecurityMiddlewareName];
48-49: Update deprecation message with new package name.The deprecation message still uses the old package name 'egg-security'.
- app.deprecate('[@eggjs/security/middleware/securities] Please use `config.security.xframe.ignore` instead, `config.security.xframe.blackUrls` will be removed very soon'); + app.deprecate('[@eggjs/security] Please use `config.security.xframe.ignore` instead, `config.security.xframe.blackUrls` will be removed very soon');test/csp.test.ts (1)
105-105: Add descriptive name for snapshot test.The snapshot test should have a descriptive name to make it easier to understand what configuration is being tested.
- snapshot(app2.config.security); + snapshot(app2.config.security, 'CSP ignore configuration');src/config/config.default.ts (1)
286-289: Enhance type safety for CSP directives.Consider using a more specific type for CSP directives to catch misconfigurations at compile time.
- policy: z.record(z.union([ z.string(), z.array(z.string()), z.boolean() ])).default({}), + policy: z.object({ + 'default-src': z.union([ z.string(), z.array(z.string()) ]).optional(), + 'script-src': z.union([ z.string(), z.array(z.string()) ]).optional(), + 'style-src': z.union([ z.string(), z.array(z.string()) ]).optional(), + 'img-src': z.union([ z.string(), z.array(z.string()) ]).optional(), + // Add other CSP directives... + }).default({}),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
__snapshots__/config.default.test.ts.js(1 hunks)__snapshots__/csp.test.ts.js(1 hunks)package.json(1 hunks)src/app.ts(1 hunks)src/app/middleware/securities.ts(1 hunks)src/config/config.default.ts(1 hunks)src/lib/extend/safe_curl.ts(1 hunks)src/lib/middlewares/index.ts(1 hunks)src/types.ts(1 hunks)test/config/config.default.test.ts(1 hunks)test/csp.test.ts(3 hunks)test/fixtures/apps/csp-ignore/config/config.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/apps/csp-ignore/config/config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/middlewares/index.ts
- src/types.ts
🧰 Additional context used
🪛 eslint
__snapshots__/config.default.test.ts.js
[error] 2-80: Unnecessarily quoted property 'security' found.
(quote-props)
[error] 2-2: Strings must use singlequote.
(quotes)
[error] 3-3: Unnecessarily quoted property 'domainWhiteList' found.
(quote-props)
[error] 3-3: Strings must use singlequote.
(quotes)
[error] 4-4: Unnecessarily quoted property 'protocolWhiteList' found.
(quote-props)
[error] 4-4: Strings must use singlequote.
(quotes)
[error] 5-15: Unnecessarily quoted property 'defaultMiddleware' found.
(quote-props)
[error] 5-5: Strings must use singlequote.
(quotes)
[error] 6-6: Strings must use singlequote.
(quotes)
[error] 7-7: Strings must use singlequote.
(quotes)
[error] 8-8: Strings must use singlequote.
(quotes)
[error] 9-9: Strings must use singlequote.
(quotes)
[error] 10-10: Strings must use singlequote.
(quotes)
[error] 11-11: Strings must use singlequote.
(quotes)
[error] 12-12: Strings must use singlequote.
(quotes)
[error] 13-13: Strings must use singlequote.
(quotes)
[error] 14-14: Strings must use singlequote.
(quotes)
[error] 14-15: Missing trailing comma.
(comma-dangle)
[error] 16-45: Unnecessarily quoted property 'csrf' found.
(quote-props)
[error] 16-16: Strings must use singlequote.
(quotes)
[error] 17-17: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 17-17: Strings must use singlequote.
(quotes)
[error] 18-18: Unnecessarily quoted property 'type' found.
(quote-props)
[error] 18-18: Strings must use singlequote.
(quotes)
[error] 18-18: Strings must use singlequote.
(quotes)
[error] 19-19: Unnecessarily quoted property 'ignoreJSON' found.
(quote-props)
[error] 19-19: Strings must use singlequote.
(quotes)
[error] 20-20: Unnecessarily quoted property 'cookieName' found.
(quote-props)
[error] 20-20: Strings must use singlequote.
(quotes)
[error] 20-20: Strings must use singlequote.
(quotes)
[error] 21-21: Unnecessarily quoted property 'sessionName' found.
(quote-props)
[error] 21-21: Strings must use singlequote.
(quotes)
[error] 21-21: Strings must use singlequote.
(quotes)
[error] 22-22: Unnecessarily quoted property 'headerName' found.
(quote-props)
[error] 22-22: Strings must use singlequote.
(quotes)
[error] 22-22: Strings must use singlequote.
(quotes)
[error] 23-23: Unnecessarily quoted property 'bodyName' found.
(quote-props)
[error] 23-23: Strings must use singlequote.
(quotes)
[error] 23-23: Strings must use singlequote.
(quotes)
[error] 24-24: Unnecessarily quoted property 'queryName' found.
(quote-props)
[error] 24-24: Strings must use singlequote.
(quotes)
[error] 24-24: Strings must use singlequote.
(quotes)
[error] 25-25: Unnecessarily quoted property 'rotateWhenInvalid' found.
(quote-props)
[error] 25-25: Strings must use singlequote.
(quotes)
[error] 26-26: Unnecessarily quoted property 'useSession' found.
(quote-props)
[error] 26-26: Strings must use singlequote.
(quotes)
[error] 27-38: Unnecessarily quoted property 'supportedRequests' found.
(quote-props)
[error] 27-27: Strings must use singlequote.
(quotes)
[error] 29-29: Unnecessarily quoted property 'path' found.
(quote-props)
[error] 29-29: Strings must use singlequote.
(quotes)
[error] 30-36: Unnecessarily quoted property 'methods' found.
(quote-props)
[error] 30-30: Strings must use singlequote.
(quotes)
[error] 31-31: Strings must use singlequote.
(quotes)
[error] 32-32: Strings must use singlequote.
(quotes)
[error] 33-33: Strings must use singlequote.
(quotes)
[error] 34-34: Strings must use singlequote.
(quotes)
[error] 35-35: Strings must use singlequote.
(quotes)
[error] 35-36: Missing trailing comma.
(comma-dangle)
[error] 36-37: Missing trailing comma.
(comma-dangle)
[error] 37-38: Missing trailing comma.
(comma-dangle)
[error] 39-39: Unnecessarily quoted property 'refererWhiteList' found.
(quote-props)
[error] 39-39: Strings must use singlequote.
(quotes)
[error] 40-44: Unnecessarily quoted property 'cookieOptions' found.
(quote-props)
[error] 40-40: Strings must use singlequote.
(quotes)
[error] 41-41: Unnecessarily quoted property 'signed' found.
(quote-props)
[error] 41-41: Strings must use singlequote.
(quotes)
[error] 42-42: Unnecessarily quoted property 'httpOnly' found.
(quote-props)
[error] 42-42: Strings must use singlequote.
(quotes)
[error] 43-43: Unnecessarily quoted property 'overwrite' found.
(quote-props)
[error] 43-43: Strings must use singlequote.
(quotes)
[error] 43-44: Missing trailing comma.
(comma-dangle)
[error] 44-45: Missing trailing comma.
(comma-dangle)
[error] 46-49: Unnecessarily quoted property 'xframe' found.
(quote-props)
[error] 46-46: Strings must use singlequote.
(quotes)
[error] 47-47: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 47-47: Strings must use singlequote.
(quotes)
[error] 48-48: Unnecessarily quoted property 'value' found.
(quote-props)
[error] 48-48: Strings must use singlequote.
(quotes)
[error] 48-48: Strings must use singlequote.
(quotes)
[error] 48-49: Missing trailing comma.
(comma-dangle)
[error] 50-54: Unnecessarily quoted property 'hsts' found.
(quote-props)
[error] 50-50: Strings must use singlequote.
(quotes)
[error] 51-51: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 51-51: Strings must use singlequote.
(quotes)
[error] 52-52: Unnecessarily quoted property 'maxAge' found.
(quote-props)
[error] 52-52: Strings must use singlequote.
(quotes)
[error] 53-53: Unnecessarily quoted property 'includeSubdomains' found.
(quote-props)
[error] 53-53: Strings must use singlequote.
(quotes)
[error] 53-54: Missing trailing comma.
(comma-dangle)
[error] 55-57: Unnecessarily quoted property 'methodnoallow' found.
(quote-props)
[error] 55-55: Strings must use singlequote.
(quotes)
[error] 56-56: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 56-56: Strings must use singlequote.
(quotes)
[error] 56-57: Missing trailing comma.
(comma-dangle)
[error] 58-60: Unnecessarily quoted property 'noopen' found.
(quote-props)
[error] 58-58: Strings must use singlequote.
(quotes)
[error] 59-59: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 59-59: Strings must use singlequote.
(quotes)
[error] 59-60: Missing trailing comma.
(comma-dangle)
[error] 61-63: Unnecessarily quoted property 'nosniff' found.
(quote-props)
[error] 61-61: Strings must use singlequote.
(quotes)
[error] 62-62: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 62-62: Strings must use singlequote.
(quotes)
[error] 62-63: Missing trailing comma.
(comma-dangle)
[error] 64-67: Unnecessarily quoted property 'xssProtection' found.
(quote-props)
[error] 64-64: Strings must use singlequote.
(quotes)
[error] 65-65: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 65-65: Strings must use singlequote.
(quotes)
[error] 66-66: Unnecessarily quoted property 'value' found.
(quote-props)
[error] 66-66: Strings must use singlequote.
(quotes)
[error] 66-66: Strings must use singlequote.
(quotes)
[error] 66-67: Missing trailing comma.
(comma-dangle)
[error] 68-71: Unnecessarily quoted property 'csp' found.
(quote-props)
[error] 68-68: Strings must use singlequote.
(quotes)
[error] 69-69: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 69-69: Strings must use singlequote.
(quotes)
[error] 70-70: Unnecessarily quoted property 'policy' found.
(quote-props)
[error] 70-70: Strings must use singlequote.
(quotes)
[error] 70-71: Missing trailing comma.
(comma-dangle)
[error] 72-75: Unnecessarily quoted property 'referrerPolicy' found.
(quote-props)
[error] 72-72: Strings must use singlequote.
(quotes)
[error] 73-73: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 73-73: Strings must use singlequote.
(quotes)
[error] 74-74: Unnecessarily quoted property 'value' found.
(quote-props)
[error] 74-74: Strings must use singlequote.
(quotes)
[error] 74-74: Strings must use singlequote.
(quotes)
[error] 74-75: Missing trailing comma.
(comma-dangle)
[error] 76-78: Unnecessarily quoted property 'dta' found.
(quote-props)
[error] 76-76: Strings must use singlequote.
(quotes)
[error] 77-77: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 77-77: Strings must use singlequote.
(quotes)
[error] 77-78: Missing trailing comma.
(comma-dangle)
[error] 79-79: Unnecessarily quoted property 'ssrf' found.
(quote-props)
[error] 79-79: Strings must use singlequote.
(quotes)
[error] 79-80: Missing trailing comma.
(comma-dangle)
[error] 81-83: Unnecessarily quoted property 'helper' found.
(quote-props)
[error] 81-81: Strings must use singlequote.
(quotes)
[error] 82-82: Unnecessarily quoted property 'shtml' found.
(quote-props)
[error] 82-82: Strings must use singlequote.
(quotes)
[error] 82-83: Missing trailing comma.
(comma-dangle)
[error] 83-84: Missing trailing comma.
(comma-dangle)
__snapshots__/csp.test.ts.js
[error] 2-2: Unnecessarily quoted property 'domainWhiteList' found.
(quote-props)
[error] 2-2: Strings must use singlequote.
(quotes)
[error] 3-3: Unnecessarily quoted property 'protocolWhiteList' found.
(quote-props)
[error] 3-3: Strings must use singlequote.
(quotes)
[error] 4-4: Unnecessarily quoted property 'defaultMiddleware' found.
(quote-props)
[error] 4-4: Strings must use singlequote.
(quotes)
[error] 4-4: Strings must use singlequote.
(quotes)
[error] 5-34: Unnecessarily quoted property 'csrf' found.
(quote-props)
[error] 5-5: Strings must use singlequote.
(quotes)
[error] 6-6: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 6-6: Strings must use singlequote.
(quotes)
[error] 7-7: Unnecessarily quoted property 'type' found.
(quote-props)
[error] 7-7: Strings must use singlequote.
(quotes)
[error] 7-7: Strings must use singlequote.
(quotes)
[error] 8-8: Unnecessarily quoted property 'ignoreJSON' found.
(quote-props)
[error] 8-8: Strings must use singlequote.
(quotes)
[error] 9-9: Unnecessarily quoted property 'cookieName' found.
(quote-props)
[error] 9-9: Strings must use singlequote.
(quotes)
[error] 9-9: Strings must use singlequote.
(quotes)
[error] 10-10: Unnecessarily quoted property 'sessionName' found.
(quote-props)
[error] 10-10: Strings must use singlequote.
(quotes)
[error] 10-10: Strings must use singlequote.
(quotes)
[error] 11-11: Unnecessarily quoted property 'headerName' found.
(quote-props)
[error] 11-11: Strings must use singlequote.
(quotes)
[error] 11-11: Strings must use singlequote.
(quotes)
[error] 12-12: Unnecessarily quoted property 'bodyName' found.
(quote-props)
[error] 12-12: Strings must use singlequote.
(quotes)
[error] 12-12: Strings must use singlequote.
(quotes)
[error] 13-13: Unnecessarily quoted property 'queryName' found.
(quote-props)
[error] 13-13: Strings must use singlequote.
(quotes)
[error] 13-13: Strings must use singlequote.
(quotes)
[error] 14-14: Unnecessarily quoted property 'rotateWhenInvalid' found.
(quote-props)
[error] 14-14: Strings must use singlequote.
(quotes)
[error] 15-15: Unnecessarily quoted property 'useSession' found.
(quote-props)
[error] 15-15: Strings must use singlequote.
(quotes)
[error] 16-27: Unnecessarily quoted property 'supportedRequests' found.
(quote-props)
[error] 16-16: Strings must use singlequote.
(quotes)
[error] 18-18: Unnecessarily quoted property 'path' found.
(quote-props)
[error] 18-18: Strings must use singlequote.
(quotes)
[error] 19-25: Unnecessarily quoted property 'methods' found.
(quote-props)
[error] 19-19: Strings must use singlequote.
(quotes)
[error] 20-20: Strings must use singlequote.
(quotes)
[error] 21-21: Strings must use singlequote.
(quotes)
[error] 22-22: Strings must use singlequote.
(quotes)
[error] 23-23: Strings must use singlequote.
(quotes)
[error] 24-24: Strings must use singlequote.
(quotes)
[error] 24-25: Missing trailing comma.
(comma-dangle)
[error] 25-26: Missing trailing comma.
(comma-dangle)
[error] 26-27: Missing trailing comma.
(comma-dangle)
[error] 28-28: Unnecessarily quoted property 'refererWhiteList' found.
(quote-props)
[error] 28-28: Strings must use singlequote.
(quotes)
[error] 29-33: Unnecessarily quoted property 'cookieOptions' found.
(quote-props)
[error] 29-29: Strings must use singlequote.
(quotes)
[error] 30-30: Unnecessarily quoted property 'signed' found.
(quote-props)
[error] 30-30: Strings must use singlequote.
(quotes)
[error] 31-31: Unnecessarily quoted property 'httpOnly' found.
(quote-props)
[error] 31-31: Strings must use singlequote.
(quotes)
[error] 32-32: Unnecessarily quoted property 'overwrite' found.
(quote-props)
[error] 32-32: Strings must use singlequote.
(quotes)
[error] 32-33: Missing trailing comma.
(comma-dangle)
[error] 33-34: Missing trailing comma.
(comma-dangle)
[error] 35-38: Unnecessarily quoted property 'xframe' found.
(quote-props)
[error] 35-35: Strings must use singlequote.
(quotes)
[error] 36-36: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 36-36: Strings must use singlequote.
(quotes)
[error] 37-37: Unnecessarily quoted property 'value' found.
(quote-props)
[error] 37-37: Strings must use singlequote.
(quotes)
[error] 37-37: Strings must use singlequote.
(quotes)
[error] 37-38: Missing trailing comma.
(comma-dangle)
[error] 39-43: Unnecessarily quoted property 'hsts' found.
(quote-props)
[error] 39-39: Strings must use singlequote.
(quotes)
[error] 40-40: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 40-40: Strings must use singlequote.
(quotes)
[error] 41-41: Unnecessarily quoted property 'maxAge' found.
(quote-props)
[error] 41-41: Strings must use singlequote.
(quotes)
[error] 42-42: Unnecessarily quoted property 'includeSubdomains' found.
(quote-props)
[error] 42-42: Strings must use singlequote.
(quotes)
[error] 42-43: Missing trailing comma.
(comma-dangle)
[error] 44-46: Unnecessarily quoted property 'methodnoallow' found.
(quote-props)
[error] 44-44: Strings must use singlequote.
(quotes)
[error] 45-45: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 45-45: Strings must use singlequote.
(quotes)
[error] 45-46: Missing trailing comma.
(comma-dangle)
[error] 47-49: Unnecessarily quoted property 'noopen' found.
(quote-props)
[error] 47-47: Strings must use singlequote.
(quotes)
[error] 48-48: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 48-48: Strings must use singlequote.
(quotes)
[error] 48-49: Missing trailing comma.
(comma-dangle)
[error] 50-52: Unnecessarily quoted property 'nosniff' found.
(quote-props)
[error] 50-50: Strings must use singlequote.
(quotes)
[error] 51-51: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 51-51: Strings must use singlequote.
(quotes)
[error] 51-52: Missing trailing comma.
(comma-dangle)
[error] 53-56: Unnecessarily quoted property 'xssProtection' found.
(quote-props)
[error] 53-53: Strings must use singlequote.
(quotes)
[error] 54-54: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 54-54: Strings must use singlequote.
(quotes)
[error] 55-55: Unnecessarily quoted property 'value' found.
(quote-props)
[error] 55-55: Strings must use singlequote.
(quotes)
[error] 55-55: Strings must use singlequote.
(quotes)
[error] 55-56: Missing trailing comma.
(comma-dangle)
[error] 57-81: Unnecessarily quoted property 'csp' found.
(quote-props)
[error] 57-57: Strings must use singlequote.
(quotes)
[error] 58-58: Unnecessarily quoted property 'ignore' found.
(quote-props)
[error] 58-58: Strings must use singlequote.
(quotes)
[error] 58-58: Strings must use singlequote.
(quotes)
[error] 59-59: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 59-59: Strings must use singlequote.
(quotes)
[error] 60-80: Unnecessarily quoted property 'policy' found.
(quote-props)
[error] 60-60: Strings must use singlequote.
(quotes)
[error] 61-61: Strings must use singlequote.
(quotes)
[error] 65-65: Strings must use singlequote.
(quotes)
[error] 65-66: Missing trailing comma.
(comma-dangle)
[error] 67-67: Strings must use singlequote.
(quotes)
[error] 69-69: Strings must use singlequote.
(quotes)
[error] 69-70: Missing trailing comma.
(comma-dangle)
[error] 71-71: Strings must use singlequote.
(quotes)
[error] 73-73: Strings must use singlequote.
(quotes)
[error] 74-74: Strings must use singlequote.
(quotes)
[error] 74-75: Missing trailing comma.
(comma-dangle)
[error] 76-76: Strings must use singlequote.
(quotes)
[error] 77-78: Missing trailing comma.
(comma-dangle)
[error] 79-79: Strings must use singlequote.
(quotes)
[error] 79-79: Strings must use singlequote.
(quotes)
[error] 79-80: Missing trailing comma.
(comma-dangle)
[error] 80-81: Missing trailing comma.
(comma-dangle)
[error] 82-85: Unnecessarily quoted property 'referrerPolicy' found.
(quote-props)
[error] 82-82: Strings must use singlequote.
(quotes)
[error] 83-83: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 83-83: Strings must use singlequote.
(quotes)
[error] 84-84: Unnecessarily quoted property 'value' found.
(quote-props)
[error] 84-84: Strings must use singlequote.
(quotes)
[error] 84-84: Strings must use singlequote.
(quotes)
[error] 84-85: Missing trailing comma.
(comma-dangle)
[error] 86-88: Unnecessarily quoted property 'dta' found.
(quote-props)
[error] 86-86: Strings must use singlequote.
(quotes)
[error] 87-87: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 87-87: Strings must use singlequote.
(quotes)
[error] 87-88: Missing trailing comma.
(comma-dangle)
[error] 89-89: Unnecessarily quoted property 'ssrf' found.
(quote-props)
[error] 89-89: Strings must use singlequote.
(quotes)
[error] 89-90: Missing trailing comma.
(comma-dangle)
🪛 Biome (1.9.4)
src/lib/extend/safe_curl.ts
[error] 20-20: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
src/app.ts
[error] 17-17: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
src/lib/extend/safe_curl.ts (1)
18-40: Verify error handling for invalid URLs.The function should validate the URL parameter before making the request to prevent potential security issues.
Let's check if there are any URL validation utilities in the codebase:
Also, let's verify if there are any existing test cases for invalid URLs:
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
package.json (1)
87-87: Verify compatibility with "type": "module".Setting
"type": "module"affects how Node.js treats.jsfiles (as ESM by default). This might break existing CommonJS imports in the ecosystem.Run this script to check for potential compatibility issues:
✅ Verification successful
"type": "module" is correctly configured
The codebase is properly set up for ESM:
- Source code is TypeScript using ESM imports
- Import paths already use .js extensions
- CommonJS usage is properly isolated to test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for .js files that might need to be .cjs or .mjs echo "Checking for .js files that might need extension changes..." fd -e js --exclude dist --exclude node_modules # Check for require() usage that might break echo -e "\nChecking for require() usage..." rg "require\(['\"]" --type js --exclude-dir={dist,node_modules}Length of output: 8574
Script:
#!/bin/bash # Check for .js files outside test directories echo "Checking for .js files outside test directories..." fd -e js --exclude test --exclude dist --exclude node_modules --exclude __snapshots__ # Check for require() usage with correct syntax echo -e "\nChecking for require() usage..." rg "require\(" --type js # Check for package.json files in test directories echo -e "\nChecking for package.json files in test directories..." fd package.json -p test/Length of output: 6221
Script:
#!/bin/bash # Check for TypeScript source files echo "Checking for TypeScript source files..." fd -e ts --exclude test --exclude dist --exclude node_modules --exclude "*.d.ts" # Check for any ESM imports in TypeScript files echo -e "\nChecking for ESM imports..." rg "^import .* from ['\"]" --type tsLength of output: 9309
test/config/config.default.test.ts (1)
1-8: LGTM! Good use of snapshot testing.The snapshot test effectively ensures stability of default configuration values, which is crucial for maintaining backward compatibility.
Let's verify that the snapshot test is running in the CI pipeline:
✅ Verification successful
Snapshot tests are included in CI pipeline ✅
The project uses a standardized Node.js test workflow that runs all tests, including snapshot tests, as part of the CI pipeline through
.github/workflows/nodejs.yml.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if snapshot tests are included in CI workflow gh api repos/eggjs/security/contents/.github/workflows -q '.[].name'Length of output: 108
Script:
#!/bin/bash # Check the content of nodejs.yml workflow file gh api repos/eggjs/security/contents/.github/workflows/nodejs.yml | jq -r '.content' | base64 -dLength of output: 193
Script:
#!/bin/bash # Search for test-related patterns in workflow files rg -i "test|npm run|yarn" .github/workflows/Length of output: 294
__snapshots__/config.default.test.ts.js (1)
79-79:⚠️ Potential issueSecurity Risk: CSP report-uri uses HTTP.
The CSP report-uri endpoint uses HTTP, which could expose violation reports to tampering.
Apply this diff:
- "report-uri": "http://pointman.domain.com/csp?app=csp" + "report-uri": "https://pointman.domain.com/csp?app=csp"Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 79-79: Unnecessarily quoted property 'ssrf' found.
(quote-props)
[error] 79-79: Strings must use singlequote.
(quotes)
[error] 79-80: Missing trailing comma.
(comma-dangle)
__snapshots__/csp.test.ts.js (1)
1-90: Security configuration issues already addressed.The security concerns in this test snapshot are identical to those identified in the default configuration snapshot.
🧰 Tools
🪛 eslint
[error] 2-2: Unnecessarily quoted property 'domainWhiteList' found.
(quote-props)
[error] 2-2: Strings must use singlequote.
(quotes)
[error] 3-3: Unnecessarily quoted property 'protocolWhiteList' found.
(quote-props)
[error] 3-3: Strings must use singlequote.
(quotes)
[error] 4-4: Unnecessarily quoted property 'defaultMiddleware' found.
(quote-props)
[error] 4-4: Strings must use singlequote.
(quotes)
[error] 4-4: Strings must use singlequote.
(quotes)
[error] 5-34: Unnecessarily quoted property 'csrf' found.
(quote-props)
[error] 5-5: Strings must use singlequote.
(quotes)
[error] 6-6: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 6-6: Strings must use singlequote.
(quotes)
[error] 7-7: Unnecessarily quoted property 'type' found.
(quote-props)
[error] 7-7: Strings must use singlequote.
(quotes)
[error] 7-7: Strings must use singlequote.
(quotes)
[error] 8-8: Unnecessarily quoted property 'ignoreJSON' found.
(quote-props)
[error] 8-8: Strings must use singlequote.
(quotes)
[error] 9-9: Unnecessarily quoted property 'cookieName' found.
(quote-props)
[error] 9-9: Strings must use singlequote.
(quotes)
[error] 9-9: Strings must use singlequote.
(quotes)
[error] 10-10: Unnecessarily quoted property 'sessionName' found.
(quote-props)
[error] 10-10: Strings must use singlequote.
(quotes)
[error] 10-10: Strings must use singlequote.
(quotes)
[error] 11-11: Unnecessarily quoted property 'headerName' found.
(quote-props)
[error] 11-11: Strings must use singlequote.
(quotes)
[error] 11-11: Strings must use singlequote.
(quotes)
[error] 12-12: Unnecessarily quoted property 'bodyName' found.
(quote-props)
[error] 12-12: Strings must use singlequote.
(quotes)
[error] 12-12: Strings must use singlequote.
(quotes)
[error] 13-13: Unnecessarily quoted property 'queryName' found.
(quote-props)
[error] 13-13: Strings must use singlequote.
(quotes)
[error] 13-13: Strings must use singlequote.
(quotes)
[error] 14-14: Unnecessarily quoted property 'rotateWhenInvalid' found.
(quote-props)
[error] 14-14: Strings must use singlequote.
(quotes)
[error] 15-15: Unnecessarily quoted property 'useSession' found.
(quote-props)
[error] 15-15: Strings must use singlequote.
(quotes)
[error] 16-27: Unnecessarily quoted property 'supportedRequests' found.
(quote-props)
[error] 16-16: Strings must use singlequote.
(quotes)
[error] 18-18: Unnecessarily quoted property 'path' found.
(quote-props)
[error] 18-18: Strings must use singlequote.
(quotes)
[error] 19-25: Unnecessarily quoted property 'methods' found.
(quote-props)
[error] 19-19: Strings must use singlequote.
(quotes)
[error] 20-20: Strings must use singlequote.
(quotes)
[error] 21-21: Strings must use singlequote.
(quotes)
[error] 22-22: Strings must use singlequote.
(quotes)
[error] 23-23: Strings must use singlequote.
(quotes)
[error] 24-24: Strings must use singlequote.
(quotes)
[error] 24-25: Missing trailing comma.
(comma-dangle)
[error] 25-26: Missing trailing comma.
(comma-dangle)
[error] 26-27: Missing trailing comma.
(comma-dangle)
[error] 28-28: Unnecessarily quoted property 'refererWhiteList' found.
(quote-props)
[error] 28-28: Strings must use singlequote.
(quotes)
[error] 29-33: Unnecessarily quoted property 'cookieOptions' found.
(quote-props)
[error] 29-29: Strings must use singlequote.
(quotes)
[error] 30-30: Unnecessarily quoted property 'signed' found.
(quote-props)
[error] 30-30: Strings must use singlequote.
(quotes)
[error] 31-31: Unnecessarily quoted property 'httpOnly' found.
(quote-props)
[error] 31-31: Strings must use singlequote.
(quotes)
[error] 32-32: Unnecessarily quoted property 'overwrite' found.
(quote-props)
[error] 32-32: Strings must use singlequote.
(quotes)
[error] 32-33: Missing trailing comma.
(comma-dangle)
[error] 33-34: Missing trailing comma.
(comma-dangle)
[error] 35-38: Unnecessarily quoted property 'xframe' found.
(quote-props)
[error] 35-35: Strings must use singlequote.
(quotes)
[error] 36-36: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 36-36: Strings must use singlequote.
(quotes)
[error] 37-37: Unnecessarily quoted property 'value' found.
(quote-props)
[error] 37-37: Strings must use singlequote.
(quotes)
[error] 37-37: Strings must use singlequote.
(quotes)
[error] 37-38: Missing trailing comma.
(comma-dangle)
[error] 39-43: Unnecessarily quoted property 'hsts' found.
(quote-props)
[error] 39-39: Strings must use singlequote.
(quotes)
[error] 40-40: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 40-40: Strings must use singlequote.
(quotes)
[error] 41-41: Unnecessarily quoted property 'maxAge' found.
(quote-props)
[error] 41-41: Strings must use singlequote.
(quotes)
[error] 42-42: Unnecessarily quoted property 'includeSubdomains' found.
(quote-props)
[error] 42-42: Strings must use singlequote.
(quotes)
[error] 42-43: Missing trailing comma.
(comma-dangle)
[error] 44-46: Unnecessarily quoted property 'methodnoallow' found.
(quote-props)
[error] 44-44: Strings must use singlequote.
(quotes)
[error] 45-45: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 45-45: Strings must use singlequote.
(quotes)
[error] 45-46: Missing trailing comma.
(comma-dangle)
[error] 47-49: Unnecessarily quoted property 'noopen' found.
(quote-props)
[error] 47-47: Strings must use singlequote.
(quotes)
[error] 48-48: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 48-48: Strings must use singlequote.
(quotes)
[error] 48-49: Missing trailing comma.
(comma-dangle)
[error] 50-52: Unnecessarily quoted property 'nosniff' found.
(quote-props)
[error] 50-50: Strings must use singlequote.
(quotes)
[error] 51-51: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 51-51: Strings must use singlequote.
(quotes)
[error] 51-52: Missing trailing comma.
(comma-dangle)
[error] 53-56: Unnecessarily quoted property 'xssProtection' found.
(quote-props)
[error] 53-53: Strings must use singlequote.
(quotes)
[error] 54-54: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 54-54: Strings must use singlequote.
(quotes)
[error] 55-55: Unnecessarily quoted property 'value' found.
(quote-props)
[error] 55-55: Strings must use singlequote.
(quotes)
[error] 55-55: Strings must use singlequote.
(quotes)
[error] 55-56: Missing trailing comma.
(comma-dangle)
[error] 57-81: Unnecessarily quoted property 'csp' found.
(quote-props)
[error] 57-57: Strings must use singlequote.
(quotes)
[error] 58-58: Unnecessarily quoted property 'ignore' found.
(quote-props)
[error] 58-58: Strings must use singlequote.
(quotes)
[error] 58-58: Strings must use singlequote.
(quotes)
[error] 59-59: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 59-59: Strings must use singlequote.
(quotes)
[error] 60-80: Unnecessarily quoted property 'policy' found.
(quote-props)
[error] 60-60: Strings must use singlequote.
(quotes)
[error] 61-61: Strings must use singlequote.
(quotes)
[error] 65-65: Strings must use singlequote.
(quotes)
[error] 65-66: Missing trailing comma.
(comma-dangle)
[error] 67-67: Strings must use singlequote.
(quotes)
[error] 69-69: Strings must use singlequote.
(quotes)
[error] 69-70: Missing trailing comma.
(comma-dangle)
[error] 71-71: Strings must use singlequote.
(quotes)
[error] 73-73: Strings must use singlequote.
(quotes)
[error] 74-74: Strings must use singlequote.
(quotes)
[error] 74-75: Missing trailing comma.
(comma-dangle)
[error] 76-76: Strings must use singlequote.
(quotes)
[error] 77-78: Missing trailing comma.
(comma-dangle)
[error] 79-79: Strings must use singlequote.
(quotes)
[error] 79-79: Strings must use singlequote.
(quotes)
[error] 79-80: Missing trailing comma.
(comma-dangle)
[error] 80-81: Missing trailing comma.
(comma-dangle)
[error] 82-85: Unnecessarily quoted property 'referrerPolicy' found.
(quote-props)
[error] 82-82: Strings must use singlequote.
(quotes)
[error] 83-83: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 83-83: Strings must use singlequote.
(quotes)
[error] 84-84: Unnecessarily quoted property 'value' found.
(quote-props)
[error] 84-84: Strings must use singlequote.
(quotes)
[error] 84-84: Strings must use singlequote.
(quotes)
[error] 84-85: Missing trailing comma.
(comma-dangle)
[error] 86-88: Unnecessarily quoted property 'dta' found.
(quote-props)
[error] 86-86: Strings must use singlequote.
(quotes)
[error] 87-87: Unnecessarily quoted property 'enable' found.
(quote-props)
[error] 87-87: Strings must use singlequote.
(quotes)
[error] 87-88: Missing trailing comma.
(comma-dangle)
[error] 89-89: Unnecessarily quoted property 'ssrf' found.
(quote-props)
[error] 89-89: Strings must use singlequote.
(quotes)
[error] 89-90: Missing trailing comma.
(comma-dangle)
src/app.ts (1)
13-31: LGTM! Good security configuration validation.The implementation properly validates CSRF configuration and ensures type safety through
SecurityConfig.parse().🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/app/middleware/securities.ts (2)
1-7: LGTM! Proper ES module imports.The imports are well-structured and use the new ES module syntax, aligning with the PR objective.
60-62: LGTM! Proper middleware composition.The middleware composition is well-implemented with appropriate logging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
package.json (1)
13-17:⚠️ Potential issueCritical: Remove misplaced exports field from eggPlugin section.
The
exportsfield is incorrectly placed inside theeggPluginsection. This should be at the root level of package.json as it's a Node.js package entry point configuration, not an Egg.js plugin configuration.
🧹 Nitpick comments (1)
__snapshots__/context.test.ts.js (1)
44-48: Consider enabling HSTS by default.HSTS is currently disabled by default. For production environments, it's recommended to enable HSTS to ensure secure communication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.eslintignore(1 hunks)__snapshots__/context.test.ts.js(1 hunks)__snapshots__/csp.test.ts.js(1 hunks)package.json(1 hunks)test/context.test.ts(1 hunks)test/csp.test.ts(3 hunks)test/fixtures/apps/csp-ignore/app/router.js(1 hunks)test/fixtures/apps/csp-ignore/config/config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .eslintignore
- test/context.test.ts
- snapshots/csp.test.ts.js
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
🔇 Additional comments (12)
test/fixtures/apps/csp-ignore/app/router.js (1)
8-10: LGTM! Test route added for CSP ignore pattern testing.The new route follows the same pattern as the existing
/api/updateroute and serves its purpose for testing CSP ignore functionality with regex patterns.test/fixtures/apps/csp-ignore/config/config.js (1)
7-7: LGTM! Enhanced CSP ignore configuration with regex support.The updated configuration properly supports both string and regex-based ignore patterns, allowing for more flexible CSP rule definitions.
__snapshots__/context.test.ts.js (2)
34-38: Review CSRF cookie security settings.The CSRF cookie configuration has
httpOnly: false, which could expose the token to XSS attacks. Consider enablinghttpOnlyfor better security.
2-7: Review domain whitelist patterns.The domain whitelist includes broad patterns like
"192.*.0.*"and"*.alibaba.com". Consider:
- Using more specific IP ranges
- Limiting wildcard domain patterns
Run this script to check for potential security implications:
✅ Verification successful
Test fixtures with broad patterns are acceptable for testing purposes
The patterns are located in test snapshots and fixtures, used to verify the security implementation's pattern matching capabilities. The actual implementation properly validates and restricts domains/IPs based on these patterns. No changes needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for domain whitelist usage across the codebase rg -A 5 "domainWhiteList|protocolWhiteList" --type ts --type jsLength of output: 14956
test/csp.test.ts (3)
91-92: Add null check for regex match.The code uses a non-null assertion operator (
!) which could lead to runtime errors if the regex doesn't match.
29-34: LGTM! Proper resource cleanup.The
afterhook ensures proper cleanup of all test applications.
112-117: LGTM! Test coverage for regex-based ignore paths.The new test case properly validates the CSP ignore functionality with regex patterns.
package.json (5)
2-6: LGTM! Package metadata is well configured.The package renaming to
@eggjs/securitywith public access configuration aligns with modern npm scoped package practices.
88-113: LGTM! Dual package configuration is well structured.The module configuration correctly handles:
- Dual package hazard with conditional exports
- Type declarations for both CJS and ESM
- Package entry points properly defined
35-37: Verify Node.js version compatibility.The minimum Node.js version requirement has been updated to 18.19.0. This is a breaking change that needs verification.
Let's verify the Node.js version compatibility:
✅ Verification successful
Node.js version requirement is appropriate
The codebase doesn't use any features specific to Node.js 18.19.0+. The version requirement appears to be a forward-looking choice for maintenance and security purposes rather than being driven by specific feature dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any code uses features only available in Node.js 18.19.0+ rg -l 'fetch|structuredClone|.findLast|.findLastIndex|.toReversed|.toSorted|.toSpliced|.with' src/Length of output: 233
Script:
#!/bin/bash # Check the actual implementation context echo "=== Checking src/lib/utils.ts ===" rg -C 3 'fetch|structuredClone|.findLast|.findLastIndex|.toReversed|.toSorted|.toSpliced|.with' src/lib/utils.ts echo -e "\n=== Checking src/lib/middlewares/methodnoallow.ts ===" rg -C 3 'fetch|structuredClone|.findLast|.findLastIndex|.toReversed|.toSorted|.toSpliced|.with' src/lib/middlewares/methodnoallow.ts echo -e "\n=== Checking src/lib/helper/spath.ts ===" rg -C 3 'fetch|structuredClone|.findLast|.findLastIndex|.toReversed|.toSorted|.toSpliced|.with' src/lib/helper/spath.ts echo -e "\n=== Checking src/lib/extend/safe_curl.ts ===" rg -C 3 'fetch|structuredClone|.findLast|.findLastIndex|.toReversed|.toSorted|.toSpliced|.with' src/lib/extend/safe_curl.ts echo -e "\n=== Checking src/app/extend/response.ts ===" rg -C 3 'fetch|structuredClone|.findLast|.findLastIndex|.toReversed|.toSorted|.toSpliced|.with' src/app/extend/response.tsLength of output: 3055
39-50: Verify dependency version compatibility.Several dependencies have been updated. Let's ensure these versions are compatible and don't have known vulnerabilities.
Run security audit on dependencies:
✅ Verification successful
Dependencies are secure and compatible
All dependencies are using appropriate version ranges and are free from known vulnerabilities. The nanoid package (^3.3.8) is at a secure version, and both csrf (^3.0.6) and xss (^1.0.3) have no reported vulnerabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known vulnerabilities in dependencies gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: NPM, package: "nanoid") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } } ' # Check if nanoid version is compatible with Node.js 18 rg -l "require.*nanoid" src/Length of output: 910
Script:
#!/bin/bash # Check for vulnerabilities in other critical dependencies gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: NPM, package: "xss") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } } ' # Check usage patterns ast-grep --pattern 'require("nanoid")' ast-grep --pattern 'import { $_ } from "nanoid"' # Check package-lock.json for exact versions fd -t f "package-lock.json" --exec cat {}Length of output: 418
Script:
#!/bin/bash # Check for vulnerabilities in csrf package gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: NPM, package: "csrf") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } } ' # Check for any alternative import patterns of nanoid rg -l "nanoid" --type js --type tsLength of output: 344
78-86: Verify the TypeScript build configuration.The new build system using tshy looks good, but let's ensure all necessary build steps are covered:
- Linting is properly configured
- Clean step is included
- Test coverage is maintained
Let's verify the TypeScript configuration:
✅ Verification successful
Build configuration is properly set up and complete ✓
The TypeScript build configuration includes all necessary components:
- TypeScript configuration is strict and uses modern settings
- Build process is properly handled by tshy
- Linting is configured with ESLint
- Test coverage is maintained with egg-bin
- Clean steps are properly integrated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if tsconfig.json exists and is properly configured cat tsconfig.json 2>/dev/null || echo "tsconfig.json missing" # Verify if all TypeScript files are included in the build fd -e ts -e tsx . src/ | wc -lLength of output: 250
Script:
#!/bin/bash # Check tshy configuration in package.json jq '.tshy' package.json # Check ESLint configuration cat .eslintrc.* 2>/dev/null || echo "No .eslintrc.* found" # List test files to verify TypeScript testing setup fd -e ts -e tsx . test/ | sortLength of output: 587
Script:
#!/bin/bash # Check if ESLint config is in package.json jq '.eslintConfig' package.json # Verify if there's any other ESLint config file fd -g '.eslintrc*'Length of output: 265
Script:
#!/bin/bash # Check for hidden ESLint config files fd -H -g '.eslintrc*' # Double check package.json for any ESLint related fields jq '.eslintrc, .eslint' package.jsonLength of output: 82
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/config/config.default.ts (2)
183-191:⚠️ Potential issueSecurity: Enable httpOnly for CSRF cookie.
The CSRF token is transmitted via forms, headers, and request parameters. No client-side JavaScript access to the cookie is required, so
httpOnly: trueshould be set for enhanced XSS protection.cookieOptions: z.object({ signed: z.boolean(), httpOnly: z.boolean(), overwrite: z.boolean(), }).default({ signed: false, - httpOnly: false, + httpOnly: true, overwrite: true, }),
335-340: 🛠️ Refactor suggestionAdd secure defaults for SSRF protection.
Consider providing default values for IP blacklist to enhance security out of the box.
ssrf: z.object({ - ipBlackList: z.array(z.string()).optional(), + ipBlackList: z.array(z.string()).default([ + '10.0.0.0/8', + '172.16.0.0/12', + '192.168.0.0/16', + '127.0.0.0/8', + ]), ipExceptionList: z.array(z.string()).optional(), hostnameExceptionList: z.array(z.string()).optional(), checkAddress: SSRFCheckAddressFunction.optional(), }).default({}),package.json (1)
13-17:⚠️ Potential issueCritical: Remove misplaced exports field from eggPlugin section.
The
exportsfield is incorrectly placed inside theeggPluginsection. This should be at the root level of package.json as it's a Node.js package entry point configuration, not an Egg.js plugin configuration.
🧹 Nitpick comments (8)
test/fixtures/apps/csrf-supported-requests-default-config/app/controller/home.js (1)
1-7: Consider enhancing test coverage.The controller only exposes methods from the first supported request configuration. Consider adding test cases for:
- Multiple supported request configurations
- Empty supportedRequests array
- Missing methods property
Here's a suggested enhancement:
exports.index = ctx => { ctx.body = { csrf: ctx.csrf, env: ctx.app.config.env, - supportedRequestsMethods: ctx.app.config.security.csrf.supportedRequests[0].methods, + supportedRequestsMethods: ctx.app.config.security.csrf.supportedRequests[0]?.methods ?? [], + allSupportedRequests: ctx.app.config.security.csrf.supportedRequests, }; };test/csrf.test.ts (1)
830-836: Consider using an enum for HTTP methods.The HTTP methods are repeated as string literals throughout the tests. Consider defining an enum to maintain consistency and prevent typos.
enum HttpMethod { POST = 'POST', PATCH = 'PATCH', DELETE = 'DELETE', PUT = 'PUT', CONNECT = 'CONNECT', }src/app.ts (1)
18-23: Enhance deprecation message.The deprecation message could be more helpful by suggesting alternatives or explaining why
ignoreJSONis not safe.- app.deprecate('[@eggjs/security/app] `config.security.csrf.ignoreJSON` is not safe now, please disable it.'); + app.deprecate('[@eggjs/security/app] `config.security.csrf.ignoreJSON` is not safe as it may expose your application to CSRF attacks. Please disable it and implement proper CSRF protection for all endpoints.');src/lib/extend/safe_curl.ts (1)
19-24: Consider making SSRF configuration mandatory.The current implementation makes SSRF protection optional with a warning. Consider making it mandatory for better security.
src/config/config.default.ts (2)
15-23: Enhance type documentation for SSRFCheckAddressFunction.The function type signature could be more explicit about its parameters. Consider adding JSDoc comments to clarify the purpose of each parameter and their expected values.
const SSRFCheckAddressFunction = z.function() .args(z.union([ z.string(), LookupAddress, LookupAddressAndStringArray ]), z.union([ z.number(), z.string() ]), z.string()) .returns(z.boolean()); /** * SSRF check address function + * @param address - The IP address to check (string | LookupAddress | Array<string | LookupAddress>) + * @param family - The IP address family (4 or 6) + * @param hostname - The hostname being resolved + * @returns boolean - true if the address is allowed, false otherwise * `(address, family, hostname) => boolean` */ export type SSRFCheckAddressFunction = z.infer<typeof SSRFCheckAddressFunction>;
363-366: Enhance deprecation notice with migration instructions.The deprecation notice should include clear instructions on how to migrate to the new configuration.
/** * domain white list - * @deprecated use `config.security.domainWhiteList` instead + * @deprecated Please migrate to `config.security.domainWhiteList`. + * Example: + * ```ts + * { + * security: { + * domainWhiteList: ['example.com'] + * } + * } + * ``` */ domainWhiteList: z.array(z.string()).optional(),package.json (2)
39-50: Consider using caret ranges consistently for dependencies.Some dependencies use exact versions while others use caret ranges. For better maintenance:
- Consider using caret ranges (
^) consistently for all dependencies- Lock only the dependencies where specific versions are critical for security/compatibility
79-87: Consider adding type checking to the test pipeline.The test pipeline includes linting and tests but doesn't explicitly validate TypeScript types. Consider:
- Adding
tsc --noEmitto verify types- Running
attwduring tests to catch type compatibility issues early
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
__snapshots__/csrf.test.ts.js(1 hunks)package.json(1 hunks)src/app.ts(1 hunks)src/config/config.default.ts(1 hunks)src/lib/extend/safe_curl.ts(1 hunks)test/csrf.test.ts(17 hunks)test/fixtures/apps/csrf-supported-requests-default-config/app/controller/home.js(1 hunks)test/fixtures/apps/csrf-supported-requests-default-config/config/config.default.js(0 hunks)test/fixtures/apps/csrf/config/config.js(0 hunks)
💤 Files with no reviewable changes (2)
- test/fixtures/apps/csrf/config/config.js
- test/fixtures/apps/csrf-supported-requests-default-config/config/config.default.js
🔇 Additional comments (6)
test/fixtures/apps/csrf-supported-requests-default-config/app/controller/home.js (1)
5-5: Verify CSRF configuration structure change.The change from
supportedRequeststo accessingsupportedRequests[0].methodsindicates a structural change in the CSRF configuration. This could be a breaking change if other parts of the application expect the fullsupportedRequestsarray.Let's verify the CSRF configuration structure:
✅ Verification successful
Accessing first CSRF rule's methods is correct for default config test.
The implementation in the test fixture is intentionally accessing
supportedRequests[0].methodsas it's specifically testing the default CSRF configuration, which by default contains only one rule. The middleware correctly handles both single and multiple rules, so this is not a breaking change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of supportedRequests to ensure consistency rg -l "supportedRequests" --type js --type ts # Look for any test assertions that might be affected by this change rg -A 5 "supportedRequests.*assert" --type js --type tsLength of output: 519
Script:
#!/bin/bash # Check default configuration structure echo "=== Default Configuration ===" cat src/config/config.default.ts # Check middleware implementation echo -e "\n=== CSRF Middleware ===" cat src/lib/middlewares/csrf.ts # Check test fixture configuration echo -e "\n=== Test Fixture Configuration ===" cat test/fixtures/apps/csrf-supported-requests-default-config/config/config.jsLength of output: 11740
__snapshots__/csrf.test.ts.js (1)
2-5: Review the ignore configuration.The
ignorearray contains an empty object and null value, which seems unusual. Consider defining proper ignore rules or removing this if not needed.test/csrf.test.ts (1)
445-447: Uncomment TRACE method test.The TRACE method test is commented out. Either remove the commented code or implement the test to ensure complete HTTP method coverage.
src/config/config.default.ts (1)
375-378: LGTM!The default export correctly validates the configuration using Zod's parse method.
package.json (2)
2-3: Consider bumping to major version 4.0.0.This PR introduces breaking changes:
- Package renamed from
egg-securityto@eggjs/security- Dropped support for Node.js versions < 18.19.0
- Added dual CJS/ESM support
According to semver, these changes warrant a major version bump.
Also applies to: 35-37
89-114: Verify dual package hazard handling.The dual CJS/ESM configuration looks good, following Node.js best practices. To ensure compatibility:
- Verify that both CJS and ESM builds work in isolation
- Test the package with
arethetypeswrongto catch any dual package hazardsRun this verification script:
✅ Verification successful
Dual package hazard handling is properly configured ✅
The package configuration correctly handles dual package hazards with:
- Proper conditional exports for both CJS and ESM formats
- Format-specific type declarations
- Correct entry points and file structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dual package hazard handling # Check for dual package hazards attw --pack # Verify both CJS and ESM builds node --input-type=module -e "import('@eggjs/security')" || exit 1 node --input-type=commonjs -e "require('@eggjs/security')" || exit 1Length of output: 1231
Script:
#!/bin/bash # Verify source files and configuration structure # Check if index.ts exists in src echo "Checking source files..." fd -t f "index.ts$" src/ # Check for tsconfig files echo -e "\nChecking TypeScript configuration..." fd -t f "tsconfig.*json$" # Verify source directory structure matches exports echo -e "\nVerifying source structure..." fd -t f . src/Length of output: 1315
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (4)
package.json (1)
13-17:⚠️ Potential issueCritical: Remove misplaced exports field from eggPlugin section.
The
exportsfield is incorrectly placed inside theeggPluginsection. This should be at the root level of package.json as it's a Node.js package entry point configuration, not an Egg.js plugin configuration.src/lib/extend/safe_curl.ts (1)
23-32:⚠️ Potential issueAdd error handling for HTTP client creation.
The HTTP client creation should include error handling in case
createHttpClientfails.if (ssrfConfig?.checkAddress) { let httpClient = app[SSRF_HTTPCLIENT] as ReturnType<EggCore['createHttpClient']>; // use the new httpClient init with checkAddress if (!httpClient) { + try { httpClient = app[SSRF_HTTPCLIENT] = app.createHttpClient({ checkAddress: ssrfConfig.checkAddress, }); + } catch (error) { + app.logger.error('Failed to create HTTP client:', error); + throw new Error('Failed to initialize secure HTTP client'); + } } return await httpClient.request<T>(url, options); }src/app/extend/application.ts (1)
9-9:⚠️ Potential issueRemove redundant closing form tag.
The INPUT_CSRF constant includes an unnecessary closing
</form>tag.-const INPUT_CSRF = '\r\n<input type="hidden" name="_csrf" value="{{ctx.csrf}}" /></form>'; +const INPUT_CSRF = '\r\n<input type="hidden" name="_csrf" value="{{ctx.csrf}}" />';src/lib/utils.ts (1)
140-149:⚠️ Potential issueImplement IPv6 support.
The code skips IPv6 addresses with a FIXME comment. Since Node.js 18.19.0+ has good IPv6 support, this should be implemented.
Would you like me to help implement IPv6 support or create a GitHub issue to track this?
🧹 Nitpick comments (29)
test/fixtures/apps/xss-close/app/router.js (1)
1-5: LGTM! Removal of generator function aligns with modern practices.The change from generator function to regular function is appropriate as part of the modernization effort.
Consider further modernizing the route handler to use async/await and current Koa conventions:
module.exports = function(app) { - app.get('/', function(){ - this.body = '123'; + app.get('/', async (ctx) => { + ctx.body = '123'; }); };test/fixtures/apps/utils-check-if-pass6/app/router.js (2)
1-1: Consider migrating to ESM syntax.Since this PR aims to support both CJS and ESM, consider updating the CommonJS
module.exportsto use the ESMexport defaultsyntax.-module.exports = function(app) { +export default function(app) {
2-10: Consider using modern response patterns.Since Node.js < 18.19.0 support is dropped, consider modernizing the response handling:
- app.get('/', function(){ - this.body = 'xx'; + app.get('/', async (ctx) => { + ctx.body = 'xx'; });This pattern:
- Uses arrow functions for consistency
- Explicitly declares the context parameter
- Allows for better TypeScript support
- Follows modern Koa practices
test/fixtures/apps/security-unset/app/router.js (2)
2-4: Consider using async/await if isSafeDomain is asynchronous.If
isSafeDomainperforms any asynchronous operations, consider updating the route handler to use async/await for better error handling and readability.- app.get('/', function(){ + app.get('/', async function(){ - this.body = this.isSafeDomain('aaa-domain.com'); + this.body = await this.isSafeDomain('aaa-domain.com'); });
5-7: Consider using async/await if isSafeDomain is asynchronous.Similar to the root route, if
isSafeDomainperforms any asynchronous operations, consider updating this route handler to use async/await.- app.get('/safe', function(){ + app.get('/safe', async function(){ - this.body = this.isSafeDomain('www.domain.com'); + this.body = await this.isSafeDomain('www.domain.com'); });test/nosniff.test.ts (4)
18-24: Consider enhancing the test coverage.While the header check is good, consider:
- Using a more descriptive test name that specifies when/why the header should be present
- Adding assertions for the response body to ensure complete functionality testing
- it('should return default no-sniff http header', async () => { + it('should return nosniff header for regular HTML responses', async () => { await app.httpRequest() .get('/') .set('accept', 'text/html') .expect('X-Content-Type-Options', 'nosniff') + .expect('content-type', /html/) + .expect(res => assert(res.text.length > 0)) .expect(200); });
26-32: Fix the test description.The test description "should not return download noopen http header" appears to have a typo and doesn't accurately describe what's being tested.
- it('should not return download noopen http header', async () => { + it('should not return X-Content-Type-Options header when disabled', async () => {
34-56: Refactor redirect tests to reduce duplication and improve consistency.The redirect tests have inconsistent async/await usage and duplicate logic. Consider using a parameterized test approach.
- it('should disable nosniff on redirect 302', async () => { - await app.httpRequest() - .get('/redirect') - .expect(res => assert(!res.headers['x-content-type-options'])) - .expect('location', '/') - .expect(302); - }); - - it('should disable nosniff on redirect 301', () => { - return app.httpRequest() - .get('/redirect301') - .expect(res => assert(!res.headers['x-content-type-options'])) - .expect('location', '/') - .expect(301); - }); - - it('should disable nosniff on redirect 307', () => { - return app.httpRequest() - .get('/redirect307') - .expect(res => assert(!res.headers['x-content-type-options'])) - .expect('location', '/') - .expect(307); - }); + const redirectCodes = [301, 302, 307]; + redirectCodes.forEach(code => { + it(`should disable nosniff on redirect ${code}`, async () => { + await app.httpRequest() + .get(`/redirect${code === 302 ? '' : code}`) + .expect(res => assert(!res.headers['x-content-type-options'])) + .expect('location', '/') + .expect(code); + }); + });
4-57: Consider adding more test cases for comprehensive coverage.The test suite could benefit from additional scenarios:
- Different content types (JSON, XML, etc.)
- Error responses (4xx, 5xx status codes)
- Various request methods (POST, PUT, etc.)
Would you like me to provide example test cases for these scenarios?
src/lib/middlewares/referrerPolicy.ts (2)
5-5: Use the English MDN documentation link for broader accessibilityThe current comment links to the Chinese version of the MDN documentation. Consider updating the URL to the English version to make it accessible to a wider audience.
Apply this diff to update the link:
-// https://developer.mozilla.org/zh-CN/docs/Web/HTTP/Headers/Referrer-Policy +// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy
27-27: Avoid using 'any' type for better type safetyCasting
ctx.securityOptionstoanybypasses TypeScript's type safety. Consider extending theSecurityOptionsinterface to includerefererPolicyfor backward compatibility, allowing you to avoid theanytype.Example modification to your type definitions:
interface SecurityOptions extends SecurityConfig { refererPolicy?: { enable?: boolean; value?: string; }; }Then adjust the code accordingly without casting to
any:- ...(ctx.securityOptions as any).refererPolicy, + ...ctx.securityOptions.refererPolicy,test/fixtures/apps/referrer-config-compatibility/config/config.js (1)
1-1: Remove redundant 'use strict' directiveIn ES modules, which are in strict mode by default, the
'use strict';directive is unnecessary. Removing it can clean up the code.Apply this diff to remove the directive:
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
package.json (2)
79-87: Add type checking script for better development workflow.Consider adding a dedicated type checking script to catch type errors early in development.
"scripts": { + "type-check": "tsc --noEmit", "lint": "eslint --cache src test --ext .ts", - "pretest": "npm run clean && npm run lint -- --fix", + "pretest": "npm run clean && npm run type-check && npm run lint -- --fix", "test": "egg-bin test",
96-107: Consider adding types export condition for better TypeScript support.While types are correctly specified in the import/require conditions, consider adding a dedicated types export condition for better compatibility with TypeScript's new "exports" resolution.
"exports": { ".": { + "types": "./dist/esm/index.d.ts", "import": { "types": "./dist/esm/index.d.ts", "default": "./dist/esm/index.js" }, "require": { "types": "./dist/commonjs/index.d.ts", "default": "./dist/commonjs/index.js" } },test/fixtures/apps/xss-close-zero/app/router.js (1)
2-2: Consider using async/await pattern for route handler.While converting from generator function is correct, consider using the async/await pattern for better compatibility with modern Node.js practices:
- app.get('/', function(){ + app.get('/', async function() {test/fixtures/apps/security-override-controller/app/router.js (1)
2-8: Consider using undefined assignment instead of delete operator.The delete operator can impact performance. Consider setting the headers to undefined instead.
- delete this.response.header['Strict-Transport-Security']; - delete this.response.header['X-Download-Options']; - delete this.response.header['X-Content-Type-Options']; - delete this.response.header['X-XSS-Protection']; + this.response.header['Strict-Transport-Security'] = undefined; + this.response.header['X-Download-Options'] = undefined; + this.response.header['X-Content-Type-Options'] = undefined; + this.response.header['X-XSS-Protection'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 3-3: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 4-4: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 5-5: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 6-6: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
test/xss.test.ts (1)
34-40: Enhance XSS protection test coverage.Consider adding tests for:
- Different content-type responses (not just text/html)
- Invalid X-XSS-Protection header values
- Edge cases where the header should be omitted
Example test case:
it('should not set X-XSS-Protection for non-HTML responses', () => { return app.httpRequest() .get('/') .set('accept', 'application/json') .expect(res => { assert(!res.headers['x-xss-protection']); }) .expect(200); });test/hsts.test.ts (1)
Line range hint
31-46: Expand HSTS test coverage for security compliance.Consider adding tests for:
- HSTS preload flag
- Invalid max-age values
- Environment-specific configurations (development vs production)
Example test case:
it('should include preload flag when configured', () => { const app4 = mm.app({ baseDir: 'apps/hsts-preload', }); return app4.httpRequest() .get('/') .expect('Strict-Transport-Security', 'max-age=31536000; includeSubdomains; preload') .expect(200); });test/referrer.test.ts (2)
56-63: Improve test case documentation for backward compatibility.The test case for backward compatibility should clearly document:
- What typo is being tested
- Why backward compatibility is needed
- When this compatibility can be removed
Consider updating the test description:
- it('should keep typo refererPolicy for backward compatibility', () => { + it('should support legacy "refererPolicy" config key (typo) for backward compatibility with < v2.0', () => {
65-73: Add context to the PR reference in comments.The comment references PR #50 without providing context about the issue being fixed.
Consider updating the comment:
- // check for fix https://github.com/eggjs/security/pull/50 + // Ensures numeric values are rejected as policy values + // Fixed security bypass reported in https://github.com/eggjs/security/pull/50test/security.test.ts (1)
Line range hint
38-83: Consider grouping related test cases.The test cases for security headers could be organized better using nested
describeblocks to group related tests. This would improve readability and maintenance.- it('should load default security headers', () => { - return app.httpRequest() - .get('/') - .set('accept', 'text/html') - .expect('X-Download-Options', 'noopen') - .expect('X-Content-Type-Options', 'nosniff') - .expect('X-XSS-Protection', '1; mode=block') - .expect(200); - }); + describe('security headers', () => { + it('should load default headers', () => { + return app.httpRequest() + .get('/') + .set('accept', 'text/html') + .expect('X-Download-Options', 'noopen') + .expect('X-Content-Type-Options', 'nosniff') + .expect('X-XSS-Protection', '1; mode=block') + .expect(200); + });test/xframe.test.ts (1)
Line range hint
65-69: Standardize on async/await pattern.Some tests use callbacks with
donewhile others use async/await. Consider standardizing on async/await for consistency and better error handling.- it('should contain X-Frame-Options: SAMEORIGIN when dont set value option', function(done) { - app2.httpRequest() - .get('/foo') - .set('accept', 'text/html') - .expect('X-Frame-Options', 'SAMEORIGIN', done); + it('should contain X-Frame-Options: SAMEORIGIN when dont set value option', async () => { + await app2.httpRequest() + .get('/foo') + .set('accept', 'text/html') + .expect('X-Frame-Options', 'SAMEORIGIN'); });test/safe_redirect.test.ts (1)
Line range hint
89-127: LGTM! Comprehensive security test coverage.The test cases effectively validate both safe and unsafe redirect scenarios. Consider adding comments explaining why specific URLs are considered unsafe to help future maintainers.
const blackurls = [ + // Protocol-relative URLs that could bypass restrictions '//baidu.com', '///baidu.com/', + // Invalid or potentially malicious protocols 'xxx://baidu.com', 'ftp://baidu.com/', + // URLs with special characters that could be used for attacks 'http://www.baidu.com?', 'http://www.baidu.com#', 'http://www.baidu.com%3F',src/lib/middlewares/methodnoallow.ts (2)
4-4: Consider adding OPTIONS to METHODS_NOT_ALLOWED.The OPTIONS method can be used for probing server capabilities and should typically be blocked unless explicitly needed.
-const METHODS_NOT_ALLOWED = [ 'TRACE', 'TRACK' ]; +const METHODS_NOT_ALLOWED = [ 'TRACE', 'TRACK', 'OPTIONS' ];
Line range hint
15-22: Enhance error response with allowed methods.The middleware could be more informative by including the allowed methods in the response headers.
return function notAllow(ctx: Context, next: Next) { // ctx.method is upper case if (!safeHttpMethodsMap[ctx.method]) { + ctx.set('Allow', Object.keys(safeHttpMethodsMap).join(', ')); ctx.throw(405); } return next(); };src/lib/extend/safe_curl.ts (1)
4-4: Consider making SSRF_HTTPCLIENT symbol unique.Add a descriptive string key to make the symbol more debuggable and unique.
-const SSRF_HTTPCLIENT = Symbol('SSRF_HTTPCLIENT'); +const SSRF_HTTPCLIENT = Symbol('eggjs.security.ssrf.httpClient');src/app/extend/response.ts (1)
Line range hint
73-82: Enhance security logging with URL details.The security warning could include more context about the blocked redirection attempt.
if (!this.ctx.isSafeDomain(urlObject.hostname)) { - const message = `a security problem has been detected for url "${url}", redirection is prohibited.`; + const message = `Blocked redirection to non-whitelisted domain "${urlObject.hostname}" (full URL: "${url}")`; if (process.env.NODE_ENV === 'production') { this.app.coreLogger.warn('[@eggjs/security/response/redirect] %s', message); + this.app.coreLogger.warn('[@eggjs/security/response/redirect] Allowed domains: %j', domainWhiteList); url = '/'; } else { // Exception will be thrown out in a non-PROD env. return this.ctx.throw(500, message); } }src/lib/utils.ts (2)
119-121: Use optional chaining for better code readability.The static analysis tool suggests using optional chaining in these locations.
- if (ssrf && ssrf.ipBlackList && !ssrf.checkAddress) { + if (ssrf?.ipBlackList && !ssrf.checkAddress) { const blackList = ssrf.ipBlackList.map(getContains); - if (config.csrf && config.csrf.refererWhiteList) { + if (config.csrf?.refererWhiteList) { config.csrf.refererWhiteList = config.csrf.refererWhiteList.map((ref: string) => ref.toLowerCase());Also applies to: 176-177
🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
182-187: Consider using a predefined set of safe protocols.The protocol whitelist should be defined as a constant to prevent accidental modifications.
+const DEFAULT_SAFE_PROTOCOLS = new Set(['http', 'https', 'file', 'data']); + const protocolWhiteListSet = new Set(config.protocolWhiteList); -protocolWhiteListSet.add('http'); -protocolWhiteListSet.add('https'); -protocolWhiteListSet.add('file'); -protocolWhiteListSet.add('data'); +for (const protocol of DEFAULT_SAFE_PROTOCOLS) { + protocolWhiteListSet.add(protocol); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
__snapshots__/csp.test.ts.js(1 hunks)__snapshots__/csrf.test.ts.js(1 hunks)__snapshots__/dta.test.ts.js(1 hunks)__snapshots__/xss.test.ts.js(1 hunks)package.json(1 hunks)src/app.ts(1 hunks)src/app/extend/agent.ts(1 hunks)src/app/extend/application.ts(1 hunks)src/app/extend/context.ts(7 hunks)src/app/extend/response.ts(4 hunks)src/config/config.default.ts(1 hunks)src/lib/extend/safe_curl.ts(1 hunks)src/lib/middlewares/methodnoallow.ts(1 hunks)src/lib/middlewares/referrerPolicy.ts(1 hunks)src/lib/utils.ts(5 hunks)test/dta.test.ts(4 hunks)test/fixtures/apps/dta/app/router.js(1 hunks)test/fixtures/apps/dta/config/config.js(0 hunks)test/fixtures/apps/hsts-nosub/app/router.js(1 hunks)test/fixtures/apps/noopen/app/router.js(1 hunks)test/fixtures/apps/nosniff/app/router.js(1 hunks)test/fixtures/apps/referrer-config-compatibility/app/router.js(1 hunks)test/fixtures/apps/referrer-config-compatibility/config/config.js(1 hunks)test/fixtures/apps/referrer-config-compatibility/package.json(1 hunks)test/fixtures/apps/referrer-config/app/router.js(1 hunks)test/fixtures/apps/referrer/app/router.js(1 hunks)test/fixtures/apps/security-override-controller/app/router.js(1 hunks)test/fixtures/apps/security-override-middleware/app/router.js(1 hunks)test/fixtures/apps/security-unset/app/router.js(1 hunks)test/fixtures/apps/security/app/router.js(1 hunks)test/fixtures/apps/utils-check-if-pass/app/router.js(1 hunks)test/fixtures/apps/utils-check-if-pass2/app/router.js(1 hunks)test/fixtures/apps/utils-check-if-pass3/app/router.js(1 hunks)test/fixtures/apps/utils-check-if-pass4/app/router.js(1 hunks)test/fixtures/apps/utils-check-if-pass5/app/router.js(1 hunks)test/fixtures/apps/utils-check-if-pass6/app/router.js(1 hunks)test/fixtures/apps/xss-close-zero/app/router.js(1 hunks)test/fixtures/apps/xss-close-zero/config/config.js(0 hunks)test/fixtures/apps/xss-close/app/router.js(1 hunks)test/fixtures/apps/xss/app/router.js(1 hunks)test/hsts.test.ts(1 hunks)test/inject.test.ts(2 hunks)test/method_not_allow.test.ts(2 hunks)test/noopen.test.ts(1 hunks)test/nosniff.test.js(0 hunks)test/nosniff.test.ts(1 hunks)test/referrer.test.js(0 hunks)test/referrer.test.ts(1 hunks)test/safe_redirect.test.ts(5 hunks)test/security.test.ts(1 hunks)test/ssrf.test.ts(10 hunks)test/utils.test.ts(3 hunks)test/xframe.test.ts(1 hunks)test/xss.test.js(0 hunks)test/xss.test.ts(1 hunks)
💤 Files with no reviewable changes (5)
- test/fixtures/apps/dta/config/config.js
- test/fixtures/apps/xss-close-zero/config/config.js
- test/referrer.test.js
- test/nosniff.test.js
- test/xss.test.js
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/apps/referrer-config-compatibility/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app.ts
- snapshots/csp.test.ts.js
- snapshots/csrf.test.ts.js
🧰 Additional context used
🪛 Biome (1.9.4)
test/fixtures/apps/referrer-config-compatibility/config/config.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/apps/security-override-controller/app/router.js
[error] 3-3: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 4-4: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 5-5: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 6-6: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/lib/utils.ts
[error] 120-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 176-176: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
🔇 Additional comments (43)
test/fixtures/apps/hsts-nosub/app/router.js (1)
1-1: 🛠️ Refactor suggestionConsider converting to ESM syntax for dual CJS/ESM support.
Since this PR aims to support both CJS and ESM modules, consider updating the export syntax to support both module systems.
-module.exports = function(app) { +export default function(app) {Likely invalid or redundant comment.
test/fixtures/apps/xss/app/router.js (2)
2-4: LGTM! Function signature updated appropriately.The transition from generator function to regular function is appropriate here since there are no async operations or yield points needed.
6-11: Verify the XSS protection test case behavior.While the function signature change is appropriate, this route explicitly disables XSS protection for testing purposes. Ensure this test case is properly documented and isolated.
Let's verify other XSS protection test cases to ensure comprehensive coverage:
✅ Verification successful
XSS protection test case is properly structured and documented
The test case is part of a comprehensive test suite that verifies different XSS protection configurations. The explicit disabling of XSS protection is properly isolated in its own test case with clear assertions and is accompanied by other test scenarios that verify the default protection behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other XSS protection test cases and their documentation # Look for other XSS-related test cases rg -l "xssProtection" "test/fixtures" # Check for test documentation rg -l "describe.*xss" "test/"Length of output: 256
Script:
#!/bin/bash # Check the main test file content echo "=== Main XSS Test File ===" cat test/xss.test.ts echo -e "\n=== XSS Config Files ===" # Check config files content for file in test/fixtures/apps/xss*/config/config.js; do echo -e "\nFile: $file" cat "$file" done echo -e "\n=== Router File ===" cat test/fixtures/apps/xss/app/router.jsLength of output: 3071
test/fixtures/apps/utils-check-if-pass6/app/router.js (1)
2-10: Verify async handling in route handlers.The removal of generator functions (
function *) changes how async operations are handled. Ensure that:
- The route handlers properly handle async operations if needed
this.bodyassignment still works as expected with the new middleware system- The test cases covering these routes are updated accordingly
Let's verify the middleware context handling:
✅ Verification successful
Simple synchronous routes don't require async handling
These route handlers only perform simple synchronous
this.bodyassignments with static strings. No async operations are present, and the removal of generator functions doesn't impact their functionality. The middleware context (this.body) usage remains valid for these basic routes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related test files and middleware context usage rg -l "this.body" "test/fixtures/apps/utils-check-if-pass6" # Check if there are any remaining generator functions in route handlers rg "function \*" "test/fixtures/apps/utils-check-if-pass6"Length of output: 170
Script:
#!/bin/bash # Find test files covering this fixture fd "test" "test/fixtures/apps/utils-check-if-pass6" # Check for async/await or Promise usage in the app rg -l "async|await|Promise" "test/fixtures/apps/utils-check-if-pass6" # Look for middleware or app configuration fd "config|middleware" "test/fixtures/apps/utils-check-if-pass6" # Check app.js or similar bootstrap files fd "^app.js$|^index.js$" "test/fixtures/apps/utils-check-if-pass6"Length of output: 356
test/fixtures/apps/security-override-middleware/app/router.js (1)
2-7: Verify the async middleware chain behaviorSince this is a test fixture for security middleware, we need to ensure that the async flow is preserved after removing generator functions.
✅ Verification successful
Async middleware chain is correctly implemented
The async flow is properly preserved:
isSafeDomainis a synchronous method, so no await is needed in the route handlers- The security middleware correctly implements the async/await pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if isSafeDomain is an async function and if there are any await calls in the middleware chain # Search for isSafeDomain implementation ast-grep --pattern 'isSafeDomain($$$) { $$$ }' # Search for middleware registration to verify the execution chain rg -l "app.use.*security" --type jsLength of output: 88
Script:
#!/bin/bash # Find files containing isSafeDomain rg -l "isSafeDomain" # Look for middleware setup files in the security-override-middleware app fd -t f . test/fixtures/apps/security-override-middleware # Search for async/await patterns in the app directory rg "async|await" test/fixtures/apps/security-override-middlewareLength of output: 1436
Script:
#!/bin/bash # Check the core implementation of isSafeDomain rg -A 5 "isSafeDomain" src/app/extend/context.ts # Check the middleware implementation cat test/fixtures/apps/security-override-middleware/app/middleware/override.jsLength of output: 1537
test/fixtures/apps/security-unset/app/router.js (2)
1-1: Verify if CommonJS is intended for test fixtures.Given that the PR aims to support both CJS and ESM, verify if test fixtures should maintain CommonJS syntax or be updated to use ESM.
1-8: Verify async behavior after generator function removal.The removal of generator functions aligns with modern Node.js practices, but we should ensure that the async flow is maintained correctly.
Let's verify the usage of
this.bodyandthis.isSafeDomainin the codebase:test/nosniff.test.ts (2)
1-17: Well-structured test setup!The test setup follows best practices with proper TypeScript types, cleanup in afterEach, and correct application lifecycle management.
8-11: Verify the test fixture app exists.The test references a fixture app at 'apps/nosniff'. Let's ensure it exists and contains the necessary routes.
✅ Verification successful
Test fixture is properly configured ✅
The fixture app exists at the correct location with all necessary components:
- Complete router implementation with test endpoints
- Configuration file present
- Proper middleware configuration options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the test fixture exists and contains required routes echo "Checking test fixture..." if [ -d "test/fixtures/apps/nosniff" ]; then echo "Found fixture directory" # Look for route definitions rg -l "app.get|router.get" "test/fixtures/apps/nosniff" else echo "Missing fixture directory: test/fixtures/apps/nosniff" fiLength of output: 252
Script:
#!/bin/bash # Check the router implementation echo "Router implementation:" cat test/fixtures/apps/nosniff/app/router.js echo -e "\nChecking for config files:" fd "config\.(js|ts)$" test/fixtures/apps/nosniffLength of output: 730
src/lib/middlewares/referrerPolicy.ts (1)
18-39: LGTM: Middleware implementation is correctThe referrer policy middleware is correctly implemented, handling options and ensuring backward compatibility. The logic for setting the
referrer-policyheader is sound.test/fixtures/apps/referrer-config-compatibility/config/config.js (1)
3-11: Configuration settings are correctThe security configuration is properly set up with the
referrerPolicymiddleware enabled and set to'origin'. The structure aligns with the application's requirements.test/fixtures/apps/referrer-config-compatibility/app/router.js (1)
1-13: Routes are correctly definedThe route handlers are properly set up, and the middleware configurations are correctly applied. The application should handle requests as expected.
package.json (1)
39-50: Verify security of production dependencies.Several security-critical dependencies are used. Let's verify their versions for known vulnerabilities:
✅ Verification successful
Dependencies are using secure versions
All production dependencies are using versions that include patches for known vulnerabilities. The current versions of nanoid (3.3.8) and zod (3.24.1) are specifically newer than their respective vulnerable versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for known vulnerabilities in dependencies # Check for security advisories for critical dependencies for pkg in "@eggjs/core" "@eggjs/ip" "csrf" "nanoid" "xss" "zod"; do echo "Checking $pkg..." gh api graphql -f query='{ securityVulnerabilities(first: 5, ecosystem: NPM, package: "'$pkg'") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' doneLength of output: 3814
test/fixtures/apps/referrer/app/router.js (1)
2-2: Same async/await consideration applies here.Similar to the previous file, consider updating to async/await pattern.
test/fixtures/apps/dta/app/router.js (1)
2-2: Same async/await consideration applies here.Similar to previous files, consider updating to async/await pattern.
test/fixtures/apps/utils-check-if-pass3/app/router.js (1)
2-7: Same modernization needed as in utils-check-if-pass/router.js.The same Koa pattern updates are needed here for Node.js 18+ compatibility.
test/fixtures/apps/utils-check-if-pass4/app/router.js (1)
2-7: Same modernization needed as in utils-check-if-pass/router.js.The same Koa pattern updates are needed here for Node.js 18+ compatibility.
test/fixtures/apps/security/app/router.js (1)
2-7: 🛠️ Refactor suggestionVerify if isSafeDomain is asynchronous.
The removal of generator functions could break functionality if
isSafeDomainis asynchronous. Additionally, the Koa patterns should be modernized.module.exports = function(app) { - app.get('/', function(){ - this.body = this.isSafeDomain('aaa-domain.com'); + app.get('/', async (ctx) => { + ctx.body = await ctx.isSafeDomain('aaa-domain.com'); }); - app.get('/safe', function(){ - this.body = this.isSafeDomain('www.domain.com'); + app.get('/safe', async (ctx) => { + ctx.body = await ctx.isSafeDomain('www.domain.com'); }); };test/fixtures/apps/noopen/app/router.js (1)
2-4: Verify async operation handling after generator function removal.The transition from generator functions to regular functions is generally good for modernization. However, ensure that:
- No async operations were previously using
yield- If async operations are needed in the future, use async/await instead
Run this script to check for any async operations in related test files:
Also applies to: 6-9
✅ Verification successful
Generator function removal is safe in this context
The route handlers only perform synchronous operations (setting response body and security options) and are part of test fixtures. No async operations were present that would require
yieldorasync/await.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for async operations in related test files rg -l "yield|async|await" "test/fixtures/apps/noopen/"Length of output: 54
Script:
#!/bin/bash # Verify file structure and content echo "=== File Structure ===" fd . "test/fixtures/apps/noopen/" echo -e "\n=== Router Content ===" cat "test/fixtures/apps/noopen/app/router.js" echo -e "\n=== Framework Patterns ===" # Look for similar router implementations rg -A 2 "app.get\(" "test/fixtures/"Length of output: 39309
test/fixtures/apps/utils-check-if-pass5/app/router.js (1)
2-10: LGTM! Simple and safe changes.The removal of generator functions is appropriate here as these routes only perform synchronous operations.
test/fixtures/apps/utils-check-if-pass2/app/router.js (1)
2-10: LGTM! Consistent with the modernization effort.The changes align well with the codebase modernization, removing generator functions where they're not needed for async operations.
test/fixtures/apps/referrer-config/app/router.js (2)
2-4: LGTM! Generator function removal is appropriate.The route handler performs only synchronous operations, making the removal of the generator function safe.
5-12:⚠️ Potential issueSecurity concern: Validate referrer policy value.
The code directly uses a query parameter to set security policy without validation. This could lead to security issues if invalid or malicious values are provided.
Add validation before setting the policy:
app.get('/referrer', function() { const policy = this.query.policy; + const validPolicies = ['no-referrer', 'no-referrer-when-downgrade', 'origin', 'origin-when-cross-origin', 'same-origin', 'strict-origin', 'strict-origin-when-cross-origin', 'unsafe-url']; + if (!validPolicies.includes(policy)) { + this.status = 400; + this.body = 'Invalid referrer policy'; + return; + } this.body = '123'; this.securityOptions.referrerPolicy = { enable: true, value: policy } });Run this script to check for similar unvalidated security options:
test/fixtures/apps/security-override-controller/app/router.js (1)
9-11: LGTM! Generator function correctly converted to regular function.The conversion from generator function to regular function is correct and aligns with the PR objective.
test/fixtures/apps/nosniff/app/router.js (1)
2-4: LGTM! Generator functions correctly converted to regular functions.All route handlers have been properly converted from generator functions to regular functions, maintaining the original functionality while aligning with the PR objective to modernize the codebase.
Also applies to: 6-9, 11-13, 15-18, 20-23
test/method_not_allow.test.ts (2)
1-4: LGTM! Proper TypeScript and ESM conversion.The conversion to TypeScript with proper type annotations and ES module imports is well done.
17-18: Verify test coverage after simplification.The test suite has been simplified to only test GET and TRACE methods. Please ensure that removing tests for POST, HEAD, PUT, and DELETE methods doesn't reduce necessary coverage.
Also applies to: 21-25
✅ Verification successful
Test coverage is adequately maintained ✅
The removed tests from
method_not_allow.test.tswere redundant as the HTTP methods (POST, HEAD, PUT, DELETE) are comprehensively covered incsrf.test.ts, which includes:
- CSRF token validation for POST requests
- Explicit tests for allowed methods
- Multiple test suites with different configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any existing tests covering these HTTP methods elsewhere rg -l "app\.httpRequest\(\).*\.(post|head|put|delete)" "test/**/*test.ts"Length of output: 132
Script:
#!/bin/bash # Search for HTTP method tests with correct glob pattern rg -l "app\.httpRequest\(\).*\.(post|head|put|delete)" test/ # Search for router/middleware HTTP method handlers rg -l "(post|head|put|delete)\s*\(['\"]" . # Search for alternative HTTP request patterns in tests rg -l "request.*\.(post|head|put|delete)" test/Length of output: 644
Script:
#!/bin/bash # Check the specific test cases in csrf.test.ts rg -A 5 "app\.httpRequest\(\).*\.(post|head|put|delete)" test/csrf.test.ts # Look for test descriptions related to these methods rg "describe|it.*('|\").*(?i)(post|head|put|delete)" test/csrf.test.tsLength of output: 1244
test/noopen.test.ts (2)
1-2: LGTM! Proper module imports with node: prefix.Good use of the
node:prefix for built-in modules and ES module imports.
4-13: LGTM! Proper TypeScript conversion and test cleanup.The conversion to TypeScript with proper type annotations and the addition of the
afterhook for cleanup is well done.test/security.test.ts (2)
1-2: LGTM! Clean transition to TypeScript with proper type safety.The conversion from CommonJS to ESM imports and addition of proper type annotations for the MockApplication instances improves code quality and maintainability.
Also applies to: 5-8
28-33: LGTM! Proper resource cleanup.Good addition of the
afterhook to ensure all test applications are properly closed, preventing resource leaks.test/dta.test.ts (3)
19-19: LGTM! Good use of snapshot testing.Using snapshots for security configuration is a good practice for detecting unintended changes.
78-80: Verify the necessity of platform-specific delay.The Windows-specific delay of 2 seconds seems arbitrary. This could make tests flaky or unnecessarily slow on Windows.
✅ Verification successful
Add comment explaining Windows-specific delay
The 2-second delay is necessary due to Windows-specific logging behavior, ensuring the log message is written before verification. However, consider:
- Adding a comment explaining why this delay is needed on Windows
- Using a more robust approach like polling for the log message instead of a fixed delay
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar platform-specific delays rg "platform === 'win32'" -A 2 -B 2Length of output: 251
Script:
#!/bin/bash # Get the full test case context rg -B 10 -A 10 "platform === 'win32'" test/dta.test.ts # Search for scheduler.wait usage rg "scheduler\.wait" -A 2 -B 2 # Search for any fs operations in the test file rg "fs\." test/dta.test.tsLength of output: 743
Line range hint
55-73: Verify why these Directory Traversal Attack tests are skipped.Multiple critical security tests are skipped. This could leave the application vulnerable to directory traversal attacks.
src/app/extend/agent.ts (1)
9-14: LGTM! Clean and type-safe implementation.The
SecurityAgentclass is well-structured with proper TypeScript types. Note that this implementation requires Node.js ≥ 18.19.0 due to the ESM imports.test/ssrf.test.ts (2)
160-161: Address the TODO comment for local server testing.The skipped test for exception IPs should be implemented to ensure proper functionality.
Would you like me to help implement the local server test or create a GitHub issue to track this task?
108-109: Verify the commented-out test URLs.Several test URLs are commented out. We should either:
- Remove them if they're no longer needed
- Re-enable them if they're still relevant
- Add a comment explaining why they're commented out
Also applies to: 147-148
test/utils.test.ts (1)
77-82: LGTM! Good cleanup practices.The explicit typing of app variables and proper cleanup in after hooks improve code quality and prevent resource leaks.
Also applies to: 115-122
src/app/extend/application.ts (1)
24-32:⚠️ Potential issueMake nonce attribute check case-insensitive and escape values.
The current implementation has two issues:
- The nonce check should be case-insensitive
- The nonce value should be escaped to prevent XSS
injectNonce(html: string) { - html = html.replace(/<script(.*?)>([\s\S]*?)<\/script[^>]*?>/gi, (_, $1, $2) => { - if (!$1.includes('nonce=')) { - $1 += ' nonce="{{ctx.nonce}}"'; + html = html.replace(/<script(.*?)>([\s\S]*?)<\/script[^>]*?>/gi, (_, $1, $2) => { + if (!$1.toLowerCase().includes('nonce=')) { + const escapedNonce = this.ctx.nonce.replace(/['"<>]/g, ''); + $1 += ` nonce="${escapedNonce}"`; } return '<script' + $1 + '>' + $2 + '</script>'; }); return html; }Likely invalid or redundant comment.
src/app/extend/context.ts (2)
271-282: LGTM! Module augmentation is well-typed.The Context interface extension properly declares all security-related methods with correct types.
104-108:⚠️ Potential issueSecurity: Enable httpOnly for CSRF cookies.
Setting
httpOnly: falsefor CSRF cookies exposes them to potential XSS attacks. Since CSRF tokens are transmitted via forms, headers, and request parameters, client-side JavaScript access to the cookie is not required.Also applies to: 146-147
src/config/config.default.ts (2)
214-233:⚠️ Potential issueSecurity: Enable HSTS by default.
HSTS is currently disabled by default. For enhanced security, it should be enabled by default with secure options.
hsts: z.object({ match: IgnoreOrMatchOption, ignore: IgnoreOrMatchOption, /** - * Default to `false` + * Default to `true` */ - enable: z.boolean().default(false), + enable: z.boolean().default(true), /** * Max age of Strict-Transport-Security in seconds */ maxAge: z.number().default(365 * 24 * 3600), /** * Whether include sub domains - * Default to `false` + * Default to `true` */ - includeSubdomains: z.boolean().default(false), + includeSubdomains: z.boolean().default(true), }).default({}),Likely invalid or redundant comment.
336-341:⚠️ Potential issueSecurity: Add secure defaults for SSRF protection.
The SSRF configuration lacks default IP blacklist values. Consider providing secure defaults to protect against SSRF attacks targeting internal networks.
ssrf: z.object({ - ipBlackList: z.array(z.string()).optional(), + ipBlackList: z.array(z.string()).default([ + '10.0.0.0/8', + '172.16.0.0/12', + '192.168.0.0/16', + '127.0.0.0/8', + 'fc00::/7', + 'fe80::/10', + ]), ipExceptionList: z.array(z.string()).optional(), hostnameExceptionList: z.array(z.string()).optional(), checkAddress: SSRFCheckAddressFunction.optional(), }).default({}),Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
[skip ci] ## [4.0.0](v3.7.0...v4.0.0) (2025-01-17) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit Based on the comprehensive changes, here are the updated release notes: - **New Features** - Migrated security plugin to TypeScript. - Enhanced type safety for security configurations. - Improved middleware and helper utilities. - Introduced new middleware for handling `Strict-Transport-Security`, `X-Frame-Options`, and `X-XSS-Protection` headers. - Added support for new security configurations and helper functions. - **Breaking Changes** - Renamed package from `egg-security` to `@eggjs/security`. - Dropped support for Node.js versions below 18.19.0. - Restructured module exports and configurations. - Removed several deprecated middleware and utility functions. - **Security Improvements** - Updated CSRF, XSS, and SSRF protection mechanisms. - Enhanced middleware for handling security headers. - Refined configuration options for various security features. - **Performance** - Modernized codebase with ES module syntax. - Improved type definitions and module structure. - Enhanced test suite with TypeScript support and better resource management. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#101](#101)) ([a11661f](a11661f))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
Strict-Transport-Security,X-Frame-Options, andX-XSS-Protectionheaders.Breaking Changes
egg-securityto@eggjs/security.Security Improvements
Performance