Javascriptのまとめかたについてアドバイスお願い致します

実現したいこと

JavascriptでDom XSSの脆弱性対策を行いたい

前提

画像、短い動画をアップロード(ファイル)、名前、メッセージを送信できる掲示板を作成していて、名前、メッセージを入力する際にヤフー知恵袋のような入力可能な文字数をカウントする機能を実装するためにHTMLElement.innerHTMLを使っていたのですが、エレメント事作る必要があり＝そこでDOM要素が発生してしまうリスクがありdocument.addEventListenerを使うよう修正いたしました。

既存の文字入力が上限を超えていた場合に送信できないようにする機能とファイルアップロード箇所にデフォルトで表示されているカメラの画像をファイルアップロード時に非表示にするコードもまとめて実装する必要があるため上手くまとめることが出来ず困っております。

カメラ画像をファイルアップロード時に非表示にするコードは、イベント設定以前に要素を取得しているため、HTML解釈後に実行することが必要となっていて、window.addEventListener("load", init);でまとめたのですが、無理やりコードをつなぎ合わせた形になっております。

発生している問題・エラーメッセージ

文字入力数の条件にあわせて送信ボタンの使用権限を操作する機能を制限する機能とDOMツリー構築完了後にカメラ画像をファイルアップロード時に非表示にするコードを両立したい為、入力中の文字数チェックと、ボタン制御のための文字数チェックが別々になっている。

該当のソースコード

<script>
    function validation_submit(f) {
        const submit = document.getElementById("submit_button");
        /* 判定は逆なので、逆に渡す */
        submit.disabled = f ? false : true;
    };
    function validation_text(parts) {
        /* このpartsグループの、inputを抽出 */
        let text = parts.getElementsByClassName('input')[0];
        /* 最小チェック */
        if (text.value.length == 0) {
            return false;
        }
        /* 最大チェック */
        if (text.value.length >= text.dataset.length) {
            return false;
        }
        return true;
    };
    /* バリデーション条件判断部分 */
    function validation() {
        let parts = document.getElementsByClassName('parts');
        let submit = true;
        for (let i = 0; i < parts.length; i++) {
            if (validation_text(parts[i]) != true) {
                submit = false;
            }
        }
        validation_submit(submit);
    };
    /* 例えばのチェック */
    function init() {
        /* カメラ画像をファイルアップロード時に非表示にする */
        /* カメラ画像をファイルアップロード時に非表示にする */
        const attach = document.querySelectorAll('.attach');
        const del = document.querySelectorAll('.attachdel');
        const clear = document.querySelectorAll('.attachclear');
        const viewer = document.querySelectorAll('.viewer');
        const changeImg = document.querySelectorAll('.changeImg'); // 入力されたら消す画像
        for (let i = 0; i < attach.length; i++) {
        attach[i].addEventListener('change', () => {
        if (attach[i].files[0].size > 15 * 1024 * 1024) {
        alert('ファイルサイズが 15MBバイトを超えています');
        return;
     }
        del[i].value = "";
        viewer[i].innerHTML = "";
        if (attach[i].files.length !== 0) {
        const reader = new FileReader();
        reader.readAsDataURL(attach[i].files[0]);
     }
     });
        clear[i].addEventListener('click', () => {
        attach[i].value = "";
        del[i].value = "1";
        viewer[i].innerHTML = "";
        changeImg[i].classList.remove('hideItems');
     });
     }
        /* 文字数表示 */
        document.addEventListener('input', e => {
            if (!['name', 'message'].includes(e.target.id)) return;
            const
                t = e.target,
                m = t.nextElementSibling,
                n = t.value.length - (t.dataset.length | 0),
                c = document.createElement('span');
            c.append(Math.abs(n));
            m.style.color = n > 0 ? 'red' : 'black';
            m.replaceChildren(n > 0 ? '' : '残り', c,
                `文字${n > 0 ? '超過してい' : '入力でき'}ます。`);
            /* 毎回判定によるボタン制御 */
            validation();
        });
        /* 初回判定のボタン制御 */
        validation();
    };
    window.addEventListener("load", init);
</script>

<body>
    <form method="post" enctype="multipart/form-data">
        <div class=parts>
            <h2>名前(name)<span>※必須</span></h2>
            <input class=input type="text" name="name1" id="name" data-length="32" placeholder="未入力の場合は、匿名で表示されます" value="">
            <div></div>
        </div>
        <div class=parts>
            <h2>コメント(comment)<span class="required">※必須</span></h2>
            <input class=input type="text" name="name2" id="message" data-length="40" placeholder="荒らし行為や誹謗中傷や著作権の侵害はご遠慮ください">
            <div></div>
        </div>
        <div class="post-button"><!-- ボタンを押せなくする -->
            <button type="submit" id="submit_button" name="mode" value="confirm">表示画面へ進む</button>
        </div>
    </form>
</body>

修正前のソースコード

<script>
function lengthCheck() {
const left = this.dataset.maxlength - this.value.length;
if (left >= 0) {
this.nextElementSibling.innerHTML = 'あと<strong>' + left + '</strong>文字';
this.dataset.submit_disabled = this.value.length === 0;
} else {
this.nextElementSibling.innerHTML = '<strong>' + -left + '</strong>文字超過しています';
this.dataset.submit_disabled = true;
}
let disabled = false;
for (let i = 0; i < length_input.length; i++) {
if (length_input[i].dataset.submit_disabled === "true") {
disabled = true;
}
}
submit_button.disabled = disabled;
}
/* カメラ画像をファイルアップロード時に非表示にする */
const attach = document.querySelectorAll('.attach');
const del = document.querySelectorAll('.attachdel');
const clear = document.querySelectorAll('.attachclear');
const viewer = document.querySelectorAll('.viewer');
const changeImg = document.querySelectorAll('.changeImg'); // 入力されたら消す画像
for (let i = 0; i < attach.length; i++) {
attach[i].addEventListener('change', () => {
if (attach[i].files[0].size > 15 * 1024 * 1024) {
alert('ファイルサイズが 15MBバイトを超えています');
return;
}
del[i].value = "";
viewer[i].innerHTML = "";
if (attach[i].files.length !== 0) {
const reader = new FileReader();
reader.readAsDataURL(attach[i].files[0]);
}
});
clear[i].addEventListener('click', () => {
attach[i].value = "";
del[i].value = "1";
viewer[i].innerHTML = "";
changeImg[i].classList.remove('hideItems');
});
}
</script>

<div class="title-partial">
<h2>名前(name)<span class="required">※必須</span></h2>
<input class="length_input" data-maxlength="<?php echo MAX_LENGTH::NAME; ?>" type="text" name="namae" id="name" placeholder="未入力の場合は、匿名で表示されます" value="<?php echo $namae; ?>">
<div class="msg_partial"></div>
</div>
<div class="body-partial">
<h2>コメント(comment)<span class="required">※必須</span></h2>
<textarea class="length_input" data-maxlength="<?php echo MAX_LENGTH::MESSAGE; ?>" name="message" id="message" placeholder="荒らし行為や誹謗中傷や著作権の侵害はご遠慮ください"><?php echo $message; ?></textarea>
<div class="msg_partial"></div>
</div>

参考サイト

https://gihyo.jp/dev/serial/01/javascript-security/0006

_peanuts_

2023/09/28 16:40

2つほど質問をさせてください。 1. 修正前のコードについて、lengthCheck関数はclass属性がlength_inputなtextarea要素やinput要素にaddEventListenerで登録していると考えて良いのでしょうか？ 2. 修正前のコードについて、攻撃者のソースと、ソースコード内のシンクが結びつく箇所が見当たらないため、DOM-Based XSSの危険性はなさそうに見えるのですが、具体的にどの箇所にXSSの危険性があると考えていますか？

homepage-site

2023/09/28 17:46 編集

回答ありがとうご回答ありがとうございます、修正前のコードでaddEventListenerが抜けておりました申し訳ありません。 /* 名前とメッセージの文字数チェック */ const length_input = document.querySelectorAll('.length_input'); const submit_button = document.getElementById('submit_button'); for (let i = 0; i < length_input.length; i++) { length_input[i].addEventListener('input', lengthCheck); let event = new Event("input"); length_input[i].dispatchEvent(event); } 1,lengthCheck関数は登録しております。 2,参考サイトに説明があるように、脆弱性はinnerHTMLに設定する内容が完全制御できない内容である場合であり、修正前のコードの場合変数leftが数値であることが保証されていれば問題ないとアドバイスは以前いただいているのですが、innerHTML を使う場合 HTMLのエスケープ処理を行った方が良いと別の方からもアドバイス頂いているためHTMLのエスケープ処理は加えたほうが良いようです。安全なウェブサイトの作り方 - クロスサイト・スクリプティング https://www.ipa.go.jp/security/vuln/websecurity/cross-site-scripting.html 上記のエスケープが元でコードを書き換えたのではなく、文字入力数の条件にあわせて送信ボタンの使用権限を操作する機能を制限する機能とDOMツリー構築完了後にカメラ画像をファイルアップロード時に非表示にするコードの両方がDOMの発生源になるリスクがあるようで、そちらの問題を解消するために修正前のコードから大きく変更しております。変更した理由になるのですが、送信ボタンを活性または非活性にする this.dataset.submit_disabled = true; において初期のDOM構築の時の値と後から書き換えた物で2つの値が発生しているため、JS内変数に格納するように変更いたしました。 ※該当コード function validation_submit(f) { const submit = document.getElementById("submit_button"); /* 判定は逆なので、逆に渡す */ submit.disabled = f ? false : true; }; /* カメラ画像をファイルアップロード時に非表示にする */const attach = document.querySelectorAll('.attach'); において画面表示前に呼ばれた場合無限DOMの原因になってしまうため、修正直後は function init() {} window.onload = init; でまとめてonload以降（DOM構築後）に呼ぶことで確実にセレクタの取り込みに成功するように修正を考えていたのですが、 window.onload が動かない原因になるリスクがあるため、 function init() {} window.addEventListener("load", init); でHTML解釈後に実行するようにまとめたのが修正後のコードになります。 ※参考サイト https://gihyo.jp/dev/serial/01/javascript-security/0006 ※window.onloadを使わないほうが良い理由 https://took.jp/window-onload/

_peanuts_

2023/09/28 18:41

変数leftはこの場合、数値であることが保証されていると思います。任意の文字列が変数leftに代入される可能性があるならば、実体参照への変換などのサニタイズが必要でしょうが、このケースではその心配はなさそうです。あと、”文字入力数の条件にあわせて　…　コードの両方がDOMの発生源になるリスクがある” とありますが、どういう意味ですか？「該当のソースコード」と「修正前のソースコード」のどちらも、要素を変数に代入する場面が見られますが、どういったリスクがあるとお考えですか？

homepage-site

2023/09/29 04:59

A.回答ありがとうございます、文字入力数の条件にあわせて送信ボタンの使用権限を操作する機能を制限する機能については理由が分からないのですが、アドバイスくださった方が修正に苦労したことがあるようで、複数のDOMが発生している場合テキスト解析させる必要になるため、それが大変になってしまいJS内変数で格納するほうが簡単で良いとのことでした。 DOMツリー構築完了後にカメラ画像をファイルアップロード時に非表示にするコードについては画面表示前に置くと動いたり動かなかったりするなど機能面でも問題があるため修正する必要がありました。 /* カメラ画像をファイルアップロード時に非表示にする */ const attach = document.querySelectorAll('.attach'); 以下略 ※参考サイト https://note.affi-sapo-sv.com/js-wait-dombuild-async-defer.php

_peanuts_

2023/09/29 07:54

読解力や知識量の不足だったら申し訳ないのですが、 “複数のDOMが発生している場合テキスト解析させる必要” ←これと、 “document.querySelectorAll('.attach'); 　…　無限DOMの原因” ←これの意味がよくわかりません。確かに仰るとおり、修正前のコードではthis.dataset.submit_disabledとsubmit_button.disabledで、ボタンを操作不能にするための状態を2つのプロパティで管理していたので、それをシンプルな形に修正できたのは良かったことだと思います。また、DOM構築前にquerySelectorAllを呼び出した場合、対応する要素を取得できない場合があるため、onloadやaddEventListenerのloadでquerySelectorAllを使うようにしたのも良いことだと思います。アドバイスした方がどういった状況でテキスト解析の必要性に迫られ、修正に苦労したのかわからないですが、homepage-siteさんはinnerHTMLを過剰に怖がっているように見受けられます。

行動規範の内容に同意します

回答2件

回答

JavascriptでDom XSSの脆弱性対策を行いたい

何度かコメントでやりとりした上での回答ですが、やはり修正前のコード、修正後のコードのどちらにもDOM based XSSの危険性は見られませんでした。また、

無理やりコードをつなぎ合わせた形になっております。

についても、validation関数やその周辺の作りを見る限りでは、特に問題は感じられませんでした。validation→validation_textによる判定→validation_submitでボタンの制御、となっており、比較的意図が伝わりやすいコードでした。

入力中の文字数チェックと、ボタン制御のための文字数チェックが別々になっている

とのことですが、文字数チェック自体はvalidation_text関数の中でだけ行われ、その結果をvalidation_submit関数に渡している作りなので、別々になっているようには感じられませんでした。

補足

input要素とtextarea要素には、maxlength属性というものがあり、規定の文字数を超えないようにすることが出来ます。ブックマークレットや開発者ツールで書き換え可能なので、結局PHP側でも文字数のチェックは必要ですが、data-maxlengthという独自の属性を使うよりも簡単に実装できると思います。

投稿2023/09/29 08:14

_peanuts_

総合スコア118

homepage-site

2023/09/29 15:15 編集

アドバイスありがとうございます、修正前のコードではinnerHTMLが使われており、エレメント事作る＝そこでDOM要素が発生してしまう→間違えたら、無限DOMになってしまうためinnerHTMLを使わないか、表示の際にエスケープさせる修正が必要でした。修正後のコードは上記の説明の通りになります。修正後のコード現在使う予定なのですが、maxlength属性を使って代用できる箇所が見当たらない為、簡略化できるように見えませんでした。具体的にまとめることが出来そうな箇所を教えて頂けると助かります。 ※HTMLElement.innerHTMLについて https://gihyo.jp/dev/serial/01/javascript-security/0006

_peanuts_

2023/09/29 18:23

修正後のコードの場合、input要素にmaxlength="32"などと設定すれば、そもそもJavaScriptによる文字数チェックをせずとも、文字数の制御はできます。「残り○○文字」という表示を作りたい場合でも、data-lengthという独自の属性の代わりにmaxlengthを使うことができます。質問に対するコメントはお読みいただけたでしょうか？リンク先の文章を読みましたが、やはり「無限DOM」が何を意味するのかわからないので、教えていただきたいです。可能ならinnerHTMLに対する誤解を解きたいです。

行動規範の内容に同意します

ベストアンサー

JavascriptでDom XSSの脆弱性対策を行いたい

この質問のどこにも DOM XSS になるような要因はないように思います。
コメントにあるように、すべての innerHTML 代入が危険なわけではありません。

window.addEventListener("load", init);でまとめたのですが、無理やりコードをつなぎ合わせた形になっております。

現状のコードで期待通りの動作をしているのなら問題はないでしょう。

敢えて言うなら、

インデントがところどころおかしいため読みにくい
window の load イベントで処理すると実行タイミングがとても遅れることがあるので、document の DOMContentLoaded イベントのほうが安全
maxlength 属性を使えばかなりコードは減らせそう

投稿2023/09/29 00:37

int32_t

総合スコア20884

homepage-site

2023/09/29 14:43

アドバイスありがとうございます、window.addEventListener("load", init); を document.addEventListener("DOMContentLoaded", init);に変更いたしました。 maxlength 属性を使いコードを大幅に減らせるという事ですが、該当のソースコードのどこの部分で代用できるのでしょうか？

int32_t

2023/10/01 22:13

> maxlength 属性を使いコードを大幅に減らせる挙動がまったく同じではないのですけども、maxlength 属性を使うとそれより長いテキストはそもそも入力できないので、validation_text() 全体が不要になります。 (この質問の本題とは関係ないので、これ以上聞きたいことがあるなら別の質問を立ててください)

行動規範の内容に同意します

あなたの回答