Chapter 5: Secure Coding and Code Review

5.1 Why.

Welcome to Chapter 5.

I am Ravindra Bagale.

Raju sat with two cups of tea and a printed diff. He still had no engineering degree. He had Chapter 4's ten names in his notebook. The diff was twenty lines. It added a refund button for a shop in the shape of Amazon, and a note field a Zomato-style partner could type.

"Guru-ji, the feature works," Raju said. "I clicked it in my lab. The refund happens. Why are we still reading?"

Guru-ji tapped a line where the refund amount came from the request body. "Working is not the same as safe. A review is a second pair of eyes before the button reaches a customer. Infosys on a client project does not merge because the author smiled. Netflix does not merge a player change because the video played once on a laptop. Paytm does not merge a bill change because the happy path returned 200."

The author of a change is proud of the happy path. The reviewer is paid, in attention, to ask what the line does when the caller is wrong, bored, or hostile. That question is secure coding. The habit of asking it on every diff is code review.

If you skip the review, the bug rides the conveyor from Chapter 1 straight into production. The cost table still applies. A comment on a pull request is cheap. A wrong refund on a live ledger is not.

This chapter is for learning only, and only inside a system you are allowed to test. The samples are small on purpose. They are not weapons. They are the lines you either write or you send back.

5.2 What.

Secure coding is writing the check in the same change as the feature. Code review is refusing the change when the check is missing. Chapter 4 taught the ten risk names and the core samples. This chapter teaches how to see those risks in a diff, in five languages, and in the tests that keep the fix from rotting.

Input validation and output encoding. Validate on the way in. Encode on the way out. They are not the same job.

Validation asks: is this value one of the values we accept? An order id is a number you issued, or a UUID you issued. A refund amount is a whole number of paise from 1 up to the captured amount. A file type is png or jpg if that is the list you published. Prefer an allowlist. A denylist of "bad characters" always forgets one.

Encoding asks: when this value is shown, will the browser treat it as text? Chapter 4's template pair is the HTML case. A URL context needs URL encoding. A JavaScript string context needs JavaScript encoding. If you are not sure, you are not ready to concatenate. Use the template engine that encodes by default.

Injection, four more interpreters. SQL was Chapter 4. The same mistake has cousins.

OS command. The bad shape puts user text in a shell string. The safe shape passes an argument list, and only if you must run a program at all. Most web handlers should not shell out.

import subprocess

# BAD shape: a shell string. Do not do this with user text.
# subprocess.run("convert " + user_name, shell=True)

# GOOD shape: the program and the arguments are separate.
subprocess.run(["convert", user_name, "out.png"], check=True)

Even the good shape is wrong if user_name is a path you did not check. Validation still applies. The argument list only stops the shell from reading extra commands.

LDAP. A directory filter is a little language. User text glued into "(uid=" + name + ")" can change the filter. Use the library's escape or parameter function. If you do not have one, do not build the filter by hand.

Template. The template source is code. User text is data. Never render(user_text). Render a fixed template and pass the text as a variable.

NoSQL. A document database still has queries. If you merge a client JSON object into a query, the client can add operators you did not mean. Pick the fields yourself. Set them from validated scalars. Do not spread the request body into the query.

Memory safety. Some languages will not let you read past the end of a list. Python, Java, JavaScript, and C# usually throw. C and C++ often do not. A missing length check becomes a crash or a quiet read of neighboring memory. Heartbleed, disclosed in April 2014 in OpenSSL, was that class of bug. This chapter's incident is a different famous miss, in a review of a branch. The rule you can use tonight: for new application code, prefer a memory-safe language. When you must review C, look at every length. Ask "who decided how many bytes?" If the answer is "the caller," you are not done.

Unsafe deserialization. You already saw pickle.loads beside json.loads. The review rule is wider than Python. Java's object serialization, .NET's BinaryFormatter, and any "load this blob back into a live object" API are the same family. If the blob crossed a trust boundary, reject that API. Use a data format. Then check the types. A JSON number is not automatically an order id you issued.

Authentication and session flaws. Review questions, not a catalog of tricks.

1. Does login issue a new session id after success?
2. Does logout delete the server-side session, not only the cookie in the browser?
3. Is the cookie HttpOnly, Secure, and SameSite, as in Chapter 4?
4. Can a token for the catalog API be replayed at the refund API? If yes, the audience check is missing.
5. Does the error text differ for "no such user" and "wrong password"? It should not. One sentence for both.

Authorization and object-level access. A role is not enough. "Customer" may refund. That does not mean this customer may refund that order. The review looks for the object check on the server. A hidden button is not a check. An admin route that only hides the link is not a check. The handler must load the order, compare the owner, and compare the role the server stored.

async function assertRefund(user, orderId) {
  const order = await DB.findOrder(orderId);
  if (!order || order.ownerId !== user.id) {
    throw new Error("forbidden");
  }
  if (order.state !== "paid") {
    throw new Error("not-refundable");
  }
  return order;
}

Call this before any money moves. Do not catch the error and continue.

Path traversal, SSRF, XXE, request smuggling. Four names a reviewer must recognize. Four stops. No procedures.

Path traversal. The client offers a file name that climbs out of the upload folder. Stop: os.path.basename, an allowlist of extensions, and a root you join only after the base name is clean. Reject a name that still contains a slash or .. after that.

SSRF. The client offers a URL the server will fetch. Stop: the allowlist function from Chapter 4. Reviewers reject a new fetch(req.body.url) with no allowlist.

XXE. An XML parser with outside entities left on. Stop: turn the feature off in the parser config. If the product does not need XML, do not parse XML.

Request smuggling. Two servers on the path disagree about where one HTTP request ends and the next begins. The impact is that a second request can be trusted as if a different user sent it. The stop is operational: one proxy and one app that agree on the HTTP version, both kept current, and no clever header split in your own code. You do not need the header trick to review this. You need to ask "do we have two parsers?" If the answer is yes, that is a design note, and it goes back to the threat model.

Race conditions and TOCTOU. A race is two true stories at once. Two customers take the last coupon. Both reads see "one left." Both writes succeed. You sold two of one.

The bad shape is check, then later act. The good shape is one statement that acts only if the rule is still true.

UPDATE coupons
SET left_count = left_count - 1
WHERE id = %s AND left_count > 0;

If the update changes zero rows, the coupon is gone. Say so. Do not read left_count in the app and then write left_count - 1 a moment later.

TOCTOU means time of check, time of use. You check a path, then you open it, and the name now points somewhere else. The stop is to make the check and the use the same step. Hold a database row lock. Or open the file and then check the descriptor you already hold. Do not check a name, sleep, and use the name again.

Money uses the same shape. Debit the balance in one statement with WHERE balance >= amount. If zero rows change, the customer cannot pay. Swiggy's last dish, Paytm's wallet, and a PayFast transfer all need this. The app's if-statement is too late.

Cryptographic misuse and weak randomness. A review looks for homemade crypto and for random used as if it were a secret.

import secrets
token = secrets.token_urlsafe(32)

random.random() is for games and shuffles of public data. It is not for a reset token, a session id, or a payment nonce. A hash of the user id is not a token either. Anyone can recompute it.

Keys do not sit in source. Modes are the ones your library documents as current. ECB mode is a review comment, not a style choice. A comparison of two hashes or two tokens should be constant-time if the language gives you that helper, so a timing difference does not become a clue. You do not need to measure timings to leave the comment.

Sensitive data in logs and errors. Log the decision. Do not log the secret. A review searches the diff for logger, print, console, and exception text. If a password, a card, an OTP, a cookie, or a full token is in the format string, the review stops. Chapter 4 showed newline stripping. Add masking: store the last four digits only if the product truly needs them, and only in the place meant to hold them.

Secure file upload and download. Upload: size limit, allowlisted type, server-chosen name, non-executable directory. Download: the caller may download this object, checked on the server, and the name you send to the browser is a base name you chose. A download route that takes a raw path is a traversal bug wearing a feature's coat.

Error handling, exception safety, fail closed. Chapter 4's A10 is the risk name. Here is the review habit. Every catch or except must say what it returns. If it returns the object, the permission, or true, it is probably failing open.

def can_refund(user, order):
    try:
        return user.id == order.owner_id and order.state == "paid"
    except Exception:
        return False

False on surprise is closed. True on surprise is a hole. A bare except that passes is also a hole, because the next line runs. Empty catches are review comments.

The security diff. Read a diff in this order.

1. New routes and new jobs. Those are entry points. They update the threat model.
2. Anything that loads by id. Look for the owner check.
3. Anything that builds a string for SQL, a shell, HTML, or a path.
4. Anything that fetches a URL, parses XML, or loads a blob into objects.
5. Anything that moves money, stock, or a coupon. Look for one atomic statement.
6. Logs and error responses.
7. New dependencies. Look at the name, the version, and whether the lockfile changed.
8. Tests. A security rule with no test will be "cleaned up" by the next author.

Language notes. The idea is one idea. The spelling changes.

Python. Parameterized queries. secrets for tokens. No pickle on untrusted bytes. subprocess with a list. Django and Flask templates escape by default if you do not mark the string safe. Do not mark user text safe.

Java. PreparedStatement and placeholders. Do not build the SQL with +. Do not enable XML external entities on DocumentBuilderFactory. Session cookies are set on the response with HttpOnly and Secure. An object check still lives in the service method, not only in a filter that checks "is logged in."

PreparedStatement ps = conn.prepareStatement(
    "SELECT id, amount FROM orders WHERE id = ? AND owner_id = ?");
ps.setString(1, orderId);
ps.setString(2, userId);

JavaScript and TypeScript. The Chapter 4 owner check and the Helmet setup still apply. Do not use eval or new Function on user text. innerHTML is a review comment when the value is untrusted. textContent is the dull, correct choice for plain text. A type on a TypeScript interface does not check a JSON body at runtime. Validate at runtime too.

Go. html/template escapes. text/template does not. Use the first for pages. database/sql with ? or the driver's placeholder. crypto/rand for tokens, not math/rand. A handler that writes err.Error() to the client is leaking the surprise. Log it. Return a short message.

.NET. SqlCommand with parameters. [Authorize] on a controller means the user is logged in. It does not mean they own this order. Add the owner check in the action. Do not use BinaryFormatter. The cookie options class has HttpOnly and Secure. Set them.

using (var cmd = new SqlCommand(
    "SELECT id FROM orders WHERE id = @id AND owner_id = @owner", conn))
{
    cmd.Parameters.AddWithValue("@id", orderId);
    cmd.Parameters.AddWithValue("@owner", userId);
}

Tests that lock the rule. A security test is a small, dull test. It does not attack a stranger. It calls your function.

def test_other_user_cannot_refund():
    order = Order(owner_id="a", state="paid", amount=5000)
    assert can_refund(User(id="b"), order) is False

def test_amount_is_not_taken_from_the_client():
    order = Order(owner_id="a", state="paid", amount=5000)
    # The server function ignores a client amount and uses the order.
    assert refund_amount(order, client_amount=1) == 5000

Add a test that a second coupon update changes zero rows when left_count is 0. Add a test that the error helper returns the short message and not the exception text.

Pull request checks. A checklist in the template is the human gate. A robot can help. A SAST comment from Semgrep or SonarQube can point at a concatenated query. The human still decides. Do not merge because the robot was quiet. Robots miss design. Chapter 3's question, "does the server know the amount?", is still a human question.

What you say in the review comment. Be specific. "Please load the order and compare ownerId before refund. Add a test where user B calls user A's order. Expected result: denied." That is a gift. "This looks insecure" is a mood. Authors can fix a gift.

5.3 How.

1. Open the diff and list every new or changed route, job, and file path before you read the style.
2. For each id that comes from the client, find the server-side owner or tenant comparison, and request changes if it is missing.
3. Reject a hidden button or a disabled field as an access-control story.
4. For each query, require a placeholder and a parameter tuple, and reject string concatenation.
5. For each process start, require an argument list, and reject a shell string built from user text.
6. For LDAP, templates, and NoSQL, require a parameter or a field list the server chose.
7. For HTML, require the encoding template, and reject a raw HTML sink for user text.
8. For file names, require a base name, an extension allowlist, and a refusal of ...
9. For outbound URLs, require the Chapter 4 allowlist, and reject a naked fetch of client text.
10. For XML, require external entities to be off, or require that XML be removed.
11. If two HTTP parsers sit in front of the app, write that down as a design question and do not invent a header trick.
12. For stock, coupons, and balances, require one conditional update, and reject a read-then-write in the app.
13. For tokens and reset links, require secrets or crypto/rand, and reject random and homemade ciphers.
14. Search the diff for log and print calls, and reject passwords, cards, cookies, and full tokens.
15. Require the error path to deny, and reject an empty catch that continues.
16. Require a new session id after login and a dead session after logout.
17. Check the cookie flags from Chapter 4 if the diff touches login.
18. Read the dependency diff and the lockfile, and send new packages back to the supply-chain rule.
19. Ask for a test: other user denied, client amount ignored, and coupon update affecting zero rows when none remain.
20. Update the threat model file if a new entry point appeared.
21. Write the review comment as wrong thing, change, and expected test.
22. Run those tests in your own lab only.
23. Do not clone a stranger's private bug and do not point tests at a host you do not own.

5.4 A real public incident.

Name. Apple's Secure Transport validation failure, often called "goto fail," CVE-2014-1266.

Date. Apple published the fix on 21 February 2014, in iOS 7.0.6 and in a matching update for older devices. The security note is article HT6147.

Source. Apple's security note "About the security content of iOS 7.0.6," last modified 21 February 2014 in the copy archived from that month. Apple's wording: an attacker with a privileged network position may capture or modify data in sessions protected by SSL/TLS. Secure Transport failed to validate the authenticity of the connection. The issue was addressed by restoring missing validation steps. The CVE id is CVE-2014-1266. Outside reviewers, reading Apple's published code that week, pointed at a repeated goto that skipped the check. This chapter shows that shape because it is the bug a review should have caught. It is not a guide to intercept traffic.

What happened. A handshake check was skipped. The code was meant to leave the function when an update step failed. A second unconditional jump sat on the next line, because the if had no braces. The signature check below it did not run. Devices trusted a connection they had not finished checking. Apple turned the check back on with the update.

The shape, as published and discussed from that code, is this:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail; /* this second line is not inside the if */

The indentation lies. Only the first goto is conditional. The second always runs. Braces would have made the lie harder to write. A review that asked "what runs when the update succeeds?" would have seen it. Tests that presented a bad certificate and expected failure would have seen it too, if that case was in the suite.

How to stop.

1. Install the vendor update. In 2014 that was Apple's iOS update and the matching computer update.
2. Do not keep a private fork of the broken function.
3. Rotate nothing mystical. The fix is to restore the check the jump skipped. Sessions that mattered could be restarted after the update.

How to prevent it again.

1. Put braces on every if, even when the body is one line.
2. Review control flow, not only the names of functions.
3. Add a test where the certificate check must fail, and fail the build if that test is missing.
4. Prefer a review checklist line: "does any jump skip a security check?"
5. Study this on the historical snippet above. Do not test it against Apple, against a classmate, or against any live TLS service.

This is for learning only, and only inside a system you are allowed to test.

5.5 A simple picture.

A pull request walks through owner, input, and query before anyone says approve.

5.6 A short animation.

The pen marks the concatenated query. The parameterized line is the one that stays. The word Fixed is the review, not a trophy.

5.7 Conclusion.

Raju drank the tea, which had gone cold, and wrote one comment in the gift shape. "The refund amount comes from the client. Load the order and use order.amount. Test that user B is denied and that a client amount of 1 still refunds 5000."

Guru-ji smiled. "That is the job. The ten names from Chapter 4 are the vocabulary. The diff is where they earn their keep. In Chapter 6 we will lock the repository itself, because a perfect handler still loses if the key is sitting in Git history."

• Validate in, encode out, parameterize every interpreter.
• The object check is on the server. The race check is one SQL statement.
• Fail closed. Log the decision, not the secret.
• Python, Java, JavaScript, Go, and .NET spell the same rules.
• Apple's 21 February 2014 note is the lesson about a jump that skipped a check.
• Your lab, your diff, your tests.

END OF CHAPTER 5 MASTER CLASS