Bug（バグ） #2233
完了
max-resultsの値に整数以外の値を入力した際に全件表示されてしまう
100%
説明
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 (修正内容)¶
修正内容を記入
Hiroshi Kato さんが13年以上前に更新
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);
Kousuke Ebihara さんが13年以上前に更新
- ステータス を New（新規） から Pending Review（レビュー待ち） に変更
- 進捗率 を 0 から 50 に変更
https://github.com/ebihara/opWebAPIPlugin/pull/7 で pull request をいただいているのでレビュー待ちにします。
Kousuke Ebihara さんが13年以上前に更新
- ステータス を Pending Review（レビュー待ち） から Pending Testing（テスト待ち） に変更
- 進捗率 を 50 から 70 に変更
Yuma Sakata さんが13年以上前に更新
- ステータス を 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)を入力する
- 試験結果
空白ページが表示されます。
- 修正方針
負の値を入力した際に、レスポンスを返すように修正お願いします。
Hiroshi Kato さんが13年以上前に更新
負の値への対応が抜けていたため、真吾さんと相談の上、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へ代入
Hiroshi Kato さんが13年以上前に更新
- ステータス を Rejected（差し戻し） から Pending Review（レビュー待ち） に変更
0の場合もデフォルト値を代入するよう、1未満の値の場合で条件式を追加し pull request しました
Kousuke Ebihara さんが13年以上前に更新
- ステータス を Pending Review（レビュー待ち） から Pending Testing（テスト待ち） に変更
- 進捗率 を 50 から 70 に変更
Yuma Sakata さんが13年以上前に更新
- ステータス を Pending Testing（テスト待ち） から Rejected（差し戻し） に変更
- 進捗率 を 70 から 50 に変更
max-resultsの値を負の数値に設定して確認したのですが、空白ページが表示されるので修正お願いします。
0を入力した際に、デフォルト件数が表示されることは確認済みです。
Hiroshi Kato さんが13年以上前に更新
- ステータス を Rejected（差し戻し） から Pending Review（レビュー待ち） に変更
max-results, start に1が渡された際、デフォルト値を代入していたため、修正を行いました。
pull request 済みです。
https://github.com/ebihara/opWebAPIPlugin/pull/9
Minoru Takai さんが13年以上前に更新
レビュワーとしての指摘ではないため、ステータスの変更はしませんが、今回の修正について気になる点があるのでコメントしておきます。
クエリ文字列として数値を受け取るような箇所で、特に件数などの（符号なしの非負整数を前提とした）パラメータ値をバリデートしたい場合、 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
Yuya Watanabe さんが13年以上前に更新
note-14の指摘について反映したものを下記のコミットで反映しました
https://github.com/ebihara/opWebAPIPlugin/commit/d797b71be89d9318a7c40bb2c53405fa32c9d3e3
Kousuke Ebihara さんが約13年前に更新
- ステータス を Pending Review（レビュー待ち） から Pending Testing（テスト待ち） に変更
- 進捗率 を 50 から 70 に変更
