A merge conflict resolution silently dropped a teammate's security fix

A security researcher reported that our file upload endpoint accepted file names with path traversal sequences such as ../../config/app.yml. We had fixed exactly that bug two weeks earlier, verified it in production, and added a test. The researcher had found the original report's fix missing.

The function in the current main:

def safe_upload_path(base: Path, filename: str) -> Path:
    return base / filename

The fix, as merged two weeks earlier in commit 5e2a9d1:

def safe_upload_path(base: Path, filename: str) -> Path:
    name = Path(filename).name                      # strip directory parts
    if not name or name in {".", ".."}:
        raise InvalidFilename(filename)
    target = (base / name).resolve()
    if not target.is_relative_to(base.resolve()):
        raise InvalidFilename(filename)
    return target

Nobody had reverted it. There was no revert commit, and git log on the file showed no commit that looked like it changed this function back.

Finding when the lines disappeared

git log -S finds commits that add or remove a given string:

git log --oneline -S "is_relative_to(base.resolve())" -- app/uploads.py

5e2a9d1 Fix path traversal in upload filenames

Only the commit that added it. A plain git log -S does not look inside merge commits by default, because merges have more than one parent and Git does not show a diff for them unless asked. With -m, Git diffs a merge against each of its parents:

git log --oneline -m -S "is_relative_to(base.resolve())" -- app/uploads.py

c41b7f0 (from 9a8e112) Merge branch 'feature/bulk-import' into main
5e2a9d1 Fix path traversal in upload filenames

There it was. The merge of feature/bulk-import removed the line, as seen from the main side.

git show c41b7f0 -m --first-parent -- app/uploads.py

diff --git a/app/uploads.py b/app/uploads.py
@@ -18,11 +18,7 @@ def save_upload(...)
 def safe_upload_path(base: Path, filename: str) -> Path:
-    name = Path(filename).name                      # strip directory parts
-    if not name or name in {".", ".."}:
-        raise InvalidFilename(filename)
-    target = (base / name).resolve()
-    if not target.is_relative_to(base.resolve()):
-        raise InvalidFilename(filename)
-    return target
+    return base / filename

Relative to main before the merge, the merge commit reverted the fix.

How a merge drops a change

feature/bulk-import had been open for six weeks. Early on, its author refactored app/uploads.py, moving code around and touching safe_upload_path's neighbouring lines. Meanwhile, the security fix landed on main. When the feature branch was merged, Git found that both sides had changed the same region and reported a conflict:

<<<<<<< HEAD
def safe_upload_path(base: Path, filename: str) -> Path:
    name = Path(filename).name
    ...
=======
def safe_upload_path(base: Path, filename: str) -> Path:
    return base / filename
>>>>>>> feature/bulk-import

The developer resolving it was familiar with the feature branch's version and not with the fix. Seeing two versions of a function, they kept "theirs", the branch's version, assuming the HEAD side was older code that the refactor replaced. Nothing failed: the code compiled, and the path traversal test had been moved by the same refactor into a file the merge also resolved in favour of the branch, which did not have it.

Why review missed it

The pull request for the feature showed the diff between the branch and main's merge base, which is how most code review tools display a branch. The conflict resolution happened in the merge commit itself, created when the PR was merged locally and pushed. Review tools generally show merge commits' diffs only in a limited way, or not at all, and nobody looked at it.

Even git show c41b7f0 without options would have hidden it. For merges, Git's default is a combined diff, which shows only lines that differ from all parents. A line taken unchanged from one parent, even if it erases the other parent's change, does not appear. The conflict resolution looked like no change at all.

Environment

1. Restore the fix

We cherry-picked 5e2a9d1 onto main, re-added the test, and checked the rest of the merge for anything else lost. git diff between the merge's first parent and the merge, filtered to files that main had changed since the merge base, gave a short list to review by hand:

base=$(git merge-base c41b7f0^1 c41b7f0^2)
git diff --name-only "$base" c41b7f0^1 > main_changed.txt    # files main changed while the branch was open
git diff c41b7f0^1 c41b7f0 -- $(cat main_changed.txt)        # what the merge did to them

Two other small changes from main had been dropped in the same merge: a log message fix and a dependency version bump. Both restored.

2. Stop merging locally, merge through the PR

Merges to main now go through the hosting platform's merge button only; direct pushes to main are blocked by branch protection. When a branch has conflicts, the author resolves them on the branch (by merging or rebasing main into it), pushes, and the resolution then appears in the PR diff for review, before it reaches main.

3. Review conflict resolutions explicitly

When resolving conflicts, we ask authors to:

• Look at both sides with context, not just the markers. git checkout --conflict=diff3 (or setting merge.conflictStyle = zdiff3) shows the merge base too, so you can see what each side changed rather than just two versions:

<<<<<<< HEAD
    name = Path(filename).name
    ...
||||||| base
    return base / filename
=======
    return base / filename
>>>>>>> feature/bulk-import

With the base visible, it is obvious the branch did not change this function at all; only HEAD did. The correct resolution keeps HEAD.

• Mention conflict resolutions in the PR description, with the files involved.

4. git rerere for repeated conflicts

For long-lived branches that are repeatedly updated from main, git rerere ("reuse recorded resolution") records how you resolved a conflict and reapplies it the next time the same conflict appears:

git config --global rerere.enabled true

This does not prevent a wrong resolution, but it prevents having to resolve the same conflict many times, which is where tired, hasty resolutions come from.

5. Keep branches short

The underlying cause was a six-week branch touching a file that others changed too. We now aim for branches that live a few days, use feature flags to merge unfinished work safely, and treat any branch older than two weeks as a risk to discuss at stand-up.

1. The test is back and protects the fix.

@pytest.mark.parametrize("name", ["../../config/app.yml", "..", "/etc/passwd", "a/../../b"])
def test_rejects_traversal(tmp_path, name):
    with pytest.raises(InvalidFilename):
        safe_upload_path(tmp_path, name)

2. A CI check for lost commits. A nightly job lists commits on main from the last 30 days that touched files under app/ and checks that their added lines still exist in main unless a later commit explicitly changed them (using git log -m -S for each added security-relevant line, for commits tagged security). It reported nothing after the restoration.

3. Branch protection. We confirmed that a direct git push origin main is rejected and that merges require an up-to-date branch with passing checks.

4. Security response. We confirmed with the researcher that the fix was effective again, checked access logs for traversal patterns in upload names during the two weeks the fix was missing (none had succeeded in reading files outside the upload directory, because the web server process could not read the config directory), and thanked them in our security acknowledgements.

Takeaways

• A conflict resolution can silently revert another change, and a merge commit's default diff hides it.
• Use git log -m -S to find which merge removed a line, and git show -m --first-parent to see what a merge did to main.
• Resolve conflicts on the branch so the resolution is reviewed, with zdiff3 to see the base.
• Keep branches short; long-lived branches make bad resolutions much more likely.

References: git log, -S and -m options, git show and combined diff format, git rerere, git merge conflict styles.