【WordPress】プラグイン公式ディレクトリ申請時のコードレビューで指摘された点まとめ！

自作プラグインをワードプレスの公式ディレクトリに申請しようとした時に、必ず通過しないといけないのが、ワードプレスのレビューチームによるコードレビュー。

「Ad Auto Insert H」というプラグインを申請した時を例に、どんなレビュー結果が来るのかと、その時の対応を備忘録的にまとめてます。

プラグイン申請時の参考になりましたら幸いです。

Contents

1 レビューについて
2 レビュー結果
3 指摘に対する具体的修正
4 ポイントまとめ

レビューについて

ワードプレスのプラグインを公式ディレクトリに登録するには、レビューチームによるコードレビューを受ける必要がありますが、このレビュー、不具合（バグ）をチェックするようなものではなく、あくまで用意されている公式ドキュメント（規約やガイドライン）に沿っているかどうかのチェックのようですね。

プラグインとして動作するものを提出するのは当たり前ですが、普通に動作するにしてもその中にバグがあっても特に指摘はしてくれません。

（レビューチームはあくまでコードのレビューチームで合ってテストチームではないですから。あまりにも明らかなものがあれば指摘してくれるかもしれませんが期待はできないかも）

ということから、申請に出す前にはワードプレスのプラグイン開発ハンドブックを参照して、ガイドラインや規約とあっているかどうかをチェックしておくのが良いですね。
（無駄なやり取りを発生させず、またレビューチームに余計な負荷をかけないようにするため）

レビュー結果

プラグイン「Ad Auto Insert H」の場合、計3回のレビューを受け（3回目で承認された）、合計で7つぐらいの指摘とそれに対する修正を行ってます。

明らかに基本的な指摘から、「え？そうなの？」というものまでありましたが、この「え？そうなの」が2つあって、その1つがかなり衝撃的で大きな修正が必要になりました。

申請すると以下のような感じで、指摘内容（レビュー結果）がメールできます。

何が良くないのか、具体的箇所やその内容が詳しく説明されていて、非常に丁寧な感じです。全部英語なんですが、良くわからないところはグーグル翻訳をフル活用。 ^-^;）

承認された3回目のメールを除いて、それ以前の2回分の指摘内容をまとめると以下の通り。

指摘１）## Incorrect Stable Tag
readme.txt とプラグインメインファイルのバージョン違い
- readme.text：0.9.2.0
- プラグインメインファイルのヘッダ：0.9.2
指摘２）## Calling core loading files directly
ワードプレスのコアファイルを直接インクルードしてはいけない
- インクルードを介してwp-config.php、wp-blog-header.php、wp-load.phpを直接インクルードすることは許可されていない。
- 1か所のみ、必要上「wp-load.php」をインクルードして呼び出していた箇所あり
指摘３）## Do not use HEREDOC or NOWDOC syntax in your plugins
ヒアドキュメント、ナウドキュメントは使用禁止
- コード内のエスケープ処理を認識できないから
指摘４）## Data Must be Sanitized, Escaped, and Validated
データは必ずサニタイズ、検証、エスケープが必要
- POST/GET/REQUEST/FILEはできるだけ早くサニタイズすること
  （変数に代入する時）
指摘５）## Variables and options must be escaped when echo'd
変数は、エコーされるときにエスケープする必要あり
- 最後に出力するときにエスケープする必要あり
  （セキュリティ観点から）
指摘６）## Attempting to process custom CSS/JS/PHP
プラグイン内でユーザー独自のCSS、JavaScript、PHPを保存させてはいけない
- セキュリティ観点から、CSS/JS/PHPの保存ができるプラグインは（これまでは良かったんだけど）今後は許可しない。
  （We no longer permit arbitrary plugins to allow users to save custom CSS, JavaScript, or PHP within the plugin.）

この最後にある指摘（プラグイン内でCSS/JS/PHP の保存は許可しない）が2回目のレビュー結果で来たもので一番衝撃的。大きな修正が必要になりました．．．

指摘に対する具体的修正

上でまとめた指摘１から順に修正対応した内容は以下のような感じです。

１）readme.txtプラグインメインファイルのバージョン違い

readme.txt では「0.9.2.0」、
プラグインメインファイルのヘッダでは「0.9.2」となっていて記述があっていなかった。

単純なケアレスミスですみません。
次回提出時に向けてバージョンを揃えた。

２）コアファイルを直接インクルードしてはいけない

プラグインの中でリセットボタンを置いて、そのリセットボタンが押されたら変数をすべて初期値戻す、ということをしています。

（データのリセットは滅多やたら使うものではないし、ユーザーの利便性向上、というより、リセットボタンがあった方がチェック時に楽、という意味合いの機能）

このデータのリセットを実現するために、form では以下のようにしてました。

<form action="リセット処理用ファイル指定" method="post" onsubmit="return resetAllDataChk()">

リセット用のボタンが押されると、プラグインフォルダ内のリセット処理用ファイルを呼び出し、その中でデータリセットの処理を行う。

でもこの場合、include とかでリセット処理用のファイルをロードするわけではなく別に呼び出しているので、つまりワードプレスのプラグインをロードした状態とは異なり、ワードプレス外、となって、ワードプレス関連の関数がそのファイル内では使えない。

そのため、このリセット処理用ファイル内で改めて wp-load.php を require_once で呼び出し、ワードプレス関連の関数を使えるようにしていたところ、こうした呼び出しがNGということです。
（同様に、wp-config.php、wp-blog-header.php の呼び出しもNG）

## Calling core loading files directly

Including wp-config.php, wp-blog-header.php, wp-load.php directly via an include is not permitted.

These calls are prone to failure as not all WordPress installs have the exact same file structure. In addition it opens your plugin to security issues, as WordPress can be easily tricked into running code in an unauthenticated manner.

（以下略）

NG理由は以下のように書かれてます。

すべてのWordPressインストールがまったく同じファイル構造を持っているわけではないため、これらの呼び出しは失敗する傾向があること
さらに、認証されていない方法でコードを実行するようにワードプレスが簡単にだまされる可能性があり、プラグインがセキュリティの問題にさらされる可能性があること

こうしたことから（別ファイル呼び出しではなく）あくまでプラグイン内での動作が期待され、解決策として以下2点の連絡も付いてました。（提案もしてくれるのが凄いですね）

１）query_varsや書き換えルールを使用して、関数を呼び出す仮想ページを作成する
- https://developer.wordpress.org/reference/hooks/query_vars/
- https://codepen.io/the_ruther4d/post/custom-query-string-vars-in-wordpress
AJAXを使用する
- https://developer.wordpress.org/plugins/javascript/ajax/

提案された２つで対応するのではなく、form の action は空文字の指定として、単に元ファイルを呼び出す、という形で対応。

<form action="" method="post" onsubmit="return resetAllDataChk()">

初めからこうしておけばよかったんですが、何か無駄な表示を繰り返す、みたいな感じがして、必要なものだけを別ファイルで呼び出す、ということをしてました。

でもそうすると、ワードプレス内の動作から離れてしまい、ワードプレスの関数（ノンスのチェックなど）が使えない。だから wp-load.php を改めて読みこむ、ということをしたわけで、こうした使い方はNGということになりますね。

３）ヒアドキュメント、ナウドキュメントは使用禁止

ヒアドキュメント、ナウドキュメントはどちらも有効だしPHPの望ましい機能ではある、とコメントもらいつつも、コード内のエスケ―プ処理が検知できないので使用禁止なのだとか。

## Do not use HEREDOC or NOWDOC syntax in your plugins

（　中略　）

The primary issue is that most (if not all) codesniffers won't detect lack of escaping in code when you use HEREDOC or NOWDOC. While there are ways around this they have the end result of dashing all that readability to the rubbish pile and leaving you with a jumbled mess that won't properly be scanned.

We feel the risk here is much higher than the benefits, which is why we don't permit their use.

例えばこの行がそうだよ、と具体的にヒアドキュメントを使っている箇所がダダダダっと書かれてましたが、ヒアドキュメントを使用していたところは順次 echo を使って書き換えたりして対応。

４）データは必ずサニタイズ、検証、エスケープ

## Data Must be Sanitized, Escaped, and Validated

When you include POST/GET/REQUEST/FILE calls in your plugin, it's important to sanitize, validate, and escape them. The goal here is to prevent a user from accidentally sending trash data through the system, as well as protecting them from potential security issues.

（中略）

To help you with this, WordPress comes with a number of sanitization and escaping functions. You can read about those here:

https://developer.wordpress.org/plugins/security/securing-input/
https://developer.wordpress.org/plugins/security/securing-output/

Remember: You must use the most appropriate functions for the context. If you’re sanitizing email, use sanitize_email(), if you’re outputting HTML, use esc_html(), and so on.

（以降略）

POST で受け取ったデータを以下のような感じでいったん変数に入れ、その後にサニタイズをしていたら「出来るだけ早く行うことが必要」との指摘。

まず変数に代入：$meta_value = $_POST[ $key ];
この後サニタイズ：$meta_settings[ $key ] = sanitize_xxx( $meta_value );

これではだめ、ということで、変数代入時にサニタイズするように変更。

$meta_settings[ $key ] = sanitize_xxx( $_POST[ $key ] );

ちなみに、POSTで受け取るデータは全てサニタイズが必要、と解釈して、POSTで受け取ったノンス（nonce）もいったん変数に入れその時サニタイズするようにしたら、FYI（参考までに）として「ノンスはサニタイズしなくていいよ」というコメントが2回目のレビュー結果についてきた。

ということで、この部分は3回目のレビュー提出時に元に戻してますが、サニタイズのつもりで esc_attr() を使っていたら、そのコメントに「サニタイズするにしても esc_attr()は適切な選択じゃないよ」とも書かれてた。
（サニタイズするなら sanitize_xxx() を使え、ということですね）

５）変数はエコーされるときにエスケープする必要あり

ここは1回目の修正後の再提出でもまた指摘されてしまったところ。

## Variables and options must be escaped when echo'd

Much related to sanitizing everything, all variables that are echoed need to be escaped when they're echoed, so it can't hijack users or (worse) admin screens. There are many esc_*() functions you can use to make sure you don't show people the wrong data, as well as some that will allow you to echo HTML safely.

（中略）

There are a number of options to secure all types of content (html, email, etc). Yes, even HTML needs to be properly escaped.

https://developer.wordpress.org/plugins/security/securing-output/

プラグイン「Auto Ad Insert H」は、アドセンスのサイトから取得した広告コードをテキストエリアにベタっとコピペだけで使える、といった使い勝手を想定していて、さらにプラグインの機能の中に、自動広告のコードをヘッダに挿入する、とか、他にもグーグルアナリティクスのコード含めて、自由にコード入力してそれをヘッダに挿入できる、というようにしてました。

（自分で便利に使いたい、というのがまずあり、アドセンスで収益化するブログではプラグインをあれこれ複数使わなくてもこれ1つで事足りる、というものにしたかったため）

でもヘッダにコードを挿入する echo $code みたいな「変数をエコーする」ようになっているところは全てエスケープする必要がある、との指摘。でも普通にエスケープすると、コードではなく単なる文字列になってしまって、挿入するコードが機能しなくなる。

ここで初めて似たような機能を持っているプラグイン（もちろん公式ディレクトリに登録されているもの）はどのようにしているのか（分からないなりにも）ソースを少しチェックしてみたところ、レビューチームから指摘のあったような対応がされてなさそう。

ということは少しでも無害化すればよいのかな、と勝手に解釈して、単に余計な空行を削除するなど当たり障りのなさそうな関数を作ってそれで無害化する、みたいなたいなことで対応としたら、2回目のレビューで同じ指摘を受けた。

2回目のレビューでは以下の６の指摘（そもそもプラグイン内でユーザーに広告コード（JavaScript）を入力させてはいけない）も来て、結果として、広告コードに含まれるそのユーザー独自のデータ（アドセンスで言えばパブリッシャーIDとか広告ユニットID）だけ入力させるようにしてその部分をエコー時にエスケープ。

残りのコードは変数ではなく定数で定義して、それをエコーするように対応した。

６）CSS、JavaScript、PHPを保存させてはいけない

この指摘は2回目のレビュー時に来たもので、
上の5の指摘に対して変な対応をしたことから、単なるコードレビューというより、プラグインが行おうとしている機能面も少し含めてレビューをしていただいた、ということになるかもしれません。

## Attempting to process custom CSS/JS/PHP

We no longer permit arbitrary plugins to allow users to save custom CSS, JavaScript, or PHP within the plugin.

（中略）

As for JavaScript, we recognizing that script insertion plugins are amazing and powerful. They're also incredibly dangerous and require a high level understanding of sanitization, security, and usage. And in the case of most plugins, these are entirely unnecessary. You should never be asking users to paste in arbitrary JavaScript. Instead have them paste in the values custom to their scripts, and generate the rest on your own.

（中略）

Please remove this from your plugin.

この中に対応方法も書かれてますが、ざっくりとした意味は以下のような感じです。

今後は、プラグイン内でユーザーにCSS/JavaScript/PHPのコードを入力させることを許可しない
JavaScriptはとても危険で、高度なサニタイズやセキュリティが必要とされる
（アドセンスの広告コードには JavaScript が含まれる）
ユーザーに JavaScript をコピペさせるのではなく、ユーザー固有の値を入力させて残りのコードは自分で生成せよ
ユーザーに JavaScript を入力させている箇所はプラグインから削除してね。

最後の行（プラグインから削除してね）を見たときは、めまいがしそうになりましたが（笑）「ワードプレス公式ディレクトリに登録されるプラグイン」の方針がそうなら致し方ないですね。

広告コードをコピペすればすぐ使える、といった使い勝手とは少し離れますが、それでも解決策として「ユーザー固有の部分を入力、その他のコードは自前で作る」といったヒントも書かれているので、以下のように対応してます。

＜対応前＞
- テキストエリアで広告コードをコピペ。
  （そのコードを記事内やヘッダに挿入。つまり使い方によってはアドセンスの広告コード以外のどんなコードでも使用できる/できてしまう）
＜対応後＞
- ユーザー固有の情報のみを入力
  （アドセンスのパブリッシャーIDやグーグルアナリティクスのトラッキングIDなど）
- その他のコード部分は定数として定義。

対応前では単に echo $code; としていたところ、

echo AAIH_CODE1 . esc_attr( $pub_id ) . AAIH_CODE2;

みたいに変更。
（AAIH＿CODE1、AAIH＿CODE2 に広告コードを定義して、ユーザー固有の入力（ $pub_id）をエコー時にエスケープ処理する）

以上のような対応をした結果、無事承認されました。