Skip to content

fix: 出力エスケープ(escape late)の徹底 (wp.org指摘対応, closes #60) - #61

Merged
fumikito merged 1 commit into
masterfrom
fix/escape-output
Jul 2, 2026
Merged

fumikito merged 1 commit into
masterfrom
fix/escape-output

Conversation

@fumikito

@fumikito fumikito commented Jul 2, 2026

Copy link
Copy Markdown
Member

Closes #60

概要

wp.org プラグインレビュー第2ラウンドの指摘「出力エスケープ(escape late)」対応。
phpcs:ignore WordPress.Security.EscapeOutput に頼っていた出力箇所を、出力時に実際にエスケープする。

方針

検証の結果、wp_kses_post() が <button> / data-* / aria-* / <nav> をすべて保持する(WP 5.0.1+ で data-・aria- はグローバル許可、button は post コンテキストに含まれる)ことを実機確認。
→ イシューで想定していたカスタム許可リスト(hamethread_allowed_html())は不要。全HTML出力点を wp_kses_post() に統一し、class属性値のみ esc_attr()。

変更点

  • template-parts/(comment-loop / button-thread-controller / woocommerce-my-account / button-comment-post / comments / form-comment)と UI/CommentForm.php の各出力を wp_kses_post() でラップ
  • 不要になった phpcs:ignore EscapeOutput を削除

変更しない例外(正当・ignore維持)

  • Hooks/StructuredData.php — JSON-LD。wp_json_encode() + JSON_HEX_* が正攻法
  • blocks/thread-button/render.php — get_block_wrapper_attributes()(コアがエスケープ済み)

検証

  • PHPCS: 43/43 ✅(WordPress.Security.EscapeOutput sniff を ignore なしで満たす)
  • PHPUnit: 5 tests / 22 assertions ✅
  • 実機レンダリング: front-end 経路で <button data-path> は保持・<script> は除去を確認
  • ※ Plugin Check はローカル環境の php-parser 非互換でクラッシュ(本変更と無関係)

補足(別対応が必要な発見)

検証中に、コメント本文が保存時にサニタイズされていない(RestCommentNew が wp_insert_comment を使用し kses をバイパス)ことを発見。front-end 表示は本PRの wp_kses_post で保護されるが、REST返却HTML(CommentModel::to_array())は生のまま。別issueで対応予定。

🤖 Generated with Claude Code

phcs:ignore に頼っていた出力箇所を実際にエスケープ。
wp_kses_post は button/data-*/aria-*/nav を保持するため
カスタム許可リストは不要。class属性値は esc_attr。
JSON-LD(StructuredData) と get_block_wrapper_attributes は
正当な例外として ignore を維持。

Co-authored-by: Claude <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

🔍 WordPress Plugin Check Report

⚠️ Status: Passed with warnings

📊 Report

🎯 Total Issues ❌ Errors ⚠️ Warnings
36 0 36

⚠️ Warnings (36)

📁 hamethread.php (1 warning)
📍 Line 🔖 Check 💬 Message
26 PluginCheck.CodeAnalysis.DiscouragedFunctions.load_plugin_textdomainFound load_plugin_textdomain() has been discouraged since WordPress version 4.6. When your plugin is hosted on WordPress.org, you no longer need to manually include this function call for translations under your plugin slug. WordPress will automatically load the translations for you as needed.
📁 functions.php (10 warnings)
📍 Line 🔖 Check 💬 Message
172 WordPress.DB.DirectDatabaseQuery.DirectQuery Use of a direct database call is discouraged.
172 WordPress.DB.DirectDatabaseQuery.NoCaching Direct database call without caching detected. Consider using wp_cache_get() / wp_cache_set() or wp_cache_delete().
189 WordPress.DB.DirectDatabaseQuery.DirectQuery Use of a direct database call is discouraged.
189 WordPress.DB.DirectDatabaseQuery.NoCaching Direct database call without caching detected. Consider using wp_cache_get() / wp_cache_set() or wp_cache_delete().
206 WordPress.DB.DirectDatabaseQuery.DirectQuery Use of a direct database call is discouraged.
206 WordPress.DB.DirectDatabaseQuery.NoCaching Direct database call without caching detected. Consider using wp_cache_get() / wp_cache_set() or wp_cache_delete().
521 WordPress.DB.DirectDatabaseQuery.DirectQuery Use of a direct database call is discouraged.
521 WordPress.DB.DirectDatabaseQuery.NoCaching Direct database call without caching detected. Consider using wp_cache_get() / wp_cache_set() or wp_cache_delete().
537 WordPress.DB.DirectDatabaseQuery.DirectQuery Use of a direct database call is discouraged.
537 WordPress.DB.DirectDatabaseQuery.NoCaching Direct database call without caching detected. Consider using wp_cache_get() / wp_cache_set() or wp_cache_delete().
📁 app/Hametuha/Thread/Rest/RestVote.php (2 warnings)
📍 Line 🔖 Check 💬 Message
159 WordPress.DB.DirectDatabaseQuery.DirectQuery Use of a direct database call is discouraged.
159 WordPress.DB.DirectDatabaseQuery.NoCaching Direct database call without caching detected. Consider using wp_cache_get() / wp_cache_set() or wp_cache_delete().
📁 includes/best-answer.php (1 warning)
📍 Line 🔖 Check 💬 Message
27 WordPress.DB.SlowDBQuery.slow_db_query_meta_query Detected usage of meta_query, possible slow query.
📁 app/Hametuha/Thread/Hooks/AutoClose.php (1 warning)
📍 Line 🔖 Check 💬 Message
68 WordPress.DB.SlowDBQuery.slow_db_query_meta_query Detected usage of meta_query, possible slow query.
📁 app/Hametuha/Thread/Rest/RestThreads.php (2 warnings)
📍 Line 🔖 Check 💬 Message
102 WordPress.DB.SlowDBQuery.slow_db_query_meta_query Detected usage of meta_query, possible slow query.
112 WordPress.DB.SlowDBQuery.slow_db_query_meta_query Detected usage of meta_query, possible slow query.
📁 template-parts/form-thread.php (1 warning)
📍 Line 🔖 Check 💬 Message
45 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$topic".
📁 template-parts/button-thread-controller.php (12 warnings)
📍 Line 🔖 Check 💬 Message
7 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$key".
8 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$label".
10 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$key".
11 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$label".
13 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$lock_action".
14 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$lists".
23 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$resolved".
24 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$resolve_label".
31 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$lists".
48 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$lists".
52 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$lists".
67 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$list".
📁 template-parts/button-thread.php (1 warning)
📍 Line 🔖 Check 💬 Message
10 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$attr".
📁 template-parts/comment-watcher.php (2 warnings)
📍 Line 🔖 Check 💬 Message
25 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$followers".
35 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$user".
📁 template-parts/woocommerce-my-account.php (1 warning)
📍 Line 🔖 Check 💬 Message
9 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$thread_count".
📁 app/Hametuha/Thread/Model/CommentModel.php (1 warning)
📍 Line 🔖 Check 💬 Message
36 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "comment_text".
📁 app/Hametuha/Thread/Model/ThreadModel.php (1 warning)
📍 Line 🔖 Check 💬 Message
43 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "the_content".

🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

@fumikito
fumikito merged commit ba7e1dc into master Jul 2, 2026
11 checks passed
@fumikito
fumikito deleted the fix/escape-output branch July 2, 2026 15:00

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces raw echo statements and phpcs:ignore annotations with late escaping functions like wp_kses_post and esc_attr to improve security and adhere to WordPress standards. However, applying wp_kses_post to the entire HTML block in CommentForm.php can strip out legitimate iframe elements (such as oEmbeds). It is recommended to use wp_kses with a custom allowed HTML array that explicitly permits iframe tags to prevent breaking embedded media.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

if ( $echo ) {
// phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- Safe HTML composed from comment_class() and the escaped comment-loop template part.
echo $html;
echo wp_kses_post( $html );

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

懸念点

wp_kses_post( $html ) を $html 全体に対して適用すると、コメント本文(comment_text())に含まれる oEmbed(YouTube や Vimeo などの <iframe> 埋め込み) や、特定の安全な HTML 要素・属性がすべて削除されてしまいます。
WordPress のデフォルトの post コンテキスト(wp_kses_post)では、セキュリティ上の理由から <iframe> タグが許可されていないためです。

また、comment-loop.php 内の各パーツはすでに個別に適切にエスケープされているため、結合された $html 全体に対して再度 wp_kses_post() を実行することは、パフォーマンス面でも不要なオーバーヘッド(二重サニタイズ)となります。

解決策

WordPress.org の late escape 指摘に対応しつつ、oEmbed などの <iframe> 埋め込みを壊さないようにするためには、wp_kses() を使用して iframe タグを明示的に許可したカスタム許可リストを渡す方法が推奨されます。

			$allowed_html = wp_kses_allowed_html( 'post' );
			$allowed_html['iframe'] = [
				'src'             => true,
				'width'           => true,
				'height'          => true,
				'frameborder'     => true,
				'allowfullscreen' => true,
				'allow'           => true,
				'style'           => true,
				'class'           => true,
				'id'              => true,
			];
			echo wp_kses( $html, $allowed_html );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[B] wp.org指摘: 出力エスケープ(escape late)の徹底対応

1 participant