👨‍💻 🤓 💅🏽 世界最悪のコードを解析する ✊🏻 💇 👆🏾

イタリアのFacebookページが1つあります。それは「ShittyProgrammer」を意味する「IlProgrammatorediMerda」と呼ばれています。私はこのページが大好きです。

彼らはしばしば、プログラミングに関する嫌なコードやミームを公開しています。しかし、ある日、私はそこで絶対に素晴らしい何かを見ました。

このコードは、今週の「ベストワーク」の名誉称号を獲得しています。

このコードを分解することにしましたが、間違っていることがたくさんあるので、分析のために最初の問題を選択することさえ困難です。

あなたが初心者プログラマーなら、私の資料は、このコードを書いた人たちがどんなひどい間違いをしたのかを理解するのに役立ちます。

28行のコードエラー

より便利にするために、エディターに次のコードを入力してみましょう。

<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 authenticated = authenticateUser(username, password);

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

そして...まあ、本当に-どこから分析を始めればいいのかわからない。しかし、あなたはまだ始める必要があります。このコードの欠点を次の3つのカテゴリに分類することにしました。

セキュリティ上の問題。
基本的なプログラミング知識の問題。
コードフォーマットの問題。

セキュリティ上の問題

このコードはおそらくクライアントで実行されています。これは、タグでラップされている<script>（ここでもjQueryを使用している）という事実によって示されます。

しかし、誤解しないでください。このコードは、サーバー用の場合と同じようにひどいものに見えます。ただし、クライアントで実行すると、このコードを読み取ることができるすべての人が、そのコードで使用されているデータベースにアクセスできるようになります。

関数を見てみましょうauthenticateUser：

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;
  }
}

そのコードの分析から、データベースへのSQLクエリを実行できるapiServiceメソッド.sqlを提供するオブジェクトがどこかにあると結論付けることができます。つまり、このコードを含むページを表示しているときに開発者コンソールを開くと、データベースに対して任意のクエリを実行できます。

たとえば、次のようなクエリを実行できます。

apiService.sql("show tables;");

応答として、データベーステーブルの完全なリストが返されます。そして、これはすべてプロジェクト独自のAPIを使用して行われます。しかし、すでにそこにあるものは、これが実際の問題ではないことを想像してみましょう。これをもっとよく見てみましょう：

if (account.username === username &&
    account.password === password)

このコードは、パスワードがハッシュなしでプロジェクトに保存されていることを示しています。素晴らしい動きです！これは、デバッガーを使用してプロジェクトユーザーのパスワードを確認できることを意味します。また、ソーシャルメディア、メールサービス、バンキングアプリ、その他の同様の場所で、ユーザーの大部分が同じユーザー名とパスワードのペアを使用していると思います。

このコードでCookieがどのように設定されているかも問題がありますloggedin。

$.cookie('loggedin', 'yes', { expires: 1 });

jQueryを使用して、ユーザーが認証されているかどうかをWebアプリケーションに通知するCookieを設定していることがわかりました。

注意：JavaScriptを使用してこれらのCookieを設定しないでください。

この種の情報をクライアントに保存する必要がある場合は、Cookieが最も頻繁に使用されます。そのような考えについて悪いことは何も言えません。ただし、JavaScriptを使用してCookieを設定すると、属性を構成できなくなりますhttpOnly。これは、悪意のあるスクリプトがこれらのCookieにアクセスできることを意味します。

はい、私はキーのようなものだけを知っています：値のペアがここに保存されます、'loggedin': 'yes'したがって、他の誰かのスクリプトがこのようなものを読んだとしても、それによる害はあまりありません。しかし、これはとにかく非常に悪い習慣です。

また、Chromeコンソールを開くと、いつでも$.cookie('loggedin', 'yes', { expires: 1000000000000 });。のようなコマンドを入力できます。その結果、アカウントを持っていなくても、永久にサイトにログインしていたことがわかりました。

基本的なプログラミング知識の問題

このコードに見られる同様の問題について話したり話したりすることができますが、時間があまりありません。

したがって、明らかに関数authenticateUserは非常に不十分に記述されたコードの例です。それは、それを書いた人がプログラミングの基本的な知識を欠いていることを示しています。

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;
  }
}

たぶん、データベースからユーザーの完全なリストを取得する代わりに、指定された名前とパスワードを持つユーザーを選択するだけで十分ですか？何百万ものユーザーがこのデータベースに保存されている場合はどうなりますか？

これについてはすでに話しましたが、繰り返しますが、「データベースでパスワードをハッシュしないのはなぜですか」という質問をします。

関数が何を返すかを見てみましょうauthenticateUser。私が見ることができることから、それは2つの型引数stringを取り、単一の型値を返すと推測できますboolean。

したがって、次のコードはひどく書かれていますが、無意味とは言えません。

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

通常の言語に翻訳すると、次のようになります。「Xという名前のユーザーとパスワードYはありますか？はいの場合、trueを返します。」

それでは、このコードを見てみましょう。

if ("true" === "true") {
  return false;
}

ナンセンス。関数が単に返さないのはなぜfalseですか？なぜ彼女は常に真である条件を必要とするのですか？

次に、次のコードを分析してみましょう。

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

  var authenticated = authenticateUser(username, password);

  if (authenticated === true) {
    $.cookie('loggedin', 'yes', { expires: 1 });
  } else if (authenticated === false) {
    $("error_message").show(LogInFailed);
  }
});

このコードのjQuery部分は問題ないように見えます。しかし、ここでの問題は、Cookieを使用した作業の編成ですloggedin。

アカウントがなくても、Chromeコンソールを開いてコマンド$.cookie('loggedin', 'yes', { expires: 1 });を実行するだけで、1日認証を受けることができます。

このページは、認証されたユーザーが誰であるかをどのように知るのですか？ユーザー名とパスワードの確認に合格し、個人データを表示しない人だけを対象とした、特別なものを表示するだけかもしれません。私たちはこれを知りません。

コードフォーマットの問題

スタイリングは、おそらくこのコードで最も小さく、最も重要でない問題です。しかし、それは、このコードを作成した人がその個々のフラグメントをどこかからコピーして貼り付けたことを明確に示しています。

文字列をフォーマットするときに二重引用符を使用する例を次に示します。

var username = $("#username").val();
var password = $("#password").val();

そして他の場所では、一重引用符が使用されています。

$.cookie('loggedin', 'yes', { expires: 1 });

これは重要ではないように聞こえるかもしれませんが、実際には、開発者がStackOverflowなどからコードをコピーし、プロジェクトで使用されているスタイルガイドに従って書き直していない可能性があることを示しています。もちろん、これは小さな詳細ですが、開発者がコードの動作を理解することにあまり関心がないことを示しています。この開発者は、なんらかの方法で機能するコードが必要です。

ここで、この問題に関する私の見解を明らかにしたいと思います。私は毎日Googleでコード関連のものを検索していますが、たとえば、どこかから無意識にコピーされたコードを機能させるよりも、Cookieの設定方法を理解することがはるかに重要だと思います。何らかの理由でプログラムが機能しなくなった場合はどうなりますか？コードで何が起こっているのかを理解していないプログラマーは、どのようにしてエラーを見つけることができますか？

結果

私はこのコードが偽物であると絶対に確信しています。ここで、SQLクエリの同期実行の例を最初に見ました。

var accounts = apiService.sql(
  "SELECT * FROM users"
);

通常、このようなタスクは次のように解決されます。

var accounts = apiService.sql("SELECT * FROM users", (err, res) => {
  console.log(err); // 
  console.log(res); //   
});

それらも次のように解決されます。

var accounts = await apiService.sql(
  "SELECT * FROM users"
);

メソッドapiService.sqlが同期モードで結果を返したとしても（私は疑わしいですが）、データベースへの接続を開き、要求を実行し、結果を呼び出しポイントに送信する必要があります。そして、これらの操作は（ご想像のとおり）同期して実行することはできません。

しかし、これが完全に実際のコードであっても、初心者のプログラマー、ジュニアによって書かれたと確信しています。私が最初の数週間プログラマーとして働いていたとき、私はひどいコードも書いたと確信しています（申し訳ありません：D）。

しかし、これはジュニアプログラマーのせいではありません。

これがどこかで実行されている実際のコードであると想像してみましょう。一部のジュニアは、このコードを機能させるために全力を尽くします。この開発者は、SQLクエリ、Cookie、およびその他すべてを適切に処理する方法を学ぶ必要があります。そして、この観点から、それは完全に正常です。

しかし、このプログラマーの頭は、作業を管理し、間違いを指摘し、初心者に彼の間違いを理解させる必要があります。これは、そのようなひどいコードが本番環境に到達しないという事実につながります。

実稼働環境に入るコードの品質を気にしない企業があることは確かです。コードは問題を解決しますか？もしそうなら、それは何の問題もなく展開されています。コードはジュニアによって書かれ、経験豊富なプログラマーによってテストされていませんか？生産中です！

一般的に、人生には悲しみがあります。

2020年8月8日現在の更新

この記事を書いているとき、私が分析しているコードは偽物だと思っていました。しかし、Redditで資料について話し合った後、このスレッドへのリンクが与えられました。結局のところ、このコードは1,500人のユーザーをサポートする一部の内部システムで使用されています。だから私は明らかに間違っていた。これは完全に実際のコードです。

あらゆる種類の問題に満ちた、率直に言って悪いコードフラグメントに出くわしたことがありますか？

世界最悪のコードを解析する

28行のコードエラー

セキュリティ上の問題

基本的なプログラミング知識の問題

コードフォーマットの問題

結果

2020年8月8日現在の更新

More articles: