Repository navigation
Hotfix v0.9.26 hiroba from w2026 64 hiroba - #1945
kanon-yoneji wants to merge 16 commits into
Conversation
W2025-33_early(release: 2026/03/30) from V0.9.26 hiroba develop
V0.9.26 hiroba develop
Update get_search_data API access control and tests(#63219)
…rom_W2026-64_hiroba fix charge process to use server-calculated price and handle price changes(#63646)
…vert_php_to_py Convert PHP files to Python and update related configuration(#63830)
Reviewer's GuideThis hotfix standardizes template translation extraction, tightens search API permissions, migrates the Shibboleth login proxy from PHP-FPM to Python/fcgiwrap, and prevents paid-file charges from using stale or client-tampered prices by validating billing details server-side and prompting for updated prices. Sequence diagram for server-validated paid-file chargingsequenceDiagram
participant User
participant Browser
participant ChargeAPI
participant Billing
participant Payment
User->>Browser: handleChargeBillingFile
Browser->>ChargeAPI: GET /charge with item_id, file_name, price
ChargeAPI->>Billing: get_file_charge_info
Billing-->>ChargeAPI: status, server_price, unit
alt price differs
ChargeAPI-->>Browser: price_changed with price and unit
Browser->>Browser: removeChargeItem
Browser->>Browser: showChargeConfirmation
else price matches
ChargeAPI->>Payment: create_charge
Payment-->>ChargeAPI: redirect URL or payment status
ChargeAPI-->>Browser: charge result
end
Sequence diagram for Shibboleth login proxysequenceDiagram
participant User
participant Nginx
participant LoginProxy
participant WEKO
participant Shibboleth
User->>Nginx: GET /secure/login.py
Nginx->>LoginProxy: fcgiwrap request
LoginProxy->>LoginProxy: Validate affiliation headers
LoginProxy->>WEKO: POST /weko/shib/login with attributes
WEKO->>Shibboleth: Process login
Shibboleth-->>WEKO: Redirect or login page
WEKO-->>LoginProxy: Response
LoginProxy-->>Nginx: HTTP redirect or HTML
Nginx-->>User: Login response
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 configuration
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 security issues, and 4 other issues
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
textarea.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link) - Certificate verification has been explicitly disabled. This permits insecure connections to insecure servers. Re-enable certification validation. (link)
Fixed security issues:
- SSL verification is disabled but should not be (currently CURLOPT_SSL_VERIFYPEER= false) (link)
- The 'phpinfo' function may reveal sensitive information about your environment. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/weko-records-ui/weko_records_ui/permissions.py" line_range="647" />
<code_context>
+ return 'already', None, None
+ return 'not_required', None, None
+
+ price, unit = get_file_price(record['_deposit']['id'])
+ if price is None or price <= 0:
+ return 'invalid', None, None
</code_context>
<issue_to_address>
**Users can pay for inaccessible files**
When a billed file has `open_no` or `open_restricted` access and the caller has a role with an item billing price, for a billed file with `open_no` or `open_restricted` access, `check_file_download_permission` returns false, and `get_file_charge_info` treats that denial as a reason to return a price. `/charge` can then create a payment even though the permission check still denies the user the file afterward.
Only return `ok` for access modes that support billing as an access grant; reject files whose access policy cannot be satisfied by payment.
</issue_to_address>
### Comment 2
<location path="modules/weko-records-ui/weko_records_ui/views.py" line_range="1071-1074" />
<code_context>
+ price (int) : 課金額(税込)。status が ok 以外の場合は None
+ unit (str) : 通貨単位。status が ok 以外の場合は None
+ """
+ try:
+ record = WekoRecord.get_record_by_pid(item_id)
+ except PIDDoesNotExistError:
</code_context>
<issue_to_address>
**Concurrent charges create duplicate reservations**
When two charge requests for the same user overlap before either writes the Redis key, both requests pass the checks and create payment reservations, then overwrite the same cache key; a callback can use the other request’s item ID, leaving the other callback without a key. The user can receive two 3DS flows and be charged twice.
Atomically claim or lock the user-and-item charge before creating a reservation, and prevent a concurrent request from creating another one.
Also at `modules/weko-records-ui/weko_records_ui/views.py:1136`.
</issue_to_address>
### Comment 3
<location path="modules/weko-records-ui/weko_records_ui/views.py" line_range="1133-1140" />
<code_context>
# 課金予約
try:
- redirect_url = create_charge(current_user.id, int(item_id), price, title, file_url, ret_url)
+ redirect_url = create_charge(
+ current_user.id,
+ int(item_id),
+ server_price,
+ title,
+ file_url,
+ ret_url
+ )
if redirect_url in ['connection_error', 'api_error']:
# 課金失敗
</code_context>
<issue_to_address>
**Failed requests orphan payment reservations**
When the application process crashes or the cache write fails after the payment provider accepts the reservation, `create_charge()` can create an external reservation and return its redirect URL before `datastore.put()` records the item ID. If the process fails in between, the response never gives the user the 3DS URL and the callback has no corresponding cache state; retrying starts another reservation instead of recovering the first.
Persist a pending charge record and its provider reservation identifier before returning the redirect, and make retries recover or safely cancel that pending reservation.
</issue_to_address>
### Comment 4
<location path="modules/weko-records-ui/weko_records_ui/views.py" line_range="1086-1087" />
<code_context>
+ if charge_status != 'ok':
+ # not_required / invalid は通常の画面操作では起きないため、専用の応答は設けない
+ current_app.logger.warning(
+ f'Charge rejected ({charge_status}): user: {current_user.id}, '
+ f'item_id: {item_id}, file_name: {file_name}')
+ return jsonify({'status': 'error'})
+ if int(price) != server_price:
</code_context>
<issue_to_address>
**Forged charge log entries**
When a caller sends a charge request with a newline in `file_name` that reaches the rejection warning, `file_name` comes directly from the request and is interpolated into the warning without escaping. A caller can include newline characters to forge additional log entries, undermining log review and alerting.
Remove or escape control characters in request values before writing them to logs, or use structured logging that preserves field boundaries.
</issue_to_address>
### Comment 5
<location path="modules/weko-records-ui/weko_records_ui/static/js/weko_records_ui/app.js" line_range="390" />
<code_context>
textarea.innerHTML = String(value);
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 6
<location path="modules/weko-records-ui/weko_records_ui/static/js/weko_records_ui/app.js" line_range="390" />
<code_context>
textarea.innerHTML = String(value);
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `textarea.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 7
<location path="nginx/login.py" line_range="50" />
<code_context>
response = requests.post(url, data=data, verify=False, cookies=cookie_jar)
</code_context>
<issue_to_address>
**security (python.requests.security.disabled-cert-validation):** Certificate verification has been explicitly disabled. This permits insecure connections to insecure servers. Re-enable certification validation.
*Source: opengrep*
</issue_to_address>Sourcery assessment
Needs a human reviewer. 7 findings to address first, and if the server-side file and price checks are wrong, the payment flow can charge a user an unintended amount; reverting the code does not undo a completed charge. The new CGI login proxy also changes how Shibboleth attributes are forwarded, so an authentication regression could expose access until fixed.
Blocking findings: modules/weko-records-ui/weko_records_ui/permissions.py:647, modules/weko-records-ui/weko_records_ui/views.py:1074, modules/weko-records-ui/weko_records_ui/views.py:1140, modules/weko-records-ui/weko_records_ui/views.py:1087, modules/weko-records-ui/weko_records_ui/static/js/weko_records_ui/app.js:390, and 2 more
| return 'already', None, None | ||
| return 'not_required', None, None | ||
|
|
||
| price, unit = get_file_price(record['_deposit']['id']) |
There was a problem hiding this comment.
🟡 Medium · Users can pay for inaccessible files
When a billed file has open_no or open_restricted access and the caller has a role with an item billing price, for a billed file with open_no or open_restricted access, check_file_download_permission returns false, and get_file_charge_info treats that denial as a reason to return a price. /charge can then create a payment even though the permission check still denies the user the file afterward.
Only return ok for access modes that support billing as an access grant; reject files whose access policy cannot be satisfied by payment.
Prompt for AI agents
In `modules/weko-records-ui/weko_records_ui/permissions.py` at line 647:
**Users can pay for inaccessible files**
When a billed file has `open_no` or `open_restricted` access and the caller has a role with an item billing price, for a billed file with `open_no` or `open_restricted` access, `check_file_download_permission` returns false, and `get_file_charge_info` treats that denial as a reason to return a price. `/charge` can then create a payment even though the permission check still denies the user the file afterward.
Only return `ok` for access modes that support billing as an access grant; reject files whose access policy cannot be satisfied by payment.| try: | ||
| charge_status, server_price, unit = get_file_charge_info( | ||
| int(item_id), file_name | ||
| ) |
There was a problem hiding this comment.
🟠 High · Concurrent charges create duplicate reservations
When two charge requests for the same user overlap before either writes the Redis key, both requests pass the checks and create payment reservations, then overwrite the same cache key; a callback can use the other request’s item ID, leaving the other callback without a key. The user can receive two 3DS flows and be charged twice.
Atomically claim or lock the user-and-item charge before creating a reservation, and prevent a concurrent request from creating another one.
Also at modules/weko-records-ui/weko_records_ui/views.py:1136.
Prompt for AI agents
In `modules/weko-records-ui/weko_records_ui/views.py` at lines 1071-1074:
**Concurrent charges create duplicate reservations**
When two charge requests for the same user overlap before either writes the Redis key, both requests pass the checks and create payment reservations, then overwrite the same cache key; a callback can use the other request’s item ID, leaving the other callback without a key. The user can receive two 3DS flows and be charged twice.
Atomically claim or lock the user-and-item charge before creating a reservation, and prevent a concurrent request from creating another one.
Also at `modules/weko-records-ui/weko_records_ui/views.py:1136`.| redirect_url = create_charge( | ||
| current_user.id, | ||
| int(item_id), | ||
| server_price, | ||
| title, | ||
| file_url, | ||
| ret_url | ||
| ) |
There was a problem hiding this comment.
🟡 Medium · Failed requests orphan payment reservations
When the application process crashes or the cache write fails after the payment provider accepts the reservation, create_charge() can create an external reservation and return its redirect URL before datastore.put() records the item ID. If the process fails in between, the response never gives the user the 3DS URL and the callback has no corresponding cache state; retrying starts another reservation instead of recovering the first.
Persist a pending charge record and its provider reservation identifier before returning the redirect, and make retries recover or safely cancel that pending reservation.
Prompt for AI agents
In `modules/weko-records-ui/weko_records_ui/views.py` at lines 1133-1140:
**Failed requests orphan payment reservations**
When the application process crashes or the cache write fails after the payment provider accepts the reservation, `create_charge()` can create an external reservation and return its redirect URL before `datastore.put()` records the item ID. If the process fails in between, the response never gives the user the 3DS URL and the callback has no corresponding cache state; retrying starts another reservation instead of recovering the first.
Persist a pending charge record and its provider reservation identifier before returning the redirect, and make retries recover or safely cancel that pending reservation.| f'Charge rejected ({charge_status}): user: {current_user.id}, ' | ||
| f'item_id: {item_id}, file_name: {file_name}') |
There was a problem hiding this comment.
🟡 Medium · Forged charge log entries
When a caller sends a charge request with a newline in file_name that reaches the rejection warning, file_name comes directly from the request and is interpolated into the warning without escaping. A caller can include newline characters to forge additional log entries, undermining log review and alerting.
Remove or escape control characters in request values before writing them to logs, or use structured logging that preserves field boundaries.
Prompt for AI agents
In `modules/weko-records-ui/weko_records_ui/views.py` at lines 1086-1087:
**Forged charge log entries**
When a caller sends a charge request with a newline in `file_name` that reaches the rejection warning, `file_name` comes directly from the request and is interpolated into the warning without escaping. A caller can include newline characters to forge additional log entries, undermining log review and alerting.
Remove or escape control characters in request values before writing them to logs, or use structured logging that preserves field boundaries.| function showChargeConfirmation(price, unit, priceChanged, previousPrice, previousUnit) { | ||
| const getDisplayUnit = function (value) { | ||
| const textarea = document.createElement('textarea'); | ||
| textarea.innerHTML = String(value); |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| function showChargeConfirmation(price, unit, priceChanged, previousPrice, previousUnit) { | ||
| const getDisplayUnit = function (value) { | ||
| const textarea = document.createElement('textarea'); | ||
| textarea.innerHTML = String(value); |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a textarea.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| cookie_jar = cookiejar.LWPCookieJar(temp_path) | ||
|
|
||
| # Request to the Shibboleth login | ||
| response = requests.post(url, data=data, verify=False, cookies=cookie_jar) |
There was a problem hiding this comment.
security (python.requests.security.disabled-cert-validation): Certificate verification has been explicitly disabled. This permits insecure connections to insecure servers. Re-enable certification validation.
Source: opengrep
概要 (Summary)
Summary by Sourcery
Harden access control and billing flows while migrating the Shibboleth login proxy from PHP to Python.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment:
Tests: