Conversation
Release 0.14.1-beta.1: Next.js rewrite middleware fix
- Add appId config option (fallback to portalPageId) - Export createInternalServer() to fix 'this.route is not a function' on config reload - Rename portalPageId to appId in load-pp-data middleware - Pass full miConfig to loadPPData in CLI
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test-nextjs/src/pages/_document.tsx (1)
5-8:⚠️ Potential issue | 🟡 Minor
metadataexport has no effect in Pages Router_document.tsx.The
metadataexport is an App Router API (forlayout.tsxorpage.tsxfiles). In the Pages Router,_document.tsxdoesn't support this export—it will be silently ignored. If you need to set page metadata in Pages Router, usenext/headin individual pages or_app.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs/src/pages/_document.tsx` around lines 5 - 8, The file exports a `metadata` object in `_document.tsx`, but `metadata` is App Router-only and will be ignored in Pages Router; remove the `export const metadata` from `_document.tsx` and instead set page metadata using `next/head` in each page or centrally in `_app.tsx` (or migrate to the App Router layout/page pattern if you intend to use `metadata`); update any tests or fixtures that expect `metadata` to be applied accordingly.
🧹 Nitpick comments (3)
tests/test-nextjs/pp-dev.config.ts (1)
5-5: Consider usingappIdinstead of deprecatedportalPageId.Since this PR introduces
appIdas the preferred option (withportalPageIdmarked as deprecated in the type definitions), consider updating the test config to use the new option for consistency and to serve as a usage example.Suggested change
const ppDevConfig: PPDevConfig = { backendBaseURL: 'https://stg7x.metricinsights.com', - portalPageId: 937, + appId: 937, v7Features: true, templateLess: false, miHudLess: true, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-nextjs/pp-dev.config.ts` at line 5, Update the test config to use the new appId option instead of the deprecated portalPageId: replace the portalPageId property in pp-dev.config.ts with appId (preserve the numeric value, e.g., 937) and remove the deprecated portalPageId entry so the test demonstrates current usage of appId consistent with the type definitions.src/lib/proxy-pass.middleware.ts (2)
139-142: Static analysis flags potential ReDoS from dynamic regex.The
hostvalue fromreq.headers.hostis interpolated directly into a RegExp. In a local dev server context, this is low risk since the Host header typically comes from localhost requests. However, as a defensive measure, consider usingString.prototype.replacewith a literal match or escaping special regex characters.💡 Safer alternative using string replacement
if (host && referer && typeof referer === 'string') { - proxyReq.setHeader( - 'referer', - referer.replace(new RegExp(`https?://${host}`), baseURL), - ); + proxyReq.setHeader( + 'referer', + referer + .replace(`https://${host}`, baseURL) + .replace(`http://${host}`, baseURL), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy-pass.middleware.ts` around lines 139 - 142, The code builds a RegExp from the untrusted host and uses it in referer.replace, which risks ReDoS; instead perform a literal string replacement for both schemes so no regex is constructed: in the proxyReq.setHeader call (the referer replacement near proxyReq.setHeader(... referer.replace(...))), replace occurrences of `https://${host}` and `http://${host}` using a safe literal replace (e.g. split/join or replaceAll) or check startsWith and slice, then substitute baseURL—do this using the referer and req.headers.host values (host) before setting the header.
32-46: Interceptor parameter is never used in streaming response handler.The
interceptorcallback is accepted but never invoked—proxyResis piped directly toreson line 44. The URL rewriting logic passed at lines 172-182 will have no effect on streaming responses.The TODO on line 31 indicates this is known incomplete work. Consider either removing the unused parameter to avoid confusion, or completing the implementation.
💡 Option: Remove unused parameter until implementation is complete
// TODO: Implement interceptor for streaming responses -function streamResponseInterceptor( - interceptor?: (data: Buffer, encoding: BufferEncoding) => Buffer, -) { +function streamResponseInterceptor() { return async <T extends IncomingMessage>( proxyRes: T, req: T, res: ServerResponse<T>, ) => {And simplify the call site:
- const streamInterceptor = streamResponseInterceptor( - (data, encoding) => { - return Buffer.from( - urlReplacer( - host, - req.headers.host ?? '', - data.toString(encoding), - ), - encoding, - ); - }, - ); + const streamInterceptor = streamResponseInterceptor();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy-pass.middleware.ts` around lines 32 - 46, The streamResponseInterceptor function currently accepts but never calls interceptor (it just does proxyRes.pipe(res)), so either remove the unused interceptor parameter or implement streaming interception: inside streamResponseInterceptor (symbol) replace proxyRes.pipe(res) with piping that listens to proxyRes 'data' events, buffers or transforms each chunk using the interceptor(Buffer, encoding) when provided, writes the transformed chunk to res, and forwards 'end' and 'error' events; keep existing res.setHeader(PROXY_HEADER, 1) and res.setHeaders(...) behavior and ensure you preserve correct BufferEncoding handling and backpressure by pausing/resuming proxyRes as needed to avoid memory spikes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 71: Remove the unnecessary npm override entry from package.json (the
"npm": ">=11.11.0" line under overrides) because npm is not a dependency and the
override has no effect; if this was intentionally added for a specific
environment issue, instead leave a brief comment in the PR or commit message
documenting that reason and keep the override only after documenting it.
In `@src/cli.ts`:
- Around line 953-957: The miConfig passed into initLoadPPData is missing the
base property which initLoadPPData destructures for redirect URL construction;
update the call so the options include base (e.g. Object.assign({}, miConfig, {
base })) or otherwise ensure miConfig contains base before calling
initLoadPPData so initLoadPPData (from load-pp-data.middleware.ts) receives a
defined base value.
In `@src/lib/load-pp-data.middleware.ts`:
- Line 87: The code is using non-null assertions on appId (e.g., where appId! is
passed to object creation and mi.getPageVariables) but validation only runs for
the templateLess && miHudLess path; add explicit validation or safe handling for
appId before any use in the handleTemplateLoad and any code paths where
isNeedTemplateLoad is true. Specifically, in the middleware entry (before
calling handleTemplateLoad and before calling mi.getPageVariables) check that
appId is defined and either throw a clear error or return an appropriate
response, or change call sites (handleTemplateLoad, handlePageInfoOnly, and the
mi.getPageVariables invocation) to accept appId as optional and guard against
undefined instead of using appId!. Remove the appId! non-null assertions at the
call sites to ensure runtime safety.
- Around line 263-266: Update the LoadPPDataOptions interface so appId is
optional (change appId: number to appId?: number) to reflect runtime checks;
this aligns the type with the conditional usage around templateLess and
miHudLess and the call site that uses typeof appId !== 'undefined' before
invoking mi.getPageVariables, ensuring callers can omit appId and the fallback
to mi.getPageTemplate remains valid.
In `@src/lib/proxy-pass.middleware.ts`:
- Around line 284-286: Replace the empty catch block in the
proxy-pass.middleware.ts catch section with a debug-level log of the caught
error so failures in URL parsing/header manipulation/script injection are
visible but the proxy flow remains intact; e.g. capture the error as (err) and
call the module's logger at debug level (logger.debug(...) or
processLogger.debug(...), falling back to console.debug) inside the catch in the
proxyPass middleware / handleProxyPassRequest function to record a descriptive
message and the error object.
---
Outside diff comments:
In `@tests/test-nextjs/src/pages/_document.tsx`:
- Around line 5-8: The file exports a `metadata` object in `_document.tsx`, but
`metadata` is App Router-only and will be ignored in Pages Router; remove the
`export const metadata` from `_document.tsx` and instead set page metadata using
`next/head` in each page or centrally in `_app.tsx` (or migrate to the App
Router layout/page pattern if you intend to use `metadata`); update any tests or
fixtures that expect `metadata` to be applied accordingly.
---
Nitpick comments:
In `@src/lib/proxy-pass.middleware.ts`:
- Around line 139-142: The code builds a RegExp from the untrusted host and uses
it in referer.replace, which risks ReDoS; instead perform a literal string
replacement for both schemes so no regex is constructed: in the
proxyReq.setHeader call (the referer replacement near proxyReq.setHeader(...
referer.replace(...))), replace occurrences of `https://${host}` and
`http://${host}` using a safe literal replace (e.g. split/join or replaceAll) or
check startsWith and slice, then substitute baseURL—do this using the referer
and req.headers.host values (host) before setting the header.
- Around line 32-46: The streamResponseInterceptor function currently accepts
but never calls interceptor (it just does proxyRes.pipe(res)), so either remove
the unused interceptor parameter or implement streaming interception: inside
streamResponseInterceptor (symbol) replace proxyRes.pipe(res) with piping that
listens to proxyRes 'data' events, buffers or transforms each chunk using the
interceptor(Buffer, encoding) when provided, writes the transformed chunk to
res, and forwards 'end' and 'error' events; keep existing
res.setHeader(PROXY_HEADER, 1) and res.setHeaders(...) behavior and ensure you
preserve correct BufferEncoding handling and backpressure by pausing/resuming
proxyRes as needed to avoid memory spikes.
In `@tests/test-nextjs/pp-dev.config.ts`:
- Line 5: Update the test config to use the new appId option instead of the
deprecated portalPageId: replace the portalPageId property in pp-dev.config.ts
with appId (preserve the numeric value, e.g., 937) and remove the deprecated
portalPageId entry so the test demonstrates current usage of appId consistent
with the type definitions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontests/test-nextjs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
README.mdpackage.jsonsrc/cli.tssrc/lib/internal.middleware.tssrc/lib/load-pp-data.middleware.tssrc/lib/proxy-pass.middleware.tssrc/plugin.tstests/test-nextjs/pp-dev.config.tstests/test-nextjs/src/pages/_document.tsx
| "chokidar": "^4.0.3", | ||
| "minimatch": ">=10.2.1" | ||
| "minimatch": ">=10.2.4", | ||
| "npm": ">=11.11.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the package.json to understand the overrides section
cat -n package.json | sed -n '60,85p'Repository: mi-examples/pp-dev
Length of output: 749
🏁 Script executed:
# Check if npm is actually in the dependency tree
npm ls npm 2>&1 | head -20Repository: mi-examples/pp-dev
Length of output: 130
🏁 Script executed:
# Search for any comments or documentation about this override
rg "npm" package.json -B 3 -A 1 --color=neverRepository: mi-examples/pp-dev
Length of output: 1512
🏁 Script executed:
# Check git history for context on when this override was added
git log --oneline -p package.json 2>/dev/null | grep -A 10 -B 10 '"npm"' | head -50Repository: mi-examples/pp-dev
Length of output: 471
Remove the npm override—it has no effect since npm is not in the dependency tree.
npm ls npm confirms npm is not a dependency in this project, so overriding it serves no purpose. npm is a CLI tool, not a library. If this override was added to address a specific issue, document the reason (e.g., in a comment or PR description) before merging. Otherwise, remove it to keep package.json clean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 71, Remove the unnecessary npm override entry from
package.json (the "npm": ">=11.11.0" line under overrides) because npm is not a
dependency and the override has no effect; if this was intentionally added for a
specific environment issue, instead leave a brief comment in the PR or commit
message documenting that reason and keep the override only after documenting it.
| const loadPPDataMiddleware = initLoadPPData( | ||
| isIndexRegExp, | ||
| mi, | ||
| Object.assign({}, miConfig), | ||
| ); |
There was a problem hiding this comment.
Missing base property in miConfig passed to initLoadPPData.
The initLoadPPData function destructures base from options (line 49 in load-pp-data.middleware.ts) for redirect URL construction. However, miConfig (defined at lines 849-865) doesn't include base. This means base will be undefined, affecting redirect behavior when v7Features is true.
Suggested fix
const loadPPDataMiddleware = initLoadPPData(
isIndexRegExp,
mi,
- Object.assign({}, miConfig),
+ Object.assign({}, miConfig, { base }),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const loadPPDataMiddleware = initLoadPPData( | |
| isIndexRegExp, | |
| mi, | |
| Object.assign({}, miConfig), | |
| ); | |
| const loadPPDataMiddleware = initLoadPPData( | |
| isIndexRegExp, | |
| mi, | |
| Object.assign({}, miConfig, { base }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli.ts` around lines 953 - 957, The miConfig passed into initLoadPPData
is missing the base property which initLoadPPData destructures for redirect URL
construction; update the call so the options include base (e.g.
Object.assign({}, miConfig, { base })) or otherwise ensure miConfig contains
base before calling initLoadPPData so initLoadPPData (from
load-pp-data.middleware.ts) receives a defined base value.
| templateLess, | ||
| miHudLess, | ||
| portalPageId, | ||
| appId: appId!, |
There was a problem hiding this comment.
Non-null assertions on appId may cause runtime errors.
The code uses appId! in multiple places, but the validation at lines 54-55 only throws when templateLess && miHudLess are both true. If either is false but appId is undefined:
handlePageInfoOnlyis called whenisNeedTemplateLoadis false (i.e.,templateLess && miHudLessare true) — this path is protected by validation.handleTemplateLoadat line 150 is called whenisNeedTemplateLoadis true, meaning validation didn't run forappId. IfappIdis undefined here and!templateLess, line 265 would callmi.getPageVariables(undefined!, headers).
Consider adding validation or adjusting the type to handle this edge case.
Suggested validation addition
export function initLoadPPData(
applyUrlRegExp: RegExp,
mi: MiAPI,
opts: PPDevConfig & { base?: string },
): NextHandleFunction {
const { templateLess = false, miHudLess = false, appId, base, v7Features } = opts;
const logger = createLogger();
// Validate required configuration
if (templateLess && miHudLess && typeof appId === 'undefined') {
throw new Error('Custom App ID is required when both templateLess and miHudLess are true');
}
+
+ // Also validate for template variable loading which requires appId
+ if (!templateLess && typeof appId === 'undefined') {
+ throw new Error('App ID is required when templateLess is false');
+ }Also applies to: 101-101, 133-133, 150-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/load-pp-data.middleware.ts` at line 87, The code is using non-null
assertions on appId (e.g., where appId! is passed to object creation and
mi.getPageVariables) but validation only runs for the templateLess && miHudLess
path; add explicit validation or safe handling for appId before any use in the
handleTemplateLoad and any code paths where isNeedTemplateLoad is true.
Specifically, in the middleware entry (before calling handleTemplateLoad and
before calling mi.getPageVariables) check that appId is defined and either throw
a clear error or return an appropriate response, or change call sites
(handleTemplateLoad, handlePageInfoOnly, and the mi.getPageVariables invocation)
to accept appId as optional and guard against undefined instead of using appId!.
Remove the appId! non-null assertions at the call sites to ensure runtime
safety.
| const loadPageData = | ||
| !templateLess && typeof portalPageId !== 'undefined' | ||
| ? mi.getPageVariables(portalPageId, headers) | ||
| !templateLess && typeof appId !== 'undefined' | ||
| ? mi.getPageVariables(appId, headers) | ||
| : mi.getPageTemplate(headers); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "getPageVariables|getPageTemplate" --type ts -C 2Repository: mi-examples/pp-dev
Length of output: 1177
🏁 Script executed:
sed -n '250,280p' src/lib/load-pp-data.middleware.tsRepository: mi-examples/pp-dev
Length of output: 908
🏁 Script executed:
sed -n '230,265p' src/lib/load-pp-data.middleware.tsRepository: mi-examples/pp-dev
Length of output: 977
🏁 Script executed:
rg -n "interface LoadPPDataOptions|type LoadPPDataOptions" --type ts -A 10Repository: mi-examples/pp-dev
Length of output: 793
🏁 Script executed:
rg -n "handleTemplateLoad" --type ts -B 5 -A 2Repository: mi-examples/pp-dev
Length of output: 1414
🏁 Script executed:
sed -n '90,115p' src/lib/load-pp-data.middleware.tsRepository: mi-examples/pp-dev
Length of output: 766
🏁 Script executed:
sed -n '140,165p' src/lib/load-pp-data.middleware.tsRepository: mi-examples/pp-dev
Length of output: 649
🏁 Script executed:
sed -n '40,95p' src/lib/load-pp-data.middleware.tsRepository: mi-examples/pp-dev
Length of output: 1887
🏁 Script executed:
rg -n "templateLess" --type ts -B 2 -A 2 | head -60Repository: mi-examples/pp-dev
Length of output: 3566
🏁 Script executed:
sed -n '195,215p' src/lib/pp.middleware.ts && echo "---" && sed -n '340,360p' src/lib/pp.middleware.tsRepository: mi-examples/pp-dev
Length of output: 1147
Consider updating LoadPPDataOptions.appId type definition for clarity.
The type definition declares appId: number (required), but the code defensively checks typeof appId !== 'undefined' before using it. Additionally, the validation at initialization only requires appId when both templateLess && miHudLess are true, suggesting appId can be undefined in other scenarios. To align the type definition with runtime behavior, change appId: number; to appId?: number; in the LoadPPDataOptions interface (line 13). This will eliminate the defensive check and make the conditional logic intention clearer.
Note: The fallback to getPageTemplate when templateLess is false but appId is undefined is safe because getPageVariables calls getPageTemplate internally (line 345 in pp.middleware.ts).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/load-pp-data.middleware.ts` around lines 263 - 266, Update the
LoadPPDataOptions interface so appId is optional (change appId: number to
appId?: number) to reflect runtime checks; this aligns the type with the
conditional usage around templateLess and miHudLess and the call site that uses
typeof appId !== 'undefined' before invoking mi.getPageVariables, ensuring
callers can omit appId and the fallback to mi.getPageTemplate remains valid.
| } catch { | ||
| // | ||
| } |
There was a problem hiding this comment.
Silent error swallowing hinders debugging.
The empty catch block masks failures in URL parsing, header manipulation, and script injection. Consider logging the error at debug level to aid troubleshooting without breaking the proxy flow.
🐛 Proposed fix to log errors
- } catch {
- //
+ } catch (e) {
+ logger.info(`${colors.blue('Response augmentation skipped:')} ${e instanceof Error ? e.message : e}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch { | |
| // | |
| } | |
| } catch (e) { | |
| logger.info(`${colors.blue('Response augmentation skipped:')} ${e instanceof Error ? e.message : e}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/proxy-pass.middleware.ts` around lines 284 - 286, Replace the empty
catch block in the proxy-pass.middleware.ts catch section with a debug-level log
of the caught error so failures in URL parsing/header manipulation/script
injection are visible but the proxy flow remains intact; e.g. capture the error
as (err) and call the module's logger at debug level (logger.debug(...) or
processLogger.debug(...), falling back to console.debug) inside the catch in the
proxyPass middleware / handleProxyPassRequest function to record a descriptive
message and the error object.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/pp.middleware.ts (1)
16-24:⚠️ Potential issue | 🟡 Minor
appIdis declared inMiAPIOptionsbut not consumed by theMiAPIconstructor.While the interface exposes
appIdas a valid option (intended as a synonym forportalPageId), the constructor only destructures and usesportalPageId. Currently, this works because callers normalize both fields to the same value before passing to the constructor, but this creates fragile API design. IfappIdis the only field provided, it will be silently ignored.The constructor should implement the fallback logic directly for consistency with the interface contract:
🔧 Proposed fix
constructor(baseURL: string, opts?: MiAPIOptions) { const { headers = {}, portalPageId, + appId, templateLess = true, disableSSLValidation = false, v7Features = false, personalAccessToken, } = opts || {}; // ... existing code ... - this.portalPageId = portalPageId; + this.portalPageId = appId ?? portalPageId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/pp.middleware.ts` around lines 16 - 24, The MiAPI constructor currently only reads portalPageId while MiAPIOptions also exposes appId; update the MiAPI constructor to honor appId as a synonym for portalPageId by applying a fallback (e.g., compute const portalPageIdFinal = portalPageId ?? appId) when destructuring options and then use portalPageIdFinal throughout the constructor and any methods that rely on portalPageId; modify the constructor parameter handling in the MiAPI class and any local references to portalPageId so callers can supply either appId or portalPageId.
♻️ Duplicate comments (1)
src/lib/proxy-pass.middleware.ts (1)
284-286:⚠️ Potential issue | 🟡 MinorResponse rewrite errors are still silently swallowed.
Line 284-285 still hides parse/injection failures, which makes proxy rewrite issues hard to diagnose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy-pass.middleware.ts` around lines 284 - 286, The empty catch block swallowing errors during response rewrite must be replaced with proper logging and context so parse/injection failures are visible; update the catch in src/lib/proxy-pass.middleware.ts (the try/catch around the response rewrite logic) to catch the error as e (catch (e)) and call the module logger (or processLogger) to log a warning/error that includes the error object plus identifying context (e.g., the URL, request id, or the proxy handler name like the response-rewrite block) so failures are not silent—do not suppress the exception silently (optionally rethrow or return a fallback response if the surrounding logic requires it).
🧹 Nitpick comments (5)
src/lib/load-pp-data.middleware.ts (1)
31-32: Minor: Unnecessary blank line in cached response check.The blank line between
apiResponseCache.delete(key);andreturn null;is inconsistent with the rest of the file's formatting.Suggested fix
if (Date.now() - cached.timestamp > CACHE_TTL) { apiResponseCache.delete(key); - return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/load-pp-data.middleware.ts` around lines 31 - 32, Remove the extraneous blank line between the cached-response deletion and the return in the cached check: in the function where apiResponseCache.delete(key) is called, remove the blank line before the subsequent return null; so the two statements appear consecutively and match the file's existing formatting style.src/lib/helpers/url.helper.ts (2)
20-37: Static analysis flags potential ReDoS - context mitigates risk.The static analysis tool flags
originalHostbeing interpolated into a RegExp pattern. While this is technically a ReDoS vector, in this codebaseoriginalHostderives frombackendBaseURLconfiguration (trusted source), not user input. The current usage is safe, but if this helper is ever exposed to untrusted input, consider escaping regex metacharacters.Optional defensive fix if needed in future
+function escapeRegExp(string: string): string { + return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + export const urlReplacer = ( originalHost: string, destinationHost: string, content: string, ) => { + const escapedHost = escapeRegExp(originalHost); const urlReplaceRegExp = new RegExp( - `(!!)?(https?(:(\\\\)?/(\\\\)?/)${originalHost})`, + `(!!)?(https?(:(\\\\)?/(\\\\)?/)${escapedHost})`, 'gi', );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/url.helper.ts` around lines 20 - 37, The RegExp built in urlReplacer interpolates originalHost directly into the pattern, which can be unsafe if originalHost ever comes from untrusted input; update urlReplacer to escape regex metacharacters in originalHost before constructing urlReplaceRegExp (e.g., add or use a helper like escapeRegExp and run originalHost through it), so urlReplaceRegExp = new RegExp(`(!!)?(https?(:(\\\\)?/(\\\\)?/)${escapedHost})`, 'gi'); keep the existing replacement logic intact and ensure the escape helper is deterministic and tested.
39-55: Similar ReDoS consideration forurlPathReplacer.Same consideration as above -
urlPathcomes from configuration. The existing.replace(/\\*\//gi, '\\\\/')at line 45 attempts partial escaping but doesn't fully sanitize regex metacharacters. Low priority given trusted input sources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/url.helper.ts` around lines 39 - 55, The urlPathReplacer uses a fragile partial escape (`urlPath.replace(/\\*\//gi, '\\\\/')`) that doesn't escape all regex metacharacters; replace it by fully escaping urlPath before building RegExp. Implement/inline an escapeRegExp function that returns urlPath.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), then use new RegExp(escapedUrlPath, 'gi') for urlReplaceRegExp and, if you still need the literal-match fallback, use escapedUrlPath without the partial replace for unescapedUrlReplaceRegExp; update references in urlPathReplacer to use the new escaped string.src/lib/proxy-cache.middleware.ts (1)
128-142: Return the normalized URL as the cache key.Line 130 strips query params into
cleanUrl, but Line 141 returnsurl. That keeps query variants as separate keys and weakens cache-hit improvements.♻️ Proposed fix
function generateCacheKey(url: string): string { // Remove query parameters for better cache hit rates const cleanUrl = url.split('?')[0]; @@ if (cleanUrl.includes('/auth/info.js')) { return ''; } - return url; + return cleanUrl; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy-cache.middleware.ts` around lines 128 - 142, The generateCacheKey function currently computes a normalized cleanUrl (stripping query params) but returns the original url, so query-string variants bypass normalization; update generateCacheKey to return cleanUrl (after the FILE_EXTENSION_REGEX and auth check) instead of url so cache keys use the normalized path; ensure you still perform the FILE_EXTENSION_REGEX.test against cleanUrl and the auth/info.js exclusion using cleanUrl before returning it from generateCacheKey.src/plugin.ts (1)
827-827: Remove request-path debug logging from middleware predicate.Line 827 prints on each evaluation and adds avoidable noise in dev-server output.
♻️ Proposed fix
(url) => { - console.log('initRewriteResponse.url', url); - return url.split('?')[0].endsWith('index.html'); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin.ts` at line 827, Remove the noisy debug log call in the middleware predicate by deleting the console.log('initRewriteResponse.url', url) statement inside the initRewriteResponse implementation (the middleware predicate function that evaluates request paths); if conditional debug output is desired instead, wrap the log in a dev-only or verbose-logging check (e.g., an existing debug flag or process.env.NODE_ENV === 'development') so normal dev-server output is not flooded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/constants.ts`:
- Around line 25-27: Remove the debug console.log statements that print
PP_DEV_PACKAGE_DIR, afterBundlePath, and beforeBundlePath on module load; locate
the console.log calls referencing the variables PP_DEV_PACKAGE_DIR,
afterBundlePath, and beforeBundlePath in the constants module and delete them
(or replace with a conditional debug logger behind a debug flag if runtime
diagnostics are required) so the module no longer emits these messages in
production.
In `@src/lib/proxy-pass.middleware.ts`:
- Around line 139-142: The referer replacement injects req.headers.host
(variable host) directly into new RegExp, allowing attacker-controlled regex
metacharacters; fix by escaping host before constructing the RegExp used in
proxyReq.setHeader. Add or reuse a utility like escapeRegExp that replaces
special regex chars (e.g. .[](){}*+?^$\\|) with escaped versions, and use
RegExp(escapeRegExp(host)) (or a properly anchored pattern) when calling
referer.replace, ensuring you also handle undefined host/referer and preserve
existing baseURL usage in the proxyReq.setHeader call.
In `@src/plugin.ts`:
- Around line 592-593: initLoadPPData is being called with the raw options
object so normalizedAppId can be missing downstream; update the call site that
currently passes initLoadPPData(isIndexRegExp, mi, Object.assign({}, options))
to explicitly include the normalizedAppId value (the computed normalizedAppId
variable) in the arguments so initLoadPPData receives appId deterministically
even when only portalPageId was set; locate the caller using the symbol
initLoadPPData and the surrounding variables isIndexRegExp, mi and options and
add normalizedAppId as a named/positional parameter per the function's
signature.
- Around line 724-798: The request handler currently only handles tokenType ===
'personal' and 'regular' and can hang for any other or missing tokenType; add a
final else block after the existing branches (near the tokenType checks around
the mi.get calls) that calls sendErrorResponse(res, 400, 'Unsupported or missing
tokenType') (or 422 if you prefer), ensures no further processing (return) and
does not attempt redirects or mi operations, so all code paths terminate; reuse
existing helpers like sendErrorResponse and ensure handleTokenValidationError
remains unchanged.
---
Outside diff comments:
In `@src/lib/pp.middleware.ts`:
- Around line 16-24: The MiAPI constructor currently only reads portalPageId
while MiAPIOptions also exposes appId; update the MiAPI constructor to honor
appId as a synonym for portalPageId by applying a fallback (e.g., compute const
portalPageIdFinal = portalPageId ?? appId) when destructuring options and then
use portalPageIdFinal throughout the constructor and any methods that rely on
portalPageId; modify the constructor parameter handling in the MiAPI class and
any local references to portalPageId so callers can supply either appId or
portalPageId.
---
Duplicate comments:
In `@src/lib/proxy-pass.middleware.ts`:
- Around line 284-286: The empty catch block swallowing errors during response
rewrite must be replaced with proper logging and context so parse/injection
failures are visible; update the catch in src/lib/proxy-pass.middleware.ts (the
try/catch around the response rewrite logic) to catch the error as e (catch (e))
and call the module logger (or processLogger) to log a warning/error that
includes the error object plus identifying context (e.g., the URL, request id,
or the proxy handler name like the response-rewrite block) so failures are not
silent—do not suppress the exception silently (optionally rethrow or return a
fallback response if the surrounding logic requires it).
---
Nitpick comments:
In `@src/lib/helpers/url.helper.ts`:
- Around line 20-37: The RegExp built in urlReplacer interpolates originalHost
directly into the pattern, which can be unsafe if originalHost ever comes from
untrusted input; update urlReplacer to escape regex metacharacters in
originalHost before constructing urlReplaceRegExp (e.g., add or use a helper
like escapeRegExp and run originalHost through it), so urlReplaceRegExp = new
RegExp(`(!!)?(https?(:(\\\\)?/(\\\\)?/)${escapedHost})`, 'gi'); keep the
existing replacement logic intact and ensure the escape helper is deterministic
and tested.
- Around line 39-55: The urlPathReplacer uses a fragile partial escape
(`urlPath.replace(/\\*\//gi, '\\\\/')`) that doesn't escape all regex
metacharacters; replace it by fully escaping urlPath before building RegExp.
Implement/inline an escapeRegExp function that returns
urlPath.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), then use new
RegExp(escapedUrlPath, 'gi') for urlReplaceRegExp and, if you still need the
literal-match fallback, use escapedUrlPath without the partial replace for
unescapedUrlReplaceRegExp; update references in urlPathReplacer to use the new
escaped string.
In `@src/lib/load-pp-data.middleware.ts`:
- Around line 31-32: Remove the extraneous blank line between the
cached-response deletion and the return in the cached check: in the function
where apiResponseCache.delete(key) is called, remove the blank line before the
subsequent return null; so the two statements appear consecutively and match the
file's existing formatting style.
In `@src/lib/proxy-cache.middleware.ts`:
- Around line 128-142: The generateCacheKey function currently computes a
normalized cleanUrl (stripping query params) but returns the original url, so
query-string variants bypass normalization; update generateCacheKey to return
cleanUrl (after the FILE_EXTENSION_REGEX and auth check) instead of url so cache
keys use the normalized path; ensure you still perform the
FILE_EXTENSION_REGEX.test against cleanUrl and the auth/info.js exclusion using
cleanUrl before returning it from generateCacheKey.
In `@src/plugin.ts`:
- Line 827: Remove the noisy debug log call in the middleware predicate by
deleting the console.log('initRewriteResponse.url', url) statement inside the
initRewriteResponse implementation (the middleware predicate function that
evaluates request paths); if conditional debug output is desired instead, wrap
the log in a dev-only or verbose-logging check (e.g., an existing debug flag or
process.env.NODE_ENV === 'development') so normal dev-server output is not
flooded.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsontests/test-commonjs/package-lock.jsonis excluded by!**/package-lock.jsontests/test-nextjs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
README.mdpackage.jsonsrc/cli.tssrc/config.tssrc/constants.tssrc/lib/auth.provider.tssrc/lib/client.service.tssrc/lib/dist.service.tssrc/lib/helpers/login.helper.tssrc/lib/helpers/server.tssrc/lib/helpers/token.helper.tssrc/lib/helpers/url.helper.tssrc/lib/internal.middleware.tssrc/lib/load-pp-data.middleware.tssrc/lib/next-import.tssrc/lib/pp.middleware.tssrc/lib/proxy-cache.middleware.tssrc/lib/proxy-pass.middleware.tssrc/plugin.tssrc/shortcuts.tstests/test-nextjs/pp-dev.config.tstests/test-nextjs/src/pages/_document.tsx
1 participant
Summary
Updates dependencies, adds
appIdoption, fixes internal server restart behavior, and improves proxy middleware handling of login pages.Changes
Features
appIdconfig option with fallback toportalPageIdfor custom app IDs when usingtemplateLessandmiHudLesscreateInternalServer()to fixthis.route is not a functionerrors when server restarts after config changes (e.g..env)Fixes
Cache-Control: no-cacheheaders for/loginresponses to prevent cached login pages/loginpathChore
Tests
Commits
Summary by CodeRabbit
Release Notes
New Features
appIdas a primary configuration option for application identificationBug Fixes
Chores