This fixes GHSA-wcg2-648h-mhxq, fix panic on negative shared string index (#2366)

- Update unit test and docs

Author: arpitjain099

cell.go

@@ -619,7 +619,11 @@ func (c *xlsxC) getValueFrom(f *File, d *xlsxSST, raw bool) (string, error) {
 		if c.V != "" {
 			xlsxSI, _ := strconv.Atoi(strings.TrimSpace(c.V))
 			if _, ok := f.tempFiles.Load(defaultXMLPathSharedStrings); ok {
-				return f.formattedValue(&xlsxC{S: c.S, V: f.getFromStringItem(xlsxSI)}, raw, CellTypeSharedString)
+				val, err := f.getFromStringItem(xlsxSI)
+				if err != nil {
+					return "", err
+				}
+				return f.formattedValue(&xlsxC{S: c.S, V: val}, raw, CellTypeSharedString)
 			}
 			d.mu.Lock()
 			defer d.mu.Unlock()

cell_test.go

@@ -574,6 +574,16 @@ func TestGetValueFrom(t *testing.T) {
 	value, err = c.getValueFrom(f, &xlsxSST{Count: 1, SI: []xlsxSI{{}, {T: &xlsxT{Val: "s"}}}}, false)
 	assert.Equal(t, newInvalidSharedStringIndex(-1), err)
 	assert.Empty(t, value)
+
+	f.tempFiles.Store(defaultXMLPathSharedStrings, "")
+	f.sharedStringTemp, err = os.CreateTemp(os.TempDir(), "excelize-")
+	assert.NoError(t, err)
+	c = xlsxC{T: "s", V: "-1"}
+	value, err = c.getValueFrom(f, sst, false)
+	assert.Equal(t, newInvalidSharedStringIndex(-1), err)
+	assert.Empty(t, value)
+	assert.NoError(t, f.sharedStringTemp.Close())
+	assert.NoError(t, os.Remove(f.sharedStringTemp.Name()))
 }
 
 func TestGetCellFormula(t *testing.T) {
@@ -1199,10 +1209,14 @@ func TestSharedStringsError(t *testing.T) {
 	tempFile, ok := f.tempFiles.Load(defaultXMLPathSharedStrings)
 	assert.True(t, ok)
 	f.tempFiles.Store(defaultXMLPathSharedStrings, "")
-	assert.Equal(t, "1", f.getFromStringItem(1))
+	value, err := f.getFromStringItem(1)
+	assert.Equal(t, newInvalidSharedStringIndex(1), err)
+	assert.Empty(t, value)
 	// Test get from string item with invalid offset range
 	f.sharedStringItem = [][]uint{{0}}
-	assert.Equal(t, "0", f.getFromStringItem(0))
+	value, err = f.getFromStringItem(0)
+	assert.NoError(t, err)
+	assert.Equal(t, "0", value)
 	// Cleanup undelete temporary files
 	assert.NoError(t, os.Remove(tempFile.(string)))
 	// Test reload the file error on set cell value and rich text. The error message was different between macOS and Windows
@@ -1227,7 +1241,9 @@ func TestSharedStringsError(t *testing.T) {
 			assert.NoError(t, err)
 			// Test get cell value from string item with invalid offset
 			f.sharedStringItem[1] = []uint{maxUint16 - 1, maxUint16}
-			assert.Equal(t, "1", f.getFromStringItem(1))
+			value, err := f.getFromStringItem(1)
+			assert.NoError(t, err)
+			assert.Equal(t, "1", value)
 			break
 		}
 	}

errors.go

@@ -128,11 +128,11 @@ var (
 	// length.
 	ErrPasswordLengthInvalid = errors.New("password length invalid")
 	// ErrPivotTableShowValuesAsBaseField defined the error message on enable
-	// this kind of show value as type requires a base field.
-	ErrPivotTableShowValuesAsBaseField = errors.New("this kind of show value as type requires a base field")
+	// this kind of "show values as" type requires a base field.
+	ErrPivotTableShowValuesAsBaseField = errors.New("this kind of show values as type requires a base field")
 	// ErrPivotTableShowValuesAsBaseItem defined the error message on enable
-	// this kind of show value as type and base field requires a base item.
-	ErrPivotTableShowValuesAsBaseItem = errors.New("this kind of show value as type and base field requires a base item")
+	// this kind of "show values as" type and base field requires a base item.
+	ErrPivotTableShowValuesAsBaseItem = errors.New("this kind of show values as type and base field requires a base item")
 	// ErrPivotTableClassicLayout defined the error message on enable
 	// ClassicLayout and CompactData in the same time.
 	ErrPivotTableClassicLayout = errors.New("cannot enable ClassicLayout and CompactData in the same time")
@@ -199,8 +199,8 @@ var (
 	// number format expression.
 	ErrUnsupportedNumberFormat = errors.New("unsupported number format token")
 	// ErrUnsupportedPivotTableShowValuesAsType defined the error message on
-	// receiving the unsupported pivot table show value as type.
-	ErrUnsupportedPivotTableShowValuesAsType = errors.New("unsupported pivot table show value as type")
+	// receiving the unsupported pivot table "show values as" type.
+	ErrUnsupportedPivotTableShowValuesAsType = errors.New("unsupported pivot table show values as type")
 	// ErrWorkbookFileFormat defined the error message on receive an
 	// unsupported workbook file format.
 	ErrWorkbookFileFormat = errors.New("unsupported workbook file format")
@@ -376,7 +376,7 @@ func newPivotTableRangeError(msg string) error {
 }
 
 // newPivotTableShowValuesAsBaseFieldError defined the error message on receiving
-// the invalid pivot table show value as base field.
+// the invalid pivot table "show values as" base field.
 func newPivotTableShowValuesAsBaseFieldError(field string) error {
 	return fmt.Errorf("base field %s does not exist in shared items", field)
 }

pivotTable.go

@@ -89,7 +89,7 @@ const (
 	PivotTableShowValuesAsIndex
 )
 
-// PivotTableShowValuesAs directly maps the show value as settings of the pivot
+// PivotTableShowValuesAs directly maps the show values as settings of the pivot
 // table.
 type PivotTableShowValuesAs struct {
 	Type      PivotTableShowValuesAsType
@@ -896,7 +896,7 @@ func (f *File) addPivotDataFields(pt *xlsxPivotTableDefinition, opts *PivotTable
 	return err
 }
 
-// setPivotTableShowValuesAs provides a method to set show value as for pivot
+// setPivotTableShowValuesAs provides a method to set show values as for pivot
 // table data field by given pivot table options and data field index.
 func (df *xlsxDataField) setPivotTableShowValuesAs(idx int, order []string, opts *PivotTableOptions) error {
 	showValuesAsType := opts.Data[idx].ShowValuesAs.Type
@@ -1453,7 +1453,7 @@ func (f *File) extractPivotTableFields(pt *xlsxPivotTableDefinition, pc *xlsxPiv
 	}
 }
 
-// extractPivotTableShowValuesAs provides a function to extract show value as
+// extractPivotTableShowValuesAs provides a function to extract show values as
 // settings for pivot table data field.
 func (f *File) extractPivotTableShowValuesAs(pc *xlsxPivotCacheDefinition, df *xlsxDataField, dataField *PivotTableField) {
 	order := pc.getPivotCacheFieldsName()

pivotTable_test.go

@@ -100,7 +100,7 @@ func TestPivotTable(t *testing.T) {
 		ShowColHeaders:  true,
 		ShowLastColumn:  true,
 	}))
-	// Test get pivot table with show value as with base field and base item
+	// Test get pivot table with show values as with base field and base item
 	pivotTables, err = f.GetPivotTables("Sheet1")
 	assert.NoError(t, err)
 	assert.Len(t, pivotTables, 4)
@@ -331,7 +331,7 @@ func TestPivotTable(t *testing.T) {
 		Data:            []PivotTableField{{Data: "Revenue"}},
 		Filter:          []PivotTableField{{Data: "Month"}},
 	}))
-	// Test with unsupported pivot table data field show value as type
+	// Test with unsupported pivot table data field show values as type
 	assert.Equal(t, ErrUnsupportedPivotTableShowValuesAsType, f.AddPivotTable(&PivotTableOptions{
 		DataRange:       "Sheet1!A1:E31",
 		PivotTableRange: "Sheet1!G2:M34",
@@ -340,7 +340,7 @@ func TestPivotTable(t *testing.T) {
 		Data:            []PivotTableField{{Data: "Revenue", ShowValuesAs: PivotTableShowValuesAs{Type: 15}}},
 		Filter:          []PivotTableField{{Data: "Month"}},
 	}))
-	// Test set pivot table show value as type without required base field
+	// Test set pivot table show values as type without required base field
 	assert.Equal(t, ErrPivotTableShowValuesAsBaseField, f.AddPivotTable(&PivotTableOptions{
 		DataRange:       "Sheet1!A1:E31",
 		PivotTableRange: "Sheet1!G2:M34",
@@ -349,7 +349,7 @@ func TestPivotTable(t *testing.T) {
 		Data:            []PivotTableField{{Data: "Revenue", ShowValuesAs: PivotTableShowValuesAs{Type: PivotTableShowValuesAsRunningTotalIn}}},
 		Filter:          []PivotTableField{{Data: "Month"}},
 	}))
-	// Test set pivot table show value as type without required base item
+	// Test set pivot table show values as type without required base item
 	assert.Equal(t, ErrPivotTableShowValuesAsBaseItem, f.AddPivotTable(&PivotTableOptions{
 		DataRange:       "Sheet1!A1:E31",
 		PivotTableRange: "Sheet1!G2:M34",
@@ -358,7 +358,7 @@ func TestPivotTable(t *testing.T) {
 		Data:            []PivotTableField{{Data: "Revenue", ShowValuesAs: PivotTableShowValuesAs{Type: PivotTableShowValuesAsPercentOf, BaseField: "Month"}}},
 		Filter:          []PivotTableField{{Data: "Month"}},
 	}))
-	// Test with invalid pivot table show value as base field
+	// Test with invalid pivot table show values as base field
 	assert.Equal(t, newPivotTableShowValuesAsBaseFieldError("x"), f.AddPivotTable(&PivotTableOptions{
 		DataRange:       "Sheet1!A1:E31",
 		PivotTableRange: "Sheet1!G2:M34",
@@ -367,7 +367,7 @@ func TestPivotTable(t *testing.T) {
 		Data:            []PivotTableField{{Data: "Revenue", ShowValuesAs: PivotTableShowValuesAs{Type: PivotTableShowValuesAsRunningTotalIn, BaseField: "x"}}},
 		Filter:          []PivotTableField{{Data: "Month"}},
 	}))
-	// Test with invalid pivot table show value as base item
+	// Test with invalid pivot table show values as base item
 	assert.Equal(t, newPivotTableSelectedItemError("x", "Month"), f.AddPivotTable(&PivotTableOptions{
 		DataRange:       "Sheet1!A1:E31",
 		PivotTableRange: "Sheet1!G2:M34",

rows.go

@@ -353,21 +353,23 @@ func (f *File) Rows(sheet string) (*Rows, error) {
 }
 
 // getFromStringItem build shared string item offset list from system temporary
-// file at one time, and return value by given to string index.
-func (f *File) getFromStringItem(index int) string {
+// file at one time, and return value by given to string index. An index outside
+// the shared string table returns an error, matching the in-memory path in
+// xlsxC.getValueFrom.
+func (f *File) getFromStringItem(index int) (string, error) {
 	if f.sharedStringTemp != nil {
-		if len(f.sharedStringItem) <= index {
-			return strconv.Itoa(index)
+		if index < 0 || len(f.sharedStringItem) <= index {
+			return "", newInvalidSharedStringIndex(index)
 		}
 		offsetRange := f.sharedStringItem[index]
 		if len(offsetRange) != 2 || offsetRange[0] > offsetRange[1] {
-			return strconv.Itoa(index)
+			return strconv.Itoa(index), nil
 		}
 		buf := make([]byte, offsetRange[1]-offsetRange[0])
 		if _, err := f.sharedStringTemp.ReadAt(buf, int64(offsetRange[0])); err != nil {
-			return strconv.Itoa(index)
+			return strconv.Itoa(index), nil
 		}
-		return string(buf)
+		return string(buf), nil
 	}
 	needClose, decoder, tempFile, err := f.xmlDecoder(defaultXMLPathSharedStrings)
 	if needClose && err == nil {

rows_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/xml"
 	"fmt"
+	"os"
 	"path/filepath"
 	"strconv"
 	"sync"
@@ -255,6 +256,27 @@ func TestColumns(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestGetFromStringItem(t *testing.T) {
+	f := NewFile()
+	// Test get shared string item by a negative index on the streaming path,
+	// which the in-memory path already rejects in xlsxC.getValueFrom
+	tempFile, err := os.CreateTemp(f.options.TmpDir, "excelize-")
+	assert.NoError(t, err)
+	f.sharedStringTemp = tempFile
+	f.sharedStringItem = [][]uint{{0, 0}}
+	value, err := f.getFromStringItem(-1)
+	assert.Equal(t, newInvalidSharedStringIndex(-1), err)
+	assert.Empty(t, value)
+	value, err = f.getFromStringItem(1)
+	assert.Equal(t, newInvalidSharedStringIndex(1), err)
+	assert.Empty(t, value)
+	value, err = f.getFromStringItem(0)
+	assert.NoError(t, err)
+	assert.Empty(t, value)
+	assert.NoError(t, tempFile.Close())
+	assert.NoError(t, os.Remove(tempFile.Name()))
+}
+
 func TestSharedStringsReader(t *testing.T) {
 	f := NewFile()
 	// Test read shared string with unsupported charset