Skip to content

fix(api): answer a request value git cannot take with a 4xx, and take commit messages on stdin - #748

Open
ThomasK33 wants to merge 9 commits into
mainfrom
git-argv-limits
Open

ThomasK33 wants to merge 9 commits into
mainfrom
git-argv-limits

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Nov 7, 2026 •

Copy link
Copy Markdown
Member

Why

ThomasK33 asked to fix and build the open issues of this repo and land them on main. While #739 was verified, a second bug of the same kind turned up that main already has: a client string that reaches a git command line or environment gives a 500 when it is 128 KiB or longer, or when it holds a NUL byte. On Linux one argument or environment variable cannot reach 128 KiB, so exec fails before git starts. A client must get a 4xx for a value the server cannot take, never a 500.

Found on main with a request matrix over every route and field that reaches git (argv_limits_test.go). These answered 500: the path of file, files, files/metadata, commits, diff and blame, a blame range, the grep pattern and paths, the author name and email of a note, and the commit_message, author and committer of merge and commit-pack (a message of 131,100 bytes or more, a NUL byte, a name or email over the limit).

What changes

  1. gitlimit.Start is the one place where every git child starts. It now answers ErrArgNUL for a NUL byte in an argument or variable, and ErrArgTooLong when exec fails with E2BIG (one value over the limit, or all of them together over the system limit). Git is not started.
  2. writeGitLimitErr turns them into 422 (too long) and 400 (NUL). It was reached by internalErr only. The two write funnels, wcInternal (commit-pack, diff-commit, restore-commit) and wmInternal (merge), now call it too. docs/reference/http-api.md has a new row in the table of limits.
  3. The commit message goes to git commit-tree on stdin (-F -), so git takes a message of any size. The request limits still bound it: the body of merge and restore-commit (1 MiB) and one metadata line of commit-pack and diff-commit (8 MiB). -m ends a message with a newline when it lacks one and -F - does not, so writeCommit adds it: the stored commit is byte for byte what -m made. commitTree and wmCommitTree share writeCommit. A NUL byte in a message is a 400 (git refuses it). A NUL byte in an author or committer is caught by the same validation before git starts, so `commit-pack` and `diff-commit` answer it with their result envelope. The checks run before a merge knows its outcome, so a fast-forward merge with a NUL byte in commit_message now answers 400 where it answered 200 and wrote no commit. A NUL byte in the base_ref of restore-commit is still a 404, as on every route: the default message (Restore to <base_ref>) is built without it.
  4. wmPromotedCommits passed every branch tip as an argument of rev-list --count. All the arguments of one command together must fit about 2 MiB, which is about 50,000 tips of 41 bytes (my arithmetic, not run). The tips go on stdin now.
  5. The sync worker (internal/sync) passes the upstream URL (https://{host}/{owner}/{name}.git, one argument) and the stored credential to git fetch and git push as an argument and as environment variables, and none of them has a length cap. A URL or a credential of 128 KiB or more, or with a NUL byte, was refused by exec and the run failed as failed to clone repository or failed to push to upstream, with exec's error as the cause. runGitTo now names both refusals (gitlimit.ErrArgNUL, gitlimit.ErrArgTooLong) and the run fails as repository configuration error: permanent, not retried, and the message never names the value. The worker starts git itself (its process-group handling and shutdown differ from gitlimit.Start), so it maps the two errors itself.
  6. The lane-schedule model (.agents/skills/verify-lane-schedule) covers internal/sync/worker.go and worker_push.go. The model is unchanged: the two new branches (fetch and push) fail the run with FailConfig where the existing denied-destination FailConfig is returned, and no model fact reads them. check.sh --record rewrote the fingerprints and the sync commit.
  7. files/metadata narrows its diff-tree with the common directory of the files as a pathspec. That is only an optimization, so a directory that does not fit one argument is left out.

One argument is limited to 32 pages, which is 128 KiB only on Linux with 4 KiB pages. The tests that send 200,000-byte values skip on any other system, and `TestStartReportsArgumentsTooLong` too. The production code reads the limit from exec (E2BIG) and assumes none.

Refs stay as they were: a ref that long is a 404 or 400 because no repository holds one, so ResolveRef still treats it as not found.

Audit of the git calls in internal/api

Call Client input Result
ls-tree, log, diff, blame, grep pathspecs and -L, -e path, range, pattern, paths argument, so 422 or 400 (new)
commit-tree message stdin now. Identities stay in the environment: 422 or 400
notes ... -F - note stdin before. Author name and email in the environment: 422 or 400
rev-list --count (promoted commits) branch tips stdin now
diff-tree --stdin pathspec directory of the files left out when it does not fit
update-index --index-info, apply, pack-objects --revs, merge-tree paths, patch, SHAs stdin or 40-hex SHAs: no change
check-ref-format, rev-parse, update-ref, for-each-ref ref names validated (255 bytes) or built by the server: no change

Audit of the git calls outside internal/api

Call Input Reachable?
internal/sync fetch --prune <url>, push ... <url>, credential in the environment upstream owner, name, host, stored credential (no length cap) Yes: fixed above
internal/sync ls-remote --symref <url> HEAD, rev-parse refs/heads/<branch> the same URL, which the fetch before it already passed; the branch comes from the upstream HEAD, capped at 64 KiB of output and checked by ValidBranchName (255 bytes) No. A failure is logged as a reason.
internal/sync loopback push/fetch to the server's own URL, refspecs server URL, repo id, constants No
internal/githttp symbolic-ref HEAD <default branch> default branch: validBranchName (255 bytes) on create and patch, ValidBranchName on adoption No. It runs through gitlimit.Output, so the new errors apply anyway.
internal/githttp hooks: rev-list <sha> ^<sha>, verify-commit, cat-file -e, merge-base --is-ancestor 40-hex ids from receive-pack (ref.ValidSHA first), a server path No. Ref names from a push never reach a command line: refs update through update-ref --stdin.
internal/githttp hydration: config, init --bare, index-pack, for-each-ref, commit-graph, upload-pack/receive-pack <repo path> constants, cache paths built from the server's repo id No
internal/jobs pack-objects <prefix> server temp path, object ids on stdin No
internal/lfsscan rev-list --objects, cat-file --batch constants, object ids and LFS pointers on stdin and stdout No
cmd/ no git calls No

Verification

  • gofmt, go build ./..., go vet, golangci-lint 2.15.0 on internal/api, internal/gitlimit, internal/gitread and internal/sync: 0 issues. Doc link gate: 0 broken.
  • go test -race -tags=openstorage_viewcheck -skip '/^pg$', all ok: at fd15e68 on internal/api, internal/gitlimit, internal/gitread, internal/githttp, internal/simtest, cmd/openstorage and cmd/dst. At 5585b62 on internal/api (645 s), internal/gitlimit and cmd/openstorage. At 64ea599 on internal/sync (78 s), internal/gitlimit (2 s) and cmd/openstorage (58 s). At 027c65e (main as of fix(api): preview merge conflicts at paths longer than one git argument #739, which landed after this branch was reviewed): internal/gitread (219 s), internal/gitlimit (2 s), internal/sync (100 s) and cmd/openstorage (80 s) pass, and internal/api passed twice alone (353 s, 374 s). One earlier run of internal/api at that head failed under host load (about 77) and its failing test was lost, because my lane script kept only the last 20 lines. The two reruns passed. I did not see that assertion. Main's test(api): retry a cold read that found every git-read slot busy #744 (a retry in TestMaterializeColdConcurrent, which fails when git-read slots are busy) is merged into this branch at the final head, and it changes only that test. TestMerge*, TestRestoreCommit*, TestCommitPack*, TestNotes*, TestWriteRefs* and the new internal/api tests pass on pg at 027c65e. CI runs both backends. check.sh --fingerprints-only of the lane-schedule model passes.
  • New tests:
    • TestRequestValuesGitCannotTake, TestMergeValuesGitCannotTake, TestCommitPackValuesGitCannotTake: 3 values (200,000 bytes, a 200,000-byte path of short components, a NUL byte) in 52 fields, each with its exact status: 39 of the read, ref and note routes, 5 of merge and 8 of commit-pack. TestRestoreCommitBaseRefGitCannotTake adds the base_ref of restore-commit, the 53rd field.
    • TestWriteCommitMessage: 8 messages, with git's own -m as the oracle where its argument fits. TestLongCommitMessageRoundTrips: a 216,000-byte merge message is read back whole.
    • TestStartReportsArgumentsTooLong, TestStartRefusesNUL: at 131,071 bytes git runs, at 131,072 it does not.
    • TestPromotedCommitsTakeTheTipsOnStdin: 399 tips, the count runs with 3 arguments (a recording git on PATH).
    • TestLastCommitsOfPathsTooLongForAnArgument: a path of 131,061, 131,062 and 200,000 bytes.
    • TestRestoreCommitBaseRefGitCannotTake: a base_ref of 200,000 bytes, a long path and a NUL byte, with and without a commit message, answers 404 each.
    • TestWorkerUpstreamArgumentsGitCannotTake: an owner or a name of 200,000 bytes, an owner with a NUL byte, and a credential of 200,000 bytes, each on a pull and on a push, fail as repository configuration error, and the failure does not name the value.
  • 18 mutants, each caught by a named test and then restored: E2BIG not mapped, NUL not refused, the 422 case or the 400 case dropped, merge or commit-pack errors skipping the mapping, the message back in -m, no newline completion, no NUL validation of the message or of an identity, the NUL kept in the default restore message, the tips back in arguments, the pathspec always kept, its limit one byte too high, and in internal/sync the fetch or the push failure not mapped, the E2BIG not named, and the NUL pre-check dropped.
  • Real serve and real git push, 9 requests: on main file, files, commits and blame with a 200,000-byte path, a grep pattern of 200,000 bytes, a note author of 200,000 bytes, a NUL in a path, and a merge with a 216,000-byte message all answer 500. On this branch they answer 422, 422, 422, 422, 422, 422, 400 and 200, a missing path still answers 404, and the merge commit read back through git has the message byte for byte.

Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh

… commit messages on stdin

A client string that reached a git command line or environment gave a 500
when it was 128 KiB or longer (one argument or variable cannot reach that on
Linux) or held a NUL byte. File, files, files/metadata, commits, diff and
blame paths, a blame range, a grep pattern and paths, the author of a note,
and the commit message and identities of merge and commit-pack all did.

gitlimit.Start now answers ErrArgTooLong (E2BIG) and ErrArgNUL, and the
handlers' error funnels turn them into 422 and 400. The commit message goes
to git commit-tree on stdin, so a message of any size is kept whole and
ends with a newline as -m made it. A NUL byte in a message is a 400. The
branch tips of the promoted-commits count go on stdin, and the diff-tree
pathspec of files/metadata is left out when a directory does not fit one
argument.

Change-Id: Iec43f951a0cee79092c446a858e835bf55975237
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-07T05:54:25.434007Z 89c2e87 Manual request
🔒 Security Review ✅ Completed 2026-11-07T05:57:44.152651Z 89c2e87 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: fd15e6849e

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd15e6849e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/api/handlers_write_commit.go Resolved
Comment thread internal/gitlimit/argtoolong_test.go Resolved
…ate the argument-limit tests

Validate a NUL byte in an author or committer before git starts, as the
commit message is, so commit-pack and diff-commit answer it with their result
envelope and not the plain body of gitlimit.ErrArgNUL.

One argument is limited to 32 pages, which is 128 KiB only with 4 KiB pages,
and only on Linux. The tests that send 200,000-byte values skip elsewhere.

Change-Id: Ic2c69e4c0f708329f76d5ba77bfb34a2908194d9
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 5585b6235e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 5585b6235e

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

…he upstream URL or the credential

The upstream owner, name and host, and the stored credential, reach git as one
argument or one environment variable of a pull or a push. A value of 128 KiB
or more, or with a NUL byte, was refused by exec and the run failed as a clone
or a push failure. Report it as the permanent configuration error that it is.

Change-Id: I0471def2e9632faaef035829978143dfc282ad81
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 64ea5998d8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 64ea5998d8

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

… failure

The sync worker now returns FailConfig when git cannot take the upstream URL or the credential. The new branch returns at the same site as the existing egress-denied FailConfig, and no model fact reads it, so the model is unchanged. Re-record the fingerprints of internal/sync/worker.go and worker_push.go, and the sync commit.

Change-Id: Id7adc12f066672ac8a6188342be862611f0b4811
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9b57d56416

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread docs/reference/http-api.md Resolved
@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 9b57d56416

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

A commit message goes to git on standard input, so git takes it at any size. The body of merge and restore-commit (1 MiB) and one metadata line of commit-pack and diff-commit (8 MiB) still bound it. The reference said a message of any size is kept whole.

Change-Id: Ia3651834469dc4041b70478e2a1dc2df140a9719
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 04acbd5922

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 04acbd5922

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

The default message of a restore is built from base_ref. A NUL in base_ref was reported as a NUL in a commit_message that the client did not send, with a 400. Main answers 404 for it, as every other route does for a ref that names nothing. Keep the NUL out of the default message so that base_ref is answered as before. Also say in the reference that the limit of one argument counts the whole upstream URL, not each part of it.

Change-Id: I60ec040b4d7a5f2c084b3eeacf0e2b5db7e210ad
Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Id949ca5835cb5c11a5fe85836fd74334084bff5d
Signed-off-by: Thomas Kosiewski <tk@coder.com>
…o not

The comment on wmMaxPathspecBytes said that Linux refuses an argument of 128 KiB or more, counting its terminating NUL. An argument of exactly 128 KiB with its NUL is accepted. The constant was right, and the comment now matches it.

Change-Id: I1a2a4b5558cf827913fb517cb61c2aa957051353
Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I646f2cb59fde334705cc0a75da15dfc0300315f3
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 89c2e879a1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +651 to +652
`repository configuration error`. That is 128 KiB or more on Linux, counted over the whole URL
and not over each of its parts, or a NUL byte in any part. The failure is permanent, and the

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Qualify the upstream limit by the Linux page size

On Linux systems with 64 KiB pages, such as supported arm64 configurations, MAX_ARG_STRLEN is 32 pages (2 MiB), so an upstream URL or credential between 128 KiB and the request-body limit can still be passed to git successfully. The later request-values table correctly makes 128 KiB specific to 4 KiB pages, but this paragraph presents it as universal Linux behavior; apply the same page-size qualification here.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 89c2e879a1

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

This branch has not been deployed

No deployments
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