Lesson 2 of 4 · 60 min

Review a patch as a future operator

Produce a concise review that ranks correctness, access control, and performance risks.

A code review interview is a reading exercise before it becomes a writing exercise. Establish the intended behavior, inspect the changed path, and follow data to its authoritative boundary. A useful review comment names a concrete trigger, the resulting wrong behavior, and a repair direction. It does not list every style preference you can find.

Read the success path and failure path together. A new endpoint might validate a record identifier but omit the account scope. It might update a row and return success without checking whether the row changed. It might catch every exception and return an empty list, making authorization failures look like a customer with no data. Each choice changes what users can trust.

Prioritize defects by impact and likelihood under the stated contract. Cross-account data access is more serious than a repeated local computation. A possible race needs an actual interleaving, not the word concurrency alone. Performance concerns need workload assumptions. An extra query in a loop over five fixed items has a different cost from a loop over an unbounded customer table.

GitLab's public technical interview guidance describes reviewing a self-contained change and discussing improvements. It supports practicing clear asynchronous comments. It does not establish that the fictional patch below is one of GitLab's questions. Treat employer format and original exercise content as separate evidence.

Worked example

A fictional handler performs these steps.

code

1PATCH /projects/:id21. Parse id and new title.32. Read project where id matches.43. If project exists, update title.54. Return the updated project.

The session identifies account A, but no step checks that the project belongs to A or that the user can edit it. A useful review comment is: when a logged-in user supplies another account's project ID, this lookup can return and modify that project. Scope the authorization check and mutation to the permitted account/resource relationship, and test both read and update denial. The comment identifies one actionable defect without speculating about unrelated encryption choices.

A second issue might appear if permission is checked on one version and the resource moves before mutation. The appropriate fix depends on the ownership model. Ask whether moves are allowed and whether the authorization predicate can be part of the write.

Turn a finding into a reviewable comment

A review comment should help the author reproduce the concern and choose a repair. The following artifact contrasts three versions of the same finding.

Comment	Why it helps or fails
This is insecure	No trigger, data path, or repair boundary
Please add auth	Ambiguous between authentication and resource permission
A logged-in A user can update B's project by ID; scope the mutation to current permission and test denial	Names caller, resource, effect, and verification

A concise comment can still carry uncertainty. If the repository has a middleware authorization layer that you have not yet inspected, say that the finding depends on whether that layer covers this route and resource. Then follow the route before treating the issue as confirmed. Do not ask the author to fix a defect that existing code already prevents.

Inspect a concrete patch shape

This is intentionally vulnerable pseudocode for review practice. It is not production application code.

code

1function renameProject(session, projectId, title):2  requireAuthenticated(session)3  project = database.findProject(projectId)4  if project is absent:5    return notFound6  database.updateProject(projectId, {title: title})7  return {id: projectId, title: title}

The session is authenticated, but the caller's relationship to the project is unused. A corrected design evaluates permission at the authoritative boundary and updates only an allowed resource. Depending on the data model, that may be a scoped conditional mutation or a transaction that protects the permission decision against relevant ownership changes. The exact method should match how membership and ownership can change.

The return value also deserves attention. The pseudocode returns the submitted title without checking whether the mutation actually changed a row. If the project disappears or permission changes before the write, the response can claim success without a committed update. A real implementation should inspect the mutation result and return a state consistent with it.

Prioritize a small review queue

Suppose the patch also includes an extra lookup for each tag and a variable name that differs from local convention. The review should lead with cross-account mutation because it changes access to another user's data. The unchecked update result comes next because it can misreport state. The per-tag lookup needs the tag-count bound and query cost before assigning severity. The naming issue can remain a nonblocking note if the repository standard requires it.

This ordering does not mean performance or style never matter. It means the review should allocate attention according to the concrete behavior and task context. A query loop over one million records can be severe; a query loop over three fixed tags may be acceptable. Avoid importing a generic priority ranking without the workload.

Test the repair through the contract

Use a fixture with account A user U1, account B user U2, project PA owned by A, and project PB owned by B. Verify that U1 can rename PA, cannot read or rename PB through this endpoint, and receives the defined result for a missing project. If permission can be revoked, include a schedule where revocation occurs between page load and mutation. The browser's earlier canEdit flag must not override current server authorization.

The denied response should follow the product's information-disclosure policy. Some APIs return not-found for inaccessible resources; others return forbidden when resource existence is not sensitive. The review should ask for consistency with that policy rather than insist on one status code in all products.

Misconceptions to correct

The first misconception is that a hidden button completes authorization. The caller can construct the HTTP request directly. Interface availability helps usability but cannot enforce server data access.

The second misconception is that every suspicious line deserves a blocking comment. A review should distinguish confirmed defects, conditional concerns, questions, and preferences. Overstating uncertainty as certainty wastes author attention and weakens valid findings.

Extend the exercise

Write one final comment for the unchecked mutation result. A model comment says that if the row disappears before update, the handler can return the requested title even though nothing changed; inspect the affected-row or returned-record result and test that race. Award one point for the trigger, one for the incorrect claim, and one for a verification path.

Exercise and solution

The author adds a client-side ownership check and hides the edit button. Is the original defect fixed? No. A caller can send the request directly. Write a follow-up comment limited to this gap. The model answer explains that the server still needs authorization at the data access and mutation boundary. Award two points for the boundary and one for a denied-access regression case. Do not award points for unrelated formatting criticism.

Interview probe and wrap-up

How do you express uncertainty without weakening a valid concern? State the condition you need confirmed, such as whether project IDs can cross accounts, then show the consequence if it holds. Follow up with how to rank three findings in a short review. A weak answer uses vague phrases like insecure code without a request path. Review for behavior that can fail, and make the next action obvious.

Sources

docsGitLab technical interview guidancehandbook.gitlab.com docsPostgreSQL constraintspostgresql.org docsNext.js data security guidancenextjs.org

Checkpoint

A route requires login but never checks resource permission. What is missing?

AA rule accepting any known resource ID for a logged-in user.BA resource-owner check only in the page that links to the route.CA rule trusting the account ID supplied in the request body.DAuthorization for the requested action and resource.

Checkpoint

The update changes zero rows but the handler returns submitted data as saved. What is the defect?

AValidation definitely rejected the request before SQL ran.BThe response may claim an effect that did not commit.CThe endpoint has safely made the update idempotent.DSuccessful SQL execution means the requested record was saved.

Checkpoint

A suspected defect may be covered by middleware. What should the reviewer do?

ATrace whether the actual route/resource is covered and state the condition.BDemand a second duplicate check regardless of behavior.CFile it as confirmed without inspecting the route.DDiscard it because middleware exists somewhere.

Checkpoint

An extra query runs once per tag. Which measurement pair directly estimates this loop's total database work?

AThe endpoint response size and HTTP status.BThe total endpoint latency and configured cache duration.CThe actual iteration count and measured per-lookup cost.DThe request timeout and maximum connection-pool size.

Checkpoint

Which denial status is universally required for every inaccessible resource?

AAlways 200 with an empty project.BNo single status is universal; follow the product's disclosure and API contract.CAlways 403, regardless of existence privacy.DAlways 404, regardless of contract.

Can you write a focused review comment that identifies the caller, resource, wrong effect, uncertainty, and test without confusing authentication with authorization? State the relevant identifiers, failure boundary, and evidence in your own words before selecting your confidence.

Not yetGetting thereConfident

Sources

Free to read · better with Enzo

Learn it with Enzo

Save your progress, answer the checkpoints, and let Enzo quiz you on what you just read.

Continue with Enzo Sign up free