Bug（バグ） #2233
完了
max-resultsの値に整数以外の値を入力した際に全件表示されてしまう
Mutsumi Imamura さんが13年以上前に追加.
約13年前に更新.
説明
Overview (現象)¶
max-resultsの値に整数以外の値(例えばhoge)を入力した際に全件表示されてしまうなどの問題がある。
異常な値が入力された場合、デフォルトの件数を表示するなどの挙動が望ましいと思われる。
再現バージョン¶
- OpenPNE3.6beta11-dev
- opWebAPIPlugin0.5.0
再現手順¶
/api.php/feeds/diary?max-results= に下記のような値を入力し実行する
* max-resultsの値を空白に設定した場合
* max-resultsの値を全角スペースに設定した場合
* max-resultsの値を半角スペースに設定した場合
* max-resultsの値を全角文字に設定した場合
* max-resultsの値を半角文字に設定した場合
* max-resultsの値をアルファベットに設定した場合
* max-resultsの値を記号に設定した場合
↑の場合に全件表示されるのは
(前のバージョンである0.4.0の場合は、異常な入力値はエスケープされデフォルトの件数が表示されます。)
* max-resultsの値を小数点の数値に設定した場合
↑例えば3.14とすると、3件表示される（max-resultsの値は整数にしかなりえないので問題は無い）
* max-resultsの値を負の数値に設定した場合
↑ホワイトアウトしてしまいます
再現手順ではdiaryでしか試していないが他のAPIでも再現する可能性がある。
Causes (原因)¶
バグが発生した原因を記入
Way to fix (修正内容)¶
修正内容を記入
- 優先度 を Normal（通常） から High（高め） に変更
- 3.6 で発生するか を Yes にセット
lib/api/opAPI.class.php の以下にて入力値を評価していないためと思われます。
158
159 $this->query->limit($this->getParameter('max-results', 25));
160 $this->query->offset($this->getParameter('start', 1) - 1);
161
以下のように修正する予定です。
- $this->query->limit($this->getParameter('max-results', 25));
- $this->query->offset($this->getParameter('start', 1) - 1);
+ $maxResults = $this->getParameter('max-results', 25);
+ $this->query->limit(is_numeric($maxResults) ? $maxResults : 25);
+ $start = $this->getParameter('start', 1);
+ $this->query->offset(is_numeric($start) ? $start - 1 : 0);
- ステータス を New（新規） から Pending Review（レビュー待ち） に変更
- 進捗率 を 0 から 50 に変更
- ステータス を Pending Review（レビュー待ち） から Pending Testing（テスト待ち） に変更
- 進捗率 を 50 から 70 に変更
- ステータス を Pending Testing（テスト待ち） から Rejected（差し戻し） に変更
- 進捗率 を 70 から 50 に変更
テスト実施しましたが、修正が必要な点がありましたので確認お願いします。
max-resultsの値を負の数値に設定して確認¶
- 試験手順
1. 管理画面にログイン後、WebAPIプラグインページ(/pc_backend.php/opWebAPIPlugin)にアクセスする
2. Auth typeを「IP Address」に設定後、Ip listに自分のIPアドレスを入力する
3. pc_frontend側のURLの語尾に「/api.php/feeds/diary?max-results=xx」を追加する
4. 手順3で追加したURLの「xx」の部分に負の数値(例:－1985)を入力する
- 修正方針
負の値を入力した際に、レスポンスを返すように修正お願いします。
負の値への対応が抜けていたため、真吾さんと相談の上、max-resultsの負の値に関してはデフォルト値を代入するように実装します。
文字列を扱う q, category 以外のパラメータの場合も調べたのでメモ。
(基本的にSQLインジェクション等の対策はできていると思います。)
orderby: メンバ変数 orderByTable に配列キーがない場合、ソートオーダーに関する処理を行わない
sortorder: orderby が正常値の場合、ascend ならASC 他の値の場合全て DESC
published-min, published-max, updated-min, updated-max: 入力値の評価はなく、strtotime, date 関数をへてDQLへ代入
author: getMemberIdByUrl にて返されたIDをDQLへ代入
- ステータス を Rejected（差し戻し） から Pending Review（レビュー待ち） に変更
0の場合もデフォルト値を代入するよう、1未満の値の場合で条件式を追加し pull request しました
- ステータス を Pending Review（レビュー待ち） から Pending Testing（テスト待ち） に変更
- 進捗率 を 50 から 70 に変更
- ステータス を Pending Testing（テスト待ち） から Rejected（差し戻し） に変更
- 進捗率 を 70 から 50 に変更
max-resultsの値を負の数値に設定して確認したのですが、空白ページが表示されるので修正お願いします。
0を入力した際に、デフォルト件数が表示されることは確認済みです。
- ステータス を Rejected（差し戻し） から Pending Review（レビュー待ち） に変更
レビュワーとしての指摘ではないため、ステータスの変更はしませんが、今回の修正について気になる点があるのでコメントしておきます。
クエリ文字列として数値を受け取るような箇所で、特に件数などの（符号なしの非負整数を前提とした）パラメータ値をバリデートしたい場合、 is_numeric() ではなく ctype_digit() を用いるべきではないでしょうか。
is_numeric() を用いるにしても、この違いに関する（ is_numeric() が true と判定する）値についての考慮がされているべきです。
参考までに、同様の指摘をした別チケットのコメントを転載しておきます。
ところで、2系の「 is_numeric() が偽となるかどうか」をチェックしているのは意図がよく分かりません。これでは 1eA が通るのに 1e1 が弾かれ、 0xfg が通るのに 0xff が通らない状態になっています。ここでは「 ctype_digit() が偽となるかどうか」をチェックするべきでしょう。この違いのことは以下では触れません（「数字のみではない」ことをチェックしていると考えておきます）。
http://redmine.openpne.jp/issues/2356#note-3
- ステータス を Pending Review（レビュー待ち） から Pending Testing（テスト待ち） に変更
- 進捗率 を 50 から 70 に変更
- ステータス を Pending Testing（テスト待ち） から Fixed（完了） に変更
- 進捗率 を 70 から 100 に変更
テスト完了致しました。
問題はありませんのでFixedいたします。
他の形式にエクスポート: Atom
PDF
