Code Issues Pull Requests Actions Security InsightsSettings

fix: clone URLs without ".git", and file writes that silently corrupted non-base64 content #5559

Merged⚡ AI-generatedXSccantynz wants to mergefix/clone-url-and-content-encoding→mainopened 5d ago

ccantynzcommented 5d ago

Two independent defects, both found by cross-checking notes with sessions working on other platforms that consume this API. Both were reachable from real, customer-facing paths.

1. Clone and push 404'd unless the URL carried ".git"

Git never appends .git to a remote URL — git clone https://host/o/r requests /o/r/info/refs?service=git-upload-pack verbatim. We only registered /:owner/:repo.git/..., so the URL a human copies out of the browser address bar failed on every clone and push. GitHub and Gitea both serve either spelling.

Measured on a downstream instance running this code:

/ccantynz-alt/Vapron.git/info/refs?service=git-upload-pack -> 401  (auth challenge)
/ccantynz-alt/Vapron/info/refs?service=git-upload-pack     -> 404

An origin configured without the suffix failed every push with "repository not found". On that box it had been stranding unpushed commits on a single workstation — and that repo's GitHub copy was deleted by choice, so the platform is its only source of truth.

4 Smart HTTP routes + 6 LFS routes now accept both spellings
git rate limiter extended to the new spelling (it was mounted on the suffixed path only, so suffix-less clone traffic would have run unlimited)

The part that needed care. app.tsx mounts a private-repo gate on /:owner/:repo/* that answers an HTML 404 for a repo the viewer cannot see. Git traffic used to miss that gate purely by URL shape — :repo captured "name.git", which resolves to no repo. Suffix-less git paths do match it, and an HTML 404 there would replace the 401 challenge a git client needs in order to retry with credentials — breaking clone and push of private repos for legitimate collaborators, while still looking secure from the outside. The exemption is now stated explicitly instead of inherited from a URL quirk; the git routes' own gitAccessGate keeps the privacy behaviour identical. Tests assert content-type: text/plain precisely so that regression cannot pass silently.

2. File-write APIs silently corrupted non-base64 content

PUT /api/v2/repos/:o/:r/contents/:path and its GitHub-compat twin both did:

try { bytes = Buffer.from(body.content, "base64") } catch { return 400 }

Buffer.from(s, "base64") never throws. It discards every character outside the base64 alphabet and decodes the remainder — so the catch was dead code and the 400 could not fire. A caller sending a plain UTF-8 string got 201 Created and a file of binary garbage:

"<!doctype html>\n<html>..."   57 bytes in  ->  29 bytes of "v\xb7-\xca..."

The write reported success, so the corruption stayed invisible until someone opened the file. This was live on a publish path that ships generated sites.

Sending a raw string is the natural mistake here: POST /git/blobs on this same API accepts encoding: "utf-8" | "base64" and defaults to utf-8. The two write routes disagreed about what content means — the usual two-sources-of-truth shape.

content stays base64 by default (GitHub's semantics, and what our own /api/v2 endpoint map promises). Flipping the default would corrupt the callers who are currently correct, by writing their base64 out as literal text. Instead: callers may state encoding explicitly using the vocabulary /git/blobs already accepts, and base64 payloads are now validated, so a plain string is a loud 400 that names the fix. Unpadded and line-wrapped base64 stay accepted, so no currently-working caller can break.

Verification

15 new tests, all passing
bun run typecheck clean
Full suite: 5152 pass / 11 fail — all 11 pre-existing, confirmed by re-running against a stashed clean tree (no local DATABASE_URL, plus existing lfs-store.ts empty-catch debt)
Affected-file runs show 87 pass vs 80 on clean tree = exactly the 7 new tests, with the pre-existing failure count unchanged at 23

Not changed — flagging for a decision

PUT /contents/:path is strictly owner-only (user.id !== owner.id -> 403) and is not collaborator-aware, unlike the workflow-dispatch endpoint beside it which resolves real access via resolveRepoAccess. A write collaborator gets 403 on file writes. That is an ACL inconsistency, not obviously a bug, so I did not change auth semantics on my own initiative.

🤖 Generated with Claude Code

ccantynzAI Reviewcommented 5d ago

AI Triage

(no summary)

Priority: medium Risk area: mixed

Suggested labels: (no label suggestions) Suggested reviewers: (no reviewer suggestions)

Suggestions only — nothing has been applied. The PR author stays in control.

gluecron[bot]🤖 botAI Reviewcommented 5d ago

AI review unavailable

The platform's AI balance is exhausted, so AI generation is temporarily unavailable. Nothing was lost. You can queue this as a repair for the internal agent from the repository's Health page, or try again once the balance is restored. The PR is otherwise unchanged.

ccantynzcommented 5d ago

Correction to the PR description — one motivating claim was wrong.

The description says the suffix-less 404 "had been stranding unpushed commits on a single workstation." That is not true and should not be read as part of the case for this change.

Commit cb3ff9ec on that machine was on its bare repo the whole time (refs/heads/fix/vercel-project-scoped-token, verified over Tailscale). It has since merged to that platform's main as 080ac089 and deployed. The push failure that prompted this investigation was a local DNS resolution problem on one workstation — its modem's resolver NXDOMAIN'd the host while the authoritative nameservers and every public recursor answered correctly. That is not a defect in this codebase, and no work was ever at risk.

What remains true, and is the whole justification for this change:

Git does not append .git to a remote URL. git clone https://host/o/r requests /o/r/info/refs?service=git-upload-pack verbatim.

Measured directly against a running instance, bypassing DNS:

/ccantynz-alt/Vapron.git/info/refs?service=git-upload-pack -> 401
/ccantynz-alt/Vapron/info/refs?service=git-upload-pack     -> 404

So an origin configured without the suffix — the URL a human copies out of the browser address bar — fails with "repository not found" on every clone and push. GitHub and Gitea both serve either spelling; we served one.
The rate limiter being mounted on the suffixed path only, and the private-repo gate returning an HTML 404 where a 401 challenge is required, are both independently real and are what most of this diff addresses.

The bug is real and measured. The urgency I attached to it was borrowed from a second-hand report I did not verify, and scaling a claim past its evidence is exactly the failure this project has been bitten by before. Correcting it here so the permanent record matches what was actually established.

Defect #2 (the silent base64 mis-decode) is unaffected by any of this — it was reproduced directly and independently.

ccantynzcommented 5d ago

Blast radius of defect #2: zero. No data was corrupted.

The affected integration enumerated all 47 repos under ccantynz and fetched index.html from every one that has it. All four are intact, well-formed HTML — no mojibake, no truncation (marco-demo 98KB, sharon-maxwell-realestate 50KB, voxlen, vapron-starter-static).

The corrupting path was genuinely reachable, but never executed against a real publish: that client's GLUECRON_API_KEY is unset in its production environment, so it sat in mock mode. The bug was live; nothing went through it. Recording that here so nobody goes hunting for damaged files — and so the fix isn't mistaken for incident remediation. It's prevention.

The other known caller of this endpoint (Vapron's MCP client) base64-encodes on the caller's behalf before sending, so every payload it ever sent was valid base64 — squarely in the class this change leaves byte-for-byte untouched.

Separately: the git plumbing API is now proven end-to-end in production, which closes the one thing I had verified by reading rather than execution. The full chain ran live against a throwaway repo:

POST  /git/blobs            -> 201  ac8cc050...  size 19
POST  /git/trees            -> 201  1a6339f8...
GET   /git/refs/heads/main  -> 200  parent 940caf66...
POST  /git/commits          -> 201  e8574b0d...  (tree + parents linked)
PATCH /git/refs/heads/main  -> 200  ref now at e8574b0d
GET   /tree/main?recursive=1 -> 200 {"tree":[{"path":"plumb.txt","size":19}],"totalCount":1}

The branch moved and the tree reflects it. blobs -> trees -> commits -> PATCH refs is a working atomic multi-file commit path over the authenticated API — worth knowing, because it is strictly better than shelling out to git push with a PAT in argv, and because /git/blobs accepts encoding: "utf-8" natively and therefore never touches the trap this PR fixes.

Cross-repo impact

See what breaks downstream if this PR merges.

Analyze →

⮌ Merged

This pull request was merged into main.

c comment · e edit title · m merge · a approve · r request changes · ? shortcuts