Merged
Conversation
yarn→npm 移行以降、Penetration testing ワークフローが全ジョブ
ECONNREFUSED ::1:80 で失敗していた問題を修正。
zaproxy ライブラリの ClientApi に proxy URL を文字列で渡していたが、
内部の axios は proxy 設定にオブジェクト {host, port} を期待するため、
hostname/port が undefined となり localhost:80 にフォールバックしていた。
proxy を ClientApi に渡さず、axios が HTTP_PROXY 環境変数を
proxy-from-env 経由で自動解決するよう変更。
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughe2e-tests の ZapClient が内部 API 呼び出しを位置引数からオブジェクト引数へ変更し、コンストラクタで proxy 文字列を解析して Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TestRunner as "Test Runner"
participant ZapClient as "e2e-tests/utils/ZapClient"
participant ClientApi as "ClientApi"
participant ZAP as "ZAP Proxy"
TestRunner->>ZapClient: 公開メソッド呼び出し
ZapClient->>ClientApi: API 呼び出し（オブジェクト引数, proxyConfig を含む場合あり）
ClientApi->>ZAP: HTTP リクエスト送信
ZAP-->>ClientApi: レスポンス返却
ClientApi-->>ZapClient: 結果返却
ZapClient-->>TestRunner: 結果返却
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
手動実行で動作確認できるようにする。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e-tests/utils/ZapClient.ts (1)
350-352:⚠️ Potential issue | 🟡 Minor
isAvailable()の意味論が実態とずれています。
this.proxyはもはや実際のプロキシルーティングを制御していません（HTTP_PROXY環境変数に依存）。isAvailable()がtrueを返してもHTTP_PROXYが未設定であれば ZAP API 呼び出しはプロキシなしで動作します。上記の修正案（this.proxyを直接ClientApiに渡す方針）を採用すれば、この不整合も自然に解消されます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/ZapClient.ts` around lines 350 - 352, The isAvailable() method currently returns this.proxy !== undefined but that no longer reflects whether ZAP API calls will use a proxy (they depend on HTTP_PROXY); update the code so availability matches runtime behavior: either (A) change isAvailable() to check the actual environment proxy variables (e.g. process.env.HTTP_PROXY / process.env.HTTPS_PROXY) instead of this.proxy, or (B) follow the suggested approach and pass this.proxy into the ClientApi constructor (where ClientApi is instantiated) so that ClientApi actually uses the configured proxy and isAvailable() can continue to rely on this.proxy. Locate and update the isAvailable() method, the this.proxy initialization, and the ClientApi construction to make one consistent choice.
🧹 Nitpick comments (1)
e2e-tests/utils/ZapClient.ts (1)
130-136: zaproxy ClientApi のproxyオプションは明示的に{host, port}オブジェクトで指定すべきzaproxy npm パッケージの
ClientApiはproxyオプションとして{host: string, port: number}オブジェクト形式を要求しており、文字列 URL を渡すのは正しくありません。本 PR でproxyを削除しHTTP_PROXY環境変数に依存する現在のアプローチは、CI ワークフロー（.github/workflows/penetration-tests.ymlの lines 89-90）でHTTP_PROXY: 'localhost:8090'が設定されているため機能していますが、暗黙的な依存であり、ローカル開発環境では HTTP_PROXY が設定されていない場合に機能しません。既に
this.proxyに URL 文字列が保持されているため、これをnew URL()で解析して{host, port}オブジェクト形式で直接渡す方が、より明示的で堅牢です。♻️ より明示的な修正案
constructor (proxy?: string | null, apiKey?: string | null) { this.proxy = proxy ?? PlaywrightConfig.use?.proxy?.server; this.apiKey = apiKey !== undefined ? apiKey : null; + const proxyUrl = this.proxy ? new URL(this.proxy) : undefined; this.zaproxy = new ClientApi({ - apiKey: this.apiKey + apiKey: this.apiKey, + ...(proxyUrl && { + proxy: { + host: proxyUrl.hostname, + port: parseInt(proxyUrl.port, 10), + }, + }), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/ZapClient.ts` around lines 130 - 136, The constructor currently passes a string or null as the proxy to new ClientApi, but ClientApi expects a {host: string, port: number} object; update the ZapClient constructor to, when this.proxy is non-null/defined, parse it with new URL(this.proxy) and build a proxy object { host: url.hostname, port: Number(url.port) || (url.protocol === 'https:' ? 443 : 80) } and pass that as the proxy option to new ClientApi (keep apiKey handling the same); when this.proxy is null/undefined, omit the proxy option so ClientApi falls back to environment settings. Ensure you reference this.proxy, apiKey, zaproxy and ClientApi in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@e2e-tests/utils/ZapClient.ts`:
- Around line 350-352: The isAvailable() method currently returns this.proxy !==
undefined but that no longer reflects whether ZAP API calls will use a proxy
(they depend on HTTP_PROXY); update the code so availability matches runtime
behavior: either (A) change isAvailable() to check the actual environment proxy
variables (e.g. process.env.HTTP_PROXY / process.env.HTTPS_PROXY) instead of
this.proxy, or (B) follow the suggested approach and pass this.proxy into the
ClientApi constructor (where ClientApi is instantiated) so that ClientApi
actually uses the configured proxy and isAvailable() can continue to rely on
this.proxy. Locate and update the isAvailable() method, the this.proxy
initialization, and the ClientApi construction to make one consistent choice.
---
Nitpick comments:
In `@e2e-tests/utils/ZapClient.ts`:
- Around line 130-136: The constructor currently passes a string or null as the
proxy to new ClientApi, but ClientApi expects a {host: string, port: number}
object; update the ZapClient constructor to, when this.proxy is
non-null/defined, parse it with new URL(this.proxy) and build a proxy object {
host: url.hostname, port: Number(url.port) || (url.protocol === 'https:' ? 443 :
80) } and pass that as the proxy option to new ClientApi (keep apiKey handling
the same); when this.proxy is null/undefined, omit the proxy option so ClientApi
falls back to environment settings. Ensure you reference this.proxy, apiKey,
zaproxy and ClientApi in the change.
env var 経由ではなく {host, port} オブジェクトとして渡すことで、
axios の setProxy が protocol を上書きしない元の挙動に近づける。
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
docker compose up -d --wait を使用して healthcheck 通過を待機。 healthcheck の interval を 1m30s → 10s に短縮し、 start_period: 30s を追加して ZAP 起動を待つ。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zaproxy v2.0.0-rc.7 の全メソッドは単一のオブジェクト引数を期待するが、
ZapClient は位置引数で呼び出していたため、パラメータが undefined となり
ZAP API が 400 を返していた。
例: setMode('protect') → args.mode = undefined
修正: setMode({ mode: 'protect' }) → args.mode = 'protect'
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
e2e-tests/utils/ZapClient.ts (2)
266-289: 長い行のリファクタリングを検討
activeScanAsUserとactiveScanのオブジェクト構築が1行に詰め込まれており、可読性が低下しています。パラメータオブジェクトをローカル変数に切り出すと、コードレビューやデバッグ時の見通しが良くなります。♻️ リファクタリング案（activeScanAsUser の例）
public async activeScanAsUser (url: string, contextId: number, userId: number, recurse?: boolean, scanPolicyName?: string | null, method?: 'GET' | 'POST' | 'PUT' | 'DELETE', postData?: string | null): Promise<number> { - const result = await this.zaproxy.ascan.scanAsUser({ url, contextid: contextId, userid: userId, recurse: recurse ?? false, scanpolicyname: scanPolicyName ?? null, method: method ?? 'GET', postdata: postData ?? null }); + const params = { + url, + contextid: contextId, + userid: userId, + recurse: recurse ?? false, + scanpolicyname: scanPolicyName ?? null, + method: method ?? 'GET', + postdata: postData ?? null, + }; + const result = await this.zaproxy.ascan.scanAsUser(params); return result.scan; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/ZapClient.ts` around lines 266 - 289, The call-site objects in activeScanAsUser and activeScan are hard to read; refactor by extracting the parameter objects passed to this.zaproxy.ascan.scanAsUser and this.zaproxy.ascan.scan into clearly named local constants (e.g., const params = { url, contextid: contextId, userid: userId, recurse: recurse ?? false, scanpolicyname: scanPolicyName ?? null, method: method ?? 'GET', postdata: postData ?? null } for activeScanAsUser and a similar params object for activeScan with keys url, recurse, inscopeonly, scanpolicyname, method, postdata, contextid), then pass that params variable to the API call and return result.scan as before; update the functions activeScanAsUser and activeScan accordingly to improve readability and ease debugging.
1-1: proxy オブジェクトにprotocolフィールドを明示的に指定してくださいzaproxy の ClientApi が内部で axios を使用する場合、axios の proxy 設定は
{ protocol?: string, host: string, port: number }形式です。protocolは技術的にはオプショナルですが、明示的に指定することで保守性と意図が明確になります。現在の実装（行 133-140）では protocol が含まれていないため、以下のように修正してください：
修正例
const proxyConfig = this.proxy ? (() => { const url = new URL(this.proxy); return { protocol: url.protocol.replace(':', ''), host: url.hostname, port: parseInt(url.port) }; })() : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/ZapClient.ts` at line 1, proxy 設定オブジェクトに protocol フィールドが欠けているため、this.proxy をパースして proxyConfig を作る箇所（proxyConfig / this.proxy / ZapClient）で URL を new URL(this.proxy) して protocol を url.protocol.replace(':','') として明示的に設定し、host と port をそれぞれ url.hostname と parseInt(url.port) から取得して返すように修正してください（プロキシが未指定の場合は undefined を返す既存の挙動は維持）。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e-tests/utils/ZapClient.ts`:
- Around line 133-140: The proxy port parsing in ZapClient (the proxyConfig
block that builds { host: url.hostname, port: parseInt(url.port) } used to
construct new ClientApi) can produce NaN when url.port is empty; change the
logic to detect empty/invalid port and fall back to a default numeric port
(e.g., 8090) before passing to ClientApi (ensure port is a number, not NaN), so
when this.proxy is present but URL has no explicit port the proxyConfig uses the
fallback port.
- Around line 229-232: getMessages and getAlerts can drop falsy params (0/false)
because zaproxy lib filters out non-truthy values; update the calls to pass
explicit nullish defaults using the Nullish Coalescing Operator (e.g., pass
start: start ?? undefined, count: count ?? undefined, and riskid: riskid ??
undefined) like other methods (activeScanAsUser, activeScan) do so the
zaproxy.core.messages and zaproxy.core.alerts requests receive intended falsy
values.
---
Nitpick comments:
In `@e2e-tests/utils/ZapClient.ts`:
- Around line 266-289: The call-site objects in activeScanAsUser and activeScan
are hard to read; refactor by extracting the parameter objects passed to
this.zaproxy.ascan.scanAsUser and this.zaproxy.ascan.scan into clearly named
local constants (e.g., const params = { url, contextid: contextId, userid:
userId, recurse: recurse ?? false, scanpolicyname: scanPolicyName ?? null,
method: method ?? 'GET', postdata: postData ?? null } for activeScanAsUser and a
similar params object for activeScan with keys url, recurse, inscopeonly,
scanpolicyname, method, postdata, contextid), then pass that params variable to
the API call and return result.scan as before; update the functions
activeScanAsUser and activeScan accordingly to improve readability and ease
debugging.
- Line 1: proxy 設定オブジェクトに protocol フィールドが欠けているため、this.proxy をパースして proxyConfig
を作る箇所（proxyConfig / this.proxy / ZapClient）で URL を new URL(this.proxy) して
protocol を url.protocol.replace(':','') として明示的に設定し、host と port をそれぞれ
url.hostname と parseInt(url.port) から取得して返すように修正してください（プロキシが未指定の場合は undefined
を返す既存の挙動は維持）。
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/penetration-tests.ymldocker-compose.owaspzap.daemon.ymle2e-tests/utils/ZapClient.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/penetration-tests.yml
Comment on lines
229
to
232
| public async getMessages (url: string, start?: number, count?: number): Promise<HttpMessage[]> { | ||
| const result = await this.zaproxy.core.messages(url, start, count); | ||
| const result = await this.zaproxy.core.messages({ baseurl: url, start, count }); | ||
| return result.messages; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual ZapClient.ts file to understand the context
cat -n e2e-tests/utils/ZapClient.ts | sed -n '220,240p'Repository: EC-CUBE/ec-cube2
Length of output: 944
🏁 Script executed:
# Also check line 321 mentioned for getAlerts
cat -n e2e-tests/utils/ZapClient.ts | sed -n '315,330p'Repository: EC-CUBE/ec-cube2
Length of output: 633
🏁 Script executed:
# Check if zaproxy is installed and look at its implementation
fd -type f -name 'package.json' | head -1 | xargs cat | jq '.dependencies, .devDependencies' 2>/dev/nullRepository: EC-CUBE/ec-cube2
Length of output: 230
🏁 Script executed:
# Look for zaproxy library files or documentation
fd -type f 'zaproxy' --path '*/node_modules/*' 2>/dev/null | grep -E '\.(js|ts|json)$' | head -20Repository: EC-CUBE/ec-cube2
Length of output: 42
🏁 Script executed:
# Search for how undefined parameters are handled in the codebase
rg 'zaproxy\.' e2e-tests/utils/ZapClient.ts -A 2 -B 2 | head -50Repository: EC-CUBE/ec-cube2
Length of output: 1573
🏁 Script executed:
# Search package.json to find zaproxy version and info
find . -name package.json -type f | xargs grep -l zaproxy 2>/dev/nullRepository: EC-CUBE/ec-cube2
Length of output: 74
🏁 Script executed:
# Look for all methods in ZapClient that use optional parameters to see patterns
rg 'async \w+.*\?:' e2e-tests/utils/ZapClient.ts -A 2 | head -80Repository: EC-CUBE/ec-cube2
Length of output: 1874
🏁 Script executed:
# Check if there's any utility to filter undefined values
rg 'undefined' e2e-tests/utils/ZapClient.ts -B 2 -A 2Repository: EC-CUBE/ec-cube2
Length of output: 872
🏁 Script executed:
# Look at zaproxy usage patterns in tests to see if undefined causes issues
rg 'zaproxy\.' e2e-tests/tests/ -t ts --max-count 20 | head -30Repository: EC-CUBE/ec-cube2
Length of output: 116
🏁 Script executed:
# Check zaproxy API documentation comments to understand parameter expectations
rg 'zaproxy.core' e2e-tests/utils/ZapClient.ts -B 5 | head -50Repository: EC-CUBE/ec-cube2
Length of output: 1890
🌐 Web query:
zaproxy npm package ClientApi optional parameters undefined handling
💡 Result:
In the zaproxy Node package (zap-api-nodejs), “optional parameter handling” is mostly implemented in each component method (e.g., spider.scan, spider.scanAsUser) by only adding a parameter to the request if it passes a truthy check like:
if (args.subtreeonly && args.subtreeonly !== null) { params.subtreeOnly = args.subtreeonly }if (args.recurse && args.recurse !== null) { params.recurse = args.recurse }
(and similar patterns throughout the generated API wrappers) [2].
What this means for undefined
- If you pass
undefinedfor an optional arg, it’s not added toparams, so it’s effectively omitted from the request (good/expected) [2]. - The client then sends
paramsvia Axios as query parameters for GET requests (requestConfig.params = data) [1].
The gotcha: “falsy” values are treated like “unset”
Because the code checks truthiness (not just !== undefined), these values will be dropped exactly like undefined:
false0""(empty string)
So if an API endpoint expects you to explicitly send 0/false, you must avoid passing them as boolean/number primitives in those wrappers; instead, pass strings like "0" / "false" so they remain truthy and are included [2].
Practical guidance
- To “not set” an optional param: omit the property or set it to
undefined(either way it won’t be sent) [2]. - To explicitly set values that might be falsy: pass them as strings (e.g.,
{ recurse: "false" },{ maxchildren: "0" }) so the wrapper includes them [2].
Sources: client request building (src/index.js) [1], parameter inclusion logic example (src/spider.js) [2].
getMessages と getAlerts メソッドで、ゼロや false などのfalsy値が正しく渡されない可能性があります
zaproxy ライブラリは、パラメータの真偽値チェックに基づいて、undefined だけでなく 0 や false などのfalsy値も要求から除外します。ページネーション開始位置が 0、取得件数が 0、またはリスク ID が falsy値の場合、これらの値は API に渡されずに削除されます。
この問題を回避するため、同ファイル内の他のメソッド（activeScanAsUser、activeScan など）に従い、Nullish Coalescing Operator（??）を使用してデフォルト値を明示的に設定してください：
// 現在（問題あり）
const result = await this.zaproxy.core.messages({ baseurl: url, start, count });
// 推奨される修正
const result = await this.zaproxy.core.messages({ baseurl: url, start: start ?? undefined, count: count ?? undefined });または、getAlerts の場合：
const result = await this.zaproxy.core.alerts({ baseurl: url, start: start ?? undefined, count: count ?? undefined, riskid: riskid ?? undefined });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-tests/utils/ZapClient.ts` around lines 229 - 232, getMessages and
getAlerts can drop falsy params (0/false) because zaproxy lib filters out
non-truthy values; update the calls to pass explicit nullish defaults using the
Nullish Coalescing Operator (e.g., pass start: start ?? undefined, count: count
?? undefined, and riskid: riskid ?? undefined) like other methods
(activeScanAsUser, activeScan) do so the zaproxy.core.messages and
zaproxy.core.alerts requests receive intended falsy values.
Contributor
Author
There was a problem hiding this comment.
ご指摘ありがとうございます。
提案された修正 start: start ?? undefined は no-op（変更なし）となります。0 ?? undefined は 0 を返すため、zaproxy ライブラリ内部の truthy チェックは回避できません。
実際の使用箇所を確認したところ:
getAlerts(url, 0, 1, Risk.High)—start=0は省略されてもデフォルト動作（先頭から取得）と同じ結果getMessagesはgetLastMessage経由でstart=numberOfMessages（0以外）で呼ばれているRisk.Informational=0が落ちる可能性はあるが、コード内ではRisk.High=3のみ使用
現状の使用パターンでは実害がないため、対応なしとします。
There was a problem hiding this comment.
@nobuhiko さん、ご指摘ありがとうございます。おっしゃる通りです。
私の提案した start ?? undefined は確かに no-op です。0 ?? undefined は 0 を返すため、zaproxy ライブラリ内部の truthy チェックを回避できません。Nullish Coalescing Operator は null または undefined の場合のみ右辺を返すため、0 や false などの他の falsy 値には対応していませんでした。
実際の使用パターンを確認いただき、現状では実害がないとのこと、承知いたしました。start=0 の省略がデフォルト動作と同じ結果になること、Risk.Informational=0 が実際には使用されていないことから、対応不要という判断は妥当です。
ご丁寧な説明をありがとうございました。
/_/
( o.o ) 納得しました!
^
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1349 +/- ##
=======================================
Coverage 54.42% 54.42%
=======================================
Files 84 84
Lines 10817 10817
=======================================
Hits 5887 5887
Misses 4930 4930
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- proxyConfigにprotocolフィールドを追加 - port未指定時のNaNフォールバック(デフォルト80/443)を追加 - activeScanAsUser/activeScanのパラメータオブジェクトを変数に抽出して可読性を向上 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
e2e-tests/utils/ZapClient.ts (1)
233-235:⚠️ Potential issue | 🟡 Minorstart/count/riskid の 0 が zaproxy 側で落ちる可能性
zaproxy ラッパーが truthy チェックでパラメータを落とす実装だと、
0（ページ開始位置や Informational=0）が送信されません。必要なら文字列化などで確実に送れる形にしてください。🛠️ 例: 0 を確実に送る場合
- const result = await this.zaproxy.core.messages({ baseurl: url, start, count }); + const result = await this.zaproxy.core.messages({ + baseurl: url, + start: start == null ? undefined : String(start), + count: count == null ? undefined : String(count) + });- const result = await this.zaproxy.core.alerts({ baseurl: url, start, count, riskid }); + const result = await this.zaproxy.core.alerts({ + baseurl: url, + start: start == null ? undefined : String(start), + count: count == null ? undefined : String(count), + riskid: riskid == null ? undefined : String(riskid) + });zaproxy npm package ClientApi params truthy check start count riskid 0 droppedAlso applies to: 342-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/ZapClient.ts` around lines 233 - 235, The zaproxy wrapper may drop parameters when they are falsy (0), so in getMessages ensure numeric params (start, count, riskid) are sent even when 0 by converting them to strings or by explicitly including them when they are not undefined (e.g. use start !== undefined ? String(start) : undefined), and apply the same change to the other ZapClient methods flagged around lines 342-344 so the params object always contains "start", "count" or "riskid" as string values when provided rather than relying on truthy checks.
🧹 Nitpick comments (2)
e2e-tests/utils/ZapClient.ts (2)
133-143: プロキシURLの認証情報が落ちる可能性
new URL()で取り出せるusername/passwordを捨てているので、認証付きプロキシを使う場合に接続できなくなる恐れがあります。必要なら axios のproxy.authに反映するか、サポート外であることを明記してください。🔧 例: 認証情報を反映する場合
const proxyConfig = this.proxy ? (() => { const url = new URL(this.proxy); + const auth = url.username + ? { username: decodeURIComponent(url.username), password: decodeURIComponent(url.password) } + : undefined; return { protocol: url.protocol.replace(':', ''), host: url.hostname, - port: parseInt(url.port) || (url.protocol === 'https:' ? 443 : 80) + port: parseInt(url.port) || (url.protocol === 'https:' ? 443 : 80), + ...(auth ? { auth } : {}) }; })() : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/ZapClient.ts` around lines 133 - 143, The proxyConfig construction in ZapClient (proxyConfig / this.proxy used when constructing new ClientApi) currently discards URL username/password; update it to extract url.username and url.password and, if present, set proxy.auth with { username, password } (or axios-compatible auth) so authenticated proxies work, or explicitly throw/log that proxy authentication is unsupported; ensure the updated fields are passed into the ClientApi instantiation so credentials are applied when this.proxy includes authentication.
300-309: inscopeonly のデフォルトが true になる点は意図確認を未指定時に
trueを強制すると、ZAP 側のデフォルト（もし false/未指定扱い）と挙動が変わる可能性があります。従来と同じ動作にしたいなら未指定時は送らない形にするのが安全です。🔧 例: 未指定時は送らない
- inscopeonly: inScopeOnly ?? true, + inscopeonly: inScopeOnly ?? undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/ZapClient.ts` around lines 300 - 309, The params object currently forces inscopeonly: inScopeOnly ?? true which changes default behavior; change it to only include inscopeonly when inScopeOnly is explicitly provided (e.g., omit the key if inScopeOnly is undefined) so that this.zaproxy.ascan.scan receives no inscopeonly parameter when not specified and ZAP can apply its own default; update the construction of params (the params literal around url, recurse, inscopeonly, scanpolicyname, etc.) to conditionally add the inscopeonly property rather than using the nullish-coalescing default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e-tests/utils/ZapClient.ts`:
- Around line 233-235: The zaproxy wrapper may drop parameters when they are
falsy (0), so in getMessages ensure numeric params (start, count, riskid) are
sent even when 0 by converting them to strings or by explicitly including them
when they are not undefined (e.g. use start !== undefined ? String(start) :
undefined), and apply the same change to the other ZapClient methods flagged
around lines 342-344 so the params object always contains "start", "count" or
"riskid" as string values when provided rather than relying on truthy checks.
---
Nitpick comments:
In `@e2e-tests/utils/ZapClient.ts`:
- Around line 133-143: The proxyConfig construction in ZapClient (proxyConfig /
this.proxy used when constructing new ClientApi) currently discards URL
username/password; update it to extract url.username and url.password and, if
present, set proxy.auth with { username, password } (or axios-compatible auth)
so authenticated proxies work, or explicitly throw/log that proxy authentication
is unsupported; ensure the updated fields are passed into the ClientApi
instantiation so credentials are applied when this.proxy includes
authentication.
- Around line 300-309: The params object currently forces inscopeonly:
inScopeOnly ?? true which changes default behavior; change it to only include
inscopeonly when inScopeOnly is explicitly provided (e.g., omit the key if
inScopeOnly is undefined) so that this.zaproxy.ascan.scan receives no
inscopeonly parameter when not specified and ZAP can apply its own default;
update the construction of params (the params literal around url, recurse,
inscopeonly, scanpolicyname, etc.) to conditionally add the inscopeonly property
rather than using the nullish-coalescing default.
- contact.test.ts: ZAP proxyモードでのメールアドレス期待値を修正 (forced userの zap_user@example.com を考慮) - cart.test.ts: fetch後のページ再読み込みと商品追加を追加 (アクティブスキャン後のカート空状態に対応) - shopping_deliv.test.ts: POST攻撃テスト前にページ遷移を追加 - shopping_confirm.test.ts: 注文完了後にショッピングフローをやり直す (セッションクリア後の確認ページ到達のため) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
e2e-tests/test/front_login/contact.test.ts (1)
42-44:expectedEmailの算出ロジックが重複しています（オプション）全く同じ式
PlaywrightConfig.use?.proxy !== undefined ? 'zap_user@example.com' : mypageLoginPage.emailが Line 42 と Line 67 の両テストブロックに重複しています。'zap_user@example.com'を定数として切り出し、算出ロジックをヘルパー関数化することで変更箇所を一元化できます。♻️ リファクタリング案
+const ZAP_FORCED_USER_EMAIL = 'zap_user@example.com'; + +const getExpectedEmail = (proxy: typeof PlaywrightConfig.use.proxy, fallback: string) => + proxy !== undefined ? ZAP_FORCED_USER_EMAIL : fallback; test('ログイン状態を確認します', async ( { mypageLoginPage, page } ) => { await page.goto(PlaywrightConfig.use?.baseURL ?? '/'); await page.goto(url); await expect(page.locator('#header')).toContainText('ようこそ'); inputNames.forEach(async (name) => expect(page.locator(`input[name=${name}]`)).not.toBeEmpty()); - const expectedEmail = PlaywrightConfig.use?.proxy !== undefined ? 'zap_user@example.com' : mypageLoginPage.email; + const expectedEmail = getExpectedEmail(PlaywrightConfig.use?.proxy, mypageLoginPage.email); await expect(page.locator('input[name=email]')).toHaveValue(expectedEmail); await expect(page.locator('input[name=email02]')).toHaveValue(expectedEmail); });同様に Line 67 も
getExpectedEmailに置き換えます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/test/front_login/contact.test.ts` around lines 42 - 44, Duplicate inline logic computing expectedEmail should be centralized: extract the literal 'zap_user@example.com' into a constant (e.g., ZAP_USER_EMAIL) and move the ternary expression PlaywrightConfig.use?.proxy !== undefined ? ZAP_USER_EMAIL : mypageLoginPage.email into a small helper function (e.g., getExpectedEmail()) and replace the duplicated expressions in the test with calls to getExpectedEmail(); update uses in both assertions that reference expectedEmail so they call getExpectedEmail() instead of duplicating the ternary.e2e-tests/test/front_login/cart/cart.test.ts (1)
85-89: ベースURLへの遷移は不要です（オプション）Line 86 の
page.goto(baseURL)は、直後のproductsDetailPage.goto(1)が完全なURLで直接遷移するため冗長です。また、
goto(1)/cartIn(2, '抹茶', 'S')の組み合わせがshopping_confirm.test.tsにも重複しています。共通フィクスチャやヘルパー関数へ切り出すと保守性が向上します。♻️ 冗長なナビゲーション削除の提案
- // スキャン後にカートが空になる場合があるため、商品を追加し直す - await page.goto(PlaywrightConfig.use?.baseURL ?? '/'); - const productsDetailPage = new ProductsDetailPage(page); + // スキャン後にカートが空になる場合があるため、商品を追加し直す + const productsDetailPage = new ProductsDetailPage(page); await productsDetailPage.goto(1); await productsDetailPage.cartIn(2, '抹茶', 'S');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/test/front_login/cart/cart.test.ts` around lines 85 - 89, Remove the redundant navigation to the base URL (the call to page.goto(...)) since ProductsDetailPage.goto(1) performs a full navigation; delete that line and rely on ProductsDetailPage.goto. Then factor the repeated sequence ProductsDetailPage.goto(1) + ProductsDetailPage.cartIn(2, '抹茶', 'S') into a shared helper or fixture (e.g., a function addProductToCart(page, productId, qty, name, size) used by both cart.test.ts and shopping_confirm.test.ts) so both tests call the helper instead of duplicating the steps.e2e-tests/test/front_login/shopping/shopping_confirm.test.ts (1)
45-49: ベースURLへの遷移は不要です（オプション）Line 46 の
page.goto(baseURL)はproductsDetailPage.goto(1)が完全なURLで直接遷移するため冗長です。また、goto(1)/cartIn(2, '抹茶', 'S')の組み合わせはcart.test.tsLines 87–89 と同一であり、プロダクトID・数量・カテゴリが双方にハードコードされています。共通のヘルパーまたはフィクスチャに切り出すと、テストデータ変更時の修正箇所を一元化できます。♻️ 冗長なナビゲーション削除の提案
// 注文完了後はセッションがクリアされるため、カートに商品を追加してフローをやり直す - await page.goto(PlaywrightConfig.use?.baseURL ?? '/'); - const productsDetailPage = new ProductsDetailPage(page); + const productsDetailPage = new ProductsDetailPage(page); await productsDetailPage.goto(1); await productsDetailPage.cartIn(2, '抹茶', 'S');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/test/front_login/shopping/shopping_confirm.test.ts` around lines 45 - 49, Remove the redundant base URL navigation and centralize the product add-to-cart steps: delete the call to page.goto(PlaywrightConfig.use?.baseURL ?? '/') and instead reuse or create a shared helper (e.g., addProductToCart or a fixture) that encapsulates ProductsDetailPage.goto(productId) and ProductsDetailPage.cartIn(quantity, name, size); replace the duplicated inline calls to ProductsDetailPage.goto and cartIn in this test with a call to that helper so productId/quantity/category are maintained in one place (same values currently passed to ProductsDetailPage.goto and cartIn).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e-tests/test/front_login/shopping/shopping_confirm.test.ts`:
- Around line 50-51: cartIn() may not leave the browser on the cart page, so
calling CartPage.gotoNext() directly can fail; update the test to explicitly
navigate to the cart page before interacting by calling CartPage.goto() (the
same pattern used in cart.test.ts) immediately before CartPage.gotoNext() to
ensure the page is at /cart/index.php and the next-button exists.
---
Nitpick comments:
In `@e2e-tests/test/front_login/cart/cart.test.ts`:
- Around line 85-89: Remove the redundant navigation to the base URL (the call
to page.goto(...)) since ProductsDetailPage.goto(1) performs a full navigation;
delete that line and rely on ProductsDetailPage.goto. Then factor the repeated
sequence ProductsDetailPage.goto(1) + ProductsDetailPage.cartIn(2, '抹茶', 'S')
into a shared helper or fixture (e.g., a function addProductToCart(page,
productId, qty, name, size) used by both cart.test.ts and
shopping_confirm.test.ts) so both tests call the helper instead of duplicating
the steps.
In `@e2e-tests/test/front_login/contact.test.ts`:
- Around line 42-44: Duplicate inline logic computing expectedEmail should be
centralized: extract the literal 'zap_user@example.com' into a constant (e.g.,
ZAP_USER_EMAIL) and move the ternary expression PlaywrightConfig.use?.proxy !==
undefined ? ZAP_USER_EMAIL : mypageLoginPage.email into a small helper function
(e.g., getExpectedEmail()) and replace the duplicated expressions in the test
with calls to getExpectedEmail(); update uses in both assertions that reference
expectedEmail so they call getExpectedEmail() instead of duplicating the
ternary.
In `@e2e-tests/test/front_login/shopping/shopping_confirm.test.ts`:
- Around line 45-49: Remove the redundant base URL navigation and centralize the
product add-to-cart steps: delete the call to
page.goto(PlaywrightConfig.use?.baseURL ?? '/') and instead reuse or create a
shared helper (e.g., addProductToCart or a fixture) that encapsulates
ProductsDetailPage.goto(productId) and ProductsDetailPage.cartIn(quantity, name,
size); replace the duplicated inline calls to ProductsDetailPage.goto and cartIn
in this test with a call to that helper so productId/quantity/category are
maintained in one place (same values currently passed to ProductsDetailPage.goto
and cartIn).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e-tests/test/front_login/cart/cart.test.tse2e-tests/test/front_login/contact.test.tse2e-tests/test/front_login/shopping/shopping_confirm.test.tse2e-tests/test/front_login/shopping/shopping_deliv.test.ts
Comment on lines
+50
to
+51
| const cartPage = new CartPage(page); | ||
| await cartPage.gotoNext(); |
There was a problem hiding this comment.
cartPage.gotoNext() の前に cartPage.goto() が呼ばれていません
cartIn() でカゴに入れる後、ページがカートURL（/cart/index.php）にいる保証がありません。CartPage.gotoNext() は page.goto() を呼ばずに直接ボタンをクリックするため、遷移先によってはボタンが見つからずテストが不安定になります。
参考として cart.test.ts の Lines 91–92 では cartPage.goto() を明示的に呼んでから操作を行っています。
🛠️ 修正案
const cartPage = new CartPage(page);
+ await cartPage.goto();
await cartPage.gotoNext();📝 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.
Suggested change
| const cartPage = new CartPage(page); | |
| await cartPage.gotoNext(); | |
| const cartPage = new CartPage(page); | |
| await cartPage.goto(); | |
| await cartPage.gotoNext(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-tests/test/front_login/shopping/shopping_confirm.test.ts` around lines 50
- 51, cartIn() may not leave the browser on the cart page, so calling
CartPage.gotoNext() directly can fail; update the test to explicitly navigate to
the cart page before interacting by calling CartPage.goto() (the same pattern
used in cart.test.ts) immediately before CartPage.gotoNext() to ensure the page
is at /cart/index.php and the next-button exists.
Contributor
Author
There was a problem hiding this comment.
ご指摘ありがとうございます。
EC-CUBE 2 では「カゴに入れる」ボタンクリック後、カートページ (/cart/index.php) にリダイレクトされるため、cartIn() 完了時点でページはカート上にあります。
同 PR 内の cart.test.ts でも同じパターンを採用しており（既存の cartPage.goto() を削除し、productsDetailPage.cartIn() に置き換え）、一貫した動作となっています。
そのため cartPage.goto() の追加は不要と判断します。
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the cart.test.ts file to see the pattern mentioned by the user
cat e2e-tests/test/front_login/cart/cart.test.ts
echo "===== SEPARATOR ====="
# Check the ProductsDetailPage implementation to understand cartIn() behavior
fd -t f -e ts "detail.page.ts" e2e-tests/pages | xargs cat
Length of output: 7125
@nobuhiko さん、ご説明ありがとうございます。
cart.test.ts の実装を確認したところ、Line 91–95 で productsDetailPage.cartIn() 後に cartPage.goto() を呼ばずに直接 page.locator('input[name=transactionid]') にアクセスしており、cartIn() 完了時点で既にカートページに遷移していることが確認できました。
EC-CUBE 2 の動作仕様により、カゴ入れ後は自動的に /cart/index.php へリダイレクトされるため、shopping_confirm.test.ts でも cartPage.goto() の追加は不要ですね。失礼いたしました。
cartIn() のフォーム送信でカートページに遷移済みのため、 重複する cartPage.goto() を削除 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
外部サービス httpbin.org が一時的に 502 Bad Gateway 等を返す場合、 テストが不安定に失敗していた。5xx応答時は markTestSkipped で スキップするガードを全6件の integration テストに追加。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t-zap-proxy-connection # Conflicts: # tests/class/module/HTTP/HTTP_Request_GuzzleTest.php
nanasess
approved these changes
Feb 25, 2026
Contributor
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
レビュー結果: Approve
検証
- Penetration testing ワークフロー (Run #22337747536) が
workflow_dispatchで手動実行され、全7ジョブが成功していることを確認しました - 通常の CI チェックも全て pass しています
変更内容の評価
| 変更 | 評価 |
|---|---|
ZapClient.ts proxy設定・APIメソッド呼び出し修正 |
OK (penetration test で実証済み) |
penetration-tests.yml workflow_dispatch追加, --wait |
Good |
docker-compose.owaspzap.daemon.yml healthcheck改善 |
Good |
| E2Eテスト修正 (cart, contact, shopping_confirm, shopping_deliv) | Good |
HTTP_Request_GuzzleTest.php httpbin.org 5xxスキップ |
Good |
各変更の所感
- ZapClient.ts:
ClientApiへのproxy設定をオブジェクト形式に変更し、APIメソッド呼び出しを名前付きパラメータ形式に統一。penetration test の成功結果が正しく動作することを実証しています - Workflow/Docker改善:
workflow_dispatchによる手動トリガー追加、docker compose up -d --waitによるサービス起動待機、ZAP の healthcheck をstart_period: 30s/interval: 10s/retries: 30に改善し、起動が遅い場合の待機を適切に処理 - E2Eテスト修正: アクティブスキャン後にカートが空になる問題への対応（商品追加し直し）、fetch後のDOM再読み込み、proxy経由時のemail不一致対応など、テストの安定性向上に寄与
- httpbin.org 5xxスキップ: 外部サービスの一時的な障害によるテスト失敗を防ぐ防御的な対応
軽微な指摘（対応不要）
CodeRabbit が指摘した isAvailable() の件について: this.proxy が設定されていれば proxyConfig も生成されるため、現時点で整合性は保たれており実害はありません。
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
2 participants
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
ECONNREFUSED ::1:80で失敗していた問題を修正ZapClient.tsで zaproxy ライブラリのClientApiに proxy URL を文字列で渡していたが、内部の axios はproxy設定にオブジェクト{host, port}を期待するため、hostname/portがundefinedとなりlocalhost:80にフォールバックしていたproxyをClientApiに渡さず、axios がHTTP_PROXY環境変数をproxy-from-env経由で自動解決するよう変更原因の詳細
修正後は axios が
HTTP_PROXY環境変数をproxy-from-envで読み取り、new URL()で正しくパースします。Test plan
localhost:8090への接続を確認🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート