[pull] trunk from cli:trunk

.github/codeql/codeql-config.yml

@@ -0,0 +1,19 @@
+name: "cli/cli CodeQL config"
+
+# This config extends the default `security-and-quality` suite with the
+# custom queries in `.github/codeql/queries/`. The custom queries enforce
+# project-specific invariants that are not covered by the stock packs:
+#
+#   - unsanitized-response-to-terminal.ql: HTTP response content that
+#     reaches a terminal writer (`os.Stdout` / `os.Stderr` /
+#     `iostreams.IOStreams.Out` / `ErrOut`) other than `ContentOut`
+#     without being sanitized. Writing to `ContentOut`, calling
+#     `iostreams.Untrusted.String`, wrapping with `asciisanitizer`, or
+#     decoding as structured JSON are accepted, so untrusted response
+#     content is sanitized before it can reach a terminal.
+#
+# This config is only meaningful for the Go matrix entry; the Actions
+# matrix entry ignores it.
+queries:
+  - uses: security-and-quality
+  - uses: ./.github/codeql/queries

.github/codeql/codeql-pack.lock.yml

@@ -0,0 +1,24 @@
+---
+lockVersion: 1.0.0
+dependencies:
+  codeql/concepts:
+    version: 0.0.24
+  codeql/controlflow:
+    version: 2.0.34
+  codeql/dataflow:
+    version: 2.1.6
+  codeql/go-all:
+    version: 7.1.1
+  codeql/mad:
+    version: 1.0.50
+  codeql/ssa:
+    version: 2.0.26
+  codeql/threat-models:
+    version: 1.0.50
+  codeql/tutorial:
+    version: 1.0.50
+  codeql/typetracking:
+    version: 2.0.34
+  codeql/util:
+    version: 2.0.37
+compiled: false

.github/codeql/qlpack.yml

@@ -0,0 +1,9 @@
+name: cli/cli-custom-security
+version: 0.0.1
+library: false
+extractor: go
+tests: tests
+dependencies:
+  codeql/go-all: ^7.1.1
+default-suite:
+  - queries: queries

.github/codeql/queries/ImmutableSafeURLConstruction.ql

@@ -0,0 +1,58 @@
+/**
+ * @name ImmutableSafeURL built from a hand-assembled string
+ * @description Flags a call to safeurl.NewImmutableSafeURL whose argument is a locally assembled
+ *              string, that is a value tainted by fmt.Sprintf, fmt.Sprint, fmt.Sprintln or a string
+ *              concatenation. NewImmutableSafeURL renders its argument verbatim, skipping the
+ *              percent-encoding and traversal check that JoinPath applies, so it must only receive an
+ *              already formed, trusted URL such as a server returned field or a pagination link. A
+ *              hand-built path reaching it is a way to route around safeurl and must instead be built
+ *              with safeurl.JoinPath. This query is a convention guard, it cannot and does not verify
+ *              the trustedness of URLs read from struct fields or returned by API calls.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id cli-cli/immutable-safeurl-construction
+ * @tags security
+ *       correctness
+ *       maintainability
+ */
+
+import go
+
+/**
+ * Holds when `node` is the URL argument of a call to safeurl.NewImmutableSafeURL, the escape hatch
+ * that renders its argument verbatim without percent-encoding or a traversal check.
+ */
+predicate isImmutableSafeURLArgument(DataFlow::Node node) {
+  exists(Function f, DataFlow::CallNode call |
+    f.hasQualifiedName("github.com/cli/cli/v2/internal/safeurl", "NewImmutableSafeURL") and
+    call = f.getACall() and
+    node = call.getArgument(0)
+  )
+}
+
+/**
+ * Holds when `node` is a locally assembled string: the result of fmt.Sprintf, fmt.Sprint or
+ * fmt.Sprintln, or a string concatenation expression. These are the shapes that build a URL by hand
+ * rather than reading an already formed value, so they must not reach NewImmutableSafeURL.
+ */
+predicate isHandAssembledString(DataFlow::Node node) {
+  exists(Function f |
+    f.hasQualifiedName("fmt", ["Sprintf", "Sprint", "Sprintln"]) and
+    node = f.getACall()
+  )
+  or
+  exists(AddExpr e |
+    e.getType() instanceof StringType and
+    node = DataFlow::exprNode(e)
+  )
+}
+
+from DataFlow::Node source, DataFlow::Node sink
+where
+  isImmutableSafeURLArgument(sink) and
+  isHandAssembledString(source) and
+  TaintTracking::localTaint(source, sink)
+select sink,
+  "This ImmutableSafeURL is built from a hand-assembled string ($@); build the path with safeurl.JoinPath so its components are escaped and traversal-checked.",
+  source, "assembled here"

.github/codeql/queries/SafeURLPathConstruction.ql

@@ -0,0 +1,73 @@
+/**
+ * @name HTTP request URL not built with safeurl.SafeURL
+ * @description Flags any HTTP request, a REST API call being the common case, whose URL argument is
+ *              not literally a call to (safeurl.SafeURL).String. The argument expression itself must
+ *              be a SafeURL.String call; any other form, such as a string literal, string
+ *              concatenation, or fmt.Sprintf, is reported. This keeps every hand built URL routed
+ *              through safeurl so its variable path components are percent-encoded.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id cli-cli/safeurl-path-construction
+ * @tags security
+ *       correctness
+ *       maintainability
+ */
+
+import go
+
+/**
+ * Holds when `node` is the URL argument of an HTTP request, a REST API call being the common case.
+ *
+ * Covered entry points:
+ *   - (github.com/cli/cli/v2/api.Client).REST and .RESTWithNext, where the path is argument 2.
+ *   - net/http.NewRequest, where the URL is argument 1.
+ *   - net/http.NewRequestWithContext, where the URL is argument 2.
+ *   - (net/http.Client).Get, .Head, .Post and .PostForm, where the URL is argument 0.
+ */
+predicate isHttpUrlArgument(DataFlow::Node node) {
+  exists(Method m, DataFlow::CallNode call |
+    m.hasQualifiedName("github.com/cli/cli/v2/api", "Client", ["REST", "RESTWithNext"]) and
+    call = m.getACall() and
+    node = call.getArgument(2)
+  )
+  or
+  exists(Function f, DataFlow::CallNode call |
+    f.hasQualifiedName("net/http", "NewRequest") and
+    call = f.getACall() and
+    node = call.getArgument(1)
+  )
+  or
+  exists(Function f, DataFlow::CallNode call |
+    f.hasQualifiedName("net/http", "NewRequestWithContext") and
+    call = f.getACall() and
+    node = call.getArgument(2)
+  )
+  or
+  exists(Method m, DataFlow::CallNode call |
+    m.hasQualifiedName("net/http", "Client", ["Get", "Head", "Post", "PostForm"]) and
+    call = m.getACall() and
+    node = call.getArgument(0)
+  )
+}
+
+/**
+ * Holds when `node` is a call to the String method of one of the safeurl URL types:
+ * the SafeURL interface or either of its implementations, MutableSafeURL and
+ * ImmutableSafeURL. Matching all three keeps call sites free of explicit conversions:
+ * a value of the concrete type can be passed to the sink directly without first being
+ * assigned to a SafeURL typed variable.
+ */
+predicate isSafeurlStringCall(DataFlow::Node node) {
+  exists(Method m |
+    m.hasQualifiedName("github.com/cli/cli/v2/internal/safeurl",
+      ["SafeURL", "MutableSafeURL", "ImmutableSafeURL"], "String") and
+    node = m.getACall()
+  )
+}
+
+from DataFlow::Node sink
+where
+  isHttpUrlArgument(sink) and
+  not isSafeurlStringCall(sink)
+select sink, "This HTTP request URL is not passed directly as the result of safeurl.SafeURL.String."

.github/codeql/queries/examples/UnsanitizedResponseToTerminalBad.go

@@ -0,0 +1,34 @@
+package example
+
+import (
+	"fmt"
+	"io"
+	"net/http"
+
+	"github.com/cli/cli/v2/pkg/iostreams"
+)
+
+type Options struct {
+	IO         *iostreams.IOStreams
+	HTTPClient *http.Client
+	URL        string
+}
+
+func run(opts *Options) error {
+	resp, err := opts.HTTPClient.Get(opts.URL)
+	if err != nil {
+		return err
+	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return err
+	}
+
+	// BAD: server-controlled bytes are written to IO.Out, which does not
+	// sanitize. Any ANSI escape sequences in the response will be rendered
+	// by the user's terminal.
+	fmt.Fprint(opts.IO.Out, string(body))
+	return nil
+}

.github/codeql/queries/examples/UnsanitizedResponseToTerminalGood.go

@@ -0,0 +1,52 @@
+package example
+
+import (
+	"fmt"
+	"io"
+	"net/http"
+
+	"github.com/cli/cli/v2/pkg/iostreams"
+	"github.com/spf13/cobra"
+)
+
+type Options struct {
+	IO         *iostreams.IOStreams
+	HTTPClient *http.Client
+	URL        string
+
+	AllowEscapeSequences bool
+}
+
+func newCmd() *cobra.Command {
+	opts := &Options{}
+	cmd := &cobra.Command{
+		Use:  "fetch",
+		RunE: func(*cobra.Command, []string) error { return run(opts) },
+	}
+	cmd.Flags().BoolVar(&opts.AllowEscapeSequences, "allow-escape-sequences", false,
+		"Allow printing terminal escape sequences")
+	return cmd
+}
+
+func run(opts *Options) error {
+	if opts.AllowEscapeSequences {
+		opts.IO.SetContentSanitization(false)
+	}
+
+	resp, err := opts.HTTPClient.Get(opts.URL)
+	if err != nil {
+		return err
+	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return err
+	}
+
+	// GOOD: external bytes flow through ContentOut, which sanitizes ANSI
+	// escape sequences by default. The --allow-escape-sequences flag is the
+	// documented opt-out for trusted content.
+	fmt.Fprint(opts.IO.ContentOut, string(body))
+	return nil
+}

.github/codeql/queries/unsanitized-response-to-terminal.md

@@ -0,0 +1,118 @@
+<!-- Generated from unsanitized-response-to-terminal.qhelp via 'codeql generate query-help'. Regenerate after editing the .qhelp source. -->
+# HTTP response content reaches a terminal without ContentOut or sanitization
+Bytes consumed from an HTTP response body are server-controlled and may contain ANSI escape sequences. When those bytes reach a terminal writer without sanitization, a remote attacker can move the cursor, repaint the screen, fake a shell prompt, write to the clipboard via OSC sequences, or otherwise manipulate the user's terminal session.
+
+This query flags HTTP response content, including bytes reintroduced by base64 decoding, that reaches a terminal writer (`IOStreams.Out`, `IOStreams.ErrOut`, `os.Stdout`, or `os.Stderr`) without first being written to `IOStreams.ContentOut`, sanitized, or decoded as a structured format (e.g. `encoding/json`).
+
+
+## Recommendation
+Choose the writer based on the kind of content you are printing:
+
+* `IOStreams.Out` is for application output the developer authored: tables, prompts, formatted messages, color-coded status. It does not sanitize and never should, because the developer controls every byte that reaches it.
+* `IOStreams.ContentOut` is for external content the developer did not author: HTTP response bodies, file contents fetched from a remote, anything where a third party chose the bytes. It sanitizes ANSI escape sequences by default.
+These patterns satisfy the query:
+
+1. Label the content at its source as `iostreams.Untrusted` and print it with `String()` (or any `fmt` verb, which calls `String()`); the value sanitizes itself. Its `Raw()` method is the explicit opt-out and is still flagged if it reaches a terminal.
+1. Write external bytes to `IOStreams.ContentOut`.
+1. Decode the bytes into a structured value first (`json.Unmarshal`, `(*json.Decoder).Decode`); the fields you print afterwards are no longer raw external content.
+1. For commands where the user has opted into raw output, add a per-command `--allow-escape-sequences` flag and call `opts.IO.SetContentSanitization(false)` before writing. The bytes still go through `ContentOut`, but ContentOut becomes a passthrough for that invocation.
+
+## Example
+In the following BAD example, the response body is written directly to `IOStreams.Out`. A server can embed ANSI escape sequences in the response and they will be rendered by the user's terminal:
+
+
+```go
+package example
+
+import (
+	"fmt"
+	"io"
+	"net/http"
+
+	"github.com/cli/cli/v2/pkg/iostreams"
+)
+
+type Options struct {
+	IO         *iostreams.IOStreams
+	HTTPClient *http.Client
+	URL        string
+}
+
+func run(opts *Options) error {
+	resp, err := opts.HTTPClient.Get(opts.URL)
+	if err != nil {
+		return err
+	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return err
+	}
+
+	// BAD: server-controlled bytes are written to IO.Out, which does not
+	// sanitize. Any ANSI escape sequences in the response will be rendered
+	// by the user's terminal.
+	fmt.Fprint(opts.IO.Out, string(body))
+	return nil
+}
+
+```
+In the following GOOD example, the same body is written to `IOStreams.ContentOut`, which sanitizes ANSI escape sequences. An `--allow-escape-sequences` flag is provided for users who explicitly want raw output for trusted content:
+
+
+```go
+package example
+
+import (
+	"fmt"
+	"io"
+	"net/http"
+
+	"github.com/cli/cli/v2/pkg/iostreams"
+	"github.com/spf13/cobra"
+)
+
+type Options struct {
+	IO         *iostreams.IOStreams
+	HTTPClient *http.Client
+	URL        string
+
+	AllowEscapeSequences bool
+}
+
+func newCmd() *cobra.Command {
+	opts := &Options{}
+	cmd := &cobra.Command{
+		Use:  "fetch",
+		RunE: func(*cobra.Command, []string) error { return run(opts) },
+	}
+	cmd.Flags().BoolVar(&opts.AllowEscapeSequences, "allow-escape-sequences", false,
+		"Allow printing terminal escape sequences")
+	return cmd
+}
+
+func run(opts *Options) error {
+	if opts.AllowEscapeSequences {
+		opts.IO.SetContentSanitization(false)
+	}
+
+	resp, err := opts.HTTPClient.Get(opts.URL)
+	if err != nil {
+		return err
+	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return err
+	}
+
+	// GOOD: external bytes flow through ContentOut, which sanitizes ANSI
+	// escape sequences by default. The --allow-escape-sequences flag is the
+	// documented opt-out for trusted content.
+	fmt.Fprint(opts.IO.ContentOut, string(body))
+	return nil
+}
+
+```