t0ul

Git and GitHub the Safe Way, Ch. 7: Reading a Pull Request Like a Security Person

· 6 min read · #git, #github, #git-and-github-the-safe-way

Part of Git and GitHub the Safe Way, a short book on git, GitHub, and the habits that keep a repository from hurting you.

Chapter 6 made a review mandatory. A review is only worth what the reviewer actually reads, and most pull requests are approved after a scroll. This chapter is how a security-minded person reads one, which turns out to be the same way a careful engineer reads one, with the order of attention changed.

Nothing here needs tools beyond the pull request page and a terminal. The examples are real pull requests on the Go book’s repository, including one that was too big.

First, the shape

Before reading a single line of code, read the numbers at the top of the Files changed tab. Pull request #5 on the Go book’s repository: 61 files changed, 2,324 lines added, 6 removed, 6 commits. Pull request #3: 113 files, 3,407 added, 1,871 removed.

For contrast, pull request #1 on this book’s practice repository: 1 file, 1 line added, 1 removed, 1 commit, a two-sentence description that says why and how it was tested.

Those numbers tell you how to review. Sixty files of new code is a day’s careful reading, not a coffee break, and both of those big pull requests should have been several smaller ones. If you are the author, the first review comment you can earn is “this is too big to review properly; can it be split?” and it is a kindness, not an insult. If you are the reviewer and it cannot be split, say how much of it you read and how, so the approval means what it says.

Then the description. Does it say why? Does it say how the author tested it? A description that is only a title is the first thing to ask about, because the diff can tell you what changed and never why.

Then, the files that bite

Not all files are equal. Before the code, open these, wherever they appear in the list:

  • Dependency manifests and lockfiles: go.mod and go.sum, package.json and package-lock.json, requirements.txt, Cargo.lock. A new dependency is new code from a stranger running inside your program. For each one added: who publishes it, how many people use it, when was it last touched, does the name look like a typo of something popular? Pull request #5 touched go.mod and go.sum; the two additions were an official golang.org/x package and a well-known pure-Go database driver. That took three minutes to confirm and is worth doing every time.
  • Anything under .github/. Workflow files run with the repository’s permissions and secrets. Watch for new permissions:, for pull_request_target, for secrets being passed to a step, for actions referenced by a floating tag instead of a commit, and for curl | sh anywhere in a run: step. Chapter 9 covers why each of these matters.
  • Configuration and infrastructure: anything named config, settings, env, docker, terraform, deploy. Small diffs, large consequences.
  • Binaries and large files. A .exe, .so, .jar, a .zip, an image you cannot preview. GitHub shows “Binary file not shown.” A reviewer cannot read a binary, so the question is where it came from and why it is not built from source.
  • Tests that got weaker. A deleted assertion, a t.Skip, a loosened expected value. Sometimes right, always worth a question.

Only then, the code.

Patterns that mean “stop”

A few things should end the scroll and start a conversation, whoever wrote the pull request:

  • A string that looks like a key or a token: long, random-looking, in a file that did not have one before. Chapter 4’s scanner catches the common formats before the commit exists; your eyes catch the rest.
  • A new URL, IP address or hostname the project did not talk to before. Where does the data go now?
  • Encoded blobs: a long base64 string, a hex-encoded payload, a “minified” file in a project that has none.
  • Code that reads environment variables, files or the network in a place that previously did neither.
  • A change described as a “refactor” whose diff adds behaviour.
  • Commits in the pull request by someone other than the author, or a commit without the Verified badge in a repository where everyone signs.

None of these is proof of anything. All of them are questions, and a pull request review is the right place to ask them, in public, politely, before the merge.

Run it before you merge it

A diff is a description of a change; the change is what happens when the code runs. For anything beyond a typo fix, get the branch onto your machine and run it:

gh pr checkout 5
go test ./...

gh, GitHub’s command-line tool, fetches the pull request’s branch and switches to it in one step; without it, git fetch origin pull/5/head:pr-5 and git switch pr-5 do the same. Run the tests, run the program, try the thing the description says it does. The Go book’s pull requests each list the exact commands the author ran, which is what lets a reviewer repeat them.

A merge button clicked on code the reviewer never ran is the most common way a ruleset gets satisfied while protecting nothing.

Writing the review

The review is a conversation on the diff. Three habits make it useful:

  • Be specific. Comment on the line, say what you see and what you expected: “This reads DATABASE_URL but the example file does not list it; is that intended?” beats “config looks off.”
  • Ask, then assert. Most surprising lines have a reason. “Why does this need network access?” gets an answer; “remove this” gets an argument.
  • Say what you did. “Checked out, ran the tests, ran the categoriser on the sample file, output matched the description” tells the author and the next reader exactly what the approval covers. “LGTM” tells them nothing.

Choose Approve only when you would be comfortable being the person who merged it. Request changes when something must change before it lands. Comment for everything else. And when you are the author, answer every comment, even with “done” and the commit that did it; the thread is the record of why the code is the way it is.

Conclusion

Shape first, then the files that bite, then the patterns that mean stop, then the code; run it; write a review that says what you checked. The ruleset from chapter 6 guarantees a pull request exists. This is what makes it worth existing.

← Back to the blog