Forem: Richard Klose

🦸‍♂️🔥 How to be a code review superhero

Richard Klose — Mon, 19 Aug 2019 10:27:20 +0000

During the last two years, I've put a lot of effort in improving the code review process at my work. While I introduced new code review tools, explained the concepts to my collegues and helped them, when they struggled, I have seen a lot of different pitfalls with code reviews. It wasn't easy to explain everyone the benefits of reviews and to bring them on a good path, where reviews become helpful for each other. I'd like to share some insights on this topic.

🎯 Code review goals

In order to learn, how you can bee good at reviews, you should understand what can be achieved with reviews. The obvious thing might be something like ensuring high code quality , but there is more:

By studying their code, you can learn from your co-developers , how they address problems.
By suggesting improvements, others can learn from you the same way.
Having a look at all reviews, helps to keep track of changes in your codebase.
Showing your code to others, helps to spread knowledge about the codebase and technologies used in the project.
And of course: Reviews help to write better code.

To achieve this goals, I follow at least these few simple things in every code review:

📝 My ultimate code review checklist

Try to understand every single change

Go through the changes line by line and make sure you know what the change means for the code. If you do not understand whats going on: Always ask! Asking questions, even if they feel dumb, is never bad. Let the author explain the changes, until you understand what's goning on.

Try to find at least one thing that could have been done better

Even if the review is just one or two simple changes, I try to find space for improvements. This not only helps to improve the code and the project, but also force me to understand the code deeply. But don't search too long, if you can't find any suggestions within a few minutes, continue the review with something else.

Always add some positive feedback

Telling people they've done a good job, helps to keep the good mood up in your team. Even if some (or most) of the changes have to be refactored or are useless, you should add some positivity to the review. This helps to ease diffucult situation, espacially if you are reviewing code of someone who tends to get angry fast.

Also, if you review some one-line-change (or even a on-character-change), adding a 👍 suits most of the time.

Check the scope of the change

Can the change be described with one simple task? Or are several changes packed into one review? The review should always contain only one taks at time. While this can be sometimes difficult to define, it is often easier to determine, if some completely non-relating task have been done together. Also, if the change is really big and spreads across several hundered lines, you should consider, suggesting to split it up into two or more smaller changes.

Try the changes in your environment

This is often optional, but most of the time I try the changes out locally in my environment, even if it passes CI. In our team, everyone has a slightly different environment, which means especially changes at build and environmental scripts can cause problems for someone else. I want to make sure that the changes work for everyone in my team.

Check for changes that affect the ecosystem around your project

If there are any changes, that might affect other software around your project, you and co-developers should make sure, they are documented and clearly communicated. This is particularly important, if the review contains breaking changes.

Don't fight about details

As long as those details don't introduce some new problems, you should not spend too much time and effort in difficult emotional discussion. The importance of positive mood in teams is often underestimated.

🤷‍♂️ What else?

For me, there is always one important thumb rule, that is always true and helpful: Reviews are not about you, they are about the code. Don't take things personally! Instead, try to stay calm and take a short break if something happens during a code review, that has the potential for personal conflicts.

Surely, there is more to look at in a code review. But these few tips helped me in the past to get a positive output from most of the reviews I have done by myself or where I helped others to do a great review. Hopefully they will do it for you too.

One last thing I'd like to share: If possible, automate as much as you can of your code review process. This will save you a lot of time. The most basic things can be integrated in every CI pipeline. For this I highly recommend to have a look at code style linters or tools like danger.

If you have any additonal and helpful tips for code reviews, I would be very happy to hear them.

Mirroring Releases from GitHub

Richard Klose — Mon, 17 Jun 2019 07:43:37 +0000

This post was originally posted on my blog.

At any point in time, developers should be able to create an reproducible and identical output from their source code. If your code relies on third party packages, this can be tricky sometimes. One problem is that some packages download precompiled releases from the web, often from GitHub, to speed up and simplify package installation. However, if the repository (or any other source) is removed, builds are not reproducible anymore, they even can't be compiled anymore.

I'd like to share my approach on how to face this issue. But before I'd like to explain the problem a bit more in detail. If you are not interested in the details, you can safely skip the next paragraph. If you just came here for a working solution, check out my GitHub repository.

Contents:

The problem in detail: How packages download binaries from GitHub
Building the caching server: Download GitHub Releases to a local server
Using the caching server: Setup your environment
Optionally package everything into a docker container

In this post I will use electron and node-sass as examples, which can be installed with npm. They are quite popular and are the packages, that caused this issue for me at first.

The problem in detail: How packages download binaries from GitHub

Compiling node-sass from source takes some time. In most cases it takes even more time than compiling or packaging the project itself that is using it. Because node-sass must be compiled individually for each platform and operating system, its install script can compile it from scratch. Let's have a deeper look in how node-sass is installed.

When running npm install node-sass, the install script from package.json is executed. (See https://docs.npmjs.com/misc/scripts for more info on npm scripts.)

{
  ...
  "scripts": {
    "install": "node scripts/install.js",
  },
  ...
}

calling the install script from package.json of node-sass

if (process.env.SKIP_SASS_BINARY_DOWNLOAD_FOR_CI) {
  console.log('Skipping downloading binaries on CI builds');
  return;
}
...
if (sass.hasBinary(binaryPath)) {
  console.log('node-sass build', 'Binary found at', binaryPath);
  return;
}
...
if (cachedBinary) {
  console.log('Cached binary found at', cachedBinary);
  ...
  return;
}

download(sass.getBinaryUrl(), binaryPath, function(err) { ... }
...

the first lines from scripts/install.js that will be executed

As you can see from the first lines of the install script, three checks are performed, before the actual binary download is started.

Should the download be skipped? If the environment variable SKIP_SASS_BINARY_DOWNLOAD_FOR_CI is set, the script exits immediately. This will ensure, that nothing is downloaded and the binary is compiled from scratch. (Please keep in mind, that this might change in the future.)
Is there already a binary, at the compile destination? If the scripts finds a binary, it wouldn't need to recompile it.
Is there already a binary in some caches? I won't go into detail about this caches, but there might already be a binary, which the script could take instead of downloading or compiling it again.

If all three checks fail, download(sass.getBinaryUrl(), ...) will be called. This is where things get interesting.

function getBinaryUrl() {
  var site = getArgument('--sass-binary-site') ||
             process.env.SASS_BINARY_SITE ||
             process.env.npm_config_sass_binary_site ||
             (pkg.nodeSassConfig && pkg.nodeSassConfig.binarySite) ||
             'https://github.com/sass/node-sass/releases/download';

  return [site, 'v' + pkg.version, getBinaryName()].join('/');
}

getBinaryUrl is the function, that decides, from where the binaries will be downloaded

getBinaryUrl() has several sources from where it can take the download URL. They are used in the following order:

npm install --sass-binary-site=<url>
The environment variable SASS_BINARY_SITE
The variable sass_binary_site from .npmrc
pkg.nodeSassConfig.binarySite, where pkg is the content of the package.json of node-sass
The default URL: https://github.com/sass/node-sass/releases/download

As you can see, downloading from GitHub is the default way for getting the binary for node-sass, however, you can manually set another download source. This enables us to be no longer dependent on GitHub. We could use any other server instead by changing the download URL, using one of those settings. But before we can do that, we need such a server, that also has all the files, that are available at https://github.com/sass/node-sass/releases/download. Of course we could download them all manually, but it would be much nicer, if we had a server, that also updates itself automatically with all the files from GitHub.

Building the caching server: Download GitHub Releases to a local server

In order to be no longer dependent on GitHub for your own build steps, we can build a local custom server for that. You will need the following tools:

An operating system that can run bash scripts. In this post, I'm using Debian, but any other Linux distribution should also work as well as macOS.
cron, for scheduling the mirror updates.
curl, for fetching the metadate for releases from the GitHub API.
jq, for parsing those metadata.
wget, for downloading the files. (We could also do this with curl, but wget can automatically create subfolders for us)
nginx, for delivering the downloaded files via http(s). (Of course, any other webserver will also work)

For the following steps, I assume that the command are executed as root. If you can't or do not want to use root, you should use sudo, when necessary.

Step 1: Install packages

Starting with a minimal debian system, the required packages must be installed first:

apt-get install -y cron curl jq wget nginx

Step 2: Create a folder and the mirror script

At first we need a place, where the downloaded artifacts will be stored. I will be using /mirror here. Create the directory with:

mkdir /mirror

Th script will just be a simply bash script. So create a new file at /opt/mirror.sh and make it executable:

touch /opt/mirror.sh
chmod +x /opt/mirror.sh

Step 3: Setup the script

Before starting with the implementation of the actual script, a bit of setup should be made, so the script can be used easily:

#!/usr/bin/env bash

REPO=${1}
MIRROR_DIR="/mirror"
SRC_URL="https://api.github.com/repos/${REPO}/releases"
DEST="${MIRROR_DIR}/${REPO}"

Some basic variables, that make the script easier to maintaincc

With REPO=${1} the script can be called with /opt/mirror.sh <repo>, so we can use it with any GitHub repository, we want. (e.g. /opt/mirror.sh sass/node-sass or /opt/mirror.sh electron/electron)
If, for some reason, the mirror directory must be changed, using MIRROR_DIR="/mirror" helps us to do this in one place, so we don't have to change every usage of the directory in the script.
The GitHub API has an endpoint for the releases of every repository with the repository name in the URL, so we will just do a GET Request against the enpoint in the next step.
Of course, we want to be able to use this script with multiple repositories, so every repository gets its own subdirectory in /mirror.

Step 4: Query the GitHub API and parse the data

For querying the data from GitHub, we simply can use curl ${SRC_URL}. The GitHub API returns JSON Data per default, that looks like this:

GET https://api.github.com/repos/sass/node-sass/releases

Response:
[
  {
    url: "https://api.github.com/repos/sass/node-sass/releases/16997851",
    html_url: "https://github.com/sass/node-sass/releases/tag/v4.12.0",
    id: 16997851,
    tag_name: "v4.12.0",
    target_commitish: "master",
    name: "v4.12.0",
    draft: false,
    author: { ... },
    created_at: "2019-04-26T10:18:21Z",
    assets: [
      {
        url: "https://api.github.com/repos/sass/node-sass/releases/assets/12251871",
        id: 12251871,
        name: "win32-x64-11_binding.node",
        browser_download_url: "https://github.com/sass/node-sass/releases/download/v4.12.0/win32-x64-11_binding.node",
        ...
      },
      {
        url: "https://api.github.com/repos/sass/node-sass/releases/assets/12251872",
        id: 12251872,
        name: "win32-x64-11_binding.pdb",
        browser_download_url: "https://github.com/sass/node-sass/releases/download/v4.12.0/win32-x64-11_binding.pdb"
      }
    ],
    ...
  },
  ...
]

Stripped down response from the GitHub API

As you can see, GitHub already gives us a lot of information about the releases of node-sass in this example. Basically it's an array of the releases, where every release itself has an array of all the assets of that particular release and their browser_download_url. That are exactly the files, we want to download and keep in our release cache. Also, every release has a tag_name which is in most cases the version number of that release, coming from the git tag of that repository. We also should make sure, that we are only mirroring actual releases and no drafts, so we should have a look at the draft attribute. All the other information about the release date, the author, etc. might be interesting, but is not relevant for us now.

This means we could strip down, the JSON like this:

[
  {
    tag_name: "v4.12.0",
    draft: false,
    assets: [
      browser_download_url: "https://github.com/sass/node-sass/releases/download/v4.12.0/win32-x64-11_binding.node",
      browser_download_url: "https://github.com/sass/node-sass/releases/download/v4.12.0/win32-x64-11_binding.pdb"
    ],
    ...
  },
  ...
]

This can easily be done by passing the output to jq:

curl ${SRC_URL} | jq '[.[] | {tag_name: .tag_name, draft: .draft, assets: [.assets[].browser_download_url]}]'

Querying the data from GitHub and passing it to jq

Let me explain the jq filter in detail:

.[] | { ... }: This means, that alle objects from the array (.[]) are taken and passed (|) into a new template.

{tag_name: .tag_name, draft: .draft, assets: [.assets[].browser_download_url]}: The template has three attributes: tag_name, draft and assets. tag_name and draft are filled with the original values from the source data, while assets is defined as a new array, containing all values of browser_download_url from every object in assets of the source data.

For better handling, I'll save that in a new variable.

RELEASES=$(curl ${SRC_URL} | jq '[.[] | {tag_name: .tag_name, draft: .draft, assets: [.assets[].browser_download_url]}]')

Step 5: Preparing the download for each release

Now, that we have an JSON array, containing only the data we need, we can interate over the objects and prepare the download of the assets, using a for loop:

for RELEASE in $(echo ${RELEASES} | jq -r '.[] | @base64'); do
  DRAFT=$(echo ${RELEASE} | base64 --decode | jq -r '.draft')
  NAME=$(echo ${RELEASE} | base64 --decode | jq -r '.tag_name')
  URLS=$(echo ${RELEASE} | base64 --decode | jq -r '.assets | .[]')
  ...
done

In this loop, we create three variables out of the JSON data:

DRAFT: contains the value of draft from the JSON data, which will be true or false.
NAME: contains the value of tag_name from the JSON data. We will use this for creating a subfolder for this particular release later. In most cases, this is the version number of the release.
URLS: contains all asset URLs as a space separated string. Wget can handle this easily.

Ruben Koster has written an excellent blog post, explaining why we have to base64 encode and decode the JSON in bash loops.

Before we can start the download, we have to do two checks. At first, we should make sure, that we skip drafts:

if [["${DRAFT}" != "false"]]; then
  continue
fi

After that, we should check, if the assets already have been downloaded.

RELEASE_DEST=${DEST}/${NAME}
if [[-d ${RELEASE_DEST}]]; then
  continue
fi

Step 6: Download all files

Now we can easily download all assets from that particular release.

mkdir -p ${RELEASE_DEST}
wget -P ${RELEASE_DEST} --no-verbose ${URLS}

Before the download is started, we make sure that the destination directory exists. Also, wget is very chatty. By using --no-verbose we can supress its output so logs are not polluted with unneccessary download progress messages.

Script completed!

#!/usr/bin/env bash

REPO=${1}
MIRROR_DIR="/mirror"
SRC_URL="https://api.github.com/repos/${REPO}/releases"
DEST="${MIRROR_DIR}/${REPO}"

RELEASES=$(curl ${SRC_URL} | jq '[.[] | {tag_name: .tag_name, draft: .draft, assets: [.assets[].browser_download_url]}]')

for RELEASE in $(echo ${RELEASES} | jq -r '.[] | @base64'); do
  DRAFT=$(echo ${RELEASE} | base64 --decode | jq -r '.draft')
  NAME=$(echo ${RELEASE} | base64 --decode | jq -r '.tag_name')
  URLS=$(echo ${RELEASE} | base64 --decode | jq -r '.assets | .[]')

  if [["${DRAFT}" != "false"]]; then
    continue
  fi

  RELEASE_DEST=${DEST}/${NAME}
  if [[-d ${RELEASE_DEST}]]; then
    continue
  fi

  mkdir -p ${RELEASE_DEST}
  wget -P ${RELEASE_DEST} --no-verbose ${URLS}

done

The complete mirror script

Step 7: Automate the cache update

With cron, we can run the script regularly, e.g. daily, to update our cache. For every repository, that should be mirrored, a new line must be added to crontab (crontab -e).

0 4 * * * /opt/mirror.sh sass/node-sass
0 5 * * * /opt/mirror.sh electron/electron

Updating a node-sass and electron cache over night (e.g. at 2:00 AM and 3:00 AM)

You should run the script at least once by hand to initialize the cache and to verify that it works.

Step 8: A simple static webserver for http access to the cache

The last thing we need is a webserver, that delivers the downloaded files via HTTP. (Hint: Although I'm not covering HTTPS setup in this post, you should consider a HTTPS configuration for you webserver.)

With nginx, we can use a simple lightweight webserver, with the following configuration placed at /etc/nginx/sites-enabled/mirror.conf:

server {
    listen 80 default_server;
    listen [::]:80 default_server;
    root /mirror;

    location / {
        autoindex on;
    }
}

After creating the file, you should reload nginx. (e.g. with systemd in Debian: systemctl reload nginx)

You can use your favorite browser and browse to the servers IP address, to verify that the webserver works.

Using the caching server: Setup your environment

Now you can use that mirror in you projects. As you might remember from the beginning of this post, there are several ways e.g. for node-sass to define the download source. I personally prefer using .npmrc but you can also use the other ways.

If you do not have a .npmrc yet, create an empty file with that name and for node-sass and electron add:

sass_binary_site=http://<ip-of-your-server>/sass/node-sass
electron_mirror=http://<ip-of-your-server>/electron/electron/v

Both packages now look for folders at that location for the desired version. We have created this folders with our script before (the $RELEASE_DEST). Note that I've added a v for electron to the end. This is because the git tags, we used for this subfolder names always start with v (e.g. v2.0.0), but the electron install script uses the version number without the leading v when searching for the download.

Optionally package everything into a docker container

I've published a full working solution of the script in a docker container on GitHub and Docker Hub.

We are using a solution with docker very similar to this at Auerswald, but with a few company specific modifications. This way, we can easily maintain and update our mirrors.

Learn from others mistakes: How not to write a PHP install script

Richard Klose — Sat, 21 Jul 2018 07:07:58 +0000

I was playing around with different photo library software latetly, mainly because I was looking for something I could selfhost in my home network. I was using Apple Photos and Lightroom before, but this always became a difficult part as soon as I wanted to share my photo library with my wife, including all tags, annotation and metadata.
I stumbled over koken and wanted to give it a try.

Koken is not open source but written in PHP, so it is more or less easy to investigate the code and have a look under the hood. Its so called "Installer" is just a PHP script with some HTML and JavaScript, so I started there.

The last modified date of the "Installer" file is 2017-02-24, which already made me wonder if it's up to date according to security. Having a look inside I discovered it's definitly not.

Outdated jQuery (1.8.0)

The first step of the installation process is a server compatibility check. This is triggered by a HTML button click, that executes a JavaScript function called test(). This function uses jQuerys $.post() function to trigger the PHP part of the server compatibility check.

...
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.8.0/jquery.min.js"></script>
...

function test() {
  ...
  $.post('index.php', {
    server_test: true,
    custom_magick_path: magick_path
    }, function(data) {
      ...
    }
  }
  ...
}

The jQuery version that is used here is 1.8.0 which is not only 6 years old by now, but also has security vulnerabilities known since January 2018. The latest 1.x version of jQuery at 2017-02-24 (the last modified date of the script) was 1.12.4 which at this point already was half a year old. I would expect at least a non-outdated version like 1.12.4, but as this is also known to have security vulnerabilities, better jQuery > 3.0.0 should be used.

Script itself must be writeable

On the server side, the server_test variable from the client is evaluated by PHP:

if ($_POST)
{
  if (isset($_POST['database_check']))
  { ... }
  else if ($_POST['server_test'])
  {
    $current_dir = dirname(__FILE__);
    $writable = is_writable($current_dir) && is_writable(__FILE__);
    ...
    if (!$writable || (...))
    {
    $permissions['fail'][] = 'This directory does not have the necessary permissions to perform the install. Set the permissions on the koken folder and the koken/index.php file to 777.';
    }
    ...
    }
    ...
}

So the first thing that this script checks is whether the current directory and the script file itself is writable by the user that runs the webserver.
Although it totally makes sense that the folder is writable, as koken will be installed here, the script itself should definitly not be writable. Enforcing this means any process that runs with the same user can modify the script at any time. We will see later, that all the download URLs are hardcoded in the script, so they now can easily be modified and pointed to some malicous URL. Of course the script can also be modified to do whatever is possible with PHP.
I assume making the script writable is necessary as it is called index.php and may later be replaced with koken's real index.php. A better solution here would be to give it another maybe more descriptive name like install.php and force the user to delete it manually after the installation. This would not only ensure that the install script can be non-writable but also would help the user to understand a little bit better what's going on here.

Weird download

The very next step during the server_test is a download of PclZip (A PHP library for handling zip files).

list($download, $download_error) = download('https://s3.amazonaws.com/koken-installer/releases/pclzip.lib.txt', 'pclzip.lib.php');

That's really strange, because the purpose of the server_test code should be, to test if the server fulfills the requirements. But why is this downloading stuff, before anything truly relevant (e.g. the PHP version) has been tested?
It's also strange, that PclZip is not downloaded from the official release downloads but from koken's aws. Comparing koken's version and the official one reveals that it is modified, but the only change is the removal of empty lines and the closing PHP tag (?>) at the end of the file.
Side note: The version of PclZip they use here (2.8) is also outdated. Thats noteworthy because the following version (2.8.1) includes a fix for PHP 5.3 which is koken's minimum required PHP version, so it totally makes sense to upgrade here.

Weird download enables MITM attacks

By looking deeper into the previous seen download function, I found the following lines:

curl_setopt($cp, CURLOPT_FOLLOWLOCATION, true);
curl_setopt($cp, CURLOPT_SSL_VERIFYPEER, false);

This means the HTTP Request for the download of PclZip (and probably every other download that will be done later during the installation) follows every HTTP 3xx redirect while not verifying the authenticity of the server that responds. Or in other words: "I'll download whatever I find behind this URL, I don't care who you are, I'll take the file anyway, even if you serve me some malicous stuff". This opens the door for man-in-the-middle-attacks.
Especially nowadays, where you can get SSL certificates for free, there is no need to disable this verification. Just don't do this.

Checking browser far too late

This is not a security isssue but more a usability problem.

... // the previous mentionend download of PclZip
$ua = strtolower($_SERVER['HTTP_USER_AGENT']);
... // several testing stuff here
if ( strpos($ua, 'msie') !== false || strpos($ua, 'internet explorer') !== false)
{
  $browser['fail'][] = 'The Koken beta does not currently support Internet Explorer. Please use Chrome, Safari or Firefox.';
}

It's okay to not support Internet Explorer, but this test can already done by a piece of JavaScript in the browser itself before any communication between the client and the server took place. This way, the script has already downloaded stuff onto your server and then enforces you to restart the process, just because you used the wrong browser.

More insecure HTTP(S)

Later during the server_test:

$loopback_test = koken_http_get('/json');

function koken_http_get($url, $host_header = false)
{
  ...
  if ($protocol === 'https')
  {
    curl_setopt($curl, CURLOPT_SSL_VERIFYHOST, 0);
    curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, false);
  }
  ...
}

Seriously, people at koken: Please do not disable SSL verifications. This enables man-in-the-middle attacks. Get a free SSL certificate instead and prevent your users from being hacked.

Conclusion

Just by looking into the install script of koken, I got a sneak peak of how the people at koken deal with security. They seem to not really care about it. I found out:

They use outdated dependencies with known bugs and security vulnerabilites.
They force the user to have a PHP script on their server writable by the user that runs PHP, so another PHP script could modify it.
They already download stuff before they know if koken will run on the server at all.
They disable the verification of SSL certificates for their downloads during installation.

There is much more going on in this script, but I'll stop here as these are the things that made me doubt most.

If this is the way people at koken write software I really cannot recommend using koken. Even if it might be a nice software for photographers, this is such a security flaw for your server.

I asked them on twitter about this a few days ago and got no response.

Richard Klose

@richard_klose

@koken Why are you disabling security for https in your production install script?
if ($protocol === 'https')
{
curl_setopt($curl, CURLOPT_SSL_VERIFYHOST, 0);
curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, false);
}

14:12 PM - 08 Jun 2018

Richard Klose

@richard_klose

No answer so far. Looks like @koken is either dead or they don’t care about security.
twitter.com/richard_klose/…

17:40 PM - 13 Jun 2018

Richard Klose @richard_klose
@koken Why are you disabling security for https in your production install script? if ($protocol === 'https') { curl_setopt($curl, CURLOPT_SSL_VERIFYHOST, 0); curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, false); }

In combination with a last modified date from more than a year ago and also no activity in their blog since a year I have truly no hopes that this situation will change.

My plea to all of you developers: Please care more about security and don't make your (probably good) software fail by doing without.