fix(tmplvars): TVフォーム生成とURL型保存処理をPHP8対応#449
Merged
Merged
Conversation
- normalize_url_tv_value()/tv_url_prefixes() をhelpers.phpに追加し、 URL型TV保存処理を各所で共通化（dm_backend、fields.php、save_resource、qm、getPreviewObject） - rendarFormUrl() を新規追加しURL型TVをform_url.tplテンプレートで描画 - rendarFormDate() に $row 引数を追加しform_name を動的に切り替え可能に - rendarFormImage()/rendarFormFile() に hsc() を適用しXSS対策を強化 - tv.ajax.php のローカル renderFormElement を削除しコアの実装を利用 - tv.ajax.php のJOINをINNER JOINに変更しORDER BYを追加 - main.tpl にブラウザ操作JS関数を移動しPHP生成コードを削減 - renderFormElement() の引数にnullチェックとstring型キャストを追加 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughこのPRはURL型テンプレート変数の正規化ロジックを集中化し、フォーム入力レンダリングをテンプレート化して複数の保存・表示経路で一貫した正規化とエスケープを適用しています。 ChangesURL TV正規化と Form Rendering統一
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1af27caeda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manager/includes/traits/document.parser.subparser.trait.php (1)
2291-2309:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
*_prefixが付いただけの非 URL TV まで正規化しない方が安全です。いまは
tv{id}_prefixが同時送信されるだけで TV のtypeを見ずにnormalize_url_tv_value()を通すため、URL 以外の TV でも値が書き換わります。site_tmplvarsからtypeも保持して、urlのときだけ正規化してください。🔧 修正例
- $tvname[$tvid] = $row['name']; + $tvname[$tvid] = [ + 'name' => $row['name'], + 'type' => $row['type'], + ]; ... - if (isset($tvname[$k])) { + if (isset($tvname[$k])) { if (is_array($v)) { $v = implode('||', $v); } - $name = $tvname[$k]; + $name = $tvname[$k]['name']; $prefix_key = "{$k}_prefix"; - if (isset($input[$prefix_key])) { + if (($tvname[$k]['type'] ?? '') === 'url' && isset($input[$prefix_key])) { $v = normalize_url_tv_value($v, $input[$prefix_key]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@manager/includes/traits/document.parser.subparser.trait.php` around lines 2291 - 2309, 現在は `site_tmplvars` から取得した TV を名前だけで参照し、`tv{id}_prefix` が送信されていると値を無条件で `normalize_url_tv_value()` に通しているため、URL 以外の TV も書き換わってしまいます。db()->select() で取得する列に `type` を含めて（`id,name,type,...` を利用）、ループ内で `if (isset($tvname[$k]))` の代わりに TV 情報（名前と type）を参照し、`type === 'url'` のときだけ `normalize_url_tv_value($v, $input[$prefix_key])` を呼ぶように変更してください（該当関数・変数: normalize_url_tv_value, $tvname/$tvid, $input, $prefix_key, site_tmplvars）。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@manager/includes/docvars/inputform/form_url.tpl`:
- Around line 4-9: The template outputs unescaped attribute placeholders (e.g.
[+prefix_id+], [+prefix_name+], [+id+], [+name+], [+value+]) which permits
attribute-injection/XSS; update form_url.tpl to use attribute-escaped
placeholders (e.g. use the *_attr variants for id/name/value in the select and
input attributes) and update the renderer that builds these placeholders to pass
hsc()-escaped strings into the *_attr keys so all attribute values are
HTML-escaped before rendering.
---
Outside diff comments:
In `@manager/includes/traits/document.parser.subparser.trait.php`:
- Around line 2291-2309: 現在は `site_tmplvars` から取得した TV を名前だけで参照し、`tv{id}_prefix`
が送信されていると値を無条件で `normalize_url_tv_value()` に通しているため、URL 以外の TV
も書き換わってしまいます。db()->select() で取得する列に `type` を含めて（`id,name,type,...` を利用）、ループ内で
`if (isset($tvname[$k]))` の代わりに TV 情報（名前と type）を参照し、`type === 'url'` のときだけ
`normalize_url_tv_value($v, $input[$prefix_key])` を呼ぶように変更してください（該当関数・変数:
normalize_url_tv_value, $tvname/$tvid, $input, $prefix_key, site_tmplvars）。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bd0e1db-8d7d-4c27-bd89-2e1074a07bae
📒 Files selected for processing (14)
assets/modules/docmanager/classes/dm_backend.class.phpassets/modules/docmanager/templates/main.tplassets/modules/docmanager/tv.ajax.phpassets/plugins/qm/qm.inc.phpmanager/actions/document/mutate_content/fields.phpmanager/actions/element/mutate_module.dynamic.phpmanager/actions/element/mutate_tmplvars.dynamic.phpmanager/includes/docvars/inputform/form_date.tplmanager/includes/docvars/inputform/form_file.tplmanager/includes/docvars/inputform/form_image.tplmanager/includes/docvars/inputform/form_url.tplmanager/includes/helpers.phpmanager/includes/traits/document.parser.subparser.trait.phpmanager/processors/document/save_resource.processor.php
- rendarFormUrl()でid/name/prefix_id/prefix_nameにhsc()を適用しXSS対策を強化 - getPreviewObject()でtvnameにtype情報を追加し、url型TVのみnormalize_url_tv_valueを適用 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
QMフォームはrenderFormElement()にtvname(文字列)を渡すため フォームのselectのname属性はtv<tvname>_prefixになる。 保存時にtv<tvId>_prefixのみ読んでいるとプレフィックスが常に'--'扱いになり URLスキームが欠落するバグを修正。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/plugins/qm/qm.inc.php`:
- Around line 434-442: The URL TV normalization is only called inside the if
($tvId) block so name-based TV submissions skip normalize_url_tv_value; change
the logic to determine $tvType by querying either id or name (use $tvId if
present otherwise look up by $tvName) and then, regardless of $tvId, call
normalize_url_tv_value when $tvType === 'url' using the existing prefix
resolution postv('tv'.$tvId.'_prefix', postv('tv'.$tvName.'_prefix', '--')),
keeping $tvContent and function normalize_url_tv_value as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 44b2a8b4-7a7c-4234-8f4d-f1a1f65e4a08
📒 Files selected for processing (2)
assets/plugins/qm/qm.inc.phpmanager/includes/traits/document.parser.subparser.trait.php
tvIdが0の場合（tvid未送信）でもtvNameでtype照会し、 url型であればnormalize_url_tv_valueを適用するよう変更。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
closes #410 の内容を再現（ブランチ誤削除による再PR）
https://forum.modx.jp/viewtopic.php?t=2036
概要
normalize_url_tv_value()/tv_url_prefixes()をhelpers.phpに追加し、URL型TVの保存処理を各所で共通化rendarFormUrl()を新規追加し URL型TVをform_url.tplテンプレートで描画（DocIDプレフィックス対応含む）rendarFormDate()に$row引数を追加しform_nameを動的に切り替え可能に（docmanagerとmutateで異なるフォーム名に対応）rendarFormImage()/rendarFormFile()にhsc()を適用しXSS対策を強化tv.ajax.phpのローカルrenderFormElementを削除しコアの実装（tmplvars.inc.php）を利用tv.ajax.phpのJOINをINNER JOINに変更しORDER BY（tvtpl.rank, tv.rank, tv.id）を追加main.tplにブラウザ操作JS関数（OpenServerBrowser/BrowseServer/BrowseFileServer/SetUrl）を移動しPHP生成コードを削減renderFormElement()の引数にnullチェックとstring型キャストを追加しPHP8での警告を解消変更ファイル
manager/includes/helpers.phptv_url_prefixes()とnormalize_url_tv_value()を新規追加manager/includes/docvars/inputform/form_url.tplmanager/includes/docvars/inputform/form_date.tpl[+form_name+]プレースホルダーに変更manager/includes/docvars/inputform/form_file.tplform_image.tplstyle="%s"形式に修正manager/includes/traits/document.parser.subparser.trait.phprendarFormUrl()追加、null安全処理、hsc()適用、rendarFormDate()に$row引数追加assets/modules/docmanager/tv.ajax.phprenderFormElementを削除しコアのものを利用、INNER JOIN・ORDER BY追加assets/modules/docmanager/templates/main.tplassets/modules/docmanager/classes/dm_backend.class.phpnormalize_url_tv_value()を利用assets/plugins/qm/qm.inc.phpmanager/actions/document/mutate_content/fields.phpsave_resource.processor.phpnormalize_url_tv_value()を利用manager/actions/element/mutate_module.dynamic.phpmutate_tmplvars.dynamic.php$chk→$chks、$notPublic初期化Test plan
http:///https:///ftp:///mailto:/DocID/--）が正しく保存されること[~ID~]形式で保存されること🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores