前回の問題は、ちょっと内容も複雑で、取り組みづらかったかも知れませんね。
せめてフレームワークを使わず、HttpServletRequest#getSessionを使って
処理するような内容にすれば良かったか、と投稿した後になってから思ったわけですが。
今回は、その反省、、、というわけでもないですが、
Javaの標準APIのみを使った問題にしました。
問題
以下のコードの問題を指摘し、修正してください。
ただし、問題は複数あることもあれば、全くないこともあります。
/** * テンプレートを利用するメッセージ構築クラスです。 */ public class MessageBuilder { /** メッセージのテンプレート。 */ private String template; /** * リソースを読み込み、テンプレートとして利用します。 * @param resourceName リソース名 */ public MessageBuilder(String resourceName) { template = readTemplate(resourceName); } /** * テンプレートにマップの値を適用して、メッセージを作成します。 * <p> * テンプレート中に埋め込まれた ${xxx} 形式の変数を、<br> * 対応するマップの値に置き換えます。<br> * 対応するキーがマップにない場合には、置き換えは行いません。<br> * <br> * マップのキーは半角英数字のみ利用可能とし、値は任意の文字列を利用可能とします。 * * @param map 適用する値のマップ * @return メッセージ */ public String buildMessage(Map<String, String> map) { // 引数チェックは必要だけど、省略。 String result = template; for (Entry<String, String> entry : map.entrySet()) { String key = entry.getKey(); String value = entry.getValue(); result.replaceAll("${" + key + "}", value); } return result; } /** * テンプレートをリソースより読み込みます。 * @param resourceName リソース名 * @return テンプレート文字列 */ private String readTemplate(String resourceName) { String content = null; InputStream is = MessageBuilder.class.getResourceAsStream(resourceName); // 読み込み処理は省略。 return content; } } /** * メッセージ構築クラスを利用する、サービスクラスの例です。 */ public class MailService { private MessageBuilder mailBuilder; public MailService() { mailBuilder = new MessageBuilder("/mailTemplate.txt"); } public void doSomeService() { Map<String, String> map = new HashMap<String, String>(); // ... 検索や、mapへの値代入処理など。 mailBuilder.buildMessage(map); } }
今回は、簡易テンプレートエンジンのような、メッセージ構築クラスです。
そもそもテンプレートをコンパイルせず、replaceAllを使っている辺りに
性能面での懸念があるかも知れませんが、
今回は性能については検討対象外としてください。
コラム
前回話題にしたセッションの問題は、
正直なところ、かなり多くのシステムに潜んでいる、セキュリティ問題だと思います。
ただ
・攻撃手法が明確にならない（共通的な攻撃手法がない）
・攻撃可能だとしても、タイミングがシビアである
などの理由から、
あまり注目が集まっていないのかな、と思います。
逆に注目が集まってしまうと、
ステートフルなフレームワーク系とかも
軒並み危ないんじゃないのかな、と思うんですが。
では、そろそろ解答です。
解答１
まずは、脊髄反射で気づいて頂けたでしょうか、
FindBugsに怒られるレベルの間違いがあります。
result.replaceAll("${" + key + "}", value);
Stringはイミュータブルであり、replaceなどを呼び出しても
元の値は変わらないため、戻り値を利用する必要があります。
result = result.replaceAll("${" + key + "}", value);
はい、これでOK。
、、、って、もちろん、これだけでは終わりません。
解答２
String#replaceAllは、正規表現による置換を行なうメソッドです。
そのため、${key} という文字列は正規表現として解釈されるのですが、
正しい正規表現の書式となっていないため、実行時例外が発生してしまいます。
文字列として扱いたいのであれば、きちんとエスケープする必要があります。
result = result.replaceAll("\\$\\{" + key + "\\}", value);
replaceAll以外にも、splitやmatchesなど、
Stringのメソッドでは正規表現を利用するものが多いため、
きちんとJavadocで確認しておく必要があります。
大抵は単体試験で検出できるのですが、たまにすり抜けるものがあるので、要注意です。
解答３
さて、問題はまだあります。
replaceAllが正規表現により置換だと言いましたが、
これは第一引数だけでなく、第二引数にも影響します。
replaceAllのJavadocには、以下のように記述されています。
指定された正規表現に一致する、この文字列の各部分文字列に対し、指定された置換を実行します。
このフォームのメソッド呼び出し str.replaceAll(regex, repl) では、次の式と正確に同じ結果が得られます。Pattern.compile(regex).matcher(str).replaceAll(repl)
ということなので、一番最後のMatcher#replaceAllのJavadocも確認してみましょう。
置換文字列内でバックスラッシュ (\) とドル記号 ($) を使用すると、それをリテラル置換文字列として処理した場合とは結果が異なる場合があります。ドル記号は、先に説明したとおり、先方参照された部分シーケンスへの参照として処理される場合があり、バックスラッシュは置換文字列内のリテラル文字をエスケープするのに使用されます。
なんか小難しく書いていますが、
要するに、\ や $ を、あまり意識せずに第二引数に含んでしまっていると、
期待通りの置換が出来なかったり、例外が発生したりすることがあるのです。
今回、MessageBuilder#buildMessageを呼び出す人は、
たとえば "$1,000,000の夜景！！" というメッセージを埋め込みたい時に
わざわざ "\\$1,000,000の夜景！！" なんてエスケープしないでしょうから、
きちんと事前にエスケープしてあげるべきです。
実は、このエスケープのためだけに存在するメソッドが
Javaの標準APIで用意されているので、これを使いましょう。
result = result.replaceAll("\\$\\{" + key + "\\}", Matcher.quoteReplacement(value));
、、、随分と見通しが悪くなってきましたね。
というか、そもそもやりたいことは文字列の置換であって、正規表現は必要ありません。
ただの文字列置換メソッドがJava標準APIにあれば良いんですが、残念ながら存在しないので
Jakarta Commons LangのStringUtilsを使うと良いでしょう。
これを使えば、以下のように書き直せます。
result = StringUtils.replace(result, "${" + key + "}", value);
これで、随分と見やすくなりました。
要するに、普通の文字列操作を行ないたいなら、
StringUtilsを使っとけ、ってことですね。
