Fix/issue62783 - #1923
Merged
ivis-miyachi merged 2 commits intoSep 29, 2026
Merged
Fix/issue62783#1923
Conversation
Add repository-scope and ownership checks across weko-admin, weko-gridlayout, weko-items-ui, weko-records-ui, and weko-workflow. - weko-admin: add repository_scope_required to get_send_mail_history - weko-gridlayout: reuse repository_scope_required on 5 widget endpoints - weko-items-ui: skip non-owned/private records during item export - weko-records-ui: restore permission check on cites API, add edit permission check to get_uri - weko-workflow: restrict activity creation/listing to Contributor+, harden guest activity init, verify ownership on activity deletion, extend check_authority for request-maillist access Add unit tests for each fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- add _lookup_param to repository_scope_required so nested keys such as 'data.repository' can be referenced with dot notation - when both the record's current repository (source) and the requested repository (destination) are resolvable, require both to be in scope, preventing records from being moved to an unassigned community - apply repository_scope_required to save_widget_item, blocking widget creation in arbitrary communities, overwriting of widgets outside the user's scope, and moves to unassigned communities - add unit tests in weko-admin/weko-gridlayout and update existing tests that assumed the old DB-value-priority behavior Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewer's GuideThis security-focused PR fixes authentication and authorization across 21+1 API paths by introducing repository-scoped decorators, applying role and ownership checks to administrative, record, export, citation, and workflow operations, and adding guest-input validation and rate limiting. The accompanying tests cover anonymous access, role boundaries, repository ownership and transfer scenarios, invalid identifiers, guest-token binding, and export filtering. Sequence diagram for repository-scoped API authorizationsequenceDiagram
participant Client
participant API
participant repository_scope_required
participant CurrentUser
participant Repository
participant Handler
Client->>API: Request protected endpoint
API->>repository_scope_required: wrapped()
alt unauthenticated
repository_scope_required-->>Client: 401 Unauthorized
else system or repository administrator
repository_scope_required->>Handler: f()
Handler-->>Client: Response
else community administrator
repository_scope_required->>Repository: Community.get_repositories_by_user(current_user)
alt all requested repositories are in scope
repository_scope_required->>Handler: f()
Handler-->>Client: Response
else repository outside scope
repository_scope_required-->>Client: 403 Forbidden
end
end
Sequence diagram for workflow activity deletion authorizationsequenceDiagram
participant Client
participant WorkflowAPI
participant verify_deletion
participant WorkActivity
participant CurrentUser
Client->>WorkflowAPI: Request deletion with activity_id
WorkflowAPI->>verify_deletion: verify_deletion(activity_id)
alt guest user
verify_deletion->>CurrentUser: Check guest session
alt guest_token activity_id matches
verify_deletion->>WorkActivity: Delete activity
WorkActivity-->>Client: Deletion result
else token mismatch or missing
verify_deletion-->>Client: 403 Authorization required
end
else authenticated user
verify_deletion->>WorkActivity: get_activity_by_id(activity_id)
alt owner or administrator authority
verify_deletion->>WorkActivity: Delete activity
WorkActivity-->>Client: Deletion result
else unauthorized
verify_deletion-->>Client: 403 Authorization required
end
end
Flow diagram for guest workflow initialization safeguardsflowchart TD
A[Guest workflow initialization] --> B[limiter.limit]
B -->|within 5 per minute| C{guest_mail provided?}
B -->|rate limit exceeded| D[Reject request]
C -->|yes| E[validate_email]
C -->|no| F[Create guest activity]
E -->|invalid| G[400 Invalid guest_mail]
E -->|valid| F
F --> H[Return guest workflow URL]
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 |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
![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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/weko-workflow/weko_workflow/views.py" line_range="647-650" />
<code_context>
"""
post_data = request.get_json()
+ if post_data.get('guest_mail'):
+ try:
+ validate_email(post_data.get('guest_mail'), check_deliverability=False)
+ except EmailNotValidError:
</code_context>
<issue_to_address>
**issue (bug_risk):** `init_activity_guest` calls `post_data.get(...)` immediately after `request.get_json()` without checking that a JSON object was supplied; an empty body, malformed JSON, or a JSON scalar therefore raises `AttributeError` and produces a 500 response instead of a client validation error.
**Triggers:** When an unauthenticated guest submits the endpoint without a JSON object.
**Suggested fix:** Reject a non-dict payload with a 400 response before accessing `.get()`.
</issue_to_address>
### Comment 2
<location path="modules/weko-records-ui/weko_records_ui/rest.py" line_range="653-657" />
<code_context>
- # @pass_record
- # @need_record_permission('read_permission_factory')
+ @require_api_auth(allow_anonymous=True)
+ @require_oauth_scopes(item_read_scope.id)
def get(self, pid_value, **kwargs):
</code_context>
<issue_to_address>
**issue (bug_risk):** `page_permission_factory(record).can()` is evaluated inside the broad citation `except Exception` block, so a permission-check failure is logged as a citation-formatting failure and converted into a 404 response; infrastructure or database errors in authorization are silently disguised as 'Not found' rather than being escalated or handled distinctly.
**Triggers:** When the permission query or permission helper raises an unexpected exception while serving a citation.
**Suggested fix:** Perform the authorization check outside the citation-formatting `try`, or catch permission-denial and unexpected permission failures separately.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and the change adds authorization and scope checks across record export, repository administration, workflow actions, and deletion paths; a faulty check could expose private records or let an unauthorized user delete data or perform workflow actions. Reverting would stop future access decisions, but it would not undo data already exposed or deleted.
Blocking findings: modules/weko-workflow/weko_workflow/views.py:650, modules/weko-records-ui/weko_records_ui/rest.py:657
Comment on lines
647
to
+650
| post_data = request.get_json() | ||
|
|
||
| if post_data.get('guest_mail'): | ||
| try: |
There was a problem hiding this comment.
issue (bug_risk): init_activity_guest calls post_data.get(...) immediately after request.get_json() without checking that a JSON object was supplied; an empty body, malformed JSON, or a JSON scalar therefore raises AttributeError and produces a 500 response instead of a client validation error.
Triggers: When an unauthenticated guest submits the endpoint without a JSON object.
Suggested fix: Reject a non-dict payload with a 400 response before accessing .get().
Comment on lines
+653
to
657
| @require_api_auth(allow_anonymous=True) | ||
| @require_oauth_scopes(item_read_scope.id) | ||
| def get(self, pid_value, **kwargs): | ||
| """Render citation for record according to style and language.""" | ||
| from weko_records.serializers import citeproc_v1 |
There was a problem hiding this comment.
issue (bug_risk): page_permission_factory(record).can() is evaluated inside the broad citation except Exception block, so a permission-check failure is logged as a citation-formatting failure and converted into a 404 response; infrastructure or database errors in authorization are silently disguised as 'Not found' rather than being escalated or handled distinctly.
Triggers: When the permission query or permission helper raises an unexpected exception while serving a citation.
Suggested fix: Perform the authorization check outside the citation-formatting try, or catch permission-denial and unexpected permission failures separately.
| return Permission(action_class) | ||
|
|
||
|
|
||
| def _is_super_user(user): |
| return value | ||
|
|
||
|
|
||
| def repository_scope_required(repository_id_param=None, |
| return any(role.name in comadmin for role in (user.roles or [])) | ||
|
|
||
|
|
||
| def _lookup_param(data, kwargs, path): |
|
|
||
| @blueprint_api.route('/get_send_mail_history', methods=['GET']) | ||
| @repository_scope_required(repository_id_param='repo_id') | ||
| def get_send_mail_history(): |
| methods=['POST']) | ||
| @login_required | ||
| @repository_scope_required(repository_id_param='repository_id') | ||
| def load_widget_list_design_setting(): |
| @repository_scope_required(repository_id_param='repository_id', | ||
| id_param='page_id', id_model=WidgetDesignPage) | ||
| # TODO: Allow this to be used for both or make a different path | ||
| def save_widget_layout_setting(): |
| @login_required | ||
| @repository_scope_required(repository_id_param='repository_id', | ||
| id_param='page_id', id_model=WidgetDesignPage) | ||
| def save_widget_design_page(): |
| @blueprint_api.route('/delete_widget_design_page', methods=['POST']) | ||
| @login_required | ||
| @repository_scope_required(id_param='page_id', id_model=WidgetDesignPage) | ||
| def delete_widget_design_page(): |
| @repository_scope_required(repository_id_param='data.repository', | ||
| id_param='data_id', id_model=WidgetItem, | ||
| pk_attr='widget_id') | ||
| def save_widget_item(): |
| @login_required | ||
| @repository_scope_required(id_param='data_id', id_model=WidgetItem, | ||
| pk_attr='widget_id') | ||
| def delete_widget_item(): |
| include_contents, | ||
| record_path, | ||
| ) | ||
| if not exported_item: |
| record = WekoRecord.get_record_by_pid(record_id) | ||
| list_item_role = {} | ||
| if record: | ||
| roles = get_user_roles() |
|
|
||
| # @pass_record | ||
| # @need_record_permission('read_permission_factory') | ||
| @require_api_auth(allow_anonymous=True) |
| try: | ||
| pid = PersistentIdentifier.get('depid', pid_value) | ||
| record = WekoRecord.get_record(pid.object_uuid) | ||
| if not page_permission_factory(record).can(): |
| current_app.logger.exception( | ||
| 'Citation formatting for record {0} failed.'.format( | ||
| str(record.id))) | ||
| str(pid_value))) # record.id ではなく pid_value を参照(UnboundLocalError修正) |
| return True | ||
|
|
||
| @blueprint.route("/get_uri", methods=['POST']) | ||
| @login_required |
| {%- if community_id %} | ||
| <li role="presentation" {% if tab_value=='top' %}class="active"{% endif %}><a href="/?c={{community_id}}">{{ _('Top') }}</a></li> | ||
| {%- if current_user.is_authenticated and current_user.roles %} | ||
| {%- if current_user.is_authenticated and current_user.roles | selectattr('name', 'in', ['System Administrator', 'Repository Administrator', 'Community Administrator', 'Contributor']) | list %} |
Contributor
There was a problem hiding this comment.
fix no.600, 998, 602, 1000
| {%- else %} | ||
| <li role="presentation" {% if tab_value=='top' %}class="active"{% endif %}><a href="/">{{ _('Top') }}</a></li> | ||
| {%- if current_user.is_authenticated and current_user.roles %} | ||
| {%- if current_user.is_authenticated and current_user.roles | selectattr('name', 'in', ['System Administrator', 'Repository Administrator', 'Community Administrator', 'Contributor']) | list %} |
Contributor
There was a problem hiding this comment.
fix no.600, 998, 602, 1000
|
|
||
| @workflow_blueprint.route('/activity/new', methods=['GET']) | ||
| @login_required | ||
| @item_permission.require(http_exception=403) |
Contributor
There was a problem hiding this comment.
fix no.600, 998, 602, 1000
|
|
||
| @workflow_blueprint.route('/activity/list', methods=['GET']) | ||
| @login_required | ||
| @item_permission.require(http_exception=403) |
Contributor
There was a problem hiding this comment.
fix no.600, 998, 602, 1000
| from .sessions import upt_activity_item | ||
| from .views import depositactivity_blueprint, workflow_blueprint | ||
| self.init_config(app) | ||
| self.init_limiter(app) |
| app.register_blueprint(depositactivity_blueprint) | ||
| app.extensions['weko-workflow'] = self | ||
|
|
||
| def init_limiter(self, app): |
|
|
||
|
|
||
| @workflow_blueprint.route('/activity/init-guest', methods=['POST']) | ||
| @limiter.limit("5 per minute", key_func=get_remote_address) |
| return Limiter(app=Flask(__name__), key_func=get_remote_address, default_limits=WEKO_WORKFLOW_API_LIMIT_RATE_DEFAULT) | ||
|
|
||
|
|
||
| # NOTE: create_limmiter() above binds the Limiter to a throw-away Flask app |
| """ | ||
| post_data = request.get_json() | ||
|
|
||
| if post_data.get('guest_mail'): |
| password_for_download = hash_password(pwd) | ||
|
|
||
| if is_terms_of_use_only(post_data["workflow_id"]): | ||
| if is_terms_of_use_only(post_data.get('workflow_id')): |
| Returns: | ||
| dict: JSON response with code, is_deleted, and for_delete status. | ||
| """ | ||
| # 当事者検証。login_required_customize は session["guest_token"] の |
| ) | ||
|
|
||
|
|
||
| def _get_shared_user_ids_from_list(shared_user_ids_list): |
| if check_authority_by_admin(activity_detail): | ||
| return func(*args, **kwargs) | ||
|
|
||
| action_id = kwargs.get('action_id') |
| action_order=0): | ||
| """Check authority.""" | ||
|
|
||
| def _get_shared_user_ids_from_list(shared_user_ids_list): |
|
|
||
| @workflow_blueprint.route('/get_request_maillist/<string:activity_id>', methods=['GET']) | ||
| @login_required | ||
| @check_authority |
| return any(role.name in supers for role in (user.roles or [])) | ||
|
|
||
|
|
||
| def _is_community_admin(user): |
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
2 participants
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)
check_authorityデコレーターの修正(拡張)repository_scope_requiredデコレーター新規追加関連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
Harden authentication and authorization across administrative, record, grid layout, and workflow APIs while preventing unauthorized data access and abuse.
Bug Fixes:
Enhancements:
Tests: