This closed qax-os#1680, fixing a potential issue that stream reader temporary files can not be clear

- Delete image files from the workbook internally when deleting pictures to reduce generated workbook size and resolve potential security issues

Author: xuri

chart.go

@@ -1109,7 +1109,8 @@ func (f *File) DeleteChart(sheet, cell string) error {
 		return err
 	}
 	drawingXML := strings.ReplaceAll(f.getSheetRelationshipsTargetByID(sheet, ws.Drawing.RID), "..", "xl")
-	return f.deleteDrawing(col, row, drawingXML, "Chart")
+	_, err = f.deleteDrawing(col, row, drawingXML, "Chart")
+	return err
 }
 
 // countCharts provides a function to get chart files count storage in the

chart_test.go

@@ -120,13 +120,15 @@ func TestDeleteDrawing(t *testing.T) {
 	f := NewFile()
 	path := "xl/drawings/drawing1.xml"
 	f.Pkg.Store(path, MacintoshCyrillicCharset)
-	assert.EqualError(t, f.deleteDrawing(0, 0, path, "Chart"), "XML syntax error on line 1: invalid UTF-8")
-	f, err := OpenFile(filepath.Join("test", "Book1.xlsx"))
+	_, err := f.deleteDrawing(0, 0, path, "Chart")
+	assert.EqualError(t, err, "XML syntax error on line 1: invalid UTF-8")
+	f, err = OpenFile(filepath.Join("test", "Book1.xlsx"))
 	assert.NoError(t, err)
 	f.Drawings.Store(path, &xlsxWsDr{TwoCellAnchor: []*xdrCellAnchor{{
 		GraphicFrame: string(MacintoshCyrillicCharset),
 	}}})
-	assert.EqualError(t, f.deleteDrawing(0, 0, path, "Chart"), "XML syntax error on line 1: invalid UTF-8")
+	_, err = f.deleteDrawing(0, 0, path, "Chart")
+	assert.EqualError(t, err, "XML syntax error on line 1: invalid UTF-8")
 }
 
 func TestAddChart(t *testing.T) {

drawing.go

@@ -1391,11 +1391,14 @@ func (f *File) addSheetDrawingChart(drawingXML string, rID int, opts *GraphicOpt
 	return err
 }
 
-// deleteDrawing provides a function to delete chart graphic frame by given by
+// deleteDrawing provides a function to delete the chart graphic frame and
+// returns deleted embed relationships ID (for unique picture cell anchor) by
 // given coordinates and graphic type.
-func (f *File) deleteDrawing(col, row int, drawingXML, drawingType string) error {
+func (f *File) deleteDrawing(col, row int, drawingXML, drawingType string) (string, error) {
 	var (
 		err             error
+		rID             string
+		rIDs            []string
 		wsDr            *xlsxWsDr
 		deTwoCellAnchor *decodeCellAnchor
 	)
@@ -1407,32 +1410,74 @@ func (f *File) deleteDrawing(col, row int, drawingXML, drawingType string) error
 		"Chart": func(anchor *decodeCellAnchor) bool { return anchor.Pic == nil },
 		"Pic":   func(anchor *decodeCellAnchor) bool { return anchor.Pic != nil },
 	}
+	onAnchorCell := func(c, r int) bool { return c == col && r == row }
 	if wsDr, _, err = f.drawingParser(drawingXML); err != nil {
-		return err
+		return rID, err
 	}
 	for idx := 0; idx < len(wsDr.TwoCellAnchor); idx++ {
 		if err = nil; wsDr.TwoCellAnchor[idx].From != nil && xdrCellAnchorFuncs[drawingType](wsDr.TwoCellAnchor[idx]) {
-			if wsDr.TwoCellAnchor[idx].From.Col == col && wsDr.TwoCellAnchor[idx].From.Row == row {
+			if onAnchorCell(wsDr.TwoCellAnchor[idx].From.Col, wsDr.TwoCellAnchor[idx].From.Row) {
+				rID, _ = extractEmbedRID(wsDr.TwoCellAnchor[idx].Pic, nil, rIDs)
 				wsDr.TwoCellAnchor = append(wsDr.TwoCellAnchor[:idx], wsDr.TwoCellAnchor[idx+1:]...)
 				idx--
+				continue
 			}
+			_, rIDs = extractEmbedRID(wsDr.TwoCellAnchor[idx].Pic, nil, rIDs)
 		}
 	}
 	for idx := 0; idx < len(wsDr.TwoCellAnchor); idx++ {
 		deTwoCellAnchor = new(decodeCellAnchor)
 		if err = f.xmlNewDecoder(strings.NewReader("<decodeCellAnchor>" + wsDr.TwoCellAnchor[idx].GraphicFrame + "</decodeCellAnchor>")).
 			Decode(deTwoCellAnchor); err != nil && err != io.EOF {
-			return err
+			return rID, err
 		}
 		if err = nil; deTwoCellAnchor.From != nil && decodeCellAnchorFuncs[drawingType](deTwoCellAnchor) {
-			if deTwoCellAnchor.From.Col == col && deTwoCellAnchor.From.Row == row {
+			if onAnchorCell(deTwoCellAnchor.From.Col, deTwoCellAnchor.From.Row) {
+				rID, _ = extractEmbedRID(nil, deTwoCellAnchor.Pic, rIDs)
 				wsDr.TwoCellAnchor = append(wsDr.TwoCellAnchor[:idx], wsDr.TwoCellAnchor[idx+1:]...)
 				idx--
+				continue
 			}
+			_, rIDs = extractEmbedRID(nil, deTwoCellAnchor.Pic, rIDs)
 		}
 	}
+	if inStrSlice(rIDs, rID, true) != -1 {
+		rID = ""
+	}
 	f.Drawings.Store(drawingXML, wsDr)
-	return err
+	return rID, err
+}
+
+// extractEmbedRID returns embed relationship ID and all relationship ID lists
+// for giving cell anchor.
+func extractEmbedRID(pic *xlsxPic, decodePic *decodePic, rIDs []string) (string, []string) {
+	if pic != nil {
+		rIDs = append(rIDs, pic.BlipFill.Blip.Embed)
+		return pic.BlipFill.Blip.Embed, rIDs
+	}
+	if decodePic != nil {
+		rIDs = append(rIDs, decodePic.BlipFill.Blip.Embed)
+		return decodePic.BlipFill.Blip.Embed, rIDs
+	}
+	return "", rIDs
+}
+
+// deleteDrawingRels provides a function to delete relationships in
+// xl/drawings/_rels/drawings%d.xml.rels by giving drawings relationships path
+// and relationship ID.
+func (f *File) deleteDrawingRels(rels, rID string) {
+	drawingRels, _ := f.relsReader(rels)
+	if drawingRels == nil {
+		drawingRels = &xlsxRelationships{}
+	}
+	drawingRels.mu.Lock()
+	defer drawingRels.mu.Unlock()
+	for k, v := range drawingRels.Relationships {
+		if v.ID == rID {
+			drawingRels.Relationships = append(drawingRels.Relationships[:k], drawingRels.Relationships[k+1:]...)
+		}
+	}
+	f.Relationships.Store(rels, drawingRels)
 }
 
 // genAxID provides a function to generate ID for primary and secondary

drawing_test.go

@@ -38,3 +38,12 @@ func TestDrawingParser(t *testing.T) {
 	_, _, err = f.drawingParser("wsDr")
 	assert.NoError(t, err)
 }
+
+func TestDeleteDrawingRels(t *testing.T) {
+	f := NewFile()
+	// Test delete drawing relationships with unsupported charset
+	rels := "xl/drawings/_rels/drawing1.xml.rels"
+	f.Relationships.Delete(rels)
+	f.Pkg.Store(rels, MacintoshCyrillicCharset)
+	f.deleteDrawingRels(rels, "")
+}

lib.go

@@ -48,16 +48,22 @@ func (f *File) ReadZipReader(r *zip.Reader) (map[string][]byte, int, error) {
 			fileName = partName
 		}
 		if strings.EqualFold(fileName, defaultXMLPathSharedStrings) && fileSize > f.options.UnzipXMLSizeLimit {
-			if tempFile, err := f.unzipToTemp(v); err == nil {
+			tempFile, err := f.unzipToTemp(v)
+			if tempFile != "" {
 				f.tempFiles.Store(fileName, tempFile)
+			}
+			if err == nil {
 				continue
 			}
 		}
 		if strings.HasPrefix(strings.ToLower(fileName), "xl/worksheets/sheet") {
 			worksheets++
 			if fileSize > f.options.UnzipXMLSizeLimit && !v.FileInfo().IsDir() {
-				if tempFile, err := f.unzipToTemp(v); err == nil {
+				tempFile, err := f.unzipToTemp(v)
+				if tempFile != "" {
 					f.tempFiles.Store(fileName, tempFile)
+				}
+				if err == nil {
 					continue
 				}
 			}

picture.go

@@ -468,8 +468,7 @@ func (f *File) GetPictures(sheet, cell string) ([]Picture, error) {
 }
 
 // DeletePicture provides a function to delete all pictures in a cell by given
-// worksheet name and cell reference. Note that the image file won't be deleted
-// from the document currently.
+// worksheet name and cell reference.
 func (f *File) DeletePicture(sheet, cell string) error {
 	col, row, err := CellNameToCoordinates(cell)
 	if err != nil {
@@ -485,7 +484,36 @@ func (f *File) DeletePicture(sheet, cell string) error {
 		return err
 	}
 	drawingXML := strings.ReplaceAll(f.getSheetRelationshipsTargetByID(sheet, ws.Drawing.RID), "..", "xl")
-	return f.deleteDrawing(col, row, drawingXML, "Pic")
+	drawingRels := "xl/drawings/_rels/" + filepath.Base(drawingXML) + ".rels"
+	rID, err := f.deleteDrawing(col, row, drawingXML, "Pic")
+	if err != nil {
+		return err
+	}
+	rels := f.getDrawingRelationships(drawingRels, rID)
+	if rels == nil {
+		return err
+	}
+	var used bool
+	f.Pkg.Range(func(k, v interface{}) bool {
+		if strings.Contains(k.(string), "xl/drawings/_rels/") {
+			r, err := f.relsReader(k.(string))
+			if err != nil {
+				return true
+			}
+			for _, rel := range r.Relationships {
+				if rel.ID != rels.ID && rel.Type == SourceRelationshipImage &&
+					filepath.Base(rel.Target) == filepath.Base(rels.Target) {
+					used = true
+				}
+			}
+		}
+		return true
+	})
+	if !used {
+		f.Pkg.Delete(strings.Replace(rels.Target, "../", "xl/", -1))
+	}
+	f.deleteDrawingRels(drawingRels, rID)
+	return err
 }
 
 // getPicture provides a function to get picture base name and raw content

picture_test.go

@@ -240,10 +240,25 @@ func TestAddPictureFromBytes(t *testing.T) {
 func TestDeletePicture(t *testing.T) {
 	f, err := OpenFile(filepath.Join("test", "Book1.xlsx"))
 	assert.NoError(t, err)
+	// Test delete picture on a worksheet which does not contains any pictures
 	assert.NoError(t, f.DeletePicture("Sheet1", "A1"))
-	assert.NoError(t, f.AddPicture("Sheet1", "P1", filepath.Join("test", "images", "excel.jpg"), nil))
-	assert.NoError(t, f.DeletePicture("Sheet1", "P1"))
+	// Add same pictures on different worksheets
+	assert.NoError(t, f.AddPicture("Sheet1", "F20", filepath.Join("test", "images", "excel.jpg"), nil))
+	assert.NoError(t, f.AddPicture("Sheet1", "I20", filepath.Join("test", "images", "excel.jpg"), nil))
+	assert.NoError(t, f.AddPicture("Sheet2", "F1", filepath.Join("test", "images", "excel.jpg"), nil))
+	// Test delete picture on a worksheet, the images should be preserved
+	assert.NoError(t, f.DeletePicture("Sheet1", "F20"))
 	assert.NoError(t, f.SaveAs(filepath.Join("test", "TestDeletePicture.xlsx")))
+	assert.NoError(t, f.Close())
+
+	f, err = OpenFile(filepath.Join("test", "TestDeletePicture.xlsx"))
+	assert.NoError(t, err)
+	// Test delete same picture on different worksheet, the images should be removed
+	assert.NoError(t, f.DeletePicture("Sheet1", "F10"))
+	assert.NoError(t, f.DeletePicture("Sheet2", "F1"))
+	assert.NoError(t, f.DeletePicture("Sheet1", "I20"))
+	assert.NoError(t, f.SaveAs(filepath.Join("test", "TestDeletePicture2.xlsx")))
+
 	// Test delete picture on not exists worksheet
 	assert.EqualError(t, f.DeletePicture("SheetN", "A1"), "sheet SheetN does not exist")
 	// Test delete picture with invalid sheet name
@@ -253,6 +268,32 @@ func TestDeletePicture(t *testing.T) {
 	assert.NoError(t, f.Close())
 	// Test delete picture on no chart worksheet
 	assert.NoError(t, NewFile().DeletePicture("Sheet1", "A1"))
+
+	f, err = OpenFile(filepath.Join("test", "TestDeletePicture.xlsx"))
+	assert.NoError(t, err)
+	// Test delete picture with unsupported charset drawing
+	f.Pkg.Store("xl/drawings/drawing1.xml", MacintoshCyrillicCharset)
+	assert.EqualError(t, f.DeletePicture("Sheet1", "F10"), "XML syntax error on line 1: invalid UTF-8")
+	assert.NoError(t, f.Close())
+
+	f, err = OpenFile(filepath.Join("test", "TestDeletePicture.xlsx"))
+	assert.NoError(t, err)
+	// Test delete picture with unsupported charset drawing relationships
+	f.Relationships.Delete("xl/drawings/_rels/drawing1.xml.rels")
+	f.Pkg.Store("xl/drawings/_rels/drawing1.xml.rels", MacintoshCyrillicCharset)
+	assert.NoError(t, f.DeletePicture("Sheet2", "F1"))
+	assert.NoError(t, f.Close())
+
+	f = NewFile()
+	assert.NoError(t, err)
+	assert.NoError(t, f.AddPicture("Sheet1", "A1", filepath.Join("test", "images", "excel.jpg"), nil))
+	assert.NoError(t, f.AddPicture("Sheet1", "G1", filepath.Join("test", "images", "excel.jpg"), nil))
+	drawing, ok := f.Drawings.Load("xl/drawings/drawing1.xml")
+	assert.True(t, ok)
+	// Made two picture reference the same drawing relationship ID
+	drawing.(*xlsxWsDr).TwoCellAnchor[1].Pic.BlipFill.Blip.Embed = "rId1"
+	assert.NoError(t, f.DeletePicture("Sheet1", "A1"))
+	assert.NoError(t, f.Close())
 }
 
 func TestDrawingResize(t *testing.T) {