feat(request-filters): advanced mode operations and execution engine#912
Merged
Conversation
…on phase Introduce two new axes for request filters: rule mode (simple/advanced) and execution phase (guard/final). Advanced mode supports a JSON-defined operation DSL with set, remove, merge, and insert operations. The final phase executes after all provider overrides, enabling post-processing use cases like system message injection with dedup and cache_control manipulation. Key changes: - Schema: add rule_mode, execution_phase, operations columns + index - Types: new FilterOperation DSL (SetOp, RemoveOp, MergeOp, InsertOp) - Engine: 4-bucket cache split, applyFinal() method, operation executors with deep merge (null-as-delete), deep equal dedup, anchor matching - Forwarder: integrate applyFinal in standard and Gemini branches - Validation: validateOperations with ReDoS prevention via safe-regex - UI: rule mode toggle, execution phase selector, JSON operations editor - i18n: new keys for all 5 locales (en, zh-CN, zh-TW, ja, ru) - Tests: 29 new tests covering all operation types and edge cases
…on and ReDoS bypass - Block __proto__/constructor/prototype traversal in parsePath, deepMerge, and validation - Add runtime safe-regex check in matchElement for regex matchers - Fix ReDoS bypass in updateRequestFilterAction by checking effective target/matchType/action - Add operations array length limit (max 50) and value validation for set/merge/insert ops - Fix UI default executionPhase to match server default (guard, not final) - Fix parsePath regex corruption from edit tool (double-escaped backslashes)
- Remove console.error from filter-dialog.tsx (error already shown via toast) - Fix ja table.actions translation to "操作" - Fix ru table.createdAt to "Дата создания" - Fix zh-CN confirmDelete halfwidth ? to fullwidth ？ - Show operations count in target column for advanced mode filters - Add aria-label to filter toggle Switch for accessibility
📝 WalkthroughWalkthrough添加高级请求过滤器支持：引入规则模式（simple/advanced）、执行阶段（guard/final）与JSON操作序列；扩展数据库模式、类型定义、验证与引擎执行路径；在代理转发中加入最终阶段过滤；新增 UI 和多语言文案及全面单元测试覆盖。 Changes
估计代码审查工作量🎯 4 (复杂) | ⏱️ ~75 分钟 可能相关的PR
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the request filtering capabilities by introducing an 'advanced mode' that allows for highly granular and programmatic manipulation of request headers and bodies. This new mode operates through a defined DSL, enabling complex operations like deep merging, conditional insertions, and targeted removals. Crucially, these advanced filters are now applied in a dedicated 'final execution phase' within the proxy, ensuring they take effect after all other provider-specific overrides. The changes also include critical security measures to prevent common vulnerabilities like prototype pollution and ReDoS, alongside comprehensive UI updates for managing these new filter types and full internationalization support. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
Bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an "advanced mode" for request filters, enabling more granular manipulation of request bodies and headers through a new Domain Specific Language (DSL) of operations (set, remove, merge, insert). Key changes include database schema updates to store rule_mode, execution_phase, and operations, new UI components for configuring these advanced filters, and extensive backend logic for validating and executing the operations, with built-in protections against ReDoS and prototype pollution. The filter engine was refactored to support two execution phases: "guard" (before provider selection) and "final" (after provider overrides), with advanced operations primarily targeting the "final" phase. However, the current implementation has two critical issues: the applyFinal filter logic is not being applied to Gemini requests that lack a body, preventing header modifications, and the general applyFinal logic is incorrectly placed inside a hasBody check, meaning it won't execute for any requests without a body, thus failing to apply header-modifying filters.
Comment on lines
+1939
to
+1945
| processedHeaders = ProxyForwarder.buildGeminiHeaders( | ||
| session, | ||
| provider, | ||
| effectiveBaseUrl, | ||
| accessToken, | ||
| isApiKey | ||
| ); |
Contributor
There was a problem hiding this comment.
Final-phase filters are not being applied to Gemini requests that don't have a body (e.g., GET requests). These filters might need to modify headers, so applyFinal should be called here in the else block as well, passing an empty object for the body.
processedHeaders = ProxyForwarder.buildGeminiHeaders(
session,
provider,
effectiveBaseUrl,
accessToken,
isApiKey
);
// Final-phase request filter for Gemini (no body)
if (!session.getEndpointPolicy().bypassRequestFilters) {
const { requestFilterEngine } = await import("@/lib/request-filter-engine");
await requestFilterEngine.applyFinal(session, {}, processedHeaders);
}
Comment on lines
+2234
to
+2238
| // Final-phase request filter: after all provider overrides, before serialization | ||
| if (!session.getEndpointPolicy().bypassRequestFilters) { | ||
| const { requestFilterEngine } = await import("@/lib/request-filter-engine"); | ||
| await requestFilterEngine.applyFinal(session, messageToSend, processedHeaders); | ||
| } |
Contributor
There was a problem hiding this comment.
Final-phase filters are not applied to requests without a body (e.g., GET requests), as this logic is inside an if (hasBody) check. This logic should be moved outside the if (hasBody) block to ensure filters can operate on headers for all request types. For requests without a body, an empty object {} can be passed to applyFinal.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+540
to
+563
| const allFinal = this.collectFinalFilters(session); | ||
| if (allFinal.length === 0) return; | ||
|
|
||
| for (const filter of allFinal) { | ||
| try { | ||
| if (filter.ruleMode === "advanced" && filter.operations) { | ||
| this.executeAdvancedOps(filter.operations, body, headers); | ||
| } else { | ||
| // simple mode: reuse existing logic but on body/headers directly | ||
| this.applySimpleFilterDirect(filter, body, headers); | ||
| } | ||
| } catch (error) { | ||
| logger.error("[RequestFilterEngine] Failed to apply final filter", { | ||
| filterId: filter.id, | ||
| ruleMode: filter.ruleMode, | ||
| error, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Transport header blacklist enforcement | ||
| for (const h of TRANSPORT_HEADER_BLACKLIST) { | ||
| headers.delete(h); | ||
| } |
There was a problem hiding this comment.
Transport header blacklist silently skipped when no final-phase filters match
The early-return on line 541 exits the function before the transport header blacklist loop at lines 560–563 runs. This means if the current session has zero matching final-phase filters, the blacklist (content-length, connection, transfer-encoding) is never enforced on processedHeaders.
Although guard-phase filters operate on session.headers (a different object), advanced-mode operations in other branches of the same request, or future forwarder changes, could inject transport headers into processedHeaders. The intent of adding the blacklist here was to provide an absolute safety net, but the current structure makes it conditional on filter presence.
The blacklist enforcement should be moved before the early return so it always runs:
async applyFinal(
session: ProxySession,
body: Record<string, unknown>,
headers: Headers
): Promise<void> {
await this.ensureInitialized();
// Always enforce transport header blacklist regardless of filter count
for (const h of TRANSPORT_HEADER_BLACKLIST) {
headers.delete(h);
}
const allFinal = this.collectFinalFilters(session);
if (allFinal.length === 0) return;
for (const filter of allFinal) {
// ...
}
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/request-filter-engine.ts
Line: 540-563
Comment:
**Transport header blacklist silently skipped when no final-phase filters match**
The early-return on line 541 exits the function before the transport header blacklist loop at lines 560–563 runs. This means if the current session has **zero** matching final-phase filters, the blacklist (`content-length`, `connection`, `transfer-encoding`) is never enforced on `processedHeaders`.
Although guard-phase filters operate on `session.headers` (a different object), advanced-mode operations in other branches of the same request, or future forwarder changes, could inject transport headers into `processedHeaders`. The intent of adding the blacklist here was to provide an absolute safety net, but the current structure makes it conditional on filter presence.
The blacklist enforcement should be moved **before** the early return so it always runs:
```
async applyFinal(
session: ProxySession,
body: Record<string, unknown>,
headers: Headers
): Promise<void> {
await this.ensureInitialized();
// Always enforce transport header blacklist regardless of filter count
for (const h of TRANSPORT_HEADER_BLACKLIST) {
headers.delete(h);
}
const allFinal = this.collectFinalFilters(session);
if (allFinal.length === 0) return;
for (const filter of allFinal) {
// ...
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+92
to
+101
| if (op.op === "merge") { | ||
| if ( | ||
| !("value" in raw) || | ||
| raw.value === null || | ||
| typeof raw.value !== "object" || | ||
| Array.isArray(raw.value) | ||
| ) { | ||
| return `${prefix}: merge value must be a plain object`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Merge op.value not scanned for prototype-polluting keys
validateOperations guards against prototype pollution in op.path (line 84), but the recursive value payload of a merge operation is only checked to be a plain object — its keys are never examined. An admin could submit:
{ "op": "merge", "scope": "body", "path": "meta", "value": { "__proto__": { "isAdmin": true } } }This passes server-side validation (the value is a non-null, non-array object), is stored in the DB, and reaches the engine. The engine's deepMerge correctly blocks the __proto__ key, but the defense should exist in both layers. Server-side validation should recursively scan merge values for keys matching UNSAFE_KEYS and reject the payload proactively.
function hasPollutingKeys(obj: Record<string, unknown>): boolean {
for (const key of Object.keys(obj)) {
if (UNSAFE_KEYS.has(key)) return true; // UNSAFE_KEYS is a Set
const val = obj[key];
if (val && typeof val === "object" && !Array.isArray(val)) {
if (hasPollutingKeys(val as Record<string, unknown>)) return true;
}
}
return false;
}Where UNSAFE_KEYS is exported or duplicated from the engine, and called after the plain-object check for merge ops.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/request-filters.ts
Line: 92-101
Comment:
**Merge `op.value` not scanned for prototype-polluting keys**
`validateOperations` guards against prototype pollution in `op.path` (line 84), but the recursive `value` payload of a `merge` operation is only checked to be a plain object — its **keys** are never examined. An admin could submit:
```json
{ "op": "merge", "scope": "body", "path": "meta", "value": { "__proto__": { "isAdmin": true } } }
```
This passes server-side validation (the value is a non-null, non-array object), is stored in the DB, and reaches the engine. The engine's `deepMerge` correctly blocks the `__proto__` key, but the defense should exist in both layers. Server-side validation should recursively scan merge values for keys matching `UNSAFE_KEYS` and reject the payload proactively.
```typescript
function hasPollutingKeys(obj: Record<string, unknown>): boolean {
for (const key of Object.keys(obj)) {
if (UNSAFE_KEYS.has(key)) return true; // UNSAFE_KEYS is a Set
const val = obj[key];
if (val && typeof val === "object" && !Array.isArray(val)) {
if (hasPollutingKeys(val as Record<string, unknown>)) return true;
}
}
return false;
}
```
Where `UNSAFE_KEYS` is exported or duplicated from the engine, and called after the plain-object check for `merge` ops.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+178
to
+190
| if (matcher.field) { | ||
| if (element === null || element === undefined || typeof element !== "object") return false; | ||
| // Support dot-path extraction | ||
| const parts = matcher.field.split("."); | ||
| let cur: unknown = element; | ||
| for (const part of parts) { | ||
| if (cur === null || cur === undefined || typeof cur !== "object") return false; | ||
| cur = (cur as Record<string, unknown>)[part]; | ||
| } | ||
| fieldValue = cur; | ||
| } else { | ||
| fieldValue = element; | ||
| } |
There was a problem hiding this comment.
matchElement field traversal does not check UNSAFE_KEYS
Every other path-traversal helper in this file (parsePath, deepMerge) checks against UNSAFE_KEYS. However, the dot-path traversal inside matchElement at lines 181–186 splits matcher.field on "." and indexes directly into the element without any guard:
const parts = matcher.field.split(".");
let cur: unknown = element;
for (const part of parts) {
cur = (cur as Record<string, unknown>)[part]; // no UNSAFE_KEYS check
}A FilterMatcher with field: "__proto__" or field: "constructor.prototype" will follow the prototype chain of the element object. While this is a read-only path and cannot mutate state, it breaks the principle of least surprise, reads internal JavaScript properties rather than own-enumerable data, and may return misleading truthy values that affect filter decisions (e.g., constructor always exists on any object).
Add the guard:
for (const part of parts) {
if (UNSAFE_KEYS.has(part)) return false;
if (cur === null || cur === undefined || typeof cur !== "object") return false;
cur = (cur as Record<string, unknown>)[part];
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/request-filter-engine.ts
Line: 178-190
Comment:
**`matchElement` field traversal does not check `UNSAFE_KEYS`**
Every other path-traversal helper in this file (`parsePath`, `deepMerge`) checks against `UNSAFE_KEYS`. However, the dot-path traversal inside `matchElement` at lines 181–186 splits `matcher.field` on `"."` and indexes directly into the element without any guard:
```typescript
const parts = matcher.field.split(".");
let cur: unknown = element;
for (const part of parts) {
cur = (cur as Record<string, unknown>)[part]; // no UNSAFE_KEYS check
}
```
A `FilterMatcher` with `field: "__proto__"` or `field: "constructor.prototype"` will follow the prototype chain of the element object. While this is a read-only path and cannot mutate state, it breaks the principle of least surprise, reads internal JavaScript properties rather than own-enumerable data, and may return misleading truthy values that affect filter decisions (e.g., `constructor` always exists on any object).
Add the guard:
```typescript
for (const part of parts) {
if (UNSAFE_KEYS.has(part)) return false;
if (cur === null || cur === undefined || typeof cur !== "object") return false;
cur = (cur as Record<string, unknown>)[part];
}
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/v1/_lib/proxy/forwarder.ts (1)
1839-1848:⚠️ Potential issue | 🟠 MajorGemini final-phase 会把 DSL 改动写回
session.request.message。这里把
session.request.message的同一对象引用直接传给了applyFinal()，而applyFinal()会原地修改body。一旦当前请求重试或切换供应商，insert/text_replace这类操作会在已经改过的 payload 上再次执行，结果会漂移。建议像标准分支一样，在 provider override 之后先克隆，再把克隆体交给 final-phase。Also applies to: 1910-1914
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 1839 - 1848, The code passes the original session.request.message reference into final-phase where applyFinal() mutates the body in-place, causing repeated mutations on retries/provider switches; after applying provider overrides (applyGeminiGoogleSearchOverrideWithAudit) clone the overriddenBody (e.g., deep clone bodyToSerialize) and assign the clone to bodyToSerialize and session.request.message before calling applyFinal(), so applyFinal() mutates the clone not the original; update the same pattern in the other occurrence mentioned (around the applyFinal() call at the 1910-1914 section) and ensure session.addSpecialSetting(googleSearchAudit) still receives the audit without changing the original message object.src/lib/request-filter-engine.ts (1)
35-47:⚠️ Potential issue | 🟠 Major不安全路径不应该被静默改写。
这里命中
UNSAFE_KEYS时选择continue，会把a.__proto__.b解析成a.b，把本该拒绝的操作重定向到另一条合法路径。这样虽然挡住了 prototype pollution，但仍然会误删/误写正常字段。建议把整条 path 判为无效
function parsePath(path: string): Array<string | number> { const parts: Array<string | number> = []; const regex = /([^.[\]]+)|(\[(\d+)\])/g; let match: RegExpExecArray | null; while ((match = regex.exec(path)) !== null) { if (match[1]) { - if (UNSAFE_KEYS.has(match[1])) continue; // block prototype pollution + if (UNSAFE_KEYS.has(match[1])) return []; parts.push(match[1]); } else if (match[3]) { parts.push(Number(match[3])); } } return parts; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/request-filter-engine.ts` around lines 35 - 47, The parser silently skips unsafe segments (UNSAFE_KEYS) so "a.__proto__.b" becomes "a.b" and wrongly targets a valid property; change parsePath to treat the entire path as invalid when any unsafe key is encountered: in parsePath (the function using UNSAFE_KEYS and regex/match logic) replace the continue with immediate failure (e.g., throw a clear Error like "Invalid path: contains unsafe key" or return a null/Result indicating invalid path) and update callers to handle that failure instead of proceeding with a sanitized path.
🧹 Nitpick comments (3)
src/drizzle/schema.ts (1)
640-640: 为operations补齐 JSONB 默认值与显式类型，保持迁移一致性。Line 640 目前没有
.default(null)，后续生成迁移时会与仓库既有 JSONB 默认约定不一致，且类型推断也偏弱。Based on learnings: Maintain the existing JSONB default pattern `jsonb DEFAULT 'null'::jsonb` in drizzle migrations and keep consistency across migration files.建议修改
- operations: jsonb('operations'), + operations: jsonb('operations').$type<Array<Record<string, unknown>> | null>().default(null),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drizzle/schema.ts` at line 640, The operations column currently lacks the JSONB default and explicit type which causes inconsistent migrations and weak type inference; update the operations column definition (symbol: operations in schema.ts) to include the JSONB default pattern used across the repo by adding the Drizzle SQL default (e.g. .default(sql`'null'::jsonb`)) and ensure the column is declared with jsonb('operations') so migrations emit "jsonb DEFAULT 'null'::jsonb" and type inference remains strong.src/app/[locale]/settings/request-filters/_components/filter-table.tsx (1)
284-284: 建议优化 Switch 的 aria-label 语义。Line 284 目前使用启用/禁用状态词作为 label，读屏体验可再提升为“该开关的用途描述”，状态由控件自身
checked传达即可。可选修改示例
- aria-label={filter.isEnabled ? t("enable") : t("disable")} + aria-label={`${filter.name} ${t("table.status")}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/request-filters/_components/filter-table.tsx at line 284, 当前 Switch 的 aria-label 使用启用/禁用词（aria-label={filter.isEnabled ? t("enable") : t("disable")），应改为描述开关用途的语义文本，状态由控件的 checked/aria-checked 属性传达即可。请在 Switch 组件（使用 aria-label 的那一处）改为返回描述性标签，例如基于 filter.name 或 filter.label 构建的文本（例如 t("toggleFilter", { name: filter.name }) 或 t("filterToggleDescription", { label: filter.label })），同时保留 checked={filter.isEnabled} 不变，以便读屏器能读出用途和当前状态。src/actions/request-filters.ts (1)
276-291: 可优化：多次数据库查询同一记录。
updateRequestFilterAction中可能对同一id调用多达 3 次getRequestFilterById（分别在 Line 278、311、334）。建议在函数开头统一获取一次，减少数据库往返。♻️ 建议优化
): Promise<ActionResult<RequestFilter>> { const session = await getSession(); if (!isAdmin(session)) return { ok: false, error: "权限不足" }; + // Fetch existing filter once for all validations + const needsExisting = + updates.ruleMode !== undefined || + updates.operations !== undefined || + updates.target !== undefined || + updates.matchType !== undefined || + updates.action !== undefined || + updates.bindingType !== undefined || + updates.providerIds !== undefined || + updates.groupTags !== undefined; + + let existing: RequestFilter | null = null; + if (needsExisting) { + existing = await getRequestFilterById(id); + if (!existing) { + return { ok: false, error: "记录不存在" }; + } + } // Validate operations when ruleMode or operations change if (updates.ruleMode !== undefined || updates.operations !== undefined) { - const existing = await getRequestFilterById(id); - if (!existing) { - return { ok: false, error: "记录不存在" }; - } - const effectiveRuleMode = updates.ruleMode ?? existing.ruleMode;后续使用
existing的地方同理修改，移除重复的getRequestFilterById调用。Also applies to: 306-317, 334-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/request-filters.ts` around lines 276 - 291, The PR makes multiple redundant calls to getRequestFilterById inside updateRequestFilterAction; fetch the record once at the start (e.g., const existing = await getRequestFilterById(id)) and reuse that variable for all subsequent validations and updates (including where effectiveRuleMode/effectiveOperations are computed and later checks around Lines ~306-317 and ~334-344), remove the additional getRequestFilterById calls and rely on the single existing object to perform validateOperations, compare fields, and apply updates.
🤖 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/app/v1/_lib/proxy/forwarder.ts`:
- Around line 1910-1914: The GET/HEAD and other no-body code paths currently
skip the final-phase request filter because applyFinal(...) is only invoked
inside the body branch; update the forwarder to call
requestFilterEngine.applyFinal(session, bodyToSerialize, processedHeaders) after
processedHeaders is constructed in the no-body branches as well (respecting
session.getEndpointPolicy().bypassRequestFilters), and ensure the same change is
applied where similar logic appears (around the blocks you noted: lines near
1919-1945 and 2234-2238); reference requestFilterEngine, applyFinal,
session.getEndpointPolicy().bypassRequestFilters, bodyToSerialize, and
processedHeaders so the final-phase rules run for header-only requests like
GET/HEAD and /v1/models.
In `@src/lib/request-filter-engine.ts`:
- Around line 367-419: loadFilters currently buckets filters with
(executionPhase ?? "guard") === "guard" but the guard code path still only uses
the old applyHeaderFilter()/applyBodyFilter() logic and never runs
filter.operations, so advanced guard rules become no-ops; update the guard phase
to either (A) execute the DSL operations for guard filters by invoking the
existing DSL executor on the cached.filter.operations (reuse whatever function
runs advanced rules for final phase—call it from the guard path) for filters
loaded by loadFilters(), ensuring compiledRegex/provider/group set lookups still
apply, or (B) if you prefer prohibition, add validation that rejects filters
with executionPhase === "guard" when they contain advanced filter.operations;
modify the code paths that currently call applyHeaderFilter/applyBodyFilter to
also handle filter.operations (or prevent creation via validation) so
advanced+guard combos are not silently ineffective.
In `@src/lib/request-filter-types.ts`:
- Around line 26-28: The BaseOp's scope is too broad for ops that only support
body paths; narrow the scope for the specific operation interfaces so that the
merge and insert operation types (e.g., the MergeOp / InsertOp interfaces or
type aliases that currently extend BaseOp) require scope: "body" instead of
inheriting "header" | "body" from BaseOp; update those interfaces (and any
similar types around the MergeOp/InsertOp declarations and the other affected op
types referenced in the file) to explicitly set scope: "body" so callers and
editors cannot supply "header" for body-only ops.
---
Outside diff comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 1839-1848: The code passes the original session.request.message
reference into final-phase where applyFinal() mutates the body in-place, causing
repeated mutations on retries/provider switches; after applying provider
overrides (applyGeminiGoogleSearchOverrideWithAudit) clone the overriddenBody
(e.g., deep clone bodyToSerialize) and assign the clone to bodyToSerialize and
session.request.message before calling applyFinal(), so applyFinal() mutates the
clone not the original; update the same pattern in the other occurrence
mentioned (around the applyFinal() call at the 1910-1914 section) and ensure
session.addSpecialSetting(googleSearchAudit) still receives the audit without
changing the original message object.
In `@src/lib/request-filter-engine.ts`:
- Around line 35-47: The parser silently skips unsafe segments (UNSAFE_KEYS) so
"a.__proto__.b" becomes "a.b" and wrongly targets a valid property; change
parsePath to treat the entire path as invalid when any unsafe key is
encountered: in parsePath (the function using UNSAFE_KEYS and regex/match logic)
replace the continue with immediate failure (e.g., throw a clear Error like
"Invalid path: contains unsafe key" or return a null/Result indicating invalid
path) and update callers to handle that failure instead of proceeding with a
sanitized path.
---
Nitpick comments:
In `@src/actions/request-filters.ts`:
- Around line 276-291: The PR makes multiple redundant calls to
getRequestFilterById inside updateRequestFilterAction; fetch the record once at
the start (e.g., const existing = await getRequestFilterById(id)) and reuse that
variable for all subsequent validations and updates (including where
effectiveRuleMode/effectiveOperations are computed and later checks around Lines
~306-317 and ~334-344), remove the additional getRequestFilterById calls and
rely on the single existing object to perform validateOperations, compare
fields, and apply updates.
In `@src/app/`[locale]/settings/request-filters/_components/filter-table.tsx:
- Line 284: 当前 Switch 的 aria-label 使用启用/禁用词（aria-label={filter.isEnabled ?
t("enable") : t("disable")），应改为描述开关用途的语义文本，状态由控件的 checked/aria-checked 属性传达即可。请在
Switch 组件（使用 aria-label 的那一处）改为返回描述性标签，例如基于 filter.name 或 filter.label 构建的文本（例如
t("toggleFilter", { name: filter.name }) 或 t("filterToggleDescription", { label:
filter.label })），同时保留 checked={filter.isEnabled} 不变，以便读屏器能读出用途和当前状态。
In `@src/drizzle/schema.ts`:
- Line 640: The operations column currently lacks the JSONB default and explicit
type which causes inconsistent migrations and weak type inference; update the
operations column definition (symbol: operations in schema.ts) to include the
JSONB default pattern used across the repo by adding the Drizzle SQL default
(e.g. .default(sql`'null'::jsonb`)) and ensure the column is declared with
jsonb('operations') so migrations emit "jsonb DEFAULT 'null'::jsonb" and type
inference remains strong.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c941bdb3-52fd-449e-9638-335d80a85c51
📒 Files selected for processing (17)
drizzle/0081_strange_zarek.sqldrizzle/meta/0081_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/settings/requestFilters.jsonmessages/ja/settings/requestFilters.jsonmessages/ru/settings/requestFilters.jsonmessages/zh-CN/settings/requestFilters.jsonmessages/zh-TW/settings/requestFilters.jsonsrc/actions/request-filters.tssrc/app/[locale]/settings/request-filters/_components/filter-dialog.tsxsrc/app/[locale]/settings/request-filters/_components/filter-table.tsxsrc/app/v1/_lib/proxy/forwarder.tssrc/drizzle/schema.tssrc/lib/request-filter-engine.tssrc/lib/request-filter-types.tssrc/repository/request-filters.tstests/unit/request-filter-advanced.test.ts
Comment on lines
+367
to
+419
| private loadFilters(filters: RequestFilter[]): void { | ||
| const cachedFilters = filters.map((f) => { | ||
| const cached: CachedRequestFilter = { ...f }; | ||
|
|
||
| // Optimization #2: Pre-compile regex for text_replace (with ReDoS validation) | ||
| if (f.matchType === "regex" && f.action === "text_replace") { | ||
| if (!safeRegex(f.target)) { | ||
| logger.warn("[RequestFilterEngine] Skip unsafe regex at load", { | ||
| if (f.matchType === "regex" && f.action === "text_replace") { | ||
| if (!safeRegex(f.target)) { | ||
| logger.warn("[RequestFilterEngine] Skip unsafe regex at load", { | ||
| filterId: f.id, | ||
| target: f.target, | ||
| }); | ||
| } else { | ||
| try { | ||
| cached.compiledRegex = new RegExp(f.target, "g"); | ||
| } catch (error) { | ||
| logger.warn("[RequestFilterEngine] Failed to compile regex at load", { | ||
| filterId: f.id, | ||
| target: f.target, | ||
| error, | ||
| }); | ||
| } else { | ||
| try { | ||
| cached.compiledRegex = new RegExp(f.target, "g"); | ||
| } catch (error) { | ||
| logger.warn("[RequestFilterEngine] Failed to compile regex at load", { | ||
| filterId: f.id, | ||
| target: f.target, | ||
| error, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Optimization #3: Create Set caches for faster lookups | ||
| if (f.bindingType === "providers" && f.providerIds) { | ||
| cached.providerIdsSet = new Set(f.providerIds); | ||
| } | ||
| if (f.bindingType === "groups" && f.groupTags) { | ||
| cached.groupTagsSet = new Set(f.groupTags); | ||
| } | ||
|
|
||
| return cached; | ||
| }); | ||
|
|
||
| // Split filters by binding type | ||
| this.globalFilters = cachedFilters | ||
| .filter((f) => f.bindingType === "global" || !f.bindingType) | ||
| .sort((a, b) => a.priority - b.priority || a.id - b.id); | ||
| if (f.bindingType === "providers" && f.providerIds) { | ||
| cached.providerIdsSet = new Set(f.providerIds); | ||
| } | ||
| if (f.bindingType === "groups" && f.groupTags) { | ||
| cached.groupTagsSet = new Set(f.groupTags); | ||
| } | ||
|
|
||
| this.providerFilters = cachedFilters | ||
| .filter((f) => f.bindingType === "providers" || f.bindingType === "groups") | ||
| .sort((a, b) => a.priority - b.priority || a.id - b.id); | ||
| return cached; | ||
| }); | ||
|
|
||
| // Optimization #5: Cache whether we have group-based filters | ||
| this.hasGroupBasedFilters = this.providerFilters.some((f) => f.bindingType === "groups"); | ||
| const isGlobal = (f: CachedRequestFilter) => f.bindingType === "global" || !f.bindingType; | ||
| const isProvider = (f: CachedRequestFilter) => | ||
| f.bindingType === "providers" || f.bindingType === "groups"; | ||
| const isGuard = (f: CachedRequestFilter) => (f.executionPhase ?? "guard") === "guard"; | ||
| const isFinal = (f: CachedRequestFilter) => f.executionPhase === "final"; | ||
| const byPriority = (a: CachedRequestFilter, b: CachedRequestFilter) => | ||
| a.priority - b.priority || a.id - b.id; | ||
|
|
||
| this.globalGuardFilters = cachedFilters | ||
| .filter((f) => isGlobal(f) && isGuard(f)) | ||
| .sort(byPriority); | ||
| this.providerGuardFilters = cachedFilters | ||
| .filter((f) => isProvider(f) && isGuard(f)) | ||
| .sort(byPriority); | ||
| this.globalFinalFilters = cachedFilters | ||
| .filter((f) => isGlobal(f) && isFinal(f)) | ||
| .sort(byPriority); | ||
| this.providerFinalFilters = cachedFilters | ||
| .filter((f) => isProvider(f) && isFinal(f)) | ||
| .sort(byPriority); |
There was a problem hiding this comment.
executionPhase = "guard" 的 advanced 规则当前基本不会生效。
loadFilters() 已经把这类规则放进了 guard buckets，但这里仍然只走 applyHeaderFilter() / applyBodyFilter() 的旧字段逻辑，从不读取 filter.operations。现在 UI/API 已经能创建 advanced + guard 组合，这会生成一条“保存成功但几乎是空操作”的规则。要么在 guard phase 复用 DSL 执行器，要么在校验层直接禁止这个组合。
Also applies to: 451-518
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 378-378: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(f.target, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/request-filter-engine.ts` around lines 367 - 419, loadFilters
currently buckets filters with (executionPhase ?? "guard") === "guard" but the
guard code path still only uses the old applyHeaderFilter()/applyBodyFilter()
logic and never runs filter.operations, so advanced guard rules become no-ops;
update the guard phase to either (A) execute the DSL operations for guard
filters by invoking the existing DSL executor on the cached.filter.operations
(reuse whatever function runs advanced rules for final phase—call it from the
guard path) for filters loaded by loadFilters(), ensuring
compiledRegex/provider/group set lookups still apply, or (B) if you prefer
prohibition, add validation that rejects filters with executionPhase === "guard"
when they contain advanced filter.operations; modify the code paths that
currently call applyHeaderFilter/applyBodyFilter to also handle
filter.operations (or prevent creation via validation) so advanced+guard combos
are not silently ineffective.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR introduces a well-architected advanced mode for request filters with a typed Operation DSL, final execution phase support, and comprehensive security hardening. The implementation is solid with good test coverage.
PR Size: XL
- Lines changed: 6,496 (6,223 additions + 273 deletions)
- Files changed: 17
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Security Highlights (Positive)
- ReDoS Prevention: All regex patterns validated via
safe-regexpackage - Prototype Pollution Guards:
UNSAFE_KEYSset blocks__proto__,constructor,prototypein path traversal and deep merge - Transport Header Blacklist: Prevents user-controlled
content-length,connection,transfer-encodingheaders
Architecture Observations
The codebase follows established patterns:
- 4-bucket filter caching (globalGuard, providerGuard, globalFinal, providerFinal)
- Defense-in-depth with validation at both action layer and engine layer
- Clean separation between simple mode (backward compatible) and advanced mode
Test Coverage
- 985 lines of unit tests in
tests/unit/request-filter-advanced.test.ts - Covers all 4 operation types (set, remove, merge, insert)
- Tests for dedup, anchor positioning, matcher strategies
- Prototype pollution rejection tests
- ReDoS regex complexity limit tests
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean (hardened)
- Error handling - Clean (all errors logged)
- Type safety - Clean (well-typed DSL)
- Documentation accuracy - Clean
- Test coverage - Excellent
- Code clarity - Good
Automated review by Claude AI
| } | ||
| try { | ||
| return new RegExp(pattern).test(String(fieldValue)); | ||
| } catch { |
Contributor
There was a problem hiding this comment.
[HIGH] [ERROR-SILENT] Swallowed regex compilation error in matcher
Location: src/lib/request-filter-engine.ts:209
Evidence: The matcher regex path silently ignores invalid patterns:
try {
return new RegExp(pattern).test(String(fieldValue));
} catch {
return false;
}Why this is a problem: An invalid pattern makes the filter behave like it "doesn't match" with no log signal, which is hard to debug and violates the project's no-silent-failure expectation.
Suggested fix:
try {
return new RegExp(pattern).test(String(fieldValue));
} catch (error) {
logger.warn("[RequestFilterEngine] Invalid regex matcher pattern", { pattern, error });
return false;
}
| if (this.globalGuardFilters.length === 0) return; | ||
|
|
||
| for (const filter of this.globalFilters) { | ||
| for (const filter of this.globalGuardFilters) { |
Contributor
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] ruleMode: "advanced" filters are accepted for guard phase but never executed
Location: src/lib/request-filter-engine.ts:457
Evidence: Guard-phase application only uses scope/action/target/replacement and never branches on filter.ruleMode / filter.operations:
for (const filter of this.globalGuardFilters) {
if (filter.scope === "header") {
this.applyHeaderFilter(session, filter);
} else if (filter.scope === "body") {
this.applyBodyFilter(session, filter);
}
}Why this is a problem: The UI and server actions allow ruleMode: "advanced" with executionPhase: "guard", but the engine will silently ignore the operation DSL in guard phase (filters become no-ops unless the legacy fields are populated). This is user-visible misconfiguration with no warning.
Suggested fix (support advanced ops in guard phase; do the same in applyForProvider):
for (const filter of this.globalGuardFilters) {
try {
if (filter.ruleMode === "advanced" && filter.operations) {
const message = session.request.message as Record<string, unknown>;
this.executeAdvancedOps(filter.operations, message, session.headers);
continue;
}
if (filter.scope === "header") {
this.applyHeaderFilter(session, filter);
} else if (filter.scope === "body") {
this.applyBodyFilter(session, filter);
}
} catch (error) {
logger.error("[RequestFilterEngine] Failed to apply global filter", {
filterId: filter.id,
scope: filter.scope,
action: filter.action,
error,
});
}
}
| // Merge (body only) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| export interface MergeOp extends BaseOp { |
Contributor
There was a problem hiding this comment.
[MEDIUM] [TYPE-WEAK-INVARIANT] Merge/Insert ops are documented as body-only but types allow scope: "header"
Location: src/lib/request-filter-types.ts:59
Evidence: The file labels these as body-only, but both extend BaseOp (scope: "header" | "body").
// Merge (body only)
export interface MergeOp extends BaseOp {
op: "merge";
// ...
}Why this is a problem: The type system currently allows invalid states (e.g., { op: "merge", scope: "header", ... }), pushing correctness to runtime validation and making it easier to construct impossible operations in code.
Suggested fix (encode the invariant in types):
export interface MergeOp {
op: "merge";
scope: "body";
path: string;
value: Record<string, unknown>;
}
export interface InsertOp {
op: "insert";
scope: "body";
path: string;
value: unknown;
// ...
}
|
|
||
| function validateOperations(operations: unknown): string | null { | ||
| if (!Array.isArray(operations) || operations.length === 0) { | ||
| return "Advanced mode requires at least one operation"; |
Contributor
There was a problem hiding this comment.
[MEDIUM] [STANDARD-VIOLATION] Advanced-mode validation returns hardcoded English strings (non-i18n)
Location: src/actions/request-filters.ts:49
Evidence: New advanced-mode validation returns raw English messages:
return "Advanced mode requires at least one operation";These error strings are returned via ActionResult.error and are surfaced directly in the UI (toast.error(result.error)), so users in non-English locales will see unlocalized messages.
Why this is a problem: Project standard requires i18n for user-facing strings. Returning raw strings here also makes it hard to keep parity across the 5 supported locales.
Suggested fix (return an errorCode and translate via next-intl on the server):
import { getTranslations } from "next-intl/server";
type ValidationError = { code: string; params?: Record<string, string | number> };
function validateOperations(operations: unknown): ValidationError | null {
if (!Array.isArray(operations) || operations.length === 0) {
return { code: "REQUEST_FILTER_OPS_REQUIRED" };
}
if (operations.length > MAX_OPERATIONS) {
return { code: "REQUEST_FILTER_OPS_TOO_MANY", params: { max: MAX_OPERATIONS } };
}
return null;
}
// inside the action
const tError = await getTranslations("errors");
const err = validateOperations(data.operations);
if (err) {
return {
ok: false,
error: tError(err.code, err.params),
errorCode: err.code,
errorParams: err.params,
};
}
| // Final-phase request filter: after all provider overrides, before serialization | ||
| if (!session.getEndpointPolicy().bypassRequestFilters) { | ||
| const { requestFilterEngine } = await import("@/lib/request-filter-engine"); | ||
| await requestFilterEngine.applyFinal(session, messageToSend, processedHeaders); |
Contributor
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] Final-phase filters can override enforced host header and are skipped for GET/HEAD
Location: src/app/v1/_lib/proxy/forwarder.ts:2237
Evidence: Final filters run on processedHeaders:
await requestFilterEngine.applyFinal(session, messageToSend, processedHeaders);Why this is a problem:
processedHeadershas a requiredhostoverride for undici TLS validation earlier in this method. BecauseapplyFinal()can run headerset/removeoperations, it can overwritehostafter it was enforced, causing upstream request failures.applyFinal()is only invoked inside thehasBodyJSON-serialization path. ForGET/HEAD(e.g.,/v1/models) and raw passthrough bodies, final-phase header filters never run.
Suggested fix (apply final filters for all methods, and re-enforce host after filters):
const applyFinalFilters = async (body: Record<string, unknown>) => {
if (session.getEndpointPolicy().bypassRequestFilters) return;
const { requestFilterEngine } = await import("@/lib/request-filter-engine");
await requestFilterEngine.applyFinal(session, body, processedHeaders);
processedHeaders.set("host", actualHost);
};
// JSON body path
await applyFinalFilters(messageToSend);
// GET/HEAD + raw passthrough paths
await applyFinalFilters({});
| // Final-phase request filter for Gemini: after headers built, before body serialization | ||
| if (!session.getEndpointPolicy().bypassRequestFilters) { | ||
| const { requestFilterEngine } = await import("@/lib/request-filter-engine"); | ||
| await requestFilterEngine.applyFinal(session, bodyToSerialize, processedHeaders); |
Contributor
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] Gemini final-phase filters can override required headers and are skipped for no-body requests
Location: src/app/v1/_lib/proxy/forwarder.ts:1913
Evidence: Gemini path applies final filters directly on processedHeaders:
await requestFilterEngine.applyFinal(session, bodyToSerialize, processedHeaders);Why this is a problem:
applyFinal()can mutate headers afterbuildGeminiHeaders()has enforced critical overrides (host, auth headers, transport-header blacklist). A final filter can reintroduce invalid values (especiallyhost) and break upstream requests.- The
elsebranch (no body) never callsapplyFinal(), so final-phase header filters won't run forGET/HEADGemini requests.
Suggested fix (reuse a helper and re-enforce host after final filters in both branches):
const applyGeminiFinalFilters = async (body: Record<string, unknown>) => {
if (session.getEndpointPolicy().bypassRequestFilters) return;
const { requestFilterEngine } = await import("@/lib/request-filter-engine");
await requestFilterEngine.applyFinal(session, body, processedHeaders);
processedHeaders.set("host", HeaderProcessor.extractHost(proxyUrl));
};
await applyGeminiFinalFilters(hasBody ? bodyToSerialize : {});
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR adds an advanced request-filter operation DSL, integrates a new final execution phase into the proxy forwarder, and updates schema/UI/i18n accordingly. There are several high-priority correctness and safety gaps in the new execution flow (final-phase header safety, GET/HEAD coverage) and in the engine/action layering (advanced guard-phase behavior, non-i18n validation errors).
PR Size: XL
- Lines changed: 6496
- Files changed: 17
- Split suggestion (required for XL): Split into 3–4 PRs: (1) DB/schema + repository types + migration, (2) request-filter engine + DSL types + unit tests, (3) forwarder integration, (4) UI + i18n message updates.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 0 | 0 |
| Security | 0 | 2 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 1 | 0 |
| Comments/Docs | 0 | 0 | 1 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
- (Confidence 98) Final-phase filters can override enforced
hostheader and are skipped for no-body requests in forwarder paths (src/app/v1/_lib/proxy/forwarder.ts), risking upstream request failures.
High Priority Issues (Should Fix)
- (Confidence 90)
ruleMode: "advanced"filters are accepted for guard phase but never execute operations (engine ignoresoperationsoutsideapplyFinal). - (Confidence 90) Swallowed regex compilation errors in matcher logic hide misconfiguration and make debugging difficult.
- (Confidence 85) Advanced-mode validation in server actions returns hardcoded English strings; these surface directly in UI and bypass i18n.
- (Confidence 85) Operation DSL types allow invalid states (Merge/Insert documented as body-only but
scopetype still permits"header").
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
- Identified and reviewed PR
#912(feat/request-filters: advanced mode operations and execution engine). - Applied PR size label
size/XL(6223 additions, 273 deletions, 17 files). - Posted 6 inline review comments (diff-only) focused on:
src/app/v1/_lib/proxy/forwarder.ts(final-phase filters:hostsafety + GET/HEAD/no-body coverage; Gemini path too)src/lib/request-filter-engine.ts(advanced ops not executing in guard phase; swallowed regex errors in matcher)src/actions/request-filters.ts(advanced-mode validation errors are hardcoded/non-i18n)src/lib/request-filter-types.ts(Merge/Insert “body-only” invariant not enforced by types)
- Submitted the required “Code Review Summary” via
gh pr review --comment.
Security: - parsePath now rejects entire path on unsafe key instead of silently dropping segments (e.g. `a.__proto__.b` no longer becomes `a.b`) - getValueByPath returns undefined on rejected paths instead of root object - MergeOp/InsertOp types narrowed to scope:"body" with runtime validation Correctness: - GET/HEAD requests now run final-phase filters for header-only operations (both Gemini and non-Gemini branches) - Gemini branch clones body via structuredClone before applyFinal to prevent in-place mutation of session.request.message on retries - Reject advanced+guard combo at validation layer (create + update paths) Quality: - Consolidate 3 redundant getRequestFilterById calls into 1 per update - Fix Switch aria-label to describe toggle purpose with filter name - Add $type<FilterOperation[]|null> annotation to operations schema column - Add toggleStatus i18n key across all 5 locales
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/lib/request-filter-engine.ts (1)
208-211:⚠️ Potential issue | 🟠 Major正则匹配构造失败被静默吞掉，排障成本高
catch分支直接返回false，会把配置错误伪装成“不匹配”。建议至少记录warn日志，保留 pattern 与异常信息。建议修改
- } catch { - return false; + } catch (error) { + logger.warn("[RequestFilterEngine] Invalid regex matcher pattern", { + pattern, + error, + }); + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/request-filter-engine.ts` around lines 208 - 211, The catch branch that swallows RegExp construction errors should log a warning with the failing pattern and the exception before returning false; locate the try/catch that uses new RegExp(pattern).test(String(fieldValue)) and in the catch block call the module's logger (e.g., logger.warn or console.warn if no logger exists) with a message including the pattern and the caught error/stack, then return false as before.src/app/v1/_lib/proxy/forwarder.ts (1)
1910-1917:⚠️ Potential issue | 🟠 Major
applyFinal后未重新强制host，可能破坏上游请求
applyFinal可修改 headers；当前多个分支在执行后都没有回写host。这会让 final 规则有机会覆盖关键host，影响 undici 的目标主机校验与上游连通性。建议每次applyFinal后统一重新设置host。建议修改
+const enforceHostHeader = () => { + processedHeaders.set("host", HeaderProcessor.extractHost(proxyUrl)); +}; // after applyFinal(...) await requestFilterEngine.applyFinal(session, bodyForFinal, processedHeaders); +enforceHostHeader();在 Gemini 与标准路径（含 no-body 分支）都执行同样处理。
Also applies to: 1951-1957, 2247-2251, 2279-2285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 1910 - 1917, The final request filter (requestFilterEngine.applyFinal) can mutate headers and may overwrite Host, breaking upstream routing; after every call to applyFinal (e.g., the Gemini path and the standard/no-body branches) re-assign processedHeaders["host"] (or processedHeaders.get/set as appropriate) back to the original host value used for the request (the value computed earlier for undici target) so Host is never lost by filters; update the code paths that call requestFilterEngine.applyFinal (references: session.getEndpointPolicy(), requestFilterEngine.applyFinal, processedHeaders, bodyToSerialize) to restore the Host header immediately after each applyFinal invocation.
🤖 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/actions/request-filters.ts`:
- Around line 145-153: validatePayload is being reused without passing the
ruleMode/operations context so advanced rules fall back to simple validation and
can incorrectly reject updates (e.g., empty data.target when only binding
changed). Fix by ensuring calls to validatePayload include the ruleMode and
operations from the incoming payload (use data.ruleMode and data.operations),
and keep the existing logic in request-filters: when ruleMode === "advanced"
call validateOperations(data.operations) and skip the simple target check;
otherwise perform the simple target validation. Update both places where
validatePayload is invoked (the block around ruleMode/validateOperations and the
similar block at lines ~356-364) so advanced-mode validations run correctly.
In `@src/lib/request-filter-engine.ts`:
- Around line 182-187: The manual dot-split traversal for matcher.field bypasses
the existing UNSAFE_KEYS checks; replace the custom loop with the shared safe
path-reading utilities (use parsePath and/or getValueByPath) so matcher.field is
resolved through the same safety logic (UNSAFE_KEYS) used elsewhere.
Specifically, in request-filter-engine.ts where matcher.field is walked, call
the common getValueByPath(parsePath(...)) (or the project’s single
getValueByPath(matcher.field, element) entrypoint) and treat an undefined/null
result as a non-match (return false), preserving the original
null/undefined/type checks but relying on the shared implementation to enforce
UNSAFE_KEYS.
---
Duplicate comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 1910-1917: The final request filter
(requestFilterEngine.applyFinal) can mutate headers and may overwrite Host,
breaking upstream routing; after every call to applyFinal (e.g., the Gemini path
and the standard/no-body branches) re-assign processedHeaders["host"] (or
processedHeaders.get/set as appropriate) back to the original host value used
for the request (the value computed earlier for undici target) so Host is never
lost by filters; update the code paths that call requestFilterEngine.applyFinal
(references: session.getEndpointPolicy(), requestFilterEngine.applyFinal,
processedHeaders, bodyToSerialize) to restore the Host header immediately after
each applyFinal invocation.
In `@src/lib/request-filter-engine.ts`:
- Around line 208-211: The catch branch that swallows RegExp construction errors
should log a warning with the failing pattern and the exception before returning
false; locate the try/catch that uses new
RegExp(pattern).test(String(fieldValue)) and in the catch block call the
module's logger (e.g., logger.warn or console.warn if no logger exists) with a
message including the pattern and the caught error/stack, then return false as
before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc9dae2d-0e1b-42d0-b9cc-6494cebfbb3d
📒 Files selected for processing (11)
messages/en/settings/requestFilters.jsonmessages/ja/settings/requestFilters.jsonmessages/ru/settings/requestFilters.jsonmessages/zh-CN/settings/requestFilters.jsonmessages/zh-TW/settings/requestFilters.jsonsrc/actions/request-filters.tssrc/app/[locale]/settings/request-filters/_components/filter-table.tsxsrc/app/v1/_lib/proxy/forwarder.tssrc/drizzle/schema.tssrc/lib/request-filter-engine.tssrc/lib/request-filter-types.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/drizzle/schema.ts
- messages/en/settings/requestFilters.json
- src/lib/request-filter-types.ts
- messages/zh-CN/settings/requestFilters.json
- messages/ru/settings/requestFilters.json
Comment on lines
+145
to
+153
| const ruleMode = data.ruleMode ?? "simple"; | ||
|
|
||
| if (ruleMode === "advanced") { | ||
| // Advanced mode: validate operations, skip simple fields | ||
| const opsError = validateOperations(data.operations); | ||
| if (opsError) return opsError; | ||
| } else { | ||
| // Simple mode: existing validation | ||
| if (!data.target?.trim()) return "目标字段不能为空"; |
There was a problem hiding this comment.
binding 更新校验缺少 ruleMode 上下文，advanced 规则会被误拦截
这里复用 validatePayload 时没有传 ruleMode/operations，会退回 simple 校验路径。对于 advanced 规则（尤其 target 为空的场景），仅修改 binding 也可能被错误拒绝。
建议修改
const validationError = validatePayload({
name: existing!.name,
scope: existing!.scope,
action: existing!.action,
target: existing!.target,
+ ruleMode: updates.ruleMode ?? existing!.ruleMode,
+ operations: updates.operations !== undefined ? updates.operations : existing!.operations,
bindingType: effectiveBindingType,
providerIds: effectiveProviderIds,
groupTags: effectiveGroupTags,
});Also applies to: 356-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/request-filters.ts` around lines 145 - 153, validatePayload is
being reused without passing the ruleMode/operations context so advanced rules
fall back to simple validation and can incorrectly reject updates (e.g., empty
data.target when only binding changed). Fix by ensuring calls to validatePayload
include the ruleMode and operations from the incoming payload (use data.ruleMode
and data.operations), and keep the existing logic in request-filters: when
ruleMode === "advanced" call validateOperations(data.operations) and skip the
simple target check; otherwise perform the simple target validation. Update both
places where validatePayload is invoked (the block around
ruleMode/validateOperations and the similar block at lines ~356-364) so
advanced-mode validations run correctly.
Comment on lines
+182
to
+187
| const parts = matcher.field.split("."); | ||
| let cur: unknown = element; | ||
| for (const part of parts) { | ||
| if (cur === null || cur === undefined || typeof cur !== "object") return false; | ||
| cur = (cur as Record<string, unknown>)[part]; | ||
| } |
There was a problem hiding this comment.
matcher.field 路径遍历绕过了现有的安全路径防护
这里手动按 . 分割遍历，没有复用 parsePath/getValueByPath 的 UNSAFE_KEYS 防护，导致 matcher 路径与其余路径处理策略不一致。建议统一走同一套安全路径读取逻辑。
建议修改
- // Support dot-path extraction
- const parts = matcher.field.split(".");
- let cur: unknown = element;
- for (const part of parts) {
- if (cur === null || cur === undefined || typeof cur !== "object") return false;
- cur = (cur as Record<string, unknown>)[part];
- }
- fieldValue = cur;
+ fieldValue = getValueByPath(element as Record<string, unknown>, matcher.field);
+ if (fieldValue === undefined) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/request-filter-engine.ts` around lines 182 - 187, The manual
dot-split traversal for matcher.field bypasses the existing UNSAFE_KEYS checks;
replace the custom loop with the shared safe path-reading utilities (use
parsePath and/or getValueByPath) so matcher.field is resolved through the same
safety logic (UNSAFE_KEYS) used elsewhere. Specifically, in
request-filter-engine.ts where matcher.field is walked, call the common
getValueByPath(parsePath(...)) (or the project’s single
getValueByPath(matcher.field, element) entrypoint) and treat an undefined/null
result as a non-match (return false), preserving the original
null/undefined/type checks but relying on the shared implementation to enforce
UNSAFE_KEYS.
Comment on lines
+116
to
+118
| if (insertOp.dedupe?.byFields && !Array.isArray(insertOp.dedupe.byFields)) { | ||
| return `${prefix}: dedupe.byFields must be an array`; | ||
| } |
There was a problem hiding this comment.
dedupe.byFields entries not validated for prototype-polluting names
The current check only verifies that dedupe.byFields is an array, but never inspects the individual field name strings. If an admin submits { "dedupe": { "byFields": ["__proto__"] } }, it passes server-side validation, is stored in the DB, and reaches executeInsertOp. There, at line 769 of the engine:
return byFields.every((field) => deepEqual(existingObj[field], valueObj[field]));For any plain object, existingObj["__proto__"] returns Object.prototype. The deepEqual check immediately hits the a === b short-circuit (both sides are the same Object.prototype reference) and returns true. As a result, the dedupe check always fires, and no element is ever inserted — the operation is silently swallowed with no error.
Fix by adding a check similar to the existing op.path guard:
if (insertOp.dedupe?.byFields && !Array.isArray(insertOp.dedupe.byFields)) {
return `${prefix}: dedupe.byFields must be an array`;
}
// ADD: validate each byField entry
if (insertOp.dedupe?.byFields) {
for (const field of insertOp.dedupe.byFields) {
if (VALIDATION_UNSAFE_KEYS.test(field)) {
return `${prefix}: dedupe.byFields contains a forbidden property name`;
}
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/request-filters.ts
Line: 116-118
Comment:
**`dedupe.byFields` entries not validated for prototype-polluting names**
The current check only verifies that `dedupe.byFields` is an array, but never inspects the individual field name strings. If an admin submits `{ "dedupe": { "byFields": ["__proto__"] } }`, it passes server-side validation, is stored in the DB, and reaches `executeInsertOp`. There, at line 769 of the engine:
```ts
return byFields.every((field) => deepEqual(existingObj[field], valueObj[field]));
```
For any plain object, `existingObj["__proto__"]` returns `Object.prototype`. The `deepEqual` check immediately hits the `a === b` short-circuit (both sides are the same `Object.prototype` reference) and returns `true`. As a result, the dedupe check always fires, and **no element is ever inserted** — the operation is silently swallowed with no error.
Fix by adding a check similar to the existing `op.path` guard:
```typescript
if (insertOp.dedupe?.byFields && !Array.isArray(insertOp.dedupe.byFields)) {
return `${prefix}: dedupe.byFields must be an array`;
}
// ADD: validate each byField entry
if (insertOp.dedupe?.byFields) {
for (const field of insertOp.dedupe.byFields) {
if (VALIDATION_UNSAFE_KEYS.test(field)) {
return `${prefix}: dedupe.byFields contains a forbidden property name`;
}
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+38
to
+44
| function validateMatcher(matcher: FilterMatcher, context: string): string | null { | ||
| if (matcher.matchType === "regex" && typeof matcher.value === "string") { | ||
| if (!safeRegex(matcher.value)) { | ||
| return `${context}: regex matcher has ReDoS risk`; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
validateMatcher does not check matcher.field for prototype-polluting names
validateMatcher only validates the regex safety of matcher.value. It never inspects matcher.field, even though matcher.field is used directly as a dot-path key during element traversal in matchElement. An admin can submit { "field": "__proto__", "matchType": "contains", "value": "object" } — this passes server-side validation, gets stored, and at runtime matchElement evaluates element["__proto__"] which returns Object.prototype for any plain JS object. String(Object.prototype) is "[object Object]", so the contains: "object" check matches every array element, causing a remove operation to wipe the entire array.
This is a distinct layer from the engine-level concern already noted in thread #3: the server-side validation should reject dangerous field values before they ever reach the engine.
Add a VALIDATION_UNSAFE_KEYS check on matcher.field:
function validateMatcher(matcher: FilterMatcher, context: string): string | null {
if (matcher.field && VALIDATION_UNSAFE_KEYS.test(matcher.field)) {
return `${context}: matcher.field contains a forbidden property name`;
}
if (matcher.matchType === "regex" && typeof matcher.value === "string") {
if (!safeRegex(matcher.value)) {
return `${context}: regex matcher has ReDoS risk`;
}
}
return null;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/request-filters.ts
Line: 38-44
Comment:
**`validateMatcher` does not check `matcher.field` for prototype-polluting names**
`validateMatcher` only validates the regex safety of `matcher.value`. It never inspects `matcher.field`, even though `matcher.field` is used directly as a dot-path key during element traversal in `matchElement`. An admin can submit `{ "field": "__proto__", "matchType": "contains", "value": "object" }` — this passes server-side validation, gets stored, and at runtime `matchElement` evaluates `element["__proto__"]` which returns `Object.prototype` for any plain JS object. `String(Object.prototype)` is `"[object Object]"`, so the `contains: "object"` check matches **every** array element, causing a remove operation to wipe the entire array.
This is a distinct layer from the engine-level concern already noted in thread #3: the server-side validation should reject dangerous `field` values before they ever reach the engine.
Add a `VALIDATION_UNSAFE_KEYS` check on `matcher.field`:
```typescript
function validateMatcher(matcher: FilterMatcher, context: string): string | null {
if (matcher.field && VALIDATION_UNSAFE_KEYS.test(matcher.field)) {
return `${context}: matcher.field contains a forbidden property name`;
}
if (matcher.matchType === "regex" && typeof matcher.value === "string") {
if (!safeRegex(matcher.value)) {
return `${context}: regex matcher has ReDoS risk`;
}
}
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.
ding113
added a commit
that referenced
this pull request
Mar 14, 2026
* fix(proxy): prevent TypeError in NON_RETRYABLE_CLIENT_ERROR handler with transport errors Fixes a regression where native transport errors (SocketError, UND_ERR_SOCKET) could be misclassified as NON_RETRYABLE_CLIENT_ERROR by error rule matching, causing the error handler to crash with "TypeError: getDetailedErrorMessage is not a function" instead of properly rethrowing the original error. Changes: 1. errors.ts: Add transport-error guard in categorizeErrorAsync() at priority 1.5 - Detects SocketError, undici error codes (UND_ERR_SOCKET, ECONNREFUSED, etc.) - Returns SYSTEM_ERROR before error rule matching to prevent misclassification - Preserves correct failover behavior for network-level failures 2. forwarder.ts: Bifurcate NON_RETRYABLE_CLIENT_ERROR branch on instanceof check - ProxyError path: uses full ProxyError fields (statusCode, upstreamError, etc.) - Plain Error path: uses only base Error fields, avoids calling non-existent methods - Both paths preserve original error rethrow and provider chain recording 3. Add regression tests: - Test A: Verifies plain SocketError + forced NON_RETRYABLE_CLIENT_ERROR doesn't throw TypeError - Test B: Verifies categorizeErrorAsync returns SYSTEM_ERROR for transport errors All 657 proxy unit tests pass. Type check and lint pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: format code (dev-a3fd666) * fix: assorted UI/UX and logic small fixes * feat(request-filters): advanced mode operations and execution engine (#912) * feat(request-filters): add advanced mode operations and final execution phase Introduce two new axes for request filters: rule mode (simple/advanced) and execution phase (guard/final). Advanced mode supports a JSON-defined operation DSL with set, remove, merge, and insert operations. The final phase executes after all provider overrides, enabling post-processing use cases like system message injection with dedup and cache_control manipulation. Key changes: - Schema: add rule_mode, execution_phase, operations columns + index - Types: new FilterOperation DSL (SetOp, RemoveOp, MergeOp, InsertOp) - Engine: 4-bucket cache split, applyFinal() method, operation executors with deep merge (null-as-delete), deep equal dedup, anchor matching - Forwarder: integrate applyFinal in standard and Gemini branches - Validation: validateOperations with ReDoS prevention via safe-regex - UI: rule mode toggle, execution phase selector, JSON operations editor - i18n: new keys for all 5 locales (en, zh-CN, zh-TW, ja, ru) - Tests: 29 new tests covering all operation types and edge cases * chore: format code (feat-request-filter-advanced-mode-ccee157) * fix(security): harden request filter engine against prototype pollution and ReDoS bypass - Block __proto__/constructor/prototype traversal in parsePath, deepMerge, and validation - Add runtime safe-regex check in matchElement for regex matchers - Fix ReDoS bypass in updateRequestFilterAction by checking effective target/matchType/action - Add operations array length limit (max 50) and value validation for set/merge/insert ops - Fix UI default executionPhase to match server default (guard, not final) - Fix parsePath regex corruption from edit tool (double-escaped backslashes) * fix(i18n,a11y): address code review findings for request filters - Remove console.error from filter-dialog.tsx (error already shown via toast) - Fix ja table.actions translation to "操作" - Fix ru table.createdAt to "Дата создания" - Fix zh-CN confirmDelete halfwidth ? to fullwidth ？ - Show operations count in target column for advanced mode filters - Add aria-label to filter toggle Switch for accessibility * fix(request-filters): address CodeRabbit review findings across 8 issues Security: - parsePath now rejects entire path on unsafe key instead of silently dropping segments (e.g. `a.__proto__.b` no longer becomes `a.b`) - getValueByPath returns undefined on rejected paths instead of root object - MergeOp/InsertOp types narrowed to scope:"body" with runtime validation Correctness: - GET/HEAD requests now run final-phase filters for header-only operations (both Gemini and non-Gemini branches) - Gemini branch clones body via structuredClone before applyFinal to prevent in-place mutation of session.request.message on retries - Reject advanced+guard combo at validation layer (create + update paths) Quality: - Consolidate 3 redundant getRequestFilterById calls into 1 per update - Fix Switch aria-label to describe toggle purpose with filter name - Add $type<FilterOperation[]|null> annotation to operations schema column - Add toggleStatus i18n key across all 5 locales --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(proxy): distinguish local client abort 499 from upstream HTTP 499 Adds isLocalAbort flag to ProxyError to differentiate CCH-synthesized 499 errors (client disconnection) from upstream providers returning HTTP 499. Previously, isClientAbortError() matched any ProxyError with statusCode 499, causing upstream 499 responses to bypass circuit-breaker and fallback logic. Now only ProxyError instances explicitly marked with isLocalAbort=true are treated as client aborts. Also removes the broad "aborted" substring match from the message whitelist to avoid false positives on upstream error messages containing that word. * chore: format code (dev-bcdc6b7) * fix: speed up users last usage lookup * fix(db): add covering index for getUsers LATERAL join and fix CASE type mismatch in cache-hit-rate-alert 1. Add idx_usage_ledger_key_created_at_desc_cover index on (key, created_at DESC NULLS LAST, final_provider_id) with partial filter WHERE blocked_by IS NULL. This enables index-only scans for the LATERAL last-usage-per-key subquery in getUsers, reducing query time from ~28s to ~6.4ms. 2. Fix "CASE types integer and text cannot be matched" error in cache-hit-rate-alert by adding explicit ::integer casts to the ttlFallbackSecondsExpr CASE branches. PostgreSQL inferred parameterized values as text, which conflicted with integer literals (3600, 300) in the outer ttlSecondsExpr CASE. 3. Add regression test verifying ::integer casts are present in the generated SQL for cache-hit-rate-alert queries. * chore: format code (dev-63c4c9c) * feat(proxy): support codex desktop alias matching * feat(billing): remove 1M context premium and add Codex 272k badge Anthropic has GA'd the 1M context window for Opus 4.6 and Sonnet 4.6, eliminating the long-context surcharge. The beta header `context-1m-2025-08-07` is no longer needed. Backend: - Remove CONTEXT_1M_SUPPORTED_MODEL_PREFIXES, CONTEXT_1M_BETA_HEADER, Context1mPreference type, isContext1mSupportedModel, shouldApplyContext1m - Remove AnthropicContext1mHeaderOverrideSpecialSetting type and derived logic - Simplify forwarder 1M decision: just record client header, no injection - Remove provider-selector Step 2.5 context1mPreference filter - Add Codex 272k badge: set context1mApplied when input > 272k tokens Frontend: - Remove 1M Context Window setting from provider forms (options-section, provider-form-context, provider-form-types, batch-edit, legacy form) - Remove premium pricing tooltips from usage-logs-table i18n: - Remove context1m provider config keys (5 languages) - Remove context1mPricing premium text from dashboard (5 languages) Tests: - Remove anthropic_context_1m_header_override test cases - Remove context1mPreference UI and patch draft tests * chore: format code (dev-5a7db40) * feat(usage): add anthropic effort tracking and display. #900 (#901) * feat(usage): add anthropic effort tracking and display. #900 * chore: format code (effort-tag-558c8fd) --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * feat(deploy): support incremental updates to preserve secrets on re-deploy Previously, re-running deploy.sh/deploy.ps1 would regenerate DB_PASSWORD, ADMIN_TOKEN, and SUFFIX, causing database connection failures because the PostgreSQL data volume still held the old password. Now the scripts auto-detect existing deployments (.env + docker-compose.yaml) and enter update mode: preserving DB_PASSWORD, ADMIN_TOKEN, SUFFIX, APP_PORT, and any user-added custom env vars (e.g. LANGFUSE_*, FETCH_*). A --force-new / -ForceNew flag allows forcing a fresh install when needed. * feat: support long-context pricing metadata * feat: support long-context pricing metadata * fix: return billing metadata when pricing is missing * refactor: extract utilities, i18n fixes, and codex threshold constant - Extract getUserFacingProviderTypes() to filter internal provider types - Extract CODEX_1M_CONTEXT_TOKEN_THRESHOLD constant and maybeSetCodexContext1m helper - Refactor limit-rule-picker to use t() shorthand for getTranslation - i18n: add templateClaudeApi/GeminiApi/OpenaiApi keys to errorRules (5 locales) - i18n: replace hardcoded API names in override-section with t() calls - Add codex desktop alias matching tests for detectClientFull - Improve error logging in edit-key-form provider group fetch - Remove default parameter value from data-table actions column title * refactor: move effort badge from table to request detail dialog Move the Anthropic effort badge display from the usage logs table (both admin and my-usage) into the SummaryTab's Session Info section within the ErrorDetailsDialog. This declutters the table view and provides richer context by showing override information (original vs overridden effort) when a provider parameter override changes the effort value. - Add extractAnthropicEffortInfo utility combining anthropic_effort and provider_parameter_override special settings - Display effort badge(s) in SummaryTab with override arrow notation - Remove effort badge from ModelDisplayWithRedirect, both usage tables - Add i18n keys (effort.label, effort.overridden) for all 5 languages - Add 11 table-driven unit tests covering all branches * fix: return proper object type in updateRequestCostFromUsage (#914) Fixed TypeScript error where bare 'return;' returned undefined instead of the expected object type with costUsd, resolvedPricing, longContextPricing, and longContextPricingApplied properties. Error: src/app/v1/_lib/proxy/response-handler.ts(3111,7): error TS2322: Type 'undefined' is not assignable to type '{ costUsd: string | null; resolvedPricing: ResolvedPricing | null; longContextPricing: ResolvedLongContextPricing | null; longContextPricingApplied: boolean; }'. CI Run: https://github.com/ding113/claude-code-hub/actions/runs/23082652215 Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: return proper type in calculateAndUpdateRequestCost early exit (#915) The early return when no price data is found was returning undefined instead of the expected object type. Fixed to return consistent object structure matching other early returns in the function. Fixed: - Type 'undefined' is not assignable to type return object in response-handler.ts:3111 CI Run: https://github.com/ding113/claude-code-hub/actions/runs/23082651541 Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: miraserver <20286838+miraserver@users.noreply.github.com> Co-authored-by: Night <lzy200816@gmail.com> Co-authored-by: Hwwwww-dev <47653238+Hwwwww-dev@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
This was referenced Jun 10, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add advanced mode to request filters with a typed Operation DSL (
set,remove,merge,insert) enabling expressive header/body manipulation beyond simple scope/action/target/replacement. Implement final execution phase in proxy forwarder - advanced-mode filters are now applied to outbound requests before forwarding to upstream providers. Harden request filter engine against prototype pollution (__proto__,constructor,prototypepath traversal) and ReDoS bypass (regex complexity limits). Address i18n and a11y code review findings across all 5 supported languages.Problem
The existing request filter engine supports only simple operations (single scope/action/target/replacement pattern). This limits expressiveness for complex header/body manipulation scenarios such as:
Related Issues: None (new feature)
Solution
Introduce a two-mode system for request filters:
Key design decisions:
FilterMatcherabstraction for exact/contains/regex matching strategiesguardphase (pre-validation) vsfinalphase (pre-forward)Changes
Core Engine (
src/lib/request-filter-engine.ts)set(withoverwrite/if_missingwrite modes),remove(with matcher-based array element removal),merge(deep recursive with null-delete semantics), andinsert(with position/anchor/dedupe)FilterMatchersupportingexact,contains, andregexmatch strategiesTypes (
src/lib/request-filter-types.ts)SetOp,RemoveOp,MergeOp,InsertOp,FilterMatcherSchema & Data Layer
rule_mode,execution_phase,operationsJSONB column to request filters tableidx_request_filters_phasefor efficient phase-based queriesProxy Integration (
src/app/v1/_lib/proxy/forwarder.ts)UI (
filter-dialog.tsx,filter-table.tsx)Security (
src/actions/request-filters.ts)Breaking Changes
None. This is a backward-compatible addition:
rule_mode: 'simple'andexecution_phase: 'guard'Testing
Automated Tests
tests/unit/request-filter-advanced.test.ts)Manual Testing
Checklist
Description enhanced by Claude AI
Greptile Summary
This PR introduces advanced mode for request filters — a typed Operation DSL (
set,remove,merge,insert) with a two-phase execution model (guardfor pre-validation,finalfor pre-forward). The existing simple mode is fully backward-compatible. The engine implementation is solid: prototype pollution guards inparsePathanddeepMerge, ReDoS protection viasafe-regex, and transport header blacklisting. The DB migration, repository layer, forwarder integration, UI, and i18n are all clean additions.Two server-side validation gaps need addressing before the feature ships:
dedupe.byFieldsfield names are not validated againstVALIDATION_UNSAFE_KEYS(src/actions/request-filters.ts:116). An admin submittingbyFields: ["__proto__"]causesexecuteInsertOpto silently skip every insertion becausedeepEqual(Object.prototype, Object.prototype)always short-circuits totrue, making every element appear to already exist as a duplicate.validateMatcher.fieldis not checked for prototype-polluting names (src/actions/request-filters.ts:38). Amatcher.field = "__proto__"combined withmatchType: "contains"andvalue: "object"matches every plain object (sinceString(Object.prototype)is"[object Object]"), allowing a remove operation to silently wipe an entire array.Both issues are admin-only exploitable but represent correctness and defense-in-depth gaps that should be closed before the feature is enabled.
Confidence Score: 2/5
validateOperationsandvalidateMatcherallow__proto__-class field names to reach the engine viadedupe.byFieldsandmatcher.field, producing silent correctness failures rather than caught errors. These are in addition to the three issues already flagged in prior review threads (transport-header blacklist reachability, merge value key scanning, andmatchElementfield traversal). With five open defects across the same security/correctness surface, the feature needs another pass before merging.src/actions/request-filters.ts— two validation functions (validateMatcherand thededupe.byFieldspath invalidateOperations) needVALIDATION_UNSAFE_KEYScoverage added;src/lib/request-filter-engine.ts— three issues flagged in prior review threads remain openImportant Files Changed
applyFinal, advanced op executors (set/remove/merge/insert), andFilterMatcher. Prototype pollution guards inparsePathanddeepMergeare correct; howevermatchElementdoes not guardmatcher.fieldagainst unsafe key traversal (flagged in prior thread).dedupe.byFieldsinexecuteInsertOpalso accesses fields directly without UNSAFE_KEYS check, causing silent no-op inserts whenbyFields: ["__proto__"]is used.validateOperationscorrectly guardsop.pathfor prototype pollution but misses two gaps: (1)dedupe.byFieldsfield names are not checked againstVALIDATION_UNSAFE_KEYS, enabling a silent insert no-op; (2)validateMatcher.fieldis never validated, allowing dangerous key traversal at the engine layer.applyFinalinto the proxy forwarding pipeline for both Gemini and Anthropic paths. Gemini correctly usesstructuredClonebefore passing body toapplyFinalto avoid mutatingsession.request.message. The Anthropic path safely avoids mutation by working on the copy returned byfilterPrivateParameters. BothbypassRequestFiltersguards are in place.InsertOp.dedupe.byFieldsis typed asstring[]with no runtime safety constraints — the validation gap in actions means dangerous values can reach the engine.rule_mode,execution_phase,operations) through all CRUD operations, with safe defaults and proper type casting.(is_enabled, execution_phase). Backward compatible; no data loss risk.byFields: ["__proto__"]silent no-op edge case.useTranslations.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming Request] --> B[Guard Phase] B --> C{Filter\nbindingType?} C -->|global| D[applyGlobal\nmodifies session.headers\n& session.request.message] C -->|providers/groups| E[applyForProvider\nmodifies session] D --> F[Provider Selection\n& Auth] E --> F F --> G{Provider\nType?} G -->|Anthropic/Claude| H[filterPrivateParameters\nreturns fresh copy] G -->|Gemini| I[structuredClone\nbodyToSerialize] H --> J[applyFinal\nAnthropomorphic Path\nline 2250] I --> K[applyFinal\nGemini Path\nline 1915] J --> L{Filter ruleMode?} K --> L L -->|simple| M[applySimpleFilterDirect\nheaders / body] L -->|advanced| N[executeAdvancedOps] N --> O{op type} O -->|set| P[executeSetOp\noverwrite / if_missing] O -->|remove| Q[executeRemoveOp\nmatcher-based array filter] O -->|merge| R[executeMergeOp\ndeepMerge null-delete] O -->|insert| S[executeInsertOp\nposition / anchor / dedupe] M --> T[Transport Header\nBlacklist Enforcement] P --> T Q --> T R --> T S --> T T --> U[Serialize & Forward\nto Upstream]Comments Outside Diff (2)
src/actions/request-filters.ts, line 277-358 (link)Up to 3 sequential DB round-trips for the same record in a single update
updateRequestFilterActioncallsgetRequestFilterById(id)independently in three separate conditional blocks (lines 278, 311, 334). In the worst case — whenruleMode,target, andbindingTypeall change in the same request — three sequential DB queries hit the same row before the actualupdateRequestFilterwrite.Fetch the record once unconditionally at the top of the function, then reuse
existingthroughout all three validation blocks:Prompt To Fix With AI
src/lib/request-filter-engine.ts, line 351-364 (link)reload()early-return resolves callers before actual reload completesWhen
isLoadingistruea second concurrent call toreload()returns an already-resolvedPromise<void>(line 352). Any awaiting caller — notablyrefreshRequestFiltersCachewhich doesawait requestFilterEngine.reload(); const stats = requestFilterEngine.getStats();— will receive stalestats.countdata because the in-flight reload hasn't finished yet.Consider storing the in-flight promise and returning it for subsequent callers, similar to the pattern used in
ensureInitialized:Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: 875c0b5