[pull] trunk from cli:trunk - #201
Merged
Merged
Conversation
* Add terminal-safety mechanisms for untrusted content Introduce the building blocks for keeping untrusted external content (HTTP response bodies and the like) from reaching a terminal as live ANSI escape sequences, while leaving the application's own styled output untouched. - iostreams.Untrusted: a value type that wraps external content. The raw bytes are unexported; String sanitizes and is called automatically by fmt, so the default print path is safe. Raw and RawBytes are the explicit, greppable opt-out for non-terminal uses (disk, the API, hashing), and the type carries its label across JSON decoding so a decoded field stays marked. - iostreams.ContentOut and SetContentSanitization: a sink that sanitizes raw external streams by default and becomes a passthrough when a command opts out, so the sanitization decision is made at the moment of writing. - A CodeQL query (with help text, examples, and a per-category test suite) that flags HTTP response content reaching a terminal writer other than ContentOut without sanitization, treating ContentOut, Untrusted.String, the asciisanitizer wrap, and structured JSON decoding as the accepted resolutions. Wire it into the existing CodeQL workflow. Co-authored-by: Copilot App <[email protected]> * Migrate gist view to the terminal-safety mechanisms GetRawGistFile now returns iostreams.Untrusted, so the two callers are forced to declare intent. gist view carries the value to the render function and lets the sink decide: the raw dump goes through ContentOut (which honors --allow-escape-sequences) and the markdown path sanitizes its input with String before rendering to Out. gist edit takes Raw, because the content is opened in an editor and sent back to the API and must round-trip verbatim. Co-authored-by: Copilot App <[email protected]> * Migrate skills to the terminal-safety mechanisms FetchBlob base64-decodes blob content out of the JSON response, so it now returns iostreams.Untrusted and its callers split by intent: skills preview sanitizes for display, while the installer (writes the file to disk) and the frontmatter parsers take Raw to operate on the verbatim bytes. Co-authored-by: Copilot App <[email protected]> * Migrate agent-task streaming logs to the terminal-safety mechanisms The agent-task chat completion chunk's Content and ReasoningText fields arrive over a streaming response, which is not a JSON content type, so the JSON transport sanitizer does not run on them. Type those fields as iostreams.Untrusted so provenance survives the per-line json.Unmarshal; printing them later sanitizes, and presence checks use Empty. This covers a path that flows resp.Body through json.Unmarshal to a printed field, which value dataflow cannot track but the type expresses directly. Co-authored-by: Copilot App <[email protected]> * Migrate repo read-file to the terminal-safety mechanisms read-file already refuses to print terminal escape sequences by default (and opts in with --allow-escape-sequences), so it guards its own bytes. Route its two raw writes through ContentOut instead of Out, in passthrough mode, so it uses the one sink the terminal-safety query recognizes while keeping its refuse-by-default behavior. Passthrough is required here: sanitizing would corrupt binary files and strip the escapes that --allow-escape-sequences explicitly allows. Co-authored-by: Copilot App <[email protected]> * Migrate pr diff to the terminal-safety mechanisms pr diff neutralized escape sequences only when stdout was a terminal, leaving piped and redirected output raw. Neutralize by default in all modes instead: the plain and name-only paths write through ContentOut, and the colored path wraps the diff reader in the shared asciisanitizer, since colored output is always terminal-bound. Add --allow-escape-sequences to opt back into raw bytes for the non-colored paths, for example when piping a patch to another program. This also replaces the command's bespoke sanitizer with the shared asciisanitizer. Co-authored-by: Copilot App <[email protected]> * Migrate api response output to the terminal-safety mechanisms gh api copied non-JSON response bodies straight to stdout, so escape sequences in the response reached the terminal unneutralized (JSON bodies are already cleaned by the transport). Route the raw copy through ContentOut, which neutralizes escape sequences on an interactive terminal, stays raw when piped so binary payloads are not corrupted, and honors --allow-escape-sequences. Silent, verbose, and slurp writers are left in place. The colorized JSON path now sanitizes its input before jsoncolor adds color, keeping the color intact while removing the raw bytes. Co-authored-by: Copilot App <[email protected]> * Migrate release download stdout to the terminal-safety mechanisms release download --output - copied asset bytes straight to stdout, so escape sequences in an asset reached the terminal unneutralized. Route the stdout write through ContentOut, which neutralizes escape sequences on an interactive terminal, stays raw when piped or redirected so binary assets are not corrupted, and honors --allow-escape-sequences. File downloads are unaffected. Co-authored-by: Copilot App <[email protected]> * Migrate codespace logs to the terminal-safety mechanisms gh codespace logs streams a remote log file over ssh by running cat or tail -f, and the shared ssh helper wires the command's stdout straight to os.Stdout, so the file's bytes reach the terminal without passing through this process and cannot be neutralized. Point the logs command's stdout at ContentOut instead: on a terminal this sanitizes escape sequences and forces the remote output through the process, while piped output stays raw so a follow stream is not buffered and saved logs keep their exact bytes. The shared helper is left untouched so the interactive ssh shell still passes control sequences through. Co-authored-by: Copilot App <[email protected]> * Build the CodeQL Go database from the shipped build The Go analysis previously relied on autobuild, which walks the whole tree and extracts the nested module under .github/codeql/queries' test fixtures as well as the main module. Both declare the same module path with their own pkg/iostreams, so the analyzer conflated the two and reported a flow through unrelated code. Switch the Go job to build-mode: manual and build with make, the same command the integration tests use. Extraction is then scoped to the packages the released gh binary compiles, which excludes the separate fixtures module, so the spurious finding is gone and the analyzed code matches what we ship. The actions job builds nothing, so it is marked build-mode: none. The query's own test suite is unchanged and still runs each fixture directory in isolation. Co-authored-by: Copilot App <[email protected]> * Match guarded-content errors by kind in tests Replace the binary-error boolean with a `wantErrAs` field so the table matches typed errors with errors.As and sentinels with errors.Is, mirroring how callers detect them. Any future typed error slots in without a new field. Co-authored-by: Copilot App <[email protected]> Copilot-Session: 53357989-fb84-4835-9eee-258d59e755e5 * Clarify that Untrusted wraps string content Co-authored-by: Copilot App <[email protected]> * Convert Untrusted tests to testify assertions Co-authored-by: Copilot App <[email protected]> * Collapse guarded content cases into the table test Co-authored-by: Copilot App <[email protected]> * Match binary terminal errors with errors.AsType Co-authored-by: Copilot App <[email protected]> * Reuse the raw gist content when writing it out Co-authored-by: Copilot App <[email protected]> * Cover the non-truncated gist file path Co-authored-by: Copilot App <[email protected]> * Document the refusal guarantee on CopyGuardedContent Co-authored-by: Copilot App <[email protected]> * Clear stale content when unmarshaling JSON null Co-authored-by: Copilot App <[email protected]> --------- Co-authored-by: Copilot App <[email protected]> Copilot-Session: 53357989-fb84-4835-9eee-258d59e755e5
* feat(safeurl): add SafeURL package and CodeQL enforcement query Introduce internal/safeurl, which builds HTTP request URLs from variable components that are percent-encoded when rendered, so user or server controlled values cannot break the path or change which resource is addressed. It provides the SafeURL interface, the MutableSafeURL and ImmutableSafeURL implementations, the JoinPath and JoinPathWithHostPrefix builders, and NewImmutableSafeURL for entrusting already-formed URLs. Because percent-encoding leaves a component that is exactly ".." intact as a real path segment, JoinPath and JoinPathWithHostPrefix reject any such component and return an error, which callers bubble up to the command level. Also add a CodeQL query that flags any HTTP request URL argument that is not literally the result of a safeurl.SafeURL.String call. Co-authored-by: Copilot <[email protected]> * refactor(api): route REST paths through SafeURL Build the hand-written REST paths in the shared API layer with safeurl so their variable components are escaped. CreateRepoTransformToV4 now takes a safeurl.SafeURL path instead of a string. Co-authored-by: Copilot <[email protected]> * refactor(repo): build REST paths with SafeURL Escape the variable components of the repo command REST paths. The new read-file command builds its Contents API URL with safeurl, and the repo create and edit commands pass a safeurl.SafeURL to CreateRepoTransformToV4. Co-authored-by: Copilot <[email protected]> * refactor(run): build REST paths with SafeURL Escape the variable components of the run command REST paths. GetJobs no longer mutates the Run; it takes an entrusted jobs URL and a run id and returns the jobs, so its callers assign them explicitly. Co-authored-by: Copilot <[email protected]> * refactor(release): build REST paths with SafeURL Escape the variable components of the release command REST paths. The upload and delete-asset pipeline threads a safeurl.SafeURL through, so the asset upload URL and AssetForUpload.ExistingURL are carried as SafeURL. Co-authored-by: Copilot <[email protected]> * refactor(codespace): build REST paths with SafeURL Escape the variable components of the codespace command REST paths. The NWO validation helper moves out of safeurl into the codespace package, where it is the only remaining caller. Co-authored-by: Copilot <[email protected]> * refactor(attestation): build bundle URLs with SafeURL getBundle takes a safeurl.SafeURL, and the server-returned bundle URL is entrusted with NewImmutableSafeURL at the API boundary. Co-authored-by: Copilot <[email protected]> * refactor(gist): build REST paths with SafeURL Co-authored-by: Copilot <[email protected]> * refactor(workflow): build REST paths with SafeURL Co-authored-by: Copilot <[email protected]> * refactor: build REST paths with SafeURL in secret, variable, ssh-key, and gpg-key commands Escape the variable name components of these commands' REST paths and surface any path-traversal error from the builders. Co-authored-by: Copilot <[email protected]> * refactor(skills): build REST paths with SafeURL Co-authored-by: Copilot <[email protected]> * refactor: build remaining REST paths with SafeURL Escape the variable components of the remaining hand-written REST paths across the pr, ruleset, label, copilot, agent-task, auth, cache, extension, status, and api commands, plus the search, feature detection, and update internals, surfacing any path-traversal error from the builders. Co-authored-by: Copilot <[email protected]> * test(safeurl): cover dot-segment escaping and preservation Add regression tests confirming a pre-encoded "%2e%2e" component is double-encoded rather than treated as traversal, and that single "." components are preserved verbatim instead of being collapsed. These guard against re-introducing url.JoinPath/path.Clean and against bypassing the traversal check via pre-encoding. Covers both JoinPath and JoinPathWithHostPrefix. Co-authored-by: Copilot <[email protected]> Copilot-Session: 2f72878e-d567-4a58-937d-f03c94c0287e * feat(codeql): flag hand-assembled strings reaching NewImmutableSafeURL Add a query that reports when the argument to safeurl.NewImmutableSafeURL is tainted by fmt.Sprintf, fmt.Sprint, fmt.Sprintln or a string concatenation. NewImmutableSafeURL renders its argument verbatim, bypassing the escaping and traversal check that JoinPath applies, so it must only receive an already formed, trusted URL. This is a convention guard and cannot verify the trustedness of URLs read from struct fields or returned by API calls. Co-authored-by: Copilot <[email protected]> Copilot-Session: 2f72878e-d567-4a58-937d-f03c94c0287e * refactor: rename pagination cursor to pageURL to avoid shadowing net/url The pagination cursor was named url, which shadows the net/url import in run/shared/shared.go where GetJobs and GetRun still use it. Rename it to pageURL, which also reads more accurately, and apply the same name to the other pagination loops so they stay consistent. Co-authored-by: Copilot <[email protected]> Copilot-Session: 2f72878e-d567-4a58-937d-f03c94c0287e * refactor(safeurl): reject owner/repo values with extra slashes RepoPartsFromNWO used strings.Cut, which splits on the first slash and left any remaining slashes in the name part, so "owner/repo/extra" silently parsed as name "repo/extra". Switch to strings.Split and require exactly one slash with a non-empty owner and name, so a value carrying extra slashes cannot smuggle additional path segments through. Also document why this does not reuse ghrepo.FromFullName, which accepts the broader "[HOST/]OWNER/REPO" form. Co-authored-by: Copilot <[email protected]> Copilot-Session: 2f72878e-d567-4a58-937d-f03c94c0287e --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Kynan Ware <[email protected]> Copilot-Session: 2f72878e-d567-4a58-937d-f03c94c0287e
Signed-off-by: Babak K. Shandiz <[email protected]>
* Escape regex metacharacters in attestation SAN matching Apply regexp.QuoteMeta to user-supplied values interpolated into regex patterns in expandToGitHubURLRegex and validateSignerWorkflow. Without escaping, dots in org/repo names act as regex wildcards, allowing attestation spoofing via lookalike repositories. Co-authored-by: Copilot <[email protected]> * Address review: QuoteMeta entire static string at once Per review feedback, wrap the full formatted string in QuoteMeta rather than individual variables, to better indicate the whole piece is escaped and avoid confusion around trailing/leading chars. Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Copilot <[email protected]>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )