Keep cross-field validation in the handler so the canonical tool schema remains compatible with provider JSON Schema subsets. Add an inventory-wide regression guard against top-level schema combinators.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A 403 carrying X-RateLimit-Remaining: 0, or a secondary rate limit
documentation URL, reached copilotReviewErrMsg as a rate limit error. It
was explained as a missing repository or missing write access, and the
repository read it triggered was refused for the same reason, so the
caller paid an extra call to be told the wrong thing.
Return the base message for both rate limit error types so the rate
limit text stands on its own, and trim the helper and its tests to the
comments the code cannot state.
Co-authored-by: Dylan Pulver <dylanpulver@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The review request endpoint requires write access to the repository, and
GitHub refuses a caller without it with 404 Not Found rather than a
permission error. Authoring the pull request does not grant that access,
so a fork contributor can be offered a Copilot review by the web UI and
still be refused by request_copilot_review, with nothing in the tool
result to say why.
On 403 or 404 the tool now reads the repository once so it can name the
cause. A caller without write access is told so directly and pointed at
the web UI. When the repository cannot be read at all, or when write
access is present, the message says so and points at the likelier
cause.
The benchmarks and the reference-implementation equivalence harness
existed to justify the sanitizer rewrite. They have served that purpose,
so remove them along with the verbatim copy of the old pipeline they
carried.
Five checks move into sanitize_test.go rather than going away, because
none of them reference the old implementation and all of them guard
behaviour the rewrite introduced:
- isHTMLInert must be a fixed point of the live bluemonday policy,
checked byte by byte and as whole strings, with the accepted byte set
pinned explicitly. Nothing else fails if that set is widened, and
widening it changes sanitizer output.
- Both filters are fixed points on their own output, which is what
licenses Sanitize to skip its second pass.
- Clean ASCII sanitizes with zero allocations.
- Invalid UTF-8 is re-encoded to U+FFFD.
- Known payloads still lose content.
Net -560 lines.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sanitizing user-authored response fields ran multiple allocating passes
over every string regardless of content: FilterInvisibleCharacters
converted the whole input to []rune and back, FilterCodeFenceMetadata
split and rejoined every line, and bluemonday ran unconditionally. On
comment- and issue-heavy responses this dominated conversion CPU and
allocation.
Three changes, none of which alter output or widen what the policy
allows:
- FilterInvisibleCharacters scans first and copies only from the first
filtered rune, skipping ASCII runs without decoding them. Invalid
UTF-8 is still re-encoded to U+FFFD, matching the []rune round trip it
replaces.
- FilterCodeFenceMetadata walks lines in place and returns the input
when no line changes.
- FilterHTMLTags skips bluemonday for input that is provably a fixed
point of the policy: printable ASCII, TAB and LF, with none of the
five characters html.EscapeString rewrites. Sanitize also skips the
second invisible/code-fence pass when HTML normalization returned its
input unchanged, since both filters are fixed points there.
Equivalence is pinned by a verbatim copy of the previous pipeline: the
new code is diffed against it over a corpus of ~22k deterministic cases
plus two fuzz targets, and the fast path is checked byte by byte against
the live bluemonday policy.
Benchmarks (Intel Ultra 9 185H, n=6):
Sanitize/TitleASCII 5.35µs -> 114ns 1 -100% allocs
Sanitize/Comment1KiB 45.3µs -> 1.27µs 1 -100% allocs
Sanitize/Body64KiB 2.47ms -> 85.9µs 1 -100% allocs
30 issues x 2KiB body 3.00ms -> 88.6µs 1.55MiB -> 1.9KiB
100 comments x 1KiB 5.04ms -> 169µs 2.19MiB -> 6.3KiB
Content that genuinely needs rewriting still pays for it, and non-ASCII
text still goes through bluemonday by design.
Fixes#3117
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The cache's idle/sliding TTL is cache2go's documented behaviour and was
deliberate in both the original hand-rolled cache and the cache2go
migration: a hot repo keeps serving from cache and only idle entries are
reclaimed. Replacing it with a fixed max age traded that away for a
periodic refetch on every hot repo, which is a freshness change rather
than the isolation fix this issue is about.
Remove createdAt, the injected clock, entryExpired, the createdAt
preservation on entry updates, and the tests that only existed to prove
bounded non-sliding expiry. Restore the original sliding semantics.
Keep the per-caller isolation, which is the actual defect: entries were
keyed on owner/repo alone in a process-wide table, so a trust decision
computed under one caller's credentials could be served to another
caller whose own credentials were never checked. Entry keys now carry a
SHA-256 digest of the request identity, inside a single bounded table.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The cache changes carried explanatory comments that restated the code or
narrated what each step did. Drop them and keep only what the code cannot
express: that cache2go never reclaims a named table, that its own expiry
slides on every read, that createdAt survives entry updates, and that
RepoAccessOpts is shared across requests. Exported options keep a short
doc comment.
Comment-only; no behavior change.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This reverts commit d7d8dd25.
The lint job failed on a transient timeout fetching the golangci-lint
config schema, which is a CI infrastructure concern rather than a defect
in this change. Disabling schema verification to work around it does not
belong in a cache-hardening PR: it weakens a check for every future run,
and its root cause is out of scope here. Leaving CI configuration
untouched keeps this PR to the lockdown cache redesign.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The golangci-lint action downloads a JSON schema from golangci-lint.run on
every run to verify .golangci.yml. A blip reaching that host fails the job
before any linter runs, as it did on this PR. Linting should depend only on
the checked-out code, which is also what script/lint does locally.
Isolating identities by deriving a cache2go table name per token grew a
process-wide registry that is never reclaimed: cache2go creates each named
table on first use and never evicts it, so every distinct bearer token —
including invalid ones, since the table was built before GitHub validated
the token — permanently added a table.
Keep a single cache table and scope entries instead. WithIdentity stores a
SHA-256 digest of the identity and prefixes each entry key with it, so
different identities still cannot observe each other's trust decisions,
while per-identity state is reclaimed by the table's ordinary TTL cleanup.
WithCacheName stays for tenant/test isolation, with docs warning against
deriving names from request data.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The repo-access cache used by lockdown mode relied on cache2go's sliding
expiry: every read extends an entry's life, so a frequently-accessed
entry could keep a stale trust decision (e.g. revoked push access)
alive indefinitely instead of refreshing after its TTL.
Separately, cache2go.Cache(name) returns a process-wide singleton table
keyed by name. In HTTP mode, RequestDeps.GetRepoAccessCache built a new
RepoAccessCache per request but always reused the same default-named
table, so trust decisions computed under one caller's credentials could
be served to a different caller for the same owner/repo, without ever
validating the second caller's own access.
Fixes:
- Track each cache entry's original creation time and bound its maximum
age from that fixed point, not from last access, so entries are
refreshed after a fixed TTL regardless of read frequency.
- Add lockdown.CacheNameForIdentity, which derives a stable, hashed
cache-table name from a request identity (e.g. auth token). Two calls
for the same identity return the same name (reusing a warm cache
across a session's repeated requests); different identities always
get different names (no shared cache state).
- RequestDeps.GetRepoAccessCache now scopes each request's cache to the
requesting token's identity via CacheNameForIdentity, closing the
cross-identity leak in HTTP/multi-tenant deployments. Stdio mode is
unaffected: it constructs a single RepoAccessCache for the whole
process lifetime, as before.
Tests added:
- TestRepoAccessCacheBoundedExpiryIgnoresRepeatedAccess and
TestRepoAccessCacheNewUserDoesNotResetEntryAge exercise bounded expiry
deterministically via an injectable clock (no sleeps).
- TestCacheNameForIdentity and
TestRepoAccessCacheIdentityScopedNamesPreventCrossIdentityLeakage
cover the naming helper and cross-identity isolation at the lockdown
package level.
- TestGetRepoAccessCacheIsolatesTrustDecisionsPerIdentity in
pkg/github mirrors the HTTP server's exact construction pattern
end-to-end and fails without the dependencies.go fix.
Fixes#3107
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bounds the total HTTP request, so allow modest headroom over the MCP
SDK's 4 MiB default for JSON-RPC and tool-call envelope overhead rather
than spending the whole budget on tool content.
Because the limit now exceeds the SDK default, passing it to
StreamableHTTPOptions is load-bearing: without it the SDK would cap
requests at 4 MiB and the headroom would not exist. Covered by a test
that sends a request between the two limits.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The middleware default was an arbitrary 10 MiB, above the 4 MiB the SDK
already enforces, so it never changed which requests were accepted.
Alias mcp.DefaultMaxRequestBodyBytes instead, making the earlier
enforcement point behaviour-preserving by construction.
Also pass the effective limit to StreamableHTTPOptions. Previously the
SDK kept its own 4 MiB default, so a larger configured
MaxRequestBodyBytes was silently capped; both layers now agree.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
WithMCPParse and WithScopeChallenge tests for oversized requests were
using strings.NewReader, which gives httptest.NewRequest a known
Content-Length. That let WithMaxBodySize reject the request in its
fast path before the request ever reached the middleware's own
io.ReadAll/isMaxBytesError handling, leaving those branches untested.
Reuse the existing unknownLengthBody helper (body_limit_test.go) so
these tests actually reach the fallback read path and cover the
*http.MaxBytesError handling added in WithMCPParse and
WithScopeChallenge.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add WithMaxBodySize middleware that bounds the request body via
http.MaxBytesReader (with a fast Content-Length rejection when known),
registered first in RegisterMiddleware so it runs before any other
middleware or the MCP SDK reads or buffers the body.
WithMCPParse and WithScopeChallenge now return a clear 413 "request
body too large" response when their body read hits the limit, instead
of silently continuing.
Defaults to 10 MiB, overridable via ServerConfig.MaxRequestBodyBytes.
Fixes#3102
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Trim the GetPullRequestCommits doc comment to a single terse sentence
and remove a test comment that only restated the test name.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
rejectPathTraversal split components on literal "/" before checking
each segment, so a segment containing an encoded separator (e.g.
"%2e%2e%2fsecret.txt") decoded to "../secret.txt" instead of "..", and
the check never caught it. Percent-decoding a segment can therefore
introduce new "/"-separated subsegments that were invisible to the
original literal split.
Recursively re-split and re-check the decoded form whenever decoding
changes a segment, so a ".." revealed by one or more layers of
percent-decoding (including through an encoded separator, or
double-encoding) is rejected regardless of where it appears.
Add regression tests for encoded-separator traversal, encoded
separators in other components, and double percent-encoded dot-dot
segments, plus a benign percent-encoded filename case to confirm
non-traversal decodes still pass through.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
url.URL.JoinPath normalizes ".." segments before producing the final
URL. A path containing enough parent-directory segments could
therefore consume the owner, repo, and ref components already joined
onto the base URL, rebinding the raw.githubusercontent.com request to
a different owner/repository/ref than the caller specified.
Reject any owner, repo, ref/sha, or path component whose "/"-separated
segments are, or percent-decode to, ".." before building the URL.
Benign filenames such as "file..txt" or "..hidden" are unaffected.
URLFromOpts, refURL, and commitURL now return an error alongside the
URL string so this can be enforced at construction time; GetRawContent
propagates it. Adds table-driven tests covering normal, nested, and
benign double-dot paths as well as literal and percent-encoded
traversal attempts.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Address review feedback on the post-HTML-entity sanitization pass.
Entity decoding could still smuggle code-fence metadata past the
sanitizer. A first line such as "`​``steal secrets" is not a fence
in the raw input, so FilterCodeFenceMetadata left it alone; decoding the
entity and stripping the zero width space then produced a real fence with
its info string intact. Sanitize now re-runs the fence filter after the
input is fully normalized.
Filtering every variation selector also corrupted legitimate text: VS15
and VS16 select text or emoji presentation, so "✈️" was reduced to "✈",
and the Variation Selectors Supplement encodes registered CJK ideographic
variation sequences. Selectors are now filtered contextually. A selector
is kept when it can apply to the character it follows, and dropped when it
is orphaned, follows a removed or non-graphic character, or continues a
run of selectors. Supplement selectors additionally require a CJK
ideograph base, matching the Ideographic Variation Database. That keeps
the anti-smuggling property, since hidden payloads rely on selector runs,
without rewriting valid Unicode.
Also corrects a lowercase-hex test case that claimed uppercase digits.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
FilterInvisibleCharacters previously ran only before FilterHTMLTags,
so numeric HTML entities (e.g. ​ or ​) that bluemonday
decodes into invisible or bidirectional control characters could
survive sanitization untouched. Sanitize now applies the
invisible-character filter both before HTML processing (so raw
invisible characters don't interfere with code-fence parsing) and
again after, so entity-decoded characters cannot escape the policy.
Also expands the removal set to include:
- ARABIC LETTER MARK (U+061C), a directional format character in the
same family as the already-covered LRM/RLM marks.
- Variation selectors (U+FE00-U+FE0F) and the variation selectors
supplement (U+E0100-U+E01EF), which can be used to hide payloads
after emoji or other base characters.
Fixes#3101
Route every MinimalIssueRef/MinimalPullRequestRef construction through shared
constructors that sanitize the user-authored title, so issue_dependency_read,
issue_dependency_write and find_duplicate no longer forward raw issue titles.
Also sanitize the get_file_blame commit message headline, after truncation so
the headline is still cut at the author's real first line break.
Extends the sanitization regression suite with the project status update body,
both ref constructors and the dependency ref, and adds tool-level regression
tests for find_duplicate and get_file_blame.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sanitization was previously applied ad hoc at a handful of tool call
sites (GetIssue, GetPullRequest, ListPullRequests) rather than in the
shared convertToMinimal* converters, so equivalent user-authored text
returned by other tools (issue comments, PR reviews, review comments,
releases, commit messages, discussions, project item titles) was
returned unsanitized.
- Apply sanitize.Sanitize inside the convertToMinimal* helpers in
minimal_types.go for issue/PR titles and bodies, issue comments, PR
reviews, review comments, releases, commit messages, and project
item content titles. This is the single, shared conversion point
used by nearly every read tool, so fixing it there covers get/list
issues, pull requests, comments, reviews, review comments, releases,
commits, and project items consistently.
- Add a sanitizeIssueTitleAndBody helper and use it for the two
response paths that marshal a raw *github.Issue directly instead of
a Minimal* type: search_issues (SearchIssueResult.MarshalJSON) and
search_pull_requests (searchHandler).
- Sanitize discussion titles/bodies/comments (list_discussions,
get_discussion, get_discussion_comments), which previously had no
sanitization at all, via a new newMinimalDiscussionComment
constructor and inline fixes.
- Sanitize project status update bodies.
- Remove the now-redundant scattered sanitize calls in GetIssue,
GetPullRequest, and ListPullRequests now that the shared converters
sanitize on their own.
Patches, diffs, and raw file contents are intentionally left
untouched to preserve fidelity.
Adds table-driven regression tests covering every touched converter,
the search_issues/search_pull_requests raw-passthrough paths, and a
fidelity check that patches/diffs are not altered.
Fixes#3106
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pull_request_read's get_commits method previously returned commit
messages without any lockdown check, unlike get_diff and get_files
which restrict the whole result when the PR author lacks push access.
Commit content is part of the same untrusted head branch as the diff
and file list, so GetPullRequestCommits now reuses
enforcePullRequestLockdown for consistent, fail-closed behavior
without adding a per-commit permission lookup.
Also updates the lockdown documentation in README.md and
docs/server-configuration.md to:
- list pull_request_read:get_diff, get_files, and get_commits among
the tools that error when the PR author lacks push access (get_diff
and get_files were already implemented this way but undocumented)
- clarify that lockdown mode is a best-effort content filter to
reduce prompt-injection risk, not an authorization boundary
- document the existing intentional trusted-bot exception
(github-actions[bot], copilot) accurately
Fixes#3105
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Only tolerate GraphQL schema validation failures that show the optional Issue.issueFieldValues selection or its known fragments are unsupported. Surface client, auth, rate-limit, network, resolver, malformed response, and unrelated GraphQL failures.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Clarify that delete:false is ignored, retain mutual exclusion for delete:true, and reject invalid delete types. Add focused schema and handler regressions.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The `delete` property of issue_write's issue_fields items was declared with
Enum: []any{true}, making true its only legal value. The property is optional,
but a client that fills every property of a schema -- common, since
OpenAI-style strict function calling requires every property to appear in
`required` -- had no way to express "not deleting this field": there is no
false in the enum and no null in the type. `value` offers no alternative
either, being typed ["string","number","boolean"] with no null.
The MCP Go SDK validates arguments against the resolved input schema before
the handler runs, so delete: false was rejected at schema validation and never
reached optionalIssueWriteFields. Such clients sent delete: true alongside a
value instead and hit the handler's mutual-exclusion check, so issue_write
could never set an issue field for them.
Remove the enum so false is a legal no-op, and document that omitting the
property or setting it to false leaves the field unchanged. No handler change
is needed: the code already branches on `if deleteField`, so false falls
through to the normal value path, and the mutual-exclusion check for
delete: true still applies. Add tests for optionalIssueWriteFields, which had
none.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Report contentless large symlink targets as not returned while preserving their ResourceLink and requested-path identity.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 21420b11-5dae-49b6-ac77-965faec7f88b
Keep the lazy blob-identity check and bounded tree fallback while consolidating metadata handling and request-count tests.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 21420b11-5dae-49b6-ac77-965faec7f88b
Detect internal symlink dereferences from Git blob identity mismatches, disclose explicit links and submodules, and preserve requested-path resource output. Use bounded exact-path tree inspection only when inline content is unavailable.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove persistent repository and push-files guidance in favor of concise runtime recovery content. Resolve refs directly through Git Trees, skip inspection for explicit symlinks and opt-ins, and lock request counts in tests.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Detect existing symlinks through the Git tree and require an explicit opt-in before changing their targets. Return the resolved repository target so callers can safely update the linked file instead.
Refs #2997
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Normalize missing or zero-length tool arguments to an empty object while preserving invalid JSON and required-parameter validation.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Handle alternate issue-field validation messages, preserve primary and retry errors, and avoid runtime result type switches.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Retry list_issues without custom issue field dependencies only when the
host schema lacks them. Preserve explicit field filters and propagate
unrelated GraphQL errors.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Verify issue_read returns assigned logins and a definitive empty array for unassigned issues. Exercise the same empty-array contract through list_issues field filtering.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The list_issues GraphQL fragment never selected assignees, so the tool
could not report who an issue was assigned to. Its nearest field, user,
is the issue author, which callers conflate with the assignee. Answering
"is anything unassigned?" therefore cost one list_issues call plus one
issue_read per candidate, and a truncated sweep invites a fabricated
answer drawn from the author instead.
Add an assignees selection to IssueFragment, flatten it to logins in
fragmentToMinimalIssue, and add "assignees" to listIssuesItemFieldEnum so
it is selectable through fields. GitHub caps issue assignees at 10, so
first: 100 cannot truncate; it also matches the page size already used
for assignees in copilot.go.
Drop omitempty from MinimalIssue.Assignees and initialize the slice in
both converters so an unassigned issue serializes as [] rather than an
absent key, which is what lets a caller identify unassigned issues from
a single response. This also affects issue_read, the other MinimalIssue
consumer, which now reports "assignees": [] instead of omitting the key.
Both tools expose a delete action, so the tool-level annotation must remain
conservative even though their other actions only update preferences.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: be9c15fe-a113-4ebd-b9ab-09a520d522b6
create_branch currently forwards the compact 422 dump, which hides
ruleset details the GitHub UI already shows. Unwrap ErrorResponse so agents
can see each validation message and recover.
Use exact configured host authorities for every REST, GraphQL, and raw client so redirects cannot reattach credentials to foreign hosts or ports. Add adversarial redirect and lookalike coverage.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
BearerAuthTransport re-adds the Authorization header on every hop, which
defeats net/http's cross-host redirect stripping. Scope the credential to
the configured hosts so a redirect off them travels without the token.
An empty AllowedHosts preserves prior behavior; the three production
construction sites populate it from the configured REST, upload, GraphQL
and raw hosts.
Sam Morrow