今回は、DIフレームワークを用いたWebアプリケーションの
ソースコードに関する問題です。
特定のフレームワークではなく、あくまで一般論の問題ですので、
少なくとも、Webアプリケーションを開発された事がある方なら、
取り組むことができると思います。
問題
以下にWebアプリケーションのソースコードを示します。
コメントから動作を読み取りながらソースコードを読み、
問題を指摘し、修正してください。
ただし、問題は複数あることもあれば、全くないこともあります。
public class EmpRegisterAction { /** 従業員エンティティ（セッションで管理されているインスタンスがバインドされる） */ public Emp emp; /** 従業員サービス（シングルトンインスタンスがバインドされる） */ public EmpService empService; /** * 従業員を登録します（/emp/registerというURLにマッピングされる） * @return 遷移先のパス */ public String doRegister() { validate(emp); empService.register(emp); return "/emp/complete.jsp"; } /** * 値の検証を行います。 */ protected void validate(Emp inputEmp) { // 例外処理や、メッセージ部分は、分かりやすさのためにベタ書きしているため指摘対象外。 if (inputEmp == null) { throw new SystemException("不正なリクエストです"); } if (ValidatorUtils.checkRequired(inputEmp.getName()) == false) { throw new ValidatorException("名前は必ず入力してください。"); } // 他のチェックは割愛。 } } public class EmpService { /** 従業員テーブルアクセッサ（シングルトンインスタンスがバインドされる） */ public EmpDao empDao; /** * 従業員を登録します。 * @param emp 従業員エンティティ */ public void register(Emp emp) { empDao.insert(emp); } }
何となく、どこかにあるサンプルのような内容ですね。
コラム
解答が見えてしまわないようにするために、コラムを挟みます。
前回の問題でgetBytesやnew Stringを出しましたが、
第二引数にエンコードを指定すると、UnsupportedEncodingExceptionがスローされて
鬱陶しいことこの上ないですよね。
その対策なのか何なのか、Java6からはgetBytesやnew Stringの引数に
「Charset」という抽象クラスを渡せるようになりました。
このCharsetを使ったメソッドでは、UnsupportedEncodingExceptionがスローされません。
（2010/01/12修正）
Charsetの実装クラスは、Charset.forName()で取得するのですが
このforNameメソッドでスローされるのは、UnsupportedCharsetExceptionなどの
RuntimeExceptionになっており、わざわざキャッチする必要がありません。
Java6以降なら、こちらを使えば良いでしょうね。
また、Google製のJavaライブラリ「Guava」にも
いくつかのCharsetが定数として定義されています。
http://guava-libraries.googlecode.com/svn/trunk/javadoc/com/google/common/base/Charsets.html
あくまで Charset.forName("UTF-8") を定数として保持しているだけですが。
Guavaには、Generics対応の文字列操作のライブラリや
コレクションライブラリ（Google Collections Libraryのスーパーセット）など
たくさんのコアライブラリが用意されているようなので、
Java1.4までの案件ではJakarta Commons、
Java5以降の案件ではGuava、という使い分けになっていきそうですね。
まだ正式版がリリースされていませんが（いつ出るのかな）
PDFやAPIドキュメントなどが公開されているので、ぜひ見てみてください。
では、そろそろ解答です。
解答
今回は「Webアプリケーション」というところが最大のカギで、
問題は、以下の箇所にあります。
validate(emp); empService.register(emp);
バリデートをした後に、registerしているだけなので
一見、問題があるようには思えませんが、ここに落とし穴があります。
empは「セッションから取得するオブジェクト」ですから、
同一ユーザーが、複数リクエストを発行した際には、
このオブジェクトが共有されます。
そのため、かなりシビアなタイミングとは言え、
以下のような「バリデーションを無効化する」攻撃が可能になってしまうのです。
1. （ユーザーが）正常なリクエストを送信
2. 1のリクエストを処理するためのスレッド「Thread-1」が生成される。
3. Thread-1 : （フレームワークが）リクエストの内容をセッションのempオブジェクトに反映する。
4. Thread-1 : validateメソッドを実行し、empオブジェクトのチェックが完了する。
5. （ユーザーが）悪意を持ったリクエストを送信
6. 5のリクエストを処理するためのスレッド「Thread-2」が生成される。
7. Thread-2 : （フレームワークが）リクエストの内容をセッションのempオブジェクトに反映する。
8. Thread-1 : registerメソッドを実行する。7の値が設定されたempが登録される。
9. Thread-2 : validateメソッドを実行し、チェックエラーが発生する。
、、、少しわかりづらいですが、理解してもらえたでしょうか。
複数リクエストを発行した際に、それぞれのリクエストを処理するためのスレッドが立ち上がるものの
セッションオブジェクトである「emp」は、それぞれのスレッドで共有されているために
後から来たリクエストに、値を上書きされてしまうのです。
後から来たリクエスト（Thread-2）自体は、validateによってエラーとなるのですが
先に動いていたリクエスト（Thread-1）は、そのまま処理を継続するため、
不正な値を持ったempオブジェクトを、8で利用してしまうのです。
要するに、セッションオブジェクトに対してバリデーションを行なっても意味がない、
無効化される危険性がある、ということです。
この対策は、複数あります。
対策１：ディープコピーする（おすすめ度：低）
セッションオブジェクトをそのまま利用せず、
ディープコピーを作成してから、利用するという方法です。
Emp newEmp = new Emp(); PropertyUtils.copyProperties(newEmp, emp); // もしくは // Emp newEmp = (Emp)BeanUtils.cloneBean(emp); validate(newEmp); empService.register(newEmp);
このように、新しいオブジェクトを使って処理をすれば、値を書き換えられる心配はありません。
ただし、場当たり的な対応であり、うっかりこの処理の記述を忘れてしまっても
単体テストでは検出できないため、あまりオススメできる方法ではありません。
リリース前後、時間も予算もない場合に、
場当たり的でも良いから対応したい場合のみに使う手段、でしょうか。
対策２：画面入力オブジェクトと、サービス用オブジェクトを分ける（おすすめ度：高）
一番の王道は、この方法でしょうか。
対策１では「画面からの入力オブジェクト」と「サービス用オブジェクト」に
同じものを使っていたために、ディープコピーをし忘れても気づかない、という問題がありました。
であれば、「画面からの入力オブジェクト」と「サービス用オブジェクト」を異なったものにして、
Actionで変換するようなアーキテクチャにすれば、変換忘れを防げるはずです。
/** 従業員エンティティ（セッションで管理されているインスタンスがバインドされる） */ public EmpForm empForm; /** * 従業員を登録します（/emp/registerというURLにマッピングされる） * @return 遷移先のパス */ public String doRegister() { Emp emp = convert(EmpForm); validate(emp); empService.register(emp); return "/emp/complete.jsp"; }
画面からの入力はEmpForm、サービスで使うのはEmpとして分けています。
サービスをきちんと作っている限りは、
変換処理を忘れてempService.register(empForm)としても
コンパイルエラーが発生するので、すぐに問題に気づきます。
というか、そもそも「画面からの入力オブジェクト」と「サービス用オブジェクト」は別物なので、
似ているからと言って、共通化すべきではありません。
最初から、こうすべきなのです。
対策３：セッションで同期化する（おすすめ度：高）
対策２のようなアーキテクチャにせず、プロジェクトを進めてしまった場合の対策として、
ServletFilterを使った方法があります。
ServletFilterでセッションを同期化し、1ユーザー（1セッション）で
同時に1つのリクエストしか送信できないようにするのです。
@Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { HttpSession session = ((HttpServletRequest) request).getSession(true); synchronized (session) { chain.doFilter(request, response); } }
このように、セッションオブジェクトでsynchronizedすることで、
同一セッションで複数リクエストが来た場合に、ここで処理を待たせることができます。
多少はパフォーマンスに影響するかも知れませんが、
Ajaxなど、で「同時に複数リクエストを処理したい」場合を除き、
この方法で問題ないでしょう。
（2010/01/14追記）
コメントで指摘を頂きましたが、APサーバによっては
リクエストごとにセッションオブジェクトが変わるものがあるそうです。
言われてみれば、そういうAPサーバでなかったとしても、
シリアライズ→デシリアライズされたりすると、インスタンスは変わりますよね。
と言うことで、そういうAPサーバも考慮に入れると、以下のようなコードになります。
@Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { HttpSession session = ((HttpServletRequest) request).getSession(true); synchronized (session.<b>getId().intern()</b>) { chain.doFilter(request, response); } }
これでOK。
対策４：そもそもセッションとか使わない（おすすめ度：中）
小難しいことを考えなくて済むよう、そもそもセッションにオブジェクトを格納せず
すべてリクエスト処理だけで済ませる、という方法です。
安全で、思わぬ落とし穴もあまりないので、
私は少人数で開発する場合には、よくこの方法を使います。
セッションではなく、hiddenを中心に使う方法です。
ただ、Wicketのようなステートフルなフレームワークは当然使えないですし、
世の中のサンプルも、セッションを使うことを前提にしているものが多いので、
開発者の教育も含めた、開発コストへの影響が多少あるでしょう。
まとめ
それでは、今回の問題をおさらいしておきましょう。
- 複数リクエストが同時に来た場合の、セッションオブジェクトの値の書き換えに注意。
- 対策としては、画面入力用オブジェクトと、サービス用オブジェクトを分けて変換するのが良い。
- 次善策は、SevletFilterにて、セッションで同期化すること。
- リクエストのみのステートレスな構成にできれば、落とし穴にはまりにくい。
世の中のサンプルが、全部「対策２」の記述になってくれていて、
さらに、同時リクエストによるセッションの上書きについて書いていてくれれば
こんな問題にハマることはないと思うのですが。
そういう意味では
「なんたらスーパーサンプル」とかっていう本を出してる会社に期待、でしょうか（笑
