fix(executor): read the step summary from the checked descriptor, bounded

The size check and the read opened the file twice, and the read was unbounded.
The summary is now read through io.LimitReader(limit+1) on the descriptor that
was checked, so a file that grows afterwards cannot be read whole.

commit: 9911deb9cff59669c57c5fb2fe579f057cf6768d
author: brice · 8d ago · 2026-10-14 02:40 UTC
parent: f0e28f8
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +64 -4

M internal/executor/filecmd.go +25 -4


@@ -238,11 +238,17 @@ func readText(root *os.Root, name string) (text string, ok bool, err error) {
238	238		if err != nil {
239	239		return "", false, err
240	240		}
	241	+	return decodeText(b), true, nil
	242	+	}
	243	+
	244	+	// decodeText is the decoding of File.ReadAllText: a UTF-8 BOM is dropped and
	245	+	// invalid bytes become U+FFFD.
	246	+	func decodeText(b []byte) string {
241	247		b = bytes.TrimPrefix(b, []byte("\xef\xbb\xbf"))
242	248		if !utf8.Valid(b) {
243	249		b = bytes.ToValidUTF8(b, []byte("\ufffd"))
244	250		}
245		-	return string(b), true, nil
	251	+	return string(b)
246	252		}
247	253
248	254		// createExcl creates name in root with the content, failing if it exists (an

@@ -354,6 +360,10 @@ func readEnvFile(sink Sink, root *os.Root, name string) ([]Pair, error) {
354	360		return ParseEnvFile(text)
355	361		}
356	362
	363	+	// afterSummaryStat is called between the size check of the summary and its
	364	+	// read; tests use it to make the file grow.
	365	+	var afterSummaryStat func()
	366	+
357	367		// processStepSummary is CreateStepSummaryCommand.ProcessCommand
358	368		// (FileCommandManager.cs:230-275): a missing or empty file is ignored, a file
359	369		// above 1 MiB is an error; otherwise the lines are masked one by one and the

@@ -366,8 +376,8 @@ func processStepSummary(sink Sink, root *os.Root, name string) error {
366	376		if err != nil {
367	377		return err
368	378		}
	379	+	defer func() { _ = fh.Close() }()
369	380		st, err := fh.Stat()
370		-	_ = fh.Close()
371	381		if err != nil \|\| st.Size() == 0 {
372	382		return nil
373	383		}

@@ -382,10 +392,21 @@ func processStepSummary(sink Sink, root *os.Root, name string) error {
382	392		if !ok {
383	393		return nil
384	394		}
385		-	text, ok, err := readText(root, name)
386		-	if err != nil \|\| !ok {
	395	+	if afterSummaryStat != nil {
	396	+	afterSummaryStat()
	397	+	}
	398	+	// The file belongs to the user of the step, who can still write to it (a
	399	+	// background process of the step): what is read is bounded by the limit on
	400	+	// the descriptor that was checked, not by the size seen a moment ago.
	401	+	b, err := io.ReadAll(io.LimitReader(fh, stepSummaryLimit+1))
	402	+	if err != nil {
387	403		return err
388	404		}
	405	+	if len(b) > stepSummaryLimit {
	406	+	tooLarge(int64(len(b)))
	407	+	return nil
	408	+	}
	409	+	text := decodeText(b)
389	410		var mask func(string) string
390	411		if m, has := sink.(interface{ MaskText(string) string }); has {
391	412		mask = m.MaskText

 	if err != nil {
 		return "", false, err
 	}
+	return decodeText(b), true, nil
+}
+// decodeText is the decoding of File.ReadAllText: a UTF-8 BOM is dropped and
+// invalid bytes become U+FFFD.
+func decodeText(b []byte) string {
 	b = bytes.TrimPrefix(b, []byte("\xef\xbb\xbf"))
 	if !utf8.Valid(b) {
 		b = bytes.ToValidUTF8(b, []byte("\ufffd"))
 	}
-	return string(b), true, nil
+	return string(b)
 }
 // createExcl creates name in root with the content, failing if it exists (an
 	return ParseEnvFile(text)
 }
+// afterSummaryStat is called between the size check of the summary and its
+// read; tests use it to make the file grow.
+var afterSummaryStat func()
 // processStepSummary is CreateStepSummaryCommand.ProcessCommand
 // (FileCommandManager.cs:230-275): a missing or empty file is ignored, a file
 // above 1 MiB is an error; otherwise the lines are masked one by one and the
 	if err != nil {
 		return err
 	}
+	defer func() { _ = fh.Close() }()
 	st, err := fh.Stat()
-	_ = fh.Close()
 	if err != nil || st.Size() == 0 {
 		return nil
 	}
 	if !ok {
 		return nil
 	}
-	text, ok, err := readText(root, name)
+	if afterSummaryStat != nil {
-	if err != nil || !ok {
+		afterSummaryStat()
+	}
+	// The file belongs to the user of the step, who can still write to it (a
+	// background process of the step): what is read is bounded by the limit on
+	// the descriptor that was checked, not by the size seen a moment ago.
+	b, err := io.ReadAll(io.LimitReader(fh, stepSummaryLimit+1))
+	if err != nil {
 		return err
 	}
+	if len(b) > stepSummaryLimit {
+		tooLarge(int64(len(b)))
+		return nil
+	}
+	text := decodeText(b)
 	var mask func(string) string
 	if m, has := sink.(interface{ MaskText(string) string }); has {
 		mask = m.MaskText

M internal/executor/filecmd_test.go +39 -0


@@ -379,3 +379,42 @@ func TestCreateExclChownsTheOpenFile(t *testing.T) {
379	379		}
380	380		}
381	381		}
	382	+
	383	+	// The summary is read from the descriptor that was checked, at most the limit
	384	+	// plus one byte: a file that grows after the size check (a background process
	385	+	// of the step) cannot make the executor read it whole (GO-005).
	386	+	func TestStepSummaryGrowingAfterTheSizeCheckIsBounded(t *testing.T) {
	387	+	fc, _ := NewFileCommands(t.TempDir())
	388	+	env, _ := fc.Init()
	389	+	path := env["GITHUB_STEP_SUMMARY"]
	390	+	if err := os.WriteFile(path, []byte("small\n"), 0o644); err != nil {
	391	+	t.Fatal(err)
	392	+	}
	393	+	afterSummaryStat = func() {
	394	+	f, err := os.OpenFile(path, os.O_WRONLY\|os.O_APPEND, 0)
	395	+	if err != nil {
	396	+	t.Error(err)
	397	+	return
	398	+	}
	399	+	defer func() { _ = f.Close() }()
	400	+	if _, err := f.Write(make([]byte, 8<<20)); err != nil {
	401	+	t.Error(err)
	402	+	}
	403	+	}
	404	+	t.Cleanup(func() { afterSummaryStat = nil })
	405	+	s := &summarySinkFake{fakeSink: newSink()}
	406	+	fc.Process(s)
	407	+	if s.got != "" {
	408	+	t.Errorf("a summary of %d bytes was kept", len(s.got))
	409	+	}
	410	+	if len(s.errors) != 1 \|\| !strings.HasPrefix(s.errors[0], "$GITHUB_STEP_SUMMARY upload aborted, supports content up to a size of 1024k, got 1024k.") {
	411	+	t.Fatalf("%q", s.errors)
	412	+	}
	413	+	}
	414	+
	415	+	type summarySinkFake struct {
	416	+	*fakeSink
	417	+	got string
	418	+	}
	419	+
	420	+	func (s *summarySinkFake) Summary(m string) { s.got = m }


@@ -379,3 +379,42 @@ func TestCreateExclChownsTheOpenFile(t *testing.T) {
379	}	379		}
380	}	380		}
381	}	381		}
		382	+
		383	+	// The summary is read from the descriptor that was checked, at most the limit
		384	+	// plus one byte: a file that grows after the size check (a background process
		385	+	// of the step) cannot make the executor read it whole (GO-005).
		386	+	func TestStepSummaryGrowingAfterTheSizeCheckIsBounded(t *testing.T) {
		387	+	fc, _ := NewFileCommands(t.TempDir())
		388	+	env, _ := fc.Init()
		389	+	path := env["GITHUB_STEP_SUMMARY"]
		390	+	if err := os.WriteFile(path, []byte("small\n"), 0o644); err != nil {
		391	+	t.Fatal(err)
		392	+	}
		393	+	afterSummaryStat = func() {
		394	+	f, err := os.OpenFile(path, os.O_WRONLY\|os.O_APPEND, 0)
		395	+	if err != nil {
		396	+	t.Error(err)
		397	+	return
		398	+	}
		399	+	defer func() { _ = f.Close() }()
		400	+	if _, err := f.Write(make([]byte, 8<<20)); err != nil {
		401	+	t.Error(err)
		402	+	}
		403	+	}
		404	+	t.Cleanup(func() { afterSummaryStat = nil })
		405	+	s := &summarySinkFake{fakeSink: newSink()}
		406	+	fc.Process(s)
		407	+	if s.got != "" {
		408	+	t.Errorf("a summary of %d bytes was kept", len(s.got))
		409	+	}
		410	+	if len(s.errors) != 1 \|\| !strings.HasPrefix(s.errors[0], "$GITHUB_STEP_SUMMARY upload aborted, supports content up to a size of 1024k, got 1024k.") {
		411	+	t.Fatalf("%q", s.errors)
		412	+	}
		413	+	}
		414	+
		415	+	type summarySinkFake struct {
		416	+	*fakeSink
		417	+	got string
		418	+	}
		419	+
		420	+	func (s *summarySinkFake) Summary(m string) { s.got = m }