Zenn
🐚

史上最悪のコードを怖いもの見たさで見よう【javascript】

2024/07/12に公開
JavaScript
form
フォーム
コード
史上最悪
tech

はじめに

こんにちは。

この世には、javascriptで書かれた史上最悪のコードがあります。
とりあえず、その史上最悪のコードを見てみましょう。

<!-- todo: put this in a different file!!! -->
<script>
function authenticateUser(username, password) {
    var accounts = apiService.sql(
        "SELECT * FROM users"
    );

    for (var i = 0; i < accounts.length; i++) {
        var account = accounts[i];
        if (account.username === username &&
            account.password === password)
        {
            return true;
        }
    }
    if ("true" === "true") {
        return false;
    }
}

$('#login').click(function() {
    var username = $("#username").val();
    var password = $("#password").val();

    var authenticate = authenticateUser(username, password);

    if (authenticate === true) {
        $.cookie('loggedin', 'yes', { expires: 1 });
    } else if (authenticate === false) {
        $("#error_message").show();
    }
});
</script>

まあ一言で言いますと、吐き気を催します。
しかも実際のサービスで稼働していたそうです。そう考えるとゾッとします。

この記事ではこの史上最悪のコードを解説していきます。

問題点

まあ、もの凄い数のツッコミどころはありますが、だいたい以下のような問題点があります。

  1. そもそもこの処理をhtmlのscriptタグ内で書く事自体がかなり危険。
  2. 一行目のコメントがそこじゃない感じがする。
  3. sqlのコマンドをhtmlのscriptタグ内で書いているので悪意のあるsqlをいくらでも実行できる。
  4. if ("true" === "true") { return false; }は文字列と文字列を比較していることは明確なのに===の演算子を使い、絶対にtrueになる条件を書き、trueではなくなぜか"true"という謎、言い出したら切りがありませんが、可読性の無さ。
  5. なぜかaccountsのループをfor..ofやforEachなどではなく、原始的な方法でする。（まあこれに関しては問題点と捉えない人もいる）
  6. 文字列がシングルクォーテーションだったりダブルクオーテーションだったりする。
  7. そもそもauthenticateUser関数からはtrueかfalseしか返ってこないのにもかかわらず、また===の演算子を使う。
  8. 非推奨になったvarを多用する。

まあ他にも問題点はあると思いますが、大体こんな感じです。

添削後

まあ「そもそもこの処理をhtmlのscriptタグ内で書く事自体がかなり危険。」と述べましたが、コードを添削できるまでは添削してみました。

<script>
/*
この関数は登録されたユーザーの中に引数の名前とパスワードに
一致するユーザーがいる場合にtrueを返し、そうでなければfalseを返す。
*/
function authenticateUser(username, password) {
    var accounts = apiService.sql(
        "SELECT * FROM users"
    );

    for (const account of accounts) {
        if (account.username === username
            && account.password === password) {
            return true;
        }
    }
    
    return false;
}


/*
もし#loginがクリックされたら、名前とパスワードを取得して
もし一致する名前とパスワードを持つユーザーがいれば
cookieにログイン済みということを記録し、
そうでなければエラーメッセージを表示する。
*/
$("#login").click(() => {
    const username = $("#username").val();
    const password = $("#password").val();

    if (username === "" || password === "") {
        $("#error_message").show();
        return;
    }

    const authenticate = authenticateUser(username, password);

    if (authenticate) {
        $.cookie("loggedin", "yes", { expires: 1 });
    } else {
        $("#error_message").show();
    }
});
</script>

まあちょっとは良くなったのではないのでしょうか。
しかし、セキュリティなどに関する部分に関しては添削できませんでした。

おわりに

どうでしたか。
とにかくこのようなコードを書くことは絶対にやめましょう。
まあ反面教師として捉えるのもありですけどね。

さようなら。

Discussion