Repository navigation
feat: API 台帳の優先度判定で永続データの変更とデコレータ以外の認証を考慮する - #1957
Merged
Merged
Conversation
- P1 の状態変更系は、データベース/ファイルストレージを作成・更新・削除する ものに限る(POST で受ける取得やセッションだけの更新を含めない) - infra_auth: OAuth2 クライアント認証、nginx の location の制限、 Shibboleth SP の requireSession を判定する - credential_auth: ビュー内のパスワード・トークンの検証を判定する - 上記の認証がある行は auth_required が「不要」でも無認証として扱わない - docstring と README を実際の判定に合わせる Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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
 |
🔍 Claude によるレビュー指摘 2 件 — 🔴 高 0 ／ 🟠 中 1 ／ 🟡 低 1 ／ 🔎 未確認 8 件 🟠 [中] data_store が '-' や書式外の行では、無認証の書き込みが P1 に入らなくなった
P1 の条件に is_mutating(=data_store に PostgreSQL/ファイルが明記されていること)が加わった。data_store が未記入('-')または書式が違う行は、無認証の POST/PUT/DELETE でも P1 にならない。同じ行は P4 の公開設計に当たらなければ「分類条件に合致せず」の P3 に落ちる。 提案 data_store が '-' や空で保存先を判別できない書き込み行は、安全側として P1 のままにするか、P2 に置く。 根拠確認: prioritize.py:150-345 を読んだ。is_mutating が使われるのは P1 の2条件だけ。P1 を外れた行の落ち先は P2 の各条件、管理者向け、P4、読み取り系 P3、P5、既定の P3 の順。台帳 TSV は非公開リポジトリにあり、data_store が実際に '-' の書き込み行が何件あるかは確認していない。 🟡 [低] 区分一覧に廃止した「対象外」が残っている
prioritize.py の ORDER から「対象外」は削除されたが、レポート生成側は「対象外」を集計し続ける。常に 0 件の行が出力され、区分の廃止が反映されない。 提案 タプルから '対象外' を外す。 根拠確認: grep で tools/api-inventory 配下の *.py を検索し、_report.py:66 だけが「対象外」を区分として参照していることを確認した。_report.py:55-70 も読んだ。reconcile.py の「対象外」は別の意味(比較対象外)なので無関係。 🔎 未確認 — 裏が取れなかったもの 8 件
モデル sonnet ／ 3 回実行して和集合 ／ コスト $0.6124。同じ差分でも結果が揺れるため複数回まわし、一部のパスでしか挙がらなかったものには回数を添えています 差分のみを対象にした自動レビューです。誤りが含まれることがあります。 |
- 「対象外」を廃止し、admin-access 相当の行も確認対象にする - bola_risk に★がある行は書き込み系を P2、読み取り系を P3 にする - リポジトリ/コミュニティ管理者も使える書き込み系は、管轄の限定を 確かめるため P3 にする - 上記以外(管理者向けの読み取り系・システム管理者のみ)は P5 にする Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
API インベントリ差分（件数のみ）
ベースラインとの差分API インベントリ差分レポート
判定: ✅ PASS (FAIL 0 / WARN 2)サマリ
[WARN] W6 依存パッケージの版が変化した — 6件
[WARN] W5 監視対象 config が変化した — 1件
台帳との突き合わせスナップショット ↔ インベントリ 突き合わせ
判定: ✅ 一致 (0件)
|
Reviewer's GuideAPI 台帳の優先度判定を、永続データの変更範囲、OAuth2/nginx/Shibboleth／ビュー内認証、管理者ロールと IDOR リスクを考慮するモデルへ改訂し、認証・設定解析および分類ロジックを追加の pytest で検証している。 Sequence diagram for non-decorator authentication detectionsequenceDiagram
participant P as prioritize.py
participant I as infra_auth
participant C as credential_auth
participant S as Source and config files
P->>I: detect(uris, methods, impl_file, impl_line)
I->>S: Read OAuth2 decorators, nginx config, Shibboleth config
S-->>I: Authentication matches or none
alt infrastructure authentication found
I-->>P: OAuth2, nginx, or Shibboleth authentication
else no infrastructure authentication
P->>C: credential_auth(impl_file, impl_line, endpoint)
C->>S: Parse view and one-level package calls
S-->>C: Password or token verification matches
C-->>P: Credential authentication or empty result
end
P->>P: Treat matched authentication as authenticated
P-->>P: Classify as P5 unless stronger risk rules apply
Flow diagram for API inventory priority classificationflowchart TD
A[API inventory row] --> B[Detect authentication]
B --> C[infra_auth: OAuth2 nginx Shibboleth]
B --> D[credential_auth: password or token]
C --> E[Determine persistent data change]
D --> E
E --> F{Database or file storage changed?}
F -->|Yes| G[Apply P1 or P2 write-risk rules]
F -->|No| H[Exclude session Redis ES and external-only changes from P1]
G --> I[Apply exposure and authorization rules]
H --> I
I --> J{Administrative API?}
J -->|Yes| K[Use roles and bola_risk for P2 P3 or P5]
J -->|No| L[Apply ordinary priority rules]
K --> M[Apply test-gap bump up to P3]
L --> M
M --> N[Priority and reason]
Flow diagram for persistent-data and administrative risk rulesflowchart TD
A[API inventory row] --> B{Persistent store?}
B -->|PostgreSQL or file storage| C{Create update or delete?}
B -->|Session Redis ES or external service only| D[Not persistent mutation]
C -->|Write with missing or ineffective authorization| E{Create only?}
C -->|No| F[Apply read and exposure rules]
E -->|No| G[P1]
E -->|Yes| H[P2]
E -->|No persistent mutation| F
D --> F
F --> I{Administrative API without finding?}
I -->|bola_risk star and write| J[P2]
I -->|bola_risk star and read| K[P3]
I -->|Scoped admin write| L[P3]
I -->|Scoped admin read or system admin only| M[P5]
I -->|No| N[Ordinary classification]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
| """SP の RequestMap で path に requireSession が効いているか。""" | ||
| p = _conf_path('WEKO_SHIB_CONF', 'nginx/shibboleth2.xml') | ||
| try: | ||
| root = ET.parse(p).getroot() |
There was a problem hiding this comment.
security (python.lang.security.use-defused-xml-parse): The native Python xml library is vulnerable to XML External Entity (XXE) attacks. These attacks can leak confidential data and "XML bombs" can cause denial of service. Do not use this library to parse untrusted input. Instead the Python documentation recommends using defusedxml.
Suggested change
| root = ET.parse(p).getroot() | |
| root = defusedxml.etree.ElementTree.parse(p).getroot() |
Source: opengrep
API インベントリ差分（件数のみ）
ベースラインとの差分API インベントリ差分レポート
判定: ✅ PASS (FAIL 0 / WARN 2)サマリ
[WARN] W6 依存パッケージの版が変化した — 6件
[WARN] W5 監視対象 config が変化した — 1件
台帳との突き合わせスナップショット ↔ インベントリ 突き合わせ
判定: ✅ 一致 (0件)
|
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
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)
tools/api-inventory/scripts/prioritize.pyの優先度判定を見直すinfra_auth.py（新規）: OAuth2 クライアント認証（@oauth2.token_handler等）、nginx の location の制限（deny all・auth_basic・if ($var) { return 403; }等）、Shibboleth SP のrequireSessionを判定するcredential_auth.py（新規）: ビュー内のパスワード・トークンの検証を判定する（CSRF・CAPTCHA は除く）auth_requiredが「不要」でも無認証として扱わず P5 にするpython3 -m pytest tools/api-inventory/tests（43 件パス）関連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なしで実行して確認する。手動で確認したこと
🤖 Generated with Claude Code
Summary by Sourcery
API インベントリの優先度判定を、永続データ変更とデコレータ以外の認証方式および管理者ロールを考慮するよう見直してください。
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: