审阅史上最烂代码

InfoQ

摘要

本文作者对网络上流行的一段代码进行了Code Review，指出了代码存在的一些问题。
代码审阅是十分有必要的，否则容易埋下安全隐患。

正文

Facebook上有一个名为“Il Programmatore di Merda”（翻译为“ The Shitty Programmer”，中文含义为“糟糕的程序猿”）的社区，非常有意思，我经常前去浏览。

网站经常分享一些糟糕的代码以及和编程相关的话题。就在今天，我看到了一段令我难以置信的代码：

本周最烂代码

上面的代码错误太多了，以至于我不知从何谈起。

如果你是一个初级开发工程师，这篇文章会帮助你明白上述代码中存在的一些非常严重的问题，并让你引以为鉴。

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

我们的代码在某些地方有个叫做apiServices的接口，它公开了一个.sql方法，可以对数据库进行SQL操作。
这意味着，如果你在运行上述代码的浏览器上打开控制台，就可以执行各种查询，安全隐患极高。
比如你无需获得授权可以这样做：

apiService.sql("show tables;");

调用上述API，代码执行后会返回数据库的所有表名称。
好吧，我们暂且假装这不是一个严重的问题。但是请接着往下看：

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

所以，作者的意思是直接保存了用户所有的明文密码，而没有对它们进行哈希处理？

这简直不可思议！现在我可以打开Chrome浏览器的调试器，直接查看每个用户的明文密码了。

我非常确定，很大一部分用户会在社交网络、电子邮件服务、银行账户等服务中使用相同的用户名和密码，想象一下，别人可以没有任何障碍就可以拿到你的账户和密码，这得有多可怕。

作者在尝试设置登录cookie的方式也存在着问题：

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

所以按照代码的意思，作者使用jQuery设置cookie，让该cookie告知Web应用程序用户是否通过了身份验证。

好吧，千万不要使用JavaScript来设置此类cookie。

如果你有存储此类登陆信息的需求，那么使用cookie确实是最常见的解决方案，这没有什么问题！但是使用JavaScript设置它们意味着你无法设置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函数的返回值。
我们可以看到，该函数接收两个string类型的参数，最后返回一个布尔类型的值。
所以，下面的代码即使很糟糕（明文密码），但也有一定的意义：

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

这根本没有任何道理呀。
为什么该函数不去掉always-true条件判断，掉直接返回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获取属性值的代码部分没有什么问题。
问题在于它如何处理loggedin 用户的cookie。

我们之前已经讨论过这样一个问题，我可以在我的Chrome控制台输入$ .cookie('loggedin'，'yes'，{expires: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); // some error
  console.log(res); // query result
});

或者这样：

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

即使使用同步方式调用apiService.sql返回查询值（我对此表示怀疑），在内部也必须进行与数据库的连接、执行查询语句并发送返回查询结果，这些过程（你可能已经知道了）明显是不同步的。

但是，即使上面的代码不是伪造的，我也可以确信它是由初级开发人员编写的。
我刚刚开始入行写代码的一段时间里，我很确定自己为之前的公司也写过这么糟糕的代码（sorry :D）。

这个锅不能甩给初级开发人员。

让我们假设上面的代码是真实的。这里的初级开发人员正在竭尽所能实现功能。
他/她尚未开始学习如何正确处理SQL查询，cookie以及其他需要注意的技术点，这完全可以理解！

高级开发人员应该提供某种形式的指导，以确保初级开发人员可以理解他们的错误，保证这样的错误代码不会在生产环境中使用。

我也可以确认，有些公司其实并不真正在乎开发人员编写的代码质量。

代码能解决问题吗？ ——生产环境部署一下就知道了呀。
代码是由初级开发人员编写的，甚至都没有高级开发人员的批准吗？ ——部署运行一下就知道结果了呀。

哎，Shit happens！

后记

我在 Reddit对此进行了一番讨论后，一个非常给力的小伙伴分享了下面的Reddit话题：

“This JavaScript code powers a 1,500 user intranet application”

所以，是我错了。这段代码并不是伪造的！

作者介绍

Michele Riva

高级开发工程师，现就职于ViacomCBS

原文链接

Reviewing the worst piece of code ever