Merge pull request #13272 from cli/wm/fix-log-terminal-injection

Fix log terminal injection

acceptance/testdata/workflow/run-view-log-escape-sequences.txtar

@@ -0,0 +1,70 @@
+# This test ensures that a malicious workflow which emit terminal control sequences (ESC, OSC, CSI) in
+# its log output does not result in terminal injection when logs are displayed using `gh run view --log`
+
+# Use gh as a credential helper
+exec gh auth setup-git
+
+# Create a repository with a file so it has a default branch
+exec gh repo create $ORG/$SCRIPT_NAME-$RANDOM_STRING --add-readme --private
+
+# Defer repo cleanup
+defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING
+
+# Clone the repo
+exec gh repo clone $ORG/$SCRIPT_NAME-$RANDOM_STRING
+
+# Commit the workflow file
+cd $SCRIPT_NAME-$RANDOM_STRING
+mkdir .github/workflows
+mv ../workflow.yml .github/workflows/workflow.yml
+exec git add .github/workflows/workflow.yml
+exec git commit -m 'Create workflow with escape sequences'
+exec git push -u origin main
+
+# Sleep because it takes a second for the workflow to register
+sleep 1
+
+# Run the workflow
+exec gh workflow run 'Escape Sequence PoC'
+
+# It takes some time for a workflow run to register
+sleep 10
+
+# Get the run ID we want to view
+exec gh run list --json databaseId --jq '.[0].databaseId'
+stdout2env RUN_ID
+
+# Wait for workflow to complete
+exec gh run watch $RUN_ID --exit-status
+
+# View the logs and check that raw ESC bytes (0x1b) are NOT present in output.
+# If this assertion fails, it means terminal escape sequences from the workflow
+# log are being passed through to the user's terminal unsanitised.
+exec gh run view $RUN_ID --log
+
+# The output should contain the safe/visible text but not raw ESC bytes.
+# \x1b is the ESC byte - it must not appear in the output.
+! stdout '\x1b'
+
+# The log output should still contain the non-escape parts of the log lines.
+stdout 'ESCAPE_MARKER_START'
+stdout 'ESCAPE_MARKER_END'
+
+-- workflow.yml --
+name: Escape Sequence PoC
+
+on:
+  workflow_dispatch:
+
+jobs:
+  emit-escape-sequences:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Emit terminal escape sequences
+        run: |
+          # OSC title set: \x1b]0;TITLE\x07
+          printf 'ESCAPE_MARKER_START \033]0;HIJACKED_TITLE\007 ESCAPE_MARKER_END\n'
+          # CSI color: \x1b[31m ... \x1b[0m
+          printf 'ESCAPE_MARKER_START \033[31mRED_TEXT\033[0m ESCAPE_MARKER_END\n'
+          # Screen title set (from original PoC): \x1bk ... \x1b\\
+          printf 'ESCAPE_MARKER_START \033k;malicious command;\033\\ ESCAPE_MARKER_END\n'

pkg/cmd/run/view/view.go

@@ -22,7 +22,9 @@ import (
 	"github.com/cli/cli/v2/pkg/cmd/run/shared"
 	"github.com/cli/cli/v2/pkg/cmdutil"
 	"github.com/cli/cli/v2/pkg/iostreams"
+	"github.com/cli/go-gh/v2/pkg/asciisanitizer"
 	"github.com/spf13/cobra"
+	"golang.org/x/text/transform"
 )
 
 type RunLogCache struct {
@@ -579,7 +581,8 @@ func displayLogSegments(w io.Writer, segments []logSegment) error {
 }
 
 func copyLogWithLinePrefix(w io.Writer, r io.Reader, prefix string) error {
-	scanner := bufio.NewScanner(r)
+	sanitized := transform.NewReader(r, &asciisanitizer.Sanitizer{})
+	scanner := bufio.NewScanner(sanitized)
 	for scanner.Scan() {
 		fmt.Fprintf(w, "%s%s\n", prefix, scanner.Text())
 	}

pkg/cmd/run/view/view_test.go

@@ -2759,6 +2759,46 @@ var expectedRunLogOutput = fmt.Sprintf("%s%s", coolJobRunLogOutput, sadJobRunLog
 var expectedRunLogOutputWithNoSteps = fmt.Sprintf("%s%s", coolJobRunWithNoStepLogsLogOutput, sadJobRunWithNoStepLogsLogOutput)
 var expectedLegacyRunLogOutputWithNoSteps = fmt.Sprintf("%s%s", legacyCoolJobRunWithNoStepLogsLogOutput, legacySadJobRunWithNoStepLogsLogOutput)
 
+func TestCopyLogWithLinePrefix_TerminalEscapeSequences(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+	}{
+		{
+			name:  "OSC title set sequence",
+			input: "normal prefix\x1b]0;HIJACKED TITLE\x07trailing text\n",
+		},
+		{
+			name:  "CSI color sequence",
+			input: "\x1b[31mRED TEXT\x1b[0m normal text\n",
+		},
+		{
+			name:  "screen title set sequence used in original report",
+			input: "\x1bk;echo this is an arbitrary command;\x1b\\\n",
+		},
+		{
+			name:  "CSI window title query",
+			input: "before\x1b[21tafter\n",
+		},
+		{
+			name:  "multiple escape sequences",
+			input: "\x1b]0;title\x07\x1b[31mred\x1b[0m\x1b[21t\n",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var buf bytes.Buffer
+			err := copyLogWithLinePrefix(&buf, strings.NewReader(tt.input), "jobname\tstep\t")
+			require.NoError(t, err)
+
+			output := buf.String()
+			assert.NotContains(t, output, "\x1b",
+				"output should not contain raw ESC (0x1b) bytes, got: %q", output)
+		})
+	}
+}
+
 func TestRunLog(t *testing.T) {
 	t.Run("when the cache dir doesn't exist, exists return false", func(t *testing.T) {
 		cacheDir := t.TempDir() + "/non-existent-dir"