Feature/jgss 202609 - #1915
Feature/jgss 202609#1915MakotoASAOKA wants to merge 3 commits into
Conversation
Debian buster ミラー消滅、NodeSource Node.js 4.x の署名鍵失効、 npm間接依存(psl)のNode4.x非互換、GitHub依存リポジトリ削除の 4点を修正し、docker-compose -f docker-compose2.yml build が 通るようにした。詳細は BUILD_FIX_NOTES.md を参照。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Elasticsearch snapshot(6.8.23)を復元できるよう、elasticsearch/Dockerfile の ベースイメージを 6.8.22 から 6.8.23 に変更 - docker-compose2.yml の elasticsearch サービスに snapshot repository 用の 読み取り専用ボリューム(./es_snapshot_restore_new)を追加 - worker サービスの image タグが誤って別名(weko3_b_web)になっており ビルドしたイメージ(weko-web)と一致していなかったため修正 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- WEKO3_BUILD_RESTORE_MANUAL.md: このブランチのビルド〜JGSSデータリストア手順書 - KNOWN_ISSUE_download_button_not_shown.md: 承認済み利用申請でもダウンロード ボタンが表示されない既知の不具合（事象・対応状況・再現データへのポインタ） - scripts/pr_review_init.sh: 上記2点をレビュー開始時にすぐ確認できる初期化スクリプト - scripts/demo/download_button_bug_repro/: 上記不具合を完全な合成データ （本番データ・個人情報は一切含まない）で再現するためのfixture一式 - .gitignore: 本番データ復元の作業ファイル（DBダンプ・ESスナップショット）を追跡対象外に Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer's GuideThis bug-fix/documentation PR makes the legacy WEKO3 Docker build reproducible by handling EOL Debian, NodeSource, npm, and GitHub dependencies, aligns Elasticsearch and worker configuration for JGSS snapshot restoration, and adds detailed operational/reproduction materials including a synthetic fixture for a separately tracked download-button issue. Flow diagram for the reproducible WEKO3 buildflowchart LR
Sources["EOL Debian and legacy dependencies"] --> Dockerfile["Dockerfile stage_2"]
Dockerfile --> Provision["provision-web.sh"]
Provision --> Image["weko-web image"]
Bundles["bundles.py tarball dependency"] --> Image
Image --> Build["docker compose build succeeds"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/provision-web.sh" line_range="165-168" />
<code_context>
+`node-sass@3.8.0` の間接依存 `request → tough-cookie → psl` がバージョン固定なしのため、時間経過で ES5 のみ対応の Node.js 4.x では実行できないモダン構文（スプレッド構文等）を使うバージョンに解決されてしまう。`scripts/provision-web.sh` の該当 `npm install -g` に旧バージョンを明示的に追加する。
+
+```bash
+# Pin transitive deps of node-sass@3.8.0 (via request -> tough-cookie -> psl)
+# to old, ES5-only versions: newer psl/tough-cookie releases use syntax
+# (e.g. spread operator) that Node.js 4.x's runtime cannot parse.
+$sudo su -c "npm install -g node-sass@3.8.0 clean-css@3.4.12 requirejs uglify-js psl@1.1.31 tough-cookie@2.3.4"
+```
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Adding `psl@1.1.31` and `tough-cookie@2.3.4` as top-level global packages does not pin the nested dependencies resolved under `node-sass -> request -> tough-cookie`; npm can still install a newer nested `psl` and the Node.js 4 build still fails with a syntax error.
**Triggers:** When npm resolves the transitive dependency tree without deduplicating it against the top-level packages.
**Suggested fix:** Pin the dependency tree with a compatible lockfile or explicitly patch/install the nested dependency versions used by `request`.
</issue_to_address>
### Comment 2
<location path="scripts/provision-web.sh" line_range="73-77" />
<code_context>
+# signature can no longer be verified (key not served anymore), so
+# allow this one command to fail (it still writes the sources.list.d
+# file before erroring out) rather than aborting the whole build.
+curl -sL https://deb.nodesource.com/setup_4.x | $sudo bash - || true
+# Trust this single, already-EOL repo explicitly rather than
+# weakening apt verification globally, then refresh package lists.
+$sudo sed -i 's/\[signed-by=[^]]*\]/[trusted=yes]/' /etc/apt/sources.list.d/nodesource.list
+$sudo apt-get -y update
+```
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Replacing the NodeSource repository's signature verification with `[trusted=yes]` makes `apt-get` accept unsigned or tampered Node.js packages, allowing a compromised external repository to execute code during every image build.
**Triggers:** When the image is built while the NodeSource repository or its transport is compromised.
**Suggested fix:** Use a pinned, available Node.js package source and verified key, or vendor/cache the required packages instead of disabling repository signature verification.
</issue_to_address>
### Comment 3
<location path="scripts/demo/download_button_bug_repro/load.sh" line_range="47-52" />
<code_context>
+ < "${HERE}/fixture.sql"
+
+echo "==> bulk loading es_bulk.ndjson into ${ES_URL}/${ES_INDEX}"
+# The ndjson already carries the index name in every action line, so _bulk on
+# the cluster root is enough. A trailing newline is required by the _bulk API.
+curl -sS -H 'Content-Type: application/x-ndjson' \
+ -X POST "${ES_URL}/_bulk?refresh=true" \
+ --data-binary "@${HERE}/es_bulk.ndjson" \
+ | python3 -c 'import json,sys; r=json.load(sys.stdin); print(" errors:", r.get("errors")); [print(" ", i) for i in r.get("items", [])[:1]]'
+
+echo
</code_context>
<issue_to_address>
**issue (bug_risk):** The `ES_INDEX` override documented by `load.sh` is never used because the bulk file hard-codes `tenant1-weko-item-v1.0.0` in each action line; running with another index still writes to the hard-coded index and can leave the requested index empty.
**Triggers:** When a deployment uses a non-default Elasticsearch index and sets `ES_INDEX`.
**Suggested fix:** Generate or rewrite the bulk action lines from `ES_INDEX`, or use the index-specific `_bulk` endpoint and remove the hard-coded index metadata.
</issue_to_address>
### Comment 4
<location path="scripts/demo/download_button_bug_repro/load.sh" line_range="49-52" />
<code_context>
+# The ndjson already carries the index name in every action line, so _bulk on
+# the cluster root is enough. A trailing newline is required by the _bulk API.
+curl -sS -H 'Content-Type: application/x-ndjson' \
+ -X POST "${ES_URL}/_bulk?refresh=true" \
+ --data-binary "@${HERE}/es_bulk.ndjson" \
+ | python3 -c 'import json,sys; r=json.load(sys.stdin); print(" errors:", r.get("errors")); [print(" ", i) for i in r.get("items", [])[:1]]'
+
+echo
+echo "==> done."
</code_context>
<issue_to_address>
**issue (bug_risk):** The script only prints the bulk response's `errors` field and never exits nonzero or inspects individual item failures, so a failed Elasticsearch load is reported as `done` and reviewers can run verification against an incomplete fixture.
**Triggers:** When Elasticsearch rejects the request or one or more bulk items fail.
**Suggested fix:** Fail when the HTTP request fails or when the parsed response has `errors: true` or any item contains an error.
```suggestion
curl --fail -sS -H 'Content-Type: application/x-ndjson' \
-X POST "${ES_URL}/_bulk?refresh=true" \
--data-binary "@${HERE}/es_bulk.ndjson" \
| python3 -c 'import json,sys; r=json.load(sys.stdin); items=r.get("items", []); failed=r.get("errors") is True or any("error" in item or any(isinstance(operation, dict) and "error" in operation for operation in item.values()) for item in items); print(" errors:", r.get("errors")); [print(" ", i) for i in items[:1]]; sys.exit(1 if failed else 0)'
```
</issue_to_address>
### Comment 5
<location path="scripts/demo/download_button_bug_repro/fixture.sql" line_range="35-36" />
<code_context>
+-- 再実行できるように、本フィクスチャが作る行だけを先に削除する。
+-- (FK 依存の逆順)
+--
+DELETE FROM public.file_onetime_download WHERE record_id = '2003976';
+DELETE FROM public.file_permission WHERE record_id = '2003976';
+DELETE FROM public.workflow_action_history WHERE activity_id = 'A-20260917-00001';
+DELETE FROM public.workflow_activity_action WHERE activity_id = 'A-20260917-00001';
</code_context>
<issue_to_address>
**issue (broader_impact):** The supposedly idempotent fixture deletes every one-time-download and file-permission row for record `2003976`, including permissions created by another user or by a prior manual test, before inserting its own data.
**Triggers:** When the target record already has non-fixture permission or download rows.
**Suggested fix:** Delete only rows identified by the fixture's own activity, user, file, or synthetic primary keys rather than deleting by record ID alone.
</issue_to_address>
### Comment 6
<location path="scripts/demo/download_button_bug_repro/verify.py" line_range="78-79" />
<code_context>
+ '/files/sample_restricted_data.txt' in html)
+print('')
+if not access_permission and permission is None:
+ print('=> BUG REPRODUCED: the approved applicant gets the "Apply" button.')
+else:
+ print('=> Bug NOT reproduced (fix applied?).')
</code_context>
<issue_to_address>
**nitpick (testing):** The verifier declares the bug reproduced solely when `access_permission` is false and `permission` is `None`; it does not assert that the activity is completed or that the one-time-download row belongs to the logged-in user, so an incorrectly loaded or unrelated fixture can produce the same success message.
**Triggers:** When the fixture is partially loaded or the target record has no permission for reasons other than the approved-application regression.
**Suggested fix:** Assert the expected completed activity, applicant email, record/file identifiers, and one-time-download row before evaluating the button regression.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and changing the NodeSource entry to [trusted=yes] disables package signature verification, so a compromised or tampered repository could place arbitrary code into the built image; the tarball dependency also relies on an external source without an integrity checksum. Reverting prevents future builds from using these settings, but it does not remove compromised images that were already built or deployed.
Blocking findings: scripts/provision-web.sh:168, scripts/provision-web.sh:77, scripts/demo/download_button_bug_repro/load.sh:52, scripts/demo/download_button_bug_repro/load.sh:52, scripts/demo/download_button_bug_repro/fixture.sql:36
| # Pin transitive deps of node-sass@3.8.0 (via request -> tough-cookie -> psl) | ||
| # to old, ES5-only versions: newer psl/tough-cookie releases use syntax | ||
| # (e.g. spread operator) that Node.js 4.x's runtime cannot parse. | ||
| $sudo su -c "npm install -g node-sass@3.8.0 clean-css@3.4.12 requirejs uglify-js psl@1.1.31 tough-cookie@2.3.4" |
There was a problem hiding this comment.
issue (bug_risk): Adding psl@1.1.31 and tough-cookie@2.3.4 as top-level global packages does not pin the nested dependencies resolved under node-sass -> request -> tough-cookie; npm can still install a newer nested psl and the Node.js 4 build still fails with a syntax error.
Triggers: When npm resolves the transitive dependency tree without deduplicating it against the top-level packages.
Suggested fix: Pin the dependency tree with a compatible lockfile or explicitly patch/install the nested dependency versions used by request.
| curl -sL https://deb.nodesource.com/setup_4.x | $sudo bash - || true | ||
| # Trust this single, already-EOL repo explicitly rather than | ||
| # weakening apt verification globally, then refresh package lists. | ||
| $sudo sed -i 's/\[signed-by=[^]]*\]/[trusted=yes]/' /etc/apt/sources.list.d/nodesource.list | ||
| $sudo apt-get -y update |
There was a problem hiding this comment.
🚨 issue (security): Replacing the NodeSource repository's signature verification with [trusted=yes] makes apt-get accept unsigned or tampered Node.js packages, allowing a compromised external repository to execute code during every image build.
Triggers: When the image is built while the NodeSource repository or its transport is compromised.
Suggested fix: Use a pinned, available Node.js package source and verified key, or vendor/cache the required packages instead of disabling repository signature verification.
| # The ndjson already carries the index name in every action line, so _bulk on | ||
| # the cluster root is enough. A trailing newline is required by the _bulk API. | ||
| curl -sS -H 'Content-Type: application/x-ndjson' \ | ||
| -X POST "${ES_URL}/_bulk?refresh=true" \ | ||
| --data-binary "@${HERE}/es_bulk.ndjson" \ | ||
| | python3 -c 'import json,sys; r=json.load(sys.stdin); print(" errors:", r.get("errors")); [print(" ", i) for i in r.get("items", [])[:1]]' |
There was a problem hiding this comment.
issue (bug_risk): The ES_INDEX override documented by load.sh is never used because the bulk file hard-codes tenant1-weko-item-v1.0.0 in each action line; running with another index still writes to the hard-coded index and can leave the requested index empty.
Triggers: When a deployment uses a non-default Elasticsearch index and sets ES_INDEX.
Suggested fix: Generate or rewrite the bulk action lines from ES_INDEX, or use the index-specific _bulk endpoint and remove the hard-coded index metadata.
| curl -sS -H 'Content-Type: application/x-ndjson' \ | ||
| -X POST "${ES_URL}/_bulk?refresh=true" \ | ||
| --data-binary "@${HERE}/es_bulk.ndjson" \ | ||
| | python3 -c 'import json,sys; r=json.load(sys.stdin); print(" errors:", r.get("errors")); [print(" ", i) for i in r.get("items", [])[:1]]' |
There was a problem hiding this comment.
issue (bug_risk): The script only prints the bulk response's errors field and never exits nonzero or inspects individual item failures, so a failed Elasticsearch load is reported as done and reviewers can run verification against an incomplete fixture.
Triggers: When Elasticsearch rejects the request or one or more bulk items fail.
Suggested fix: Fail when the HTTP request fails or when the parsed response has errors: true or any item contains an error.
| curl -sS -H 'Content-Type: application/x-ndjson' \ | |
| -X POST "${ES_URL}/_bulk?refresh=true" \ | |
| --data-binary "@${HERE}/es_bulk.ndjson" \ | |
| | python3 -c 'import json,sys; r=json.load(sys.stdin); print(" errors:", r.get("errors")); [print(" ", i) for i in r.get("items", [])[:1]]' | |
| curl --fail -sS -H 'Content-Type: application/x-ndjson' \ | |
| -X POST "${ES_URL}/_bulk?refresh=true" \ | |
| --data-binary "@${HERE}/es_bulk.ndjson" \ | |
| | python3 -c 'import json,sys; r=json.load(sys.stdin); items=r.get("items", []); failed=r.get("errors") is True or any("error" in item or any(isinstance(operation, dict) and "error" in operation for operation in item.values()) for item in items); print(" errors:", r.get("errors")); [print(" ", i) for i in items[:1]]; sys.exit(1 if failed else 0)' |
| DELETE FROM public.file_onetime_download WHERE record_id = '2003976'; | ||
| DELETE FROM public.file_permission WHERE record_id = '2003976'; |
There was a problem hiding this comment.
issue (broader_impact): The supposedly idempotent fixture deletes every one-time-download and file-permission row for record 2003976, including permissions created by another user or by a prior manual test, before inserting its own data.
Triggers: When the target record already has non-fixture permission or download rows.
Suggested fix: Delete only rows identified by the fixture's own activity, user, file, or synthetic primary keys rather than deleting by record ID alone.
| print('=> BUG REPRODUCED: the approved applicant gets the "Apply" button.') | ||
| else: |
There was a problem hiding this comment.
nitpick (testing): The verifier declares the bug reproduced solely when access_permission is false and permission is None; it does not assert that the activity is completed or that the one-time-download row belongs to the logged-in user, so an incorrectly loaded or unrelated fixture can produce the same success message.
Triggers: When the fixture is partially loaded or the target record has no permission for reasons other than the approved-application regression.
Suggested fix: Assert the expected completed activity, applicant email, record/file identifiers, and one-time-download row before evaluating the button regression.
1 participant
概要 (Summary)
関連Issue / チケット (Related Issues)
変更タイプ (Type of Change)
🤖 0. CI 自動チェック (API Inventory Drift)
PR ごとに WEKO3 コンテナを起動し、
url_mapのダンプ・台帳との突き合わせ・変更行の到達可否測定を自動実行する。結果は PR コメントと Actions の artifact
(
api-inventory-summary) に出る。このリポジトリは public のため、台帳もベースラインも同梱していない。
実データはプライベートリポジトリ
RCOSDP/weko-secretにあり、CI は Secret 経由で取得する。以降この文書では、そこを単にプライベートリポジトリと呼ぶ。
台帳はブランチごとに内容が違うため、CI は weko 側と同名のブランチを
プライベートリポジトリから探して使う（head → base → 既定ブランチ の順）。
採用されたブランチ名は PR コメントの冒頭に出るので、件数を読む前にそこを見ること。
対応ブランチが無い場合は既定ブランチと比較され、コメント冒頭に警告が出る。
その件数は当てにならないので、PASS でも「確認済み」と読まないこと。
詳細:
tools/api-inventory/ci/README.md§3aSecret (
API_INVENTORY_REPO/API_INVENTORY_SSH_KEY) が未設定のリポジトリ、および fork からの PR では、このジョブは何もせずスキップされる。
API を追加・変更した場合（必須）
この PR が
fix/issue62569→develop_v2.0.4なら、プライベート側もfix/issue62569→develop_v2.0.4。同名にしておけば台帳 PR が未マージでもCI がそれを見るので、2つの PR のマージ順を気にしなくてよい。
api_snapshot.jsonを更新し、対応する PR を出したbash export WEKO_API_INVENTORY_DIR=/path/to/weko-secret ./install.sh python3 tools/api-inventory/scripts/snapshot.py --out "$WEKO_API_INVENTORY_DIR/api_snapshot.json"更新しないと CI が落ちる。公開リポジトリのコード変更とは別の PRになる。
weko3_api_list_full.tsvに行を追加・更新し、build_checklist.pyで 24 列版を再生成した（未収載だと reconcile が FAIL する）（
git statusに*.tsv/api_snapshot.jsonが出ていないこと）FAIL したときの対処（要約）
まず PR コメント冒頭の台帳ブランチを見る。警告が出ていれば、件数を追う前に
プライベートリポジトリ側の対応ブランチを用意すること（比較相手が違うので件数に意味がない）。
ジョブが落ちる条件は 3 つある。PR コメントのどのセクションに件数が出ているかで切り分ける。
drift.md)reconcile.md)*_PERMISSION_FACTORY/ CSRF 保護 等が危険側の値に変わったcan_delete/can_exportがFalse→Truedata_opを更新data_opが作成/更新/削除の経路に、未認証で到達したdata_opの記載誤りなら台帳を直すurl_mapに無いreconcile B のうち、実機に存在しないことが正当な行（プラグイン未登録・config で無効等）は
プライベートリポジトリの
reconcile_allow.jsonに理由付きで登録する。理由なしの登録は不可。登録済みの行は B'（既知・許容）として集計され、E'（endpoint が実機に無い）と併せてゲート対象外になる。
W1〜W6 は WARN でゲートは通るが、レビューでは見ること
（ModelView の追加 / 実装本体の変化 / HTTP メソッド・URL の変化 / 監視対象 config の変化 /
依存パッケージの版の変化）。特に W6（依存の版）は、ベースラインを CI と異なる環境で作ると
毎回出続けて形骸化するため、ベースラインは
install.shで作った環境から生成する。🔒 1. セキュリティ & API アクセス制御チェック (必須)
認証・認可 (Authentication & Authorization)
@login_required,@pass_record,need(...), Invenio Access Action/api/*ではPermission.require(http_exception=403)を使うこと。@login_requiredは API アプリにsecurity.loginが無いため 401 ではなく 500 になる--allow-writes付きでGET / HEAD 以外も叩く）
起動した経路のみ。ワークフロー系など未解決プレースホルダの行は skip される
Noneで無効化していない*_PERMISSION_FACTORY等を監視機能クローズ・非公開化の場合 (Feature Disable)
404 Not Foundまたは403 Forbiddenが返ることを確認した🧪 2. テストコード観点チェック (pytest / Invenio Test Suite)
権限・異常系テスト (Negative & Authorization Tests)
401 Unauthorizedまたは403 Forbidden/404 Not Foundが返ることを検証するテストがある403になるテストがある404/403を返すテストがある境界値・入力バリデーションテスト (Boundary & Validation)
400 Bad Request/ バリデーションエラーが返るテストがあるデータ整合性・トランザクションテスト (Integrity & Rollback)
🛡️ 3. データ保護 & 破壊的変更防止チェック (Data Safety)
⚙️ 4. マイグレーション & システム影響チェック (Invenio / WEKO3 Stack)
データベース (DB / Alembic)
invenio alembic upgrade（適用）およびdowngrade（ロールバック）スクリプトを作成・検証した検索インデックス (Elasticsearch / OpenSearch)
設定 & 非同期処理 (Config / Celery / Cache)
invenio.cfg/ 環境変数のデフォルト値を設定した📚 5. ドキュメント・仕様書更新チェック (weko-document)
tools/api-inventory/、台帳・調査記録はプライベートリポジトリ（public リポジトリには置かない）。
§0 のチェック項目で対応済みなら、ここは確認のみ。
weko3_api_auth_findings.md）もプライベートリポジトリに置く。台帳は二重管理しない📋 6. 動作検証エビデンス (Verification Evidence)
テスト実行結果
CI の成果物 (artifact:
api-inventory-summary)drift.mdreconcile.md明細（該当した経路名・実測結果）は公開できないため artifact に含めていない。
プライベートリポジトリ側で同じコマンドを
--summary-onlyなしで実行して確認する。手動で確認したこと
Summary by Sourcery
Restore the feature/jgss environment’s build and restore workflow while documenting and reproducing a known download-button issue.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: