Repository navigation
fix(runtime): require an access token for OAuth status - #1725
Merged
Dumbris merged 1 commit intoOct 22, 2026
Merged
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This was referenced Oct 22, 2026
Dumbris
pushed a commit
that referenced
this pull request
Oct 22, 2026
# Pull Request ## Description Round 3 of the UTF-8-safe truncation sweep started in #1647 and continued in #1664 / #1665. Three production sites still cut text with a raw byte slice (`s[:n]`), so a non-ASCII value whose cut offset lands inside a multi-byte rune is published as invalid UTF-8, and JSON encoding rewrites the tail to U+FFFD. ### 1. `internal/storage` — the activity-log per-record cap `truncateResponse` is the only thing bounding a single activity record. It now goes through `truncateRuneSafe`, the helper the per-server tool-call records in the *same package* (`internal/storage/tool_call_bounds.go`) already use for exactly this reason. **Reachability.** An upstream tool response arrives as the `response` payload of an activity event, and `ActivityService.handleEvent` → `truncateForStorage` → `storage.TruncateActivityResponse` → `truncateResponse` applies `activity_max_response_size` (default 64 KB) on the write path (`internal/runtime/activity_service.go:267`). Any response over the cap whose byte at the cut offset is a UTF-8 continuation byte — ordinary CJK, Cyrillic, or emoji text — lands in `config.db` malformed, and from there in `GET /api/v1/activities`, `GET /api/v1/activities/{id}` and the CSV export. With a 3-byte rune, 2 of every 3 cut offsets are mid-rune. ### 2. `internal/outputvalidation` — the 500-byte violation `Reason` A `jsonschema` message quotes the offending instance location, so a non-ASCII property name or value sits astride the cut. **Reachability.** `Validator.Validate` calls `truncate(err.Error(), 500)` (`validator.go:157`) on any structured tool output that fails its `outputSchema`; the `Reason` goes back to the agent and into the audit line. Measured on the pre-fix code with a 200-rune non-ASCII property name, shifting the cut by one byte at a time: offsets 0 and 2 produce invalid UTF-8, offset 1 is valid — i.e. 2 of 3. ### 3. `internal/registries` — the 300-byte Pulse description cap Registry descriptions are upstream-authored free text. **Reachability.** `parsePulseWithoutGuesser` fills `ServerEntry.Description` from `short_description` / `EXPERIMENTAL_ai_generated_description` for every result of a Pulse registry search, which the REST API and the Web UI catalog then JSON-encode. Same 2-of-3 pattern in the test. The 300 literal is now a named constant (`pulseDescriptionMaxBytes`) since it appeared twice. ASCII behaviour is unchanged at all three sites, and the retained prefix never exceeds the cap. **Out of scope,** deliberately, so this stays reviewable: - The CLI table cells in `cmd/mcpproxy` that #1664 also left alone — sent separately. - `cmd/mcpproxy/security_cmd.go` and `cmd/mcpproxy/main.go` have the same shape but are under open PRs (#1714, #1720, #1725), so I have not touched them. - `internal/configimport.SanitizeServerName` and `internal/dockernaming.SanitizeServerName` also cut at a byte offset, but both strip everything outside `[A-Za-z0-9._-]` first, so no multi-byte rune can reach the cut. Left as they are. ## Testing - [x] I have tested these changes locally - [x] I have added/updated tests that prove my fix is effective or my feature works - [x] All existing tests pass New tests, each failing by name at the base SHA (`14cb288`) and passing after: | Test | Package | |---|---| | `TestTruncateResponseKeepsRuneBoundary`, `TestTruncateResponseASCIIUnchanged` | `internal/storage` | | `TestViolationReasonIsValidUTF8`, `TestTruncateASCIIUnchanged` | `internal/outputvalidation` | | `TestPulseDescriptionTruncationIsValidUTF8` | `internal/registries` | Each boundary test sweeps every offset inside a 3-byte rune rather than asserting one fixed cap, so it cannot pass by luck. Commands run (exit status in brackets): ``` git diff --check [0] gofmt -l internal/storage internal/outputvalidation internal/registries [0, no output] go vet ./internal/storage ./internal/outputvalidation ./internal/registries [0] go test ./internal/storage ./internal/outputvalidation ./internal/registries [0] go test ./internal/runtime -run 'TestToolCallResponseTruncatedForStorage|TestInternalToolCallResponseTruncatedForStorage|TestStorageTruncatedBuiltinStillCountsDeliveredBytes|TestPromptGetResponseTruncatedForStorage|TestDefaultResponseCapAppliesWithoutConfig|TestEmitterTruncationFlagSurvivesUntruncatedRecord' [0, 6/6 pass] ``` The `internal/runtime` selection is the end-to-end activity-truncation suite that drives the storage cut through the real event path, included to show the write-path behaviour and the `ResponseTruncated` flag semantics are unchanged. Not run: `./scripts/run-all-tests.sh` and `-race` (local disk constraints on this machine). Nothing here is concurrent.
Dumbris
pushed a commit
that referenced
this pull request
Oct 22, 2026
# Pull Request ## Description Finishes the UTF-8-safe truncation sweep of #1647 / #1664 / #1665 in the part of `cmd/mcpproxy` those PRs explicitly set aside ("I left the CLI table description truncations in `cmd/mcpproxy` alone to keep this focused" — #1664). Companion to #1726, which covers the remaining `internal/` sites. Four table cuts existed as independent copies of the same arithmetic, and all four cut at a raw byte offset: | Site | Column | Data | |---|---|---| | `shorten` (`profile_cmd.go`) | 30 | `EffectiveServers`, comma-joined | | `truncateStr` (`registry_cmd.go`, `catalog_cmd.go`) | 60 / 45 / 40 | registry `description`, catalog `Title` | | inline `desc[:57] + "..."` (`upstream_cmd.go`) | 60 | upstream tool `Description` | | inline `result[:maxLen-3] + "..."` (`token_cmd.go`) | 23 | `allowed_servers`, comma-joined | **Reachability.** Every one of these cells is upstream- or operator-authored text: - `mcpproxy registry list` / `registry search` print descriptions straight from the registry API response (`mapString(r, "description")`), and `catalog list` prints `r.Title`. Both are free text from a public registry. - `mcpproxy upstream tools` prints the tool `Description` advertised by the upstream server. - `allowed_servers` and `EffectiveServers` are server names, which the config only forbids from being empty or containing `:` (`internal/config/server_edition_config.go:176`) — a non-ASCII name is legal. A value longer than its column whose byte at the cut offset is a UTF-8 continuation byte ends in a partial rune on the terminal. With a 3-byte rune, 2 of every 3 offsets are mid-rune; the test sweeps all three at each of the six real column widths. All four now go through one `truncateCell` built on `stringutil.UTF8Prefix`, in a file of its own so a fifth table does not grow a fifth copy. `joinInterfaceSlice` keeps `maxLen == 0` meaning "do not truncate". ASCII output is byte-for-byte unchanged and the result still never exceeds the column width — both asserted. **Left alone, deliberately:** - `cmd/mcpproxy/security_cmd.go` and `cmd/mcpproxy/main.go` have the same shape but are touched by open PRs (#1714, #1720, #1725), so I have not gone near them. Happy to follow up once those land. - `activity_cmd.go`'s `actType[:12]` and `upstream_cmd.go`'s `hash[:12]` cut an internal activity-type enum and a hex digest, where no multi-byte rune can reach the boundary. Not touched — a guard there would protect against a state the code cannot produce. ## Testing - [x] I have tested these changes locally - [x] I have added/updated tests that prove my fix is effective or my feature works - [x] All existing tests pass New tests in `cmd/mcpproxy/table_cell_utf8_test.go`: - `TestTruncateCellKeepsRuneBoundary` — 6 real column widths x 3 byte offsets. - `TestTruncateCellASCIIUnchanged` — pins the ASCII output of all four removed helpers, including the `maxLen <= 3` edge. - `TestJoinInterfaceSliceTruncationIsRuneSafe` — drives the composition (`joinInterfaceSlice` -> `truncateCell`), not just the helper, and pins `maxLen == 0`. **Mutation check.** With `truncateCell` reverted to the raw byte cut (`s[:maxLen-3] + "..."`) the two boundary tests fail by name — 12 sub-cases in `TestTruncateCellKeepsRuneBoundary`, plus `TestJoinInterfaceSliceTruncationIsRuneSafe` — while `TestTruncateCellASCIIUnchanged` still passes, which is what shows the change is behaviour-preserving for ASCII. Restoring the fix returns the package to green. Commands run (exit status in brackets): ``` gofmt -l cmd/mcpproxy [0, no output] git diff --check [0] CGO_ENABLED=0 go vet ./cmd/mcpproxy [0] CGO_ENABLED=0 go build ./cmd/mcpproxy [0] CGO_ENABLED=0 go test -count=1 ./cmd/mcpproxy [0, whole package] ``` `CGO_ENABLED=0` because this machine's Command Line Tools SDK is currently broken for cgo linking (`arm64e.x1-macos: unknown architecture` out of `CoreFoundation.tbd`), which is unrelated to this change. Nothing here is cgo-dependent. I did not run `-race` or `./scripts/run-all-tests.sh` (local disk constraints); none of this code is concurrent.
Member
|
Merged, and #1723 is closed with it. Thank you, @neylwalecki! A DCR-only record no longer shows up as Contributions like this are what keep MCPProxy moving, and they matter a lot to the project. One small ask, only if you feel like it: a short public note about your change (a tweet, a LinkedIn post, a line in a community you are part of) helps more people discover MCPProxy. No pressure at all, and I would be glad to repost it. |
Dumbris
added a commit
that referenced
this pull request
Oct 22, 2026
## Description `TestInformationalRescan_BusyServerIsRetriedNotDropped` (Spec 118, #1689) failed on `main` in 4 of the last 8 unit-test runs and was the only red check on contributor PRs #1727, #1725 and #1715. **Cause.** `fireInformationalRescan` deletes the server's entry from `infoRescanTimers` when the debounce timer fires and re-creates it only after the gates and the claim ran. On every busy fire there is a short window in which "is a timer armed" is false. The test sampled the map at one fixed instant (`time.Sleep(4 * debounce)`), which under CI load lands in that window. Reproduced locally: 1 failure in 150 runs with `GOMAXPROCS=1` under 2×ncpu CPU burners. **Fix (test only).** Read `infoRescanSeq` under the mutex before scheduling; the change itself takes `before+1`, so `require.Eventually(rescanSeq("srv") > before+1)` is a timing-free proof that a busy fire re-armed the timer. The "no scan attempted on top of a claimed slot" and "a busy slot is not a failed attempt" assertions are unchanged. **Verification.** 400 runs of the fixed test under the same CPU contention: 0 failures. `go test -race -count=3 -run TestInformationalRescan ./internal/server/` green. `golangci-lint` with the CI v2 config clean. Cross-reviewed with opencode (gpt-6.1-sol), two rounds: round 1 found the baseline read could hit the same gap (fixed by reading the counter before scheduling), round 2 CLEAN. No production code changes. ## Second flake, same PR `TestRefreshConcurrency_ScansDuringASlowSelfCheckSeeOldOrNew` (from #1720) failed the darwin-amd64 Build Binaries job on this PR: `119.6ms is not less than 109.7ms (baseline median 1.19ms)`. Both refresh-concurrency guards compared the single worst of 300 scans against `50*median+50ms`, which one GC pause on a macOS runner exceeds. They now bound p90 relative to the baseline and keep an absolute 2 s ceiling on the worst scan (no scheduler stall reaches it, any wait behind the gate or a fetch deadline does). 30 runs under CPU contention: 0 failures. opencode Sol 6.1 round 1 flagged that p90 alone could hide one scan absorbing a timed-out wait; the absolute ceiling is the answer to that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Runtime.GetAllServerstreats a persisted DCR registration as a valid OAuth token even whenAccessTokenis empty. That makes the status payload reportauthenticated=trueandoauth.token_valid=true, and the CLI printToken: Valid, while the token store correctly returnsErrNoToken.Require a non-empty access token before projecting authentication and expiry, matching
PersistentTokenStore.GetToken. A DCR-only record still identifies an OAuth server, including autodiscovery, so it keeps the login action. The read preserves the registration. Actual non-expiring tokens, expired-token semantics, and static-header authentication are unchanged.Fixes #1723.
Regression proof
The test creates registrations through
UpdateOAuthClientCredentialsin temporary BBolt storage and exercises the real runtime projection. Five scenarios run in both explicit and autodiscovery modes:On base
14cb288f, the four empty-access-token scenarios fail (authenticatedis true instead of false,oauth_statusis authenticated instead of none); the six real-token controls pass. With the fix, all ten pass. The test also checksErrNoTokenfor the incomplete records, the login action, absence of access-token expiry claims, and preservation of the registration.Scope
This changes the runtime status projection only. It does not change token storage, refresh, or OAuth instance retirement. #1722 separately tracks pending logins being retired by replacement and callback UX; the retirement barrier remains intact. All fixtures are synthetic and use
.invalid; no real providers or credentials are involved.Testing
Go 1.26.0, darwin/arm64:
Results: 79 base packages and six Server-edition packages passed with race detection; both builds passed; the CI-pinned golangci-lint v2.9.0 reported zero issues for both editions. The full runtime and OAuth packages also passed with race on Go 1.27.3 (
go test -race ./internal/runtime ./internal/oauth -count=1 -timeout=10m).The REST smoke from
scripts/test-api-e2e.shpassed 70/70, including the Server-edition audit companion. The harness assertions were unchanged; its scratch setup used loopback listeners, disabled local sockets/tray, and a dedicated npm cache. The first smoke attempt could not start theeverythingfixture because the global npm cache returnedEACCES; using the dedicated cache resolved that setup failure without changing global permissions.