fix(clients): prevent query injection, rate-limit masking, and window drift

[mfacenet]

Summary

Hardens the GitHub/GitLab commit-evidence collection paths against silently incorrect or incomplete evidence sets. Every behavioral fix ships with a regression test that fails before the change and passes after. Scope was limited to ghclient/, gitlabclient/, internal/patch/, internal/commitclient/, and config/resolve.go (+ their tests); config/config.go was intentionally not touched.

Findings addressed

P1 — GitHub search-qualifier injection

ghclient/ghclient.go:533-565 concatenated q.Author, q.Org, q.Repos unquoted into the commit-search string, so an author like victim author:attacker smuggled a second qualifier and even John Doe corrupted the query. Two layers of defense:

• config/resolve.go — validate GitHub author/org/repo against the provider charset (login [A-Za-z0-9-], repo owner/name, email as a whitespace/colon/quote-free token including the Name <addr> form). Embedded spaces, colons, and quotes are rejected. GitLab is deliberately left unvalidated because its client URL-encodes every value.
• ghclient/ghclient.go (authorQualifier / buildSearchQuery / new searchQualifierValue) — quote and backslash-escape any qualifier value that is not a plain identifier, as defense in depth matching GitLab's URL-encode safety level.

Tests: TestResolveRejectsGitHubIdentifierInjection (feeds victim author:attacker, John Doe, x org:secret-org, a quote), TestResolveAcceptsValidGitHubAuthors, TestResolveGitLabSkipsGitHubValidation, TestSearchQualifierValueEscaping, TestBuildSearchQueryQuotesInjection.

P1 — secondary rate limits masked as access-forbidden

ghclient/ghclient.go:66 treated every 403 during an org scan as a per-repo skip (access forbidden), so a secondary-rate-limit 403 (which go-github does not always map to RateLimitError) silently dropped repos. Now skipRepoReason inspects Retry-After / X-RateLimit-Remaining / the message and classifies rate-limit 403s as fatal (abort), only skipping genuine permission 403s. Mirrored in gitlabclient/gitlabclient.go:301: statusError now captures Retry-After and skipProjectReason treats a skippable status carrying it as fatal.

Tests: TestSkipRepoReasonRateLimit403, TestCollectScopeOrgRateLimit403Aborts, TestCollectScopeOrgPermission403Skips, TestSkipProjectReasonRetryAfter.

P1 — search-scope window truncated to whole days

ghclient/ghclient.go:543-551 formatted since/until as 2006-01-02, while repos, PR (inWindow), and GitLab all filter at second precision, so scope=search and scope=repos returned different evidence for the same sub-day window. Now emits full RFC3339 author-date bounds.

Tests: TestSearchReposWindowParity (asserts the search q qualifier and the repos since/until carry the same sub-day RFC3339 instants), updated TestBuildSearchQuery.

P2 — no HTTP timeout

gitlabclient.go:57 used http.DefaultClient; ghclient.go:86 used github.NewClient(nil). Each client now gets its own *http.Client{Timeout: 30s} (per-request backstop; the caller still bounds the whole scan via context). The global default client is never mutated.

P2 — unbounded pagination

Every pagination loop trusted the server's next-page signal with no cap. Added a maxPages (10000) guard to every loop in both clients; exceeding it aborts with a clear error.

P2 — false Truncated

ghclient.go:136 / gitlabclient.go:92 set Truncated whenever len(commits) >= MaxCommits, even when nothing was dropped. Both Collects now fetch one commit past the cap (MaxCommits+1) and only set Truncated when strictly more than MaxCommits came back.

Tests: TestCollectExactMaxCommitsNotTruncated (ghclient + gitlabclient), reworked TestCollectReposPullRequestsRespectMaxCommits and TestCollectPaginationAndMaxCommits.

P2 — UTF-8 patch split

internal/patch/patch.go:21 sliced patch[:budget] on a byte boundary, which could cut a multibyte rune. Truncation now backs off to the last whole-rune boundary so recorded evidence is always valid UTF-8.

Tests: TestConsumePatchBudgetUTF8Boundary.

Dead-code cleanup

• Removed the never-read gitlabCommit.ShortID field (JSON still ignores the incoming short_id).
• Removed the provider-defaulting in internal/commitclient/commitclient.go that duplicated config.Resolve's empty→default→github fallback. Both production callers pass an already-resolved q.Provider.

P2 doc note

Added a short code comment in gitlabclient.listProjectCommits documenting that GitLab filters/records the committed date and author_name/author_email while the GitHub paths use the author date and login. The prose docs change is left to the docs PR.

Decision needing maintainer sign-off

commitclient.New no longer defaults an empty provider. The finding suggested removing both the duplicated defaulting and the switch default branch. I removed the defaulting but kept the default branch as a guard, because dropping it would make the exported constructor silently return (nil, nil) for an unexpected/empty provider — a nil-client footgun. Consequence: New(cfg, "") now returns an error instead of defaulting (previously it fell back to cfg.DefaultProvider/github). This is safe today because both in-repo callers pass a resolved provider and the package is internal/, but it is a behavior change to an exported constructor. TestNewUsesDefaultProvider was repurposed into TestNewRejectsEmptyProvider. Please confirm this is acceptable, or say the word and I'll restore the defaulting in New.

Verification

• go build ./..., go vet ./..., go test ./... all pass.
• golangci-lint run ./... (v2.12.2): 0 issues.
• Per-package coverage gate (scripts/check-coverage.sh) passes; all touched packages stay above their thresholds.

[Copilot]

Pull request overview

Hardens sting’s GitHub/GitLab commit collection to avoid silently incomplete/incorrect evidence by preventing search-qualifier injection, correctly surfacing rate-limit failures, aligning time-window precision across scopes, and adding safety bounds (timeouts + pagination caps). This fits into the provider clients (ghclient/, gitlabclient/) and shared helpers (config/, internal/) that power both the CLI and MCP server.

Changes:

• Prevent GitHub search-qualifier injection via validation in config.Resolve plus defense-in-depth escaping/quoting in ghclient query construction.
• Treat rate-limit-like 403s as fatal (abort) instead of “access forbidden” skips; add per-request HTTP timeouts and max-pages guards to pagination loops.
• Fix correctness edge cases: second-precision search windows, correct Truncated detection by probing MaxCommits+1, and UTF-8-safe patch truncation.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/patch/patch.go	Truncate patches without splitting UTF-8 runes.
internal/patch/patch_test.go	Adds regression coverage for UTF-8-safe truncation behavior.
internal/commitclient/commitclient.go	Stops defaulting empty provider in New; documents reliance on config.Resolve.
internal/commitclient/commitclient_test.go	Updates tests to enforce error on empty provider.
gitlabclient/gitlabclient.go	Adds HTTP timeout, pagination cap, improved rate-limit signaling, and correct truncation detection.
gitlabclient/gitlabclient_test.go	Adds/updates tests for retry-after fatal handling and truncation correctness.
ghclient/ghclient.go	Adds HTTP timeout, pagination cap, rate-limit-aware 403 handling, query escaping/quoting, and RFC3339 window bounds.
ghclient/ghclient_test.go	Updates/extends unit tests for RFC3339 bounds and qualifier escaping/quoting.
ghclient/hardening_test.go	Adds end-to-end regression tests for rate-limit vs permission 403 behavior and window parity.
ghclient/collect_test.go	Updates tests for the “probe one past MaxCommits” truncation detection semantics.
config/resolve.go	Validates GitHub identifiers to prevent qualifier injection before reaching the client.
config/resolve_test.go	Adds regression tests for GitHub validation and confirms GitLab bypasses that validation.