Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
anko9801
force-pushed
the
ci/conformance-suite
branch
3 times, most recently
from
c499d1e to
f455bb9
Compare
Add CI workflow that runs the OIDC Basic Certification test plan using pre-built conformance suite images from GHCR. Includes a separate workflow_dispatch to build and publish the conformance suite images. Passes 21/34 conformance tests. Assisted-by: Claude Code (model: claude-opus-4-6)
anko9801
force-pushed
the
ci/conformance-suite
branch
from
f455bb9 to
47bd92c
Compare
CONFORMANCE_HTTPD_IMAGE and CONFORMANCE_SERVER_IMAGE were only defined in the "Start conformance suite" step env. Other steps calling docker compose (logs in wait step and failure diagnostics) lacked these variables, causing compose to fail with "service has neither an image nor a build context". Assisted-by: Claude Code (model: claude-opus-4-6)
The previous commit moved image env vars to job-level env using
${{ env.X }}, but the env context is not available at job level.
Define full image names directly at workflow level instead.
Assisted-by: Claude Code (model: claude-opus-4-6)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
.github/scripts/run-test-plan.py (3)
28-32:verify=Falseに意図を示すコメントを追加してください。ブラウザクライアントでSSL検証を無効化していますが、これは自己署名証明書を使用するローカル適合性テストスイートにアクセスするために必要です。セキュリティレビューで指摘されることを防ぐため、意図をコメントで明記してください。
📝 コメント追加案
def create_browser_client() -> httpx.Client: + # SSL verification disabled intentionally: conformance suite uses self-signed certificates return httpx.Client( verify=False, timeout=30.0, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/run-test-plan.py around lines 28 - 32, 関数 create_browser_client 内で httpx.Client(... verify=False ...) によって SSL 検証を無効化している意図を明示するコメントを追加してください。具体的には create_browser_client の直上に「verify=False は自己署名証明書を使ったローカルの適合性テストスイートにアクセスするためのもので、本番環境では絶対に無効化しない」旨（必要なら TODO や FIXME を付けて限定的な使用を明示）を短く記載してください。
90-91: ハードコードされたホスト名の置換について確認してください。
host.docker.internal:8080がハードコードされています。--oidc-server引数が指定された場合のみ置換が行われますが、元のホスト名も引数として受け取ることで、より柔軟な構成が可能になります。現在の実装でも動作しますが、将来的な拡張性のために検討してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/run-test-plan.py around lines 90 - 91, The code currently hardcodes "host.docker.internal:8080" when swapping auth_url (in the block using oidc_server_url); change this to use a new CLI/config argument (e.g., oidc_server_host or oidc_origin) instead of the literal string: add parsing for a new --oidc-server-host/--oidc-origin option that defaults to "host.docker.internal:8080", then replace auth_url = auth_url.replace(oidc_server_host, oidc_server_url) inside the if oidc_server_url branch (referencing the oidc_server_url and auth_url variables) so callers can override the original host without editing code.
93-93: 不要なf-stringプレフィックスを削除してください。静的解析ツールが指摘しているように、プレースホルダーのないf-stringがあります。
♻️ 修正案
- print(f" Browser: visiting authorize URL") + print(" Browser: visiting authorize URL")同様に、Line 106, 109, 122も修正してください：
- print(f" Browser: redirect with no location header") + print(" Browser: redirect with no location header") - print(f" Browser: following redirect to callback") + print(" Browser: following redirect to callback") - print(f" Browser: submitting fragment to implicit endpoint") + print(" Browser: submitting fragment to implicit endpoint")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/run-test-plan.py at line 93, Remove unnecessary f-string prefixes from plain string print calls so static analysis stops flagging them: replace the f-prefixed prints (e.g., the call printing " Browser: visiting authorize URL" and the similar prints at the other locations referenced) with normal string literals by dropping the leading "f" before the opening quote; update the print statements at the three other occurrences mentioned (the lines corresponding to the prints around 106, 109, 122) in the same way to ensure no f-string is used when there are no placeholders..github/workflows/conformance.yaml (2)
81-92: ヘルスチェックのタイムアウトが短い可能性があります。最大60秒（30回 × 2秒）のタイムアウトですが、CI環境ではリソース競合によりサーバーの起動が遅れることがあります。適合性スイートの待機（Line 104-116）では最大300秒待機しているのに対し、portal-oidcの待機時間が短めです。現在の21/34テスト通過状況を考慮すると、問題なければこのままで良いですが、フレーキーテストが発生する場合は延長を検討してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/conformance.yaml around lines 81 - 92, The portal-oidc health-check loop times out after 30 attempts (~60s); increase the wait to match the longer CI waits by modifying the "Wait for portal-oidc health check" step’s curl loop (the for i in $(seq 1 30); do ... sleep 2 done block) to try longer (e.g., change 30 to 150 to allow ~300s total, or adjust attempts/sleep to reach ~300s) so the step will tolerate slower CI startups.
56-58: MySQLパスワードのコマンドライン露出を避けてください。
-ppasswordでパスワードをコマンドラインに直接渡すと、プロセスリストや履歴に露出する可能性があります。CIでは重大なリスクではありませんが、MYSQL_PWD環境変数を使用する方がベストプラクティスです。♻️ 修正案
- name: Apply DB schema - run: | - mysql -h 127.0.0.1 -P 3307 -u root -ppassword oidc < db/schema.sql + run: mysql -h 127.0.0.1 -P 3307 -u root oidc < db/schema.sql + env: + MYSQL_PWD: password🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/conformance.yaml around lines 56 - 58, The CI step named "Apply DB schema" exposes the MySQL password via the `-ppassword` flag; change the run command to set MYSQL_PWD as an environment variable (using the secret, e.g. `secrets.MYSQL_PASSWORD`) and remove the `-p` flag from the `mysql` invocation so the password is not on the command line; update the step to export/define MYSQL_PWD before running `mysql -h 127.0.0.1 -P 3307 -u root oidc < db/schema.sql` (refer to the step name "Apply DB schema" and the original `mysql -h 127.0.0.1 -P 3307 -u root -ppassword oidc < db/schema.sql` command to locate the change)..github/conformance/run.sh (1)
28-32: エラー時のシークレット露出の可能性を考慮してください。
python3によるJSONパースが失敗した場合、エラーメッセージに$RESPONSE（client_secretを含む）が表示される可能性があります。また、jqを使用するとより堅牢で、シェルスクリプトでは一般的です。♻️ jqを使用した修正案
-CLIENT_ID=$(echo "$RESPONSE" | python3 -c "import sys,json; print(json.load(sys.stdin)['client_id'])") -CLIENT_SECRET=$(echo "$RESPONSE" | python3 -c "import sys,json; print(json.load(sys.stdin)['client_secret'])") +CLIENT_ID=$(echo "$RESPONSE" | jq -r '.client_id') +CLIENT_SECRET=$(echo "$RESPONSE" | jq -r '.client_secret') + +if [[ -z "$CLIENT_ID" || -z "$CLIENT_SECRET" ]]; then + echo "Error: Failed to extract client credentials from response" + exit 1 +fi
jqがワークフローで利用可能であることを確認してください（ubuntu-latestにはデフォルトでインストールされています）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/conformance/run.sh around lines 28 - 32, RESPONSE should not be echoed or parsed with python3 (which may print the full RESPONSE on error); replace the python-based parsing with jq to extract client_id and client_secret into the variables CLIENT_ID and CLIENT_SECRET and fail cleanly on parse errors, avoid printing RESPONSE anywhere, and keep the client_secret masked when logging; specifically update the extraction of CLIENT_ID and CLIENT_SECRET (the existing variables) to use jq parsing of RESPONSE and add error handling (e.g., check jq exit status) so parsing failures do not leak secrets..github/workflows/build-conformance-suite.yaml (1)
31-35: Mavenキャッシュが効果的に機能していない可能性があります。
MAVEN_CACHEを/tmp/m2に設定していますが、このディレクトリがdocker-composeのbuilderコンテナにマウントされているかはbuilder-compose.ymlの内容に依存します。また、ジョブ間でキャッシュが保持されないため、ワークフローの実行ごとに再ダウンロードが発生する可能性があります。ビルド時間が問題になる場合は、
actions/cacheを使用してMavenリポジトリをキャッシュすることを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-conformance-suite.yaml around lines 31 - 35, 現在のワークフローはMAVEN_CACHE=/tmp/m2を作成しているだけで、docker composeでそのディレクトリがビルダーコンテナにマウントされているか（builder-compose.ymlのvolumes設定）やジョブ間で保持されるかが保証されていません。対処方法としては、GitHub Actionsにactions/cacheステップを追加してMavenローカルリポジトリ（例: ~/.m2/repository）をキーにキャッシュし、ワークフロー内のMAVEN_CACHE環境変数をキャッシュ先に合わせて設定し（またはrunnerの一時ディレクトリを使う）、さらに必要ならbuilder-compose.ymlのbuilderサービスにhost側のMAVEN_CACHEディレクトリをコンテナの~/.m2にマウントするvolumesエントリを追加して、docker compose run --rm builderが実際にキャッシュを利用するようにしてください。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/conformance/docker-compose.yml:
- Around line 12-18: JVMシステムプロパティ -Djdk.tls.maxHandshakeMessageSize=65536 が現在
-jar /server/fapi-test-suite.jar の後に置かれておりプログラム引数扱いになっているので、該当プロパティ（および他の JVM
オプションがあれば）を -jar の前に移動して JVM オプションとして渡し、-jar /server/fapi-test-suite.jar
はその後に置き、 --fintechlabs.base_url=... や --fintechlabs.devmode=true
などのアプリケーション引数は引き続き -jar の後に残すように修正してください.
In @.github/conformance/expected-skips.json:
- Line 1: expected-skips.json
が空で参照されていないため、将来のプレースホルダーであることを明記するか実装を追加してください。修正案1: expected-skips.json
の先頭にコメント（JSONでは許されないので README か同ディレクトリに expected-skips.md
を追加）で「このファイルは将来のスキップリストのプレースホルダーである」旨と期待されるスキーマ（例:
[{"test_id":"<id>","reason":"<reason>"}]）を記載する。修正案2: スキップ機能を有効にする場合は run.sh または
run-test-plan.py に expected-skips.json を読み込んでテスト実行前に除外するロジックを追加し、期待スキーマ（test_id
で一致させる等）に従って該当テストをスキップする処理を実装してください。
---
Nitpick comments:
In @.github/conformance/run.sh:
- Around line 28-32: RESPONSE should not be echoed or parsed with python3 (which
may print the full RESPONSE on error); replace the python-based parsing with jq
to extract client_id and client_secret into the variables CLIENT_ID and
CLIENT_SECRET and fail cleanly on parse errors, avoid printing RESPONSE
anywhere, and keep the client_secret masked when logging; specifically update
the extraction of CLIENT_ID and CLIENT_SECRET (the existing variables) to use jq
parsing of RESPONSE and add error handling (e.g., check jq exit status) so
parsing failures do not leak secrets.
In @.github/scripts/run-test-plan.py:
- Around line 28-32: 関数 create_browser_client 内で httpx.Client(... verify=False
...) によって SSL 検証を無効化している意図を明示するコメントを追加してください。具体的には create_browser_client
の直上に「verify=False
は自己署名証明書を使ったローカルの適合性テストスイートにアクセスするためのもので、本番環境では絶対に無効化しない」旨（必要なら TODO や FIXME
を付けて限定的な使用を明示）を短く記載してください。
- Around line 90-91: The code currently hardcodes "host.docker.internal:8080"
when swapping auth_url (in the block using oidc_server_url); change this to use
a new CLI/config argument (e.g., oidc_server_host or oidc_origin) instead of the
literal string: add parsing for a new --oidc-server-host/--oidc-origin option
that defaults to "host.docker.internal:8080", then replace auth_url =
auth_url.replace(oidc_server_host, oidc_server_url) inside the if
oidc_server_url branch (referencing the oidc_server_url and auth_url variables)
so callers can override the original host without editing code.
- Line 93: Remove unnecessary f-string prefixes from plain string print calls so
static analysis stops flagging them: replace the f-prefixed prints (e.g., the
call printing " Browser: visiting authorize URL" and the similar prints at the
other locations referenced) with normal string literals by dropping the leading
"f" before the opening quote; update the print statements at the three other
occurrences mentioned (the lines corresponding to the prints around 106, 109,
122) in the same way to ensure no f-string is used when there are no
placeholders.
In @.github/workflows/build-conformance-suite.yaml:
- Around line 31-35: 現在のワークフローはMAVEN_CACHE=/tmp/m2を作成しているだけで、docker
composeでそのディレクトリがビルダーコンテナにマウントされているか（builder-compose.ymlのvolumes設定）やジョブ間で保持されるかが保証されていません。対処方法としては、GitHub
Actionsにactions/cacheステップを追加してMavenローカルリポジトリ（例:
~/.m2/repository）をキーにキャッシュし、ワークフロー内のMAVEN_CACHE環境変数をキャッシュ先に合わせて設定し（またはrunnerの一時ディレクトリを使う）、さらに必要ならbuilder-compose.ymlのbuilderサービスにhost側のMAVEN_CACHEディレクトリをコンテナの~/.m2にマウントするvolumesエントリを追加して、docker
compose run --rm builderが実際にキャッシュを利用するようにしてください。
In @.github/workflows/conformance.yaml:
- Around line 81-92: The portal-oidc health-check loop times out after 30
attempts (~60s); increase the wait to match the longer CI waits by modifying the
"Wait for portal-oidc health check" step’s curl loop (the for i in $(seq 1 30);
do ... sleep 2 done block) to try longer (e.g., change 30 to 150 to allow ~300s
total, or adjust attempts/sleep to reach ~300s) so the step will tolerate slower
CI startups.
- Around line 56-58: The CI step named "Apply DB schema" exposes the MySQL
password via the `-ppassword` flag; change the run command to set MYSQL_PWD as
an environment variable (using the secret, e.g. `secrets.MYSQL_PASSWORD`) and
remove the `-p` flag from the `mysql` invocation so the password is not on the
command line; update the step to export/define MYSQL_PWD before running `mysql
-h 127.0.0.1 -P 3307 -u root oidc < db/schema.sql` (refer to the step name
"Apply DB schema" and the original `mysql -h 127.0.0.1 -P 3307 -u root
-ppassword oidc < db/schema.sql` command to locate the change).
| command: > | ||
| java | ||
| -jar /server/fapi-test-suite.jar | ||
| -Djdk.tls.maxHandshakeMessageSize=65536 | ||
| --fintechlabs.base_url=https://localhost.emobix.co.uk:8443 | ||
| --fintechlabs.devmode=true | ||
| --fintechlabs.startredir=true |
There was a problem hiding this comment.
JVMオプションの配置が正しくありません。
-Djdk.tls.maxHandshakeMessageSize=65536は-jarの後に配置されているため、JVMオプションではなくプログラム引数として扱われます。JVMシステムプロパティは-jarの前に指定する必要があります。
🐛 修正案
server:
image: ${CONFORMANCE_SERVER_IMAGE}
command: >
java
+ -Djdk.tls.maxHandshakeMessageSize=65536
-jar /server/fapi-test-suite.jar
- -Djdk.tls.maxHandshakeMessageSize=65536
--fintechlabs.base_url=https://localhost.emobix.co.uk:8443
--fintechlabs.devmode=true
--fintechlabs.startredir=true📝 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.
| command: > | |
| java | |
| -jar /server/fapi-test-suite.jar | |
| -Djdk.tls.maxHandshakeMessageSize=65536 | |
| --fintechlabs.base_url=https://localhost.emobix.co.uk:8443 | |
| --fintechlabs.devmode=true | |
| --fintechlabs.startredir=true | |
| command: > | |
| java | |
| -Djdk.tls.maxHandshakeMessageSize=65536 | |
| -jar /server/fapi-test-suite.jar | |
| --fintechlabs.base_url=https://localhost.emobix.co.uk:8443 | |
| --fintechlabs.devmode=true | |
| --fintechlabs.startredir=true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/conformance/docker-compose.yml around lines 12 - 18, JVMシステムプロパティ
-Djdk.tls.maxHandshakeMessageSize=65536 が現在 -jar /server/fapi-test-suite.jar
の後に置かれておりプログラム引数扱いになっているので、該当プロパティ（および他の JVM オプションがあれば）を -jar の前に移動して JVM
オプションとして渡し、-jar /server/fapi-test-suite.jar はその後に置き、 --fintechlabs.base_url=...
や --fintechlabs.devmode=true などのアプリケーション引数は引き続き -jar の後に残すように修正してください.
| @@ -0,0 +1 @@ | |||
| [] | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "expected-skips" --type=sh --type=py --type=yaml --type=jsonRepository: traPtitech/portal-oidc
Length of output: 48
🏁 Script executed:
find .github/conformance -type f -name "*.md" -o -name "*.txt" -o -name "README*" | head -20Repository: traPtitech/portal-oidc
Length of output: 48
🏁 Script executed:
cat .github/conformance/*.json 2>/dev/null | head -50Repository: traPtitech/portal-oidc
Length of output: 301
🏁 Script executed:
find .github/conformance -type fRepository: traPtitech/portal-oidc
Length of output: 212
🏁 Script executed:
rg -n "conformance" --type=sh --type=py | head -20Repository: traPtitech/portal-oidc
Length of output: 214
🏁 Script executed:
rg -n "conformance" | head -30Repository: traPtitech/portal-oidc
Length of output: 269
🏁 Script executed:
cat .github/conformance/run.shRepository: traPtitech/portal-oidc
Length of output: 1973
🏁 Script executed:
cat .github/scripts/run-test-plan.pyRepository: traPtitech/portal-oidc
Length of output: 7636
expected-skips.jsonの用途を明記するか実装を追加してください。
expected-skips.jsonは現在空の配列で、run.shやrun-test-plan.pyのいずれからも参照されていません。ファイルが将来の機能のプレースホルダーであれば、その旨をコメントで明記するか、実装を追加してスキップ機能を有効にしてください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/conformance/expected-skips.json at line 1, expected-skips.json
が空で参照されていないため、将来のプレースホルダーであることを明記するか実装を追加してください。修正案1: expected-skips.json
の先頭にコメント（JSONでは許されないので README か同ディレクトリに expected-skips.md
を追加）で「このファイルは将来のスキップリストのプレースホルダーである」旨と期待されるスキーマ（例:
[{"test_id":"<id>","reason":"<reason>"}]）を記載する。修正案2: スキップ機能を有効にする場合は run.sh または
run-test-plan.py に expected-skips.json を読み込んでテスト実行前に除外するロジックを追加し、期待スキーマ（test_id
で一致させる等）に従って該当テストをスキップする処理を実装してください。
There was a problem hiding this comment.
Pull request overview
CI 上で OpenID Connect Conformance Suite を実行できるようにし、ビルドに時間がかかる conformance suite 側の成果物は GHCR に push したイメージを利用してキャッシュする構成を追加する PR です。
Changes:
- OIDC Conformance Suite を起動してテストプランを実行・結果を artifact で回収する GitHub Actions workflow を追加
- Conformance Suite（httpd/server）イメージをビルドして GHCR に push するための手動実行 workflow を追加
- テストプラン実行用のスクリプト（Python）と実行ラッパー（bash）、設定テンプレート、compose 定義を追加
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .gitignore | conformance 実行結果ディレクトリを git 管理対象外に追加 |
| .github/workflows/conformance.yaml | CI で portal-oidc + conformance suite を立ち上げてテストを実行し、結果を artifact 化 |
| .github/workflows/build-conformance-suite.yaml | conformance-suite をビルドし、httpd/server イメージを GHCR に push する workflow を追加 |
| .github/scripts/run-test-plan.py | conformance suite API を叩いて test plan を実行し、ログを JSON で保存するランナーを追加 |
| .github/conformance/run.sh | クライアント作成・設定生成・test plan 実行を行うスクリプトを追加 |
| .github/conformance/expected-skips.json | 期待スキップ一覧（現状は空）を追加 |
| .github/conformance/docker-compose.yml | conformance suite (mongo/httpd/server) 起動用 compose を追加 |
| .github/conformance/config.template.json | discovery URL / client 情報を差し込む設定テンプレートを追加 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Pugma
left a comment
Pugma
left a comment
There was a problem hiding this comment.
少々気になるポイントにコメントをつけました！
ご確認いただければと思います 🙇
- Add --expected-skips support to skip specified test modules - Add test result statistics (passed/failed/skipped counts) - Use jq instead of python3 for JSON parsing in run.sh - Move JVM property before -jar in docker-compose command - Use MYSQL_PWD env var instead of -p flag - Remove unnecessary f-string prefixes - Add SSL verification comment for clarity Assisted-by: Claude Code (model: claude-opus-4-6)
Fixes PATH hijacking vulnerability in go.opentelemetry.io/otel/sdk. Assisted-by: Claude Code (model: claude-opus-4-6)
Reduces CI check noise by consolidating govulncheck, gosec, and Trivy into a single Security Scan job. Assisted-by: Claude Code (model: claude-opus-4-6)
Combine SARIF outputs into a single upload to reduce the number of Code scanning results checks on PRs. Assisted-by: Claude Code (model: claude-opus-4-6)
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
1 similar comment
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
CI で conformance suite を回せるようにしたんですが、これのビルドが大変時間が掛かるので ghcr にビルド成果を push してキャッシュする感じで実行しています。
Summary by CodeRabbit
リリースノート