Conversation
|
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:
📝 WalkthroughWalkthroughOWASP ZAP テストワークフローを拡張し、GitHub Actions ワークフロー、docker-compose 設定、ZAP 自動化テンプレートと関連スクリプト（シード/削除/生成）を更新して、非対話実行・before_script パッチ挿入、アドオン更新、Zest 前処理、alerts マージとアーティファクト処理を追加しています。 Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant EC as EC-CUBE Container
participant DB as Database
participant ZAP as ZAP Container
participant AR as Artifact Store
GH->>EC: build/download EC-CUBE image & start (docker-compose exec -T)
GH->>DB: apply seed SQL (`zap/seed_zap.sql`) and prepare DB
GH->>EC: run non-interactive setup/seed/verify commands (-T)
GH->>ZAP: start ZAP container (entrypoint tail -f /dev/null)
GH->>ZAP: patch automation config (insert before_script)
GH->>ZAP: install add-ons / preprocess Zest (token replacement)
GH->>ZAP: autorun scans (non-interactive)
ZAP-->>GH: emit reports & alerts
GH->>AR: upload reports & zap-*-alerts artifacts (v4, overwrite)
GH->>GH: download zap-*-alerts artifacts and merge -> all_alerts.json
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/zaproxy.yml (1)
26-26:⚠️ Potential issue | 🟠 Major
actions/checkout@masterはセキュリティリスク
@masterは可変参照であり、上流リポジトリへの不正コミットによるサプライチェーン攻撃のリスクがあります。Line 166も同様です。タグ（例:@v4）またはコミットSHAにピン留めしてください。提案
- uses: actions/checkout@master + uses: actions/checkout@v4
🤖 Fix all issues with AI agents
In @.github/workflows/zaproxy.yml:
- Around line 222-237: The workflow never uses matrix-provided before scripts
because the override that assigns ZAP_BEFORE_SCRIPT to BEFORE is commented out;
either uncomment the line "if [[ -n \"${ZAP_BEFORE_SCRIPT:-}\" ]]; then
BEFORE=\"${ZAP_BEFORE_SCRIPT}\"; fi" so ZAP_BEFORE_SCRIPT (from
matrix.before_script) is applied before invoking
zap/generate_automation_config.sh (references: ZAP_BEFORE_SCRIPT, BEFORE, and
the zap/generate_automation_config.sh call), or if matrix.before_script entries
are not needed remove them from the matrix to avoid confusion.
- Line 235: 現在の展開 `${BEFORE:+"-b ${BEFORE}"}` は展開結果が一括した引数（例: "\"-b
admin_login.zst\""）になる可能性があるので、安全に分割した引数を渡すよう修正してください；例えば展開中にクォートを文字列として残さない形
`${BEFORE:+-b "$BEFORE"}` に置き換えるか、より明示的に if [ -n "$BEFORE" ]; then ARGS+=("-b"
"$BEFORE"); fi のように `-b` と値を別々の引数として追加する実装（変更箇所は `${BEFORE:+"-b ${BEFORE}"}`
を含む該当行）にしてください。
- Line 41: The workflow mixes different major versions of the artifact actions;
update every occurrence of actions/upload-artifact to the same major version
(use v6) and every occurrence of actions/download-artifact to the same major
version (use v7) so artifact upload/download calls are consistent; search for
all uses of "actions/upload-artifact" and "actions/download-artifact" in the
file and replace their version tags (e.g., v4/v5/v6/v7) accordingly, then run a
quick workflow lint to ensure no syntax/compatibility issues remain.
In `@zap/automation/template.yml`:
- Around line 64-68: The matrix's thread_per_host setting is being collected
into ZA_THREAD_PER_HOST by generate_automation_config.sh but not applied because
the job uses type: sequence-activeScan which doesn't propagate thread_per_host;
change the job to use the scanner job type that accepts per-scan thread settings
(e.g., activeScan) or add the supported parameter name (threadPerHost) to the
parameters block and bind it to ${ZA_THREAD_PER_HOST}; update template.yml's
sequence-activeScan entry (or replace it with activeScan) and ensure
generate_automation_config.sh still sets ZA_THREAD_PER_HOST so the threadPerHost
value is passed into the scan execution.
In `@zap/seed_zap.sql`:
- Line 52: シードデータの顧客ステータスが現在
customer_status_id=1（仮会員）になっており、認証が必要な画面（front_mypage、mypage_order
等）にアクセスできないためテストが失敗します；seed_zap.sql の該当レコードで customer_status_id を 1 から
2（本会員／ACTIVE）に変更して、ログイン可能な状態にしてテストが通るようにしてください。
🧹 Nitpick comments (2)
zap/generate_automation_config.sh (1)
44-52:.zstファイルの場合にrunブロックを追加しない理由が不明確
.zst（Zestスクリプト）の場合、ここではrunブロックが追加されませんが、ワークフロー側の「Patch automation config」ステップ（Python スクリプト）で後からrunブロックが挿入されます。このロジックが2箇所に分散しているため、保守性が低下しています。なぜ
.zstの場合だけワークフロー側でパッチする必要があるのか、コメントで意図を明記することを推奨します。zap/delete_data.sh (1)
14-37: コメントアウトされた旧コードを削除してください20行以上のコメントアウトコードはバージョン管理で履歴が残るため、削除して可読性を向上させることを推奨します。
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/zaproxy.yml:
- Around line 328-350: The "Debug zap logs after autorun" step runs docker
compose exec -T zap cat /home/zap/.ZAP/zap.log which will fail the step if
zap.log is missing; update this step to first check for the file (e.g., test -f
/home/zap/.ZAP/zap.log inside the container) and only run the cat when it
exists, or fall back to a no-op (or use a command that returns success on
missing files) so the debug step does not error out when /home/zap/.ZAP/zap.log
is absent.
- Around line 387-397: The current merge command jq -s 'add' assumes every
alerts.json is a valid JSON array; instead validate and filter files first:
using the existing file list (from mapfile/find) run each file through jq -e
'type == "array"' (or equivalent) and only pass the successfully-validated files
to jq -s 'add' to produce all_alerts.json (or use a single jq invocation like jq
-s 'map(select(type=="array")) | add' over the files); if no valid arrays
remain, fail with a clear error instead of producing an invalid/empty
all_alerts.json. Ensure you reference the variables/files produced by
mapfile/find and write to all_alerts.json as before.
🧹 Nitpick comments (4)
.github/workflows/zaproxy.yml (4)
241-293: Python によるYAML パッチ処理 — 正規表現ベースのYAML操作はフラジャイル正規表現でYAMLを編集するアプローチは、テンプレートのインデントやフォーマットが変わると容易に壊れます。現時点では動作しているかもしれませんが、将来的なメンテナンスコストが高くなります。
いくつかの懸念点:
- Line 267-276: 正規表現パターンがインデントに厳密に依存しています。テンプレートのフォーマットが少しでも変わるとマッチしなくなり、Line 278 で
SystemExitが発生します。- Line 280-286: 挿入する YAML フラグメントもハードコードされたインデントに依存しています。
将来的には
ruamel.yamlやpyyamlを使ったパース・編集への移行を検討してください。現時点ではCIで追加パッケージのインストールを避ける意図であれば許容範囲ですが、テンプレートとの密結合はリスクです。
302-308: ZAP Add-on の更新・再インストール手順について
sequenceアドオンを一度アンインストールしてから再インストールしています。これは最新版を確実にインストールする意図と思われますが、-addonupdateは既にインストール済みアドオンを更新するはずです。また、各
-cmd呼び出しで ZAP が毎回起動・終了するため、この4ステップでかなりの時間がかかる可能性があります。パフォーマンスが問題になる場合は、複数のアドオン操作を1回の起動にまとめることを検討してください。
226-226:matrix.targetをシェル比較で直接展開するのはインジェクションリスク
${{ matrix.target }}はワークフロー定義内の固定値なので現時点では問題ありませんが、GitHub Actions のベストプラクティスとして、式展開を環境変数経由で渡す方が安全です。Line 234, 331, 340 等でも同様のパターンが使われています。🔧 修正案（Line 226 の例）
+ env: + TARGET: ${{ matrix.target }} run: | set -eu BEFORE="" - if [[ "${{ matrix.target }}" == admin_* ]]; then + if [[ "${TARGET}" == admin_* ]]; then BEFORE="admin_login.zst" fi
189-192: シードデータの冪等性は部分的に担保されています
seed_zap.sqlはDELETE FROM dtb_customer WHERE id = 1;で既存レコードを削除した後に INSERT しているため、基本的な冪等性は実装されています。ただし、この方式は以下の点で脆弱です：
- DELETE が成功しても INSERT が失敗した場合、シーケンスだけ更新される可能性がある
- 単一レコード削除のため、部分的な失敗に対して堅牢ではない
より堅牢な実装としては、
INSERT ... ON CONFLICT DO UPDATE SETまたはTRUNCATEでの全削除 + INSERT を検討してください。現在の実装は機能しますが、ワークフロー再実行時の信頼性を高めるため、上記パターンへの改善が望ましいです。
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/zaproxy.yml:
- Around line 295-300: The debug steps using docker compose exec (the commands
invoking whoami, pwd, ls -l /zap/wrk, and ls -l /home/zap/.ZAP || true) are
missing the non-interactive -T flag; update each docker compose exec invocation
in that block to include -T (e.g., docker compose exec -T ...) so the commands
run without requiring a TTY in CI and avoid hangs/warnings.
- Around line 236-237: 展開時にリテラルのダブルクォートが混入する恐れがあるため、既に修正された `${BEFORE:+-b
"${BEFORE}"}` と同じクォーティング規則を `ZAP_CONTEXT` と `ZAP_THREAD_PER_HOST`
の条件付き展開にも適用してください；具体的には `${ZAP_CONTEXT:+"-c ${ZAP_CONTEXT}"}` と
`${ZAP_THREAD_PER_HOST:+"-n ${ZAP_THREAD_PER_HOST}"}` をそれぞれ `${ZAP_CONTEXT:+-c
"${ZAP_CONTEXT}"}` と `${ZAP_THREAD_PER_HOST:+-n "${ZAP_THREAD_PER_HOST}"}`
に置き換えて、展開結果に余分なダブルクォートが含まれないように修正してください。
🧹 Nitpick comments (1)
.github/workflows/zaproxy.yml (1)
241-293: YAML パッチ処理は妥当ですが、正規表現の脆さに注意してください正規表現によるYAMLパッチはテンプレートのインデントやフィールド順序に厳密に依存しています。テンプレート (
template.yml) が変更された場合、パッチが無言で失敗する代わりに Line 278 でエラー終了するのは適切です。将来的にテンプレート変更が頻繁になる場合は、
pyyamlやruamel.yaml等のYAMLライブラリの使用を検討してください。
zeniya0000
force-pushed
the
feature/fix_zap
branch
from
73dbab7 to
f0b619b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.3 #6619 +/- ##
============================================
- Coverage 78.84% 78.83% -0.01%
Complexity 6631 6631
============================================
Files 475 475
Lines 26539 26539
============================================
- Hits 20924 20923 -1
- Misses 5615 5616 +1
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:
|
.github/workflows/zaproxy.yml
Outdated
| - mypage_delivery | ||
| - mypage_favorite | ||
| - mypage_order | ||
| # - admin_class_category_csv |
There was a problem hiding this comment.
コメントアウトされたターゲットがありますが、一時的なものであればその旨のコメントを記載してください。
恒久的なものであれば削除してください
There was a problem hiding this comment.
試験用の一時的なコメントアウトだったため、コメントアウトを外しました。
zap/delete_data.sh
Outdated
| delete from dtb_tag where id > ${MAX_TAG_ID};"; | ||
| sleep 5 | ||
| done | ||
|
|
There was a problem hiding this comment.
コメントアウトしている理由を記載してください。恒久的なものであれば削除してください
There was a problem hiding this comment.
恒久的なものであるため削除いたしました。
|
|
||
| MAX_MEMBER_ID=$(psql -t -c 'select max(id) from dtb_member;') | ||
| MAX_TAG_ID=$(psql -t -c 'select max(id) from dtb_tag;') | ||
| psql -v ON_ERROR_STOP=1 <<'SQL' |
There was a problem hiding this comment.
何故コメントアウトされている delete from ではなく、一部のテーブルに限定した TRUNCATE にしているか理由を記述してください
There was a problem hiding this comment.
シーケンステストの使うテーブルによってはここで削除することでスキャンが出来ない部分などがあったため省いています。
（すみません、具体的な原因までは突き止められていません）
| ZA_CONTEXT=admin | ||
| ZA_USER=admin | ||
| ZA_FORCE_ADMIN_CONFIG=" | ||
| - type: script |
There was a problem hiding this comment.
これらのスクリプトを削除している理由を記載してください。
もし不要になったのであれば forceuser.groovy も削除してください
There was a problem hiding this comment.
不要であるためforceuser.groovy含めて削除いたしました。
.github/workflows/zaproxy.yml
Outdated
| docker compose exec -d -e PGUSER=dbuser -e PGDATABASE=eccubedb postgres /delete_data.sh | ||
| docker compose exec -T -e PGUSER=dbuser -e PGDATABASE=eccubedb postgres bash /delete_data.sh | ||
| docker compose cp zap/seed_zap.sql postgres:/ | ||
| docker compose exec -T -e PGUSER=dbuser -e PGDATABASE=eccubedb postgres psql -f /seed_zap.sql |
There was a problem hiding this comment.
seed_zap.sql は 1件 dtb_customer を INSERT しているのみなので、この程度であれば bin/console doctrine:query:sql コマンドを使用し、 PostgreSQL/MySQL を抽象的に利用できるようにしてください
There was a problem hiding this comment.
seed_zap.sql自体が不要でしたので、該当部分とseed_zap.sql自体を削除いたしました。
zeniya0000
force-pushed
the
feature/fix_zap
branch
from
a6a6d1c to
3180d95
Compare
zeniya0000
force-pushed
the
feature/fix_zap
branch
from
55188e2 to
d31be8d
Compare
PR #6619 コードレビュー: ZAPバージョンアップ対応ZAP環境を 1. バージョンアップの適切性概ね適切だが、いくつか懸念あり
問題点:
2. スキャンの適切性に関する懸念2a. Forced User Mode の無効化（重大）旧実装では forceUser.setForcedUser(admin.getContextId(), admin)
forceUser.setForcedUserModeEnabled(true)新実装では Forced User Mode と user パラメータ + poll verification は別の仕組み:
CIランで大半のジョブが「成功」していても、成功＝正しくスキャンできているとは限らない。ログインページの200応答をスキャンしてもZAPのシーケンスとしてはエラーにならず成功と報告される。スキャンレポートの中身（スキャン対象URLの分布）を確認しないと、管理画面の各機能が本当にスキャンされたのか判断できない。 対応案: 2b. ログインの二重実行全管理画面スクリプトでログインが重複する構造になっている:
結果として、ログインが2回実行される。致命的ではないがスキャン時間が無駄に増加する。 2c.
|
| 項目 | 詳細 |
|---|---|
| artifact名の修正 | 旧: report/alerts 両方 zap-*-report → 新: 別名に修正。バグ修正として良い |
| alerts マージの改善 | jq -s 'add // []' と mapfile による堅牢化 |
-it → -T |
CI（非対話）環境で正しい変更 |
set -eu の追加 |
エラー検出の強化 |
entry.zst の改良 |
\n → \r?\n と ZestAssignReplace による改行コード正規化は適切 |
| ECCUBE-CSRF-TOKEN ハードコード除去 | admin_create_customer.zst からハードコードされた CSRF トークンヘッダを除去。重要な修正 |
| loggedOutRegex の改善 | 旧: HTML構造依存 → 新: name="login_id" でより堅牢 |
6. 総合評価・推奨事項
マージ前に対応すべき事項（Must）:
forceuser.groovy（Forced User Mode）の仕組みを維持する。ZAP 2.16+ での動作を確認の上、generate_automation_config.shで復活させるdelete_data.shにマスターデータのクリーニングを追加するか、スキャン中にバックグラウンドで実行する旧方式を復活させるadmin_order_mail.zstのインデックス 31 重複を修正admin_customer_delivery.zstの二重ログインとurlToken/url不整合を修正- CIランのスキャンレポートを確認し、実際に管理画面の各機能（ログインページではなく）がスキャンされていることを検証する
推奨事項（Should）:
admin_customer_list.zstのインデックス不整合を修正threadPerHostの制御をactiveScan-configジョブで復活させる- ポリシーのマウントパスが ZAP に認識されることを確認
- ログインの二重実行について、
before_scriptと埋め込みのどちらかに統一する（保守性の観点からadmin_login.zstas before_script に統一し、各スクリプトからは埋め込みを除去するのが望ましい） {{customer-id}}の sed 一括置換をやめ、TRUNCATE ... RESTART IDENTITY に依存しない動的ID取得に統一する
3 participants
概要(Overview・Refs Issue)
OSSのGitHub Actionsで使用している OWASP ZAP のバージョンが古いため、最新バージョンでスキャンが実行されるよう環境の刷新を行いました。
変更内容
最新バージョンの利用に伴い、依存関係およびイメージの管理方法を以下の通り変更しています。
・セキュリティスキャンの精度向上と最新の脆弱性への対応のため。
・上記ZAPのアップデートに伴い、動作要件を満たすJDKバージョンへ更新。
・外部リポジトリから取得していたイメージを、ec-cube 本体のGitHub（GitHub Packages）内へ移行
動作確認
事前に以下の個人リポジトリにて動作確認を行っています。
1回目：https://github.com/zeniya0000/ec-cube/actions/runs/21889773054
2回目：https://github.com/zeniya0000/ec-cube/actions/runs/21892209250
※一度で回りきっていないので、1回目でまわりきらなかったものを2回目でまわしています。
マイナーバージョン互換性保持のための制限事項チェックリスト
レビュワー確認項目
Summary by CodeRabbit
新機能
バグ修正
Chores