Skip to content

fix(porcelain): report a malformed LFS pointer OID instead of panicking - #664

Open
Dev-next-gen wants to merge 1 commit into
netlify:masterfrom
Dev-next-gen:fix/lfs-pointer-oid-panic
Open

Dev-next-gen wants to merge 1 commit into
netlify:masterfrom
Dev-next-gen:fix/lfs-pointer-oid-panic

Conversation

@Dev-next-gen

@Dev-next-gen Dev-next-gen commented Sep 27, 2026 •

Copy link
Copy Markdown

What

readLFSData panics on a Git LFS pointer whose oid line has no <hash method>: prefix, so a
single malformed pointer file in the publish directory kills the whole deploy process instead of
failing it with an error.

How I ran into it

I was reading the large-media path of DoDeploy and noticed that readLFSData guards a missing
oid key but not a missing separator:

oid, ok := values["oid"]
if !ok {
	return nil, fmt.Errorf("missing LFS OID")
}

sha = strings.SplitN(oid, ":", 2)[1]

An LFS OID is <hash method>:<hash>. Without the separator, strings.SplitN returns a
one-element slice and [1] panics.

Reaching it takes no special setup: with large media enabled, walk hands every regular file in
the publish directory to readLFSData, which only checks the 41-byte version header before
parsing the rest. A file containing

version https://git-lfs.github.com/spec/v1
oid 7e56e498ccb4cbb9c672e1aed6710fb91b2fd314394a666c11c33b2059ea3d71
size 1743570

(the pointer from the existing TestGetLFSSha, minus its sha256:) takes the deploy down:

--- FAIL: TestDoDeploy_MalformedLFSPointer
panic: runtime error: index out of range [1] with length 1

github.com/netlify/open-api/v2/go/porcelain.readLFSData(...)
	go/porcelain/deploy.go:1581
github.com/netlify/open-api/v2/go/porcelain.walk.func1(...)
	go/porcelain/deploy.go:949
github.com/netlify/open-api/v2/go/porcelain.(*Netlify).DoDeploy(...)
	go/porcelain/deploy.go:408

The change

strings.Cut instead of indexing a split, and a malformed LFS OID: %s error when the separator
is absent — consistent with the missing LFS OID error right above it. A file that advertises the
LFS header but carries an unreadable pointer is now reported rather than silently deployed as if
its own bytes were the content.

Tests

Both new tests fail before the change (they panic) and pass after:

  • a TestGetLFSSha sub-case for an OID with no hash method;
  • TestDoDeploy_MalformedLFSPointer, which goes through the real entry point (DoDeploy →
    walk → readLFSData) and also asserts the deploy never reaches the API.

go test ./go/porcelain/... is green.

Unrelated to this change, go vet ./go/porcelain/ already reports four lostcancel findings on
master (including deploy.go:572); make test does not run that analyzer, so they are invisible
in CI. I left them alone to keep this PR to one fix.

Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5.

A deploy with large media enabled reads every file in the publish directory
as a possible Git LFS pointer. readLFSData took the second element of the
OID split on ":", so a pointer whose `oid` line carries no `<hash method>:`
prefix made the whole process die with "index out of range [1] with length 1"
instead of failing the deploy with an error.
@Dev-next-gen
Dev-next-gen requested a review from a team as a code owner September 27, 2026 20:42
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b0bffbfd-74fa-42ce-80c5-04e8c4da545c

📥 Commits

Reviewing files that changed from the base of the PR and between 0655676 and 380f6c1.

📒 Files selected for processing (2)
  • go/porcelain/deploy.go
  • go/porcelain/deploy_test.go
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Malformed Git LFS pointers now produce a clear error during deployment instead of causing an unexpected failure.
    • Deployments with malformed LFS pointers stop before a deploy request is made.

Walkthrough

readLFSData now checks for a colon in the LFS OID and returns a malformed LFS OID error if it is absent. The missing-OID error remains. Tests cover the parsing error and verify that DoDeploy fails without sending a deploy-creation request.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: paulo

Merge Risk: ⚪ Minimal · up to 380f6

The malformed pointer now produces an error instead of terminating deployment. No actionable merge-blocking issue remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main change: reporting malformed LFS pointer OIDs instead of panicking.
Description check ✅ Passed The description directly explains the panic, the code change, the added tests, and the reported test result.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant