fix_62782_62796 - #1921
fix_62782_62796#1921KotaroInoue1448 wants to merge 1 commit into
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
Reviewer's GuideThe PR strengthens authorization for index metadata endpoints by validating and role-filtering requested indexes, and adds an explicit authority check to the workflow feedback-mail endpoint while making a minor whitespace cleanup. Sequence diagram for index metadata authorizationsequenceDiagram
participant Client
participant View as SearchUIView
participant Indexes
participant RoleFilter as filter_index_list_by_role
Client->>View: GET journal_info/{index_id} or get_path_name_dict/{path_str}
View->>Indexes: get_index(index_id)
Indexes-->>View: requested indexes
View->>RoleFilter: filter_index_list_by_role(index_list)
alt no valid or unauthorized index
RoleFilter-->>View: empty list
View-->>Client: 403 Forbidden
else authorized indexes
RoleFilter-->>View: allowed indexes
View->>View: journal_detail(index_id) or get_path_name_dict(path_str)
View-->>Client: metadata response
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/weko-search-ui/weko_search_ui/views.py" line_range="395" />
<code_context>
@blueprint.route("/journal_info/<int:index_id>", methods=["GET"])
+@check_index_permission
def journal_detail(index_id=0):
"""Render a check view."""
</code_context>
<issue_to_address>
**issue (testing):** `check_index_permission` reads only `kwargs` for `index_id` and `path_str`, so direct positional calls such as the existing `journal_detail(33)` and `get_path_name_dict('33_44')` tests have neither value and abort with 404 instead of invoking the view.
**Triggers:** When these decorated views are called positionally, as in the existing unit tests or any non-routing caller.
**Suggested fix:** Accept the corresponding positional arguments or update the decorator to bind arguments using the wrapped function signature before checking them.
</issue_to_address>
### Comment 2
<location path="modules/weko-workflow/weko_workflow/views.py" line_range="2805" />
<code_context>
@workflow_blueprint.route('/get_feedback_maillist/<string:activity_id>',
methods=['GET'])
@login_required
+@check_authority
def get_feedback_maillist(activity_id='0'):
"""アクティビティに設定されているフィードバックメール送信先の情報を取得して返す
</code_context>
<issue_to_address>
**issue (bug_risk):** Applying `check_authority` before `get_feedback_maillist` dereferences `activity_detail.action_order` without checking whether `get_activity_by_id(activity_id)` returned `None`, so a nonexistent activity ID raises `AttributeError` and returns a 500 response instead of reaching the view's argument/error handling.
**Triggers:** When an authenticated client requests `/workflow/get_feedback_maillist/<activity_id>` for an activity ID that does not exist.
**Suggested fix:** Return a 404 or the endpoint's existing error response when `activity_detail` is `None` before accessing `action_order`.
</issue_to_address>
### Comment 3
<location path="modules/weko-workflow/weko_workflow/views.py" line_range="2805" />
<code_context>
@workflow_blueprint.route('/get_feedback_maillist/<string:activity_id>',
methods=['GET'])
@login_required
+@check_authority
def get_feedback_maillist(activity_id='0'):
"""アクティビティに設定されているフィードバックメール送信先の情報を取得して返す
</code_context>
<issue_to_address>
**issue (bug_risk):** Unauthorized users are rejected by `check_authority` with `jsonify(code=403, msg=...)` but no HTTP status argument, so the endpoint returns HTTP 200 with a body containing `code: 403`; API clients and middleware that rely on the HTTP status still treat the unauthorized request as successful.
**Triggers:** When a logged-in user is denied by the workflow action authority check.
**Suggested fix:** Return the JSON response with HTTP status 403, for example `return jsonify(code=403, msg=error_msg), 403`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and the new decorators change who can access index metadata and feedback-recipient information. If the role filtering or authority check is wrong, unauthorized data could be exposed or legitimate access denied; reverting restores the old behavior, but any information already exposed cannot be unexposed.
Blocking findings: modules/weko-search-ui/weko_search_ui/views.py:395, modules/weko-workflow/weko_workflow/views.py:2805, modules/weko-workflow/weko_workflow/views.py:2805
|
|
||
|
|
||
| @blueprint.route("/journal_info/<int:index_id>", methods=["GET"]) | ||
| @check_index_permission |
There was a problem hiding this comment.
issue (testing): check_index_permission reads only kwargs for index_id and path_str, so direct positional calls such as the existing journal_detail(33) and get_path_name_dict('33_44') tests have neither value and abort with 404 instead of invoking the view.
Triggers: When these decorated views are called positionally, as in the existing unit tests or any non-routing caller.
Suggested fix: Accept the corresponding positional arguments or update the decorator to bind arguments using the wrapped function signature before checking them.
| @workflow_blueprint.route('/get_feedback_maillist/<string:activity_id>', | ||
| methods=['GET']) | ||
| @login_required | ||
| @check_authority |
There was a problem hiding this comment.
issue (bug_risk): Applying check_authority before get_feedback_maillist dereferences activity_detail.action_order without checking whether get_activity_by_id(activity_id) returned None, so a nonexistent activity ID raises AttributeError and returns a 500 response instead of reaching the view's argument/error handling.
Triggers: When an authenticated client requests /workflow/get_feedback_maillist/<activity_id> for an activity ID that does not exist.
Suggested fix: Return a 404 or the endpoint's existing error response when activity_detail is None before accessing action_order.
| @workflow_blueprint.route('/get_feedback_maillist/<string:activity_id>', | ||
| methods=['GET']) | ||
| @login_required | ||
| @check_authority |
There was a problem hiding this comment.
issue (bug_risk): Unauthorized users are rejected by check_authority with jsonify(code=403, msg=...) but no HTTP status argument, so the endpoint returns HTTP 200 with a body containing code: 403; API clients and middleware that rely on the HTTP status still treat the unauthorized request as successful.
Triggers: When a logged-in user is denied by the workflow action authority check.
Suggested fix: Return the JSON response with HTTP status 403, for example return jsonify(code=403, msg=error_msg), 403.
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
Bug Fixes: