From 8708ae4e1c7bbdc905740cfcc43a0d66cb21fab9 Mon Sep 17 00:00:00 2001 From: xiongyuanwen-byted Date: Sat, 11 Jul 2026 16:10:08 +0800 Subject: [PATCH] fix(sheets): import mislabeled .xls workbooks by sniffing content Local .xls files that are actually OOXML (an .xlsx exported or renamed to .xls) failed +workbook-import with a cryptic backend "xml_version_not_support" because the CLI trusted the file name extension. +workbook-import now sniffs the file's leading magic bytes (PK -> xlsx, OLE2 -> xls) and passes the true extension to the drive import core via a new optional ImportParams.FileExtension override, correcting both the file_extension and the staged media file name (the latter avoids the backend's "import file extension not match", code 1069910). A declared Excel file whose bytes match neither container is rejected locally with a prescriptive error instead of the opaque backend failure. The drive import core gains only the neutral FileExtension override (empty = infer from the file name, i.e. unchanged behavior for drive +import); all Excel sniffing/correction policy lives in the sheets shortcut. --- shortcuts/drive/drive_import.go | 22 ++-- shortcuts/drive/drive_import_common.go | 50 ++++++-- shortcuts/drive/drive_import_common_test.go | 20 +-- shortcuts/sheets/lark_sheet_workbook.go | 120 ++++++++++++++++-- .../sheets/lark_sheet_workbook_import_test.go | 87 ++++++++++++- 5 files changed, 257 insertions(+), 42 deletions(-) diff --git a/shortcuts/drive/drive_import.go b/shortcuts/drive/drive_import.go index c4f7dafa8..c34a42414 100644 --- a/shortcuts/drive/drive_import.go +++ b/shortcuts/drive/drive_import.go @@ -54,15 +54,21 @@ type ImportParams struct { FolderToken string Name string TargetToken string + // FileExtension optionally overrides the extension inferred from File's + // name. Leave empty to infer from File (the default). Callers that have + // sniffed the file's real container use this to correct a mislabeled name + // so the backend receives the true format. + FileExtension string } func (p ImportParams) spec() driveImportSpec { return driveImportSpec{ - FilePath: p.File, - DocType: strings.ToLower(p.DocType), - FolderToken: p.FolderToken, - Name: p.Name, - TargetToken: p.TargetToken, + FilePath: p.File, + DocType: strings.ToLower(p.DocType), + FolderToken: p.FolderToken, + Name: p.Name, + TargetToken: p.TargetToken, + EffectiveExt: strings.TrimPrefix(strings.ToLower(p.FileExtension), "."), } } @@ -127,7 +133,7 @@ func RunImport(ctx context.Context, runtime *common.RuntimeContext, p ImportPara } // Step 1: Upload file as media - fileToken, uploadErr := uploadMediaForImport(ctx, runtime, spec.FilePath, spec.SourceFileName(), spec.DocType) + fileToken, uploadErr := uploadMediaForImport(ctx, runtime, spec) if uploadErr != nil { return uploadErr } @@ -203,14 +209,14 @@ func preflightDriveImportFile(fio fileio.FileIO, spec *driveImportSpec) (int64, if !info.Mode().IsRegular() { return 0, errs.NewValidationError(errs.SubtypeInvalidArgument, "file must be a regular file: %s", spec.FilePath).WithParam("--file") } - if err = validateDriveImportFileSize(spec.FilePath, spec.DocType, info.Size()); err != nil { + if err = validateDriveImportFileSize(spec.FileExtension(), spec.DocType, info.Size()); err != nil { return 0, err } return info.Size(), nil } func appendDriveImportUploadDryRun(dry *common.DryRunAPI, spec driveImportSpec, fileSize int64) { - extra, err := buildImportMediaExtra(spec.FilePath, spec.DocType) + extra, err := buildImportMediaExtra(spec.FileExtension(), spec.DocType) if err != nil { extra = fmt.Sprintf(`{"obj_type":"%s","file_extension":"%s"}`, spec.DocType, spec.FileExtension()) } diff --git a/shortcuts/drive/drive_import_common.go b/shortcuts/drive/drive_import_common.go index daea5e050..c93344306 100644 --- a/shortcuts/drive/drive_import_common.go +++ b/shortcuts/drive/drive_import_common.go @@ -54,14 +54,39 @@ type driveImportSpec struct { FolderToken string Name string TargetToken string // existing bitable token to import data into (only for type=bitable) + + // EffectiveExt is a caller-supplied override for the extension otherwise + // derived from FilePath (see ImportParams.FileExtension). It lets a caller + // that has detected the file's real container correct a mislabeled name + // (e.g. an OOXML workbook saved as .xls). Empty means "trust the filename". + EffectiveExt string } -func (s driveImportSpec) FileExtension() string { +// rawExtension is the lowercased extension taken verbatim from the file name. +func (s driveImportSpec) rawExtension() string { return strings.TrimPrefix(strings.ToLower(filepath.Ext(s.FilePath)), ".") } +// FileExtension is the extension the import pipeline treats as authoritative: +// the content-sniffed override when set, otherwise the file name's extension. +func (s driveImportSpec) FileExtension() string { + if s.EffectiveExt != "" { + return s.EffectiveExt + } + return s.rawExtension() +} + +// SourceFileName is the name used when staging the upload media. When content +// sniffing corrected the extension, the staged name must carry the corrected +// suffix too: the import backend cross-checks the media file name's extension +// against the file_extension in the import task and rejects a mismatch with +// "import file extension not match" (code 1069910). func (s driveImportSpec) SourceFileName() string { - return filepath.Base(s.FilePath) + base := filepath.Base(s.FilePath) + if s.EffectiveExt != "" && s.EffectiveExt != s.rawExtension() { + base = strings.TrimSuffix(base, filepath.Ext(base)) + "." + s.EffectiveExt + } + return base } func (s driveImportSpec) TargetFileName() string { @@ -92,18 +117,20 @@ func (s driveImportSpec) CreateTaskBody(fileToken string) map[string]interface{} // uploadMediaForImport uploads the source file to the temporary import media // endpoint and returns the file token consumed by import_tasks. -func uploadMediaForImport(ctx context.Context, runtime *common.RuntimeContext, filePath, fileName, docType string) (string, error) { +func uploadMediaForImport(ctx context.Context, runtime *common.RuntimeContext, spec driveImportSpec) (string, error) { + filePath := spec.FilePath + fileName := spec.SourceFileName() importInfo, err := runtime.FileIO().Stat(filePath) if err != nil { return "", driveInputStatError(err) } fileSize := importInfo.Size() - if err = validateDriveImportFileSize(filePath, docType, fileSize); err != nil { + if err = validateDriveImportFileSize(spec.FileExtension(), spec.DocType, fileSize); err != nil { return "", err } - extra, err := buildImportMediaExtra(filePath, docType) + extra, err := buildImportMediaExtra(spec.FileExtension(), spec.DocType) if err != nil { return "", err } @@ -134,12 +161,12 @@ func uploadMediaForImport(ctx context.Context, runtime *common.RuntimeContext, f }) } -func buildImportMediaExtra(filePath, docType string) (string, error) { +func buildImportMediaExtra(ext, docType string) (string, error) { // The import media endpoint uses extra to decide both the target native type // and how to interpret the uploaded source file. extraBytes, err := json.Marshal(map[string]string{ "obj_type": docType, - "file_extension": strings.TrimPrefix(strings.ToLower(filepath.Ext(filePath)), "."), + "file_extension": ext, }) if err != nil { return "", errs.NewInternalError(errs.SubtypeUnknown, "build upload extra failed: %v", err).WithCause(err) @@ -147,10 +174,10 @@ func buildImportMediaExtra(filePath, docType string) (string, error) { return string(extraBytes), nil } -func driveImportFileSizeLimit(filePath, docType string) (int64, bool) { +func driveImportFileSizeLimit(ext, docType string) (int64, bool) { // Keep the limit mapping local to import flows so we do not widen behavior // changes beyond drive +import. - switch strings.TrimPrefix(strings.ToLower(filepath.Ext(filePath)), ".") { + switch ext { case "docx", "doc": return driveImport600MBFileSizeLimit, true case "pptx": @@ -169,13 +196,12 @@ func driveImportFileSizeLimit(filePath, docType string) (int64, bool) { } } -func validateDriveImportFileSize(filePath, docType string, fileSize int64) error { - limit, ok := driveImportFileSizeLimit(filePath, docType) +func validateDriveImportFileSize(ext, docType string, fileSize int64) error { + limit, ok := driveImportFileSizeLimit(ext, docType) if !ok || fileSize <= limit { return nil } - ext := strings.TrimPrefix(strings.ToLower(filepath.Ext(filePath)), ".") if ext == "csv" { // CSV is the only source format whose limit depends on the target type. return errs.NewValidationError(errs.SubtypeInvalidArgument, diff --git a/shortcuts/drive/drive_import_common_test.go b/shortcuts/drive/drive_import_common_test.go index a59e31feb..f2cf8efa6 100644 --- a/shortcuts/drive/drive_import_common_test.go +++ b/shortcuts/drive/drive_import_common_test.go @@ -93,61 +93,61 @@ func TestValidateDriveImportFileSize(t *testing.T) { tests := []struct { name string - filePath string + ext string docType string fileSize int64 wantText string }{ { name: "docx exceeds 600mb limit", - filePath: "./report.docx", + ext: "docx", docType: "docx", fileSize: driveImport600MBFileSizeLimit + 1, wantText: "exceeds 600.0 MB import limit for .docx", }, { name: "csv sheet exceeds 20mb limit", - filePath: "./data.csv", + ext: "csv", docType: "sheet", fileSize: driveImport20MBFileSizeLimit + 1, wantText: "exceeds 20.0 MB import limit for .csv when importing as sheet", }, { name: "csv bitable exceeds 100mb limit", - filePath: "./data.csv", + ext: "csv", docType: "bitable", fileSize: driveImport100MBFileSizeLimit + 1, wantText: "exceeds 100.0 MB import limit for .csv when importing as bitable", }, { name: "xlsx within 800mb limit", - filePath: "./data.xlsx", + ext: "xlsx", docType: "sheet", fileSize: driveImport800MBFileSizeLimit, }, { name: "pptx exceeds 500mb limit", - filePath: "./deck.pptx", + ext: "pptx", docType: "slides", fileSize: driveImport500MBFileSizeLimit + 1, wantText: "exceeds 500.0 MB import limit for .pptx", }, { name: "pptx within 500mb limit", - filePath: "./deck.pptx", + ext: "pptx", docType: "slides", fileSize: driveImport500MBFileSizeLimit, }, { name: "base exceeds 20mb limit", - filePath: "./snapshot.base", + ext: "base", docType: "bitable", fileSize: driveImport20MBFileSizeLimit + 1, wantText: "exceeds 20.0 MB import limit for .base", }, { name: "base within 20mb limit", - filePath: "./snapshot.base", + ext: "base", docType: "bitable", fileSize: driveImport20MBFileSizeLimit, }, @@ -157,7 +157,7 @@ func TestValidateDriveImportFileSize(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := validateDriveImportFileSize(tt.filePath, tt.docType, tt.fileSize) + err := validateDriveImportFileSize(tt.ext, tt.docType, tt.fileSize) if tt.wantText == "" { if err != nil { t.Fatalf("expected no error, got %v", err) diff --git a/shortcuts/sheets/lark_sheet_workbook.go b/shortcuts/sheets/lark_sheet_workbook.go index 709b596e7..7cef97605 100644 --- a/shortcuts/sheets/lark_sheet_workbook.go +++ b/shortcuts/sheets/lark_sheet_workbook.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "fmt" + "io" "path/filepath" "strings" @@ -1816,9 +1817,13 @@ func lookupFirstSheetID(ctx context.Context, runtime *common.RuntimeContext, tok // // Imports a local xlsx/xls/csv file as a brand-new spreadsheet. The full // upload → create-task → poll flow is the shared drive import core -// (drive.RunImport); this shortcut only pins the target type to "sheet" and -// omits the bitable-only --target-token. Symmetric with +workbook-export. -// Not exposed as an MCP tool. +// (drive.RunImport); this shortcut only pins the target type to "sheet", +// omits the bitable-only --target-token, and — because spreadsheet source +// files are routinely misnamed (an .xlsx exported/renamed to .xls, etc.) — +// sniffs the file's real container so the drive import backend receives the +// true file_extension instead of failing with a cryptic +// "xml_version_not_support". Symmetric with +workbook-export. Not exposed as +// an MCP tool. // WorkbookImport imports a local spreadsheet file as a new Feishu spreadsheet // by delegating to the shared drive import core with type fixed to "sheet". @@ -1832,24 +1837,119 @@ var WorkbookImport = common.Shortcut{ HasFormat: true, Flags: flagsFor("+workbook-import"), Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { - return drive.ValidateImport(workbookImportParams(runtime)) + params, err := workbookImportParams(runtime) + if err != nil { + return err + } + return drive.ValidateImport(params) }, DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { - return drive.PlanImportDryRun(runtime, workbookImportParams(runtime)) + params, err := workbookImportParams(runtime) + if err != nil { + return common.NewDryRunAPI().Set("error", err.Error()) + } + dry := drive.PlanImportDryRun(runtime, params) + if note := workbookImportMislabelNote(params); note != "" { + dry.Desc(note) + } + return dry }, Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { - return drive.RunImport(ctx, runtime, workbookImportParams(runtime)) + params, err := workbookImportParams(runtime) + if err != nil { + return err + } + if note := workbookImportMislabelNote(params); note != "" { + fmt.Fprintln(runtime.IO().ErrOut, note) + } + return drive.RunImport(ctx, runtime, params) }, } // workbookImportParams builds the drive import request for +workbook-import, // pinning DocType to "sheet". The bitable-only --target-token is intentionally -// not exposed here — use drive +import for non-sheet import targets. -func workbookImportParams(runtime *common.RuntimeContext) drive.ImportParams { - return drive.ImportParams{ - File: runtime.Str("file"), +// not exposed here — use drive +import for non-sheet import targets. It also +// resolves a corrected file extension via content sniffing (see +// correctedWorkbookExtension) and surfaces it through ImportParams.FileExtension. +func workbookImportParams(runtime *common.RuntimeContext) (drive.ImportParams, error) { + file := runtime.Str("file") + params := drive.ImportParams{ + File: file, DocType: "sheet", FolderToken: runtime.Str("folder-token"), Name: runtime.Str("name"), } + ext, err := correctedWorkbookExtension(runtime.FileIO(), file) + if err != nil { + return params, err + } + params.FileExtension = ext + return params, nil +} + +// correctedWorkbookExtension returns an override extension when the file's +// declared .xls/.xlsx suffix disagrees with its real container, "" when the +// declared suffix is correct (or the extension is not in the Excel family, or +// the file cannot yet be read). A declared Excel file whose bytes match neither +// container yields a prescriptive validation error rather than deferring to the +// backend's opaque "xml_version_not_support". +func correctedWorkbookExtension(fio fileio.FileIO, filePath string) (string, error) { + declared := strings.TrimPrefix(strings.ToLower(filepath.Ext(filePath)), ".") + if declared != "xls" && declared != "xlsx" { + return "", nil + } + + sniffed, ok := sniffWorkbookContainer(fio, filePath) + if !ok { + // Not readable here; let the drive core's stat/upload surface any error. + return "", nil + } + switch sniffed { + case declared: + return "", nil + case "xls", "xlsx": + return sniffed, nil + default: + return "", errs.NewValidationError(errs.SubtypeInvalidArgument, + "file %s has a .%s extension but its content is neither an OOXML (.xlsx) nor a legacy Excel (.xls) workbook; re-save it as a real .xlsx/.xls (or export to .csv) before importing", + filePath, declared).WithParam("--file") + } +} + +// sniffWorkbookContainer inspects a file's leading magic bytes to tell an OOXML +// workbook (zip container -> .xlsx) apart from a legacy OLE2/BIFF workbook +// (compound document -> .xls). The second return value is false when the file +// cannot be read far enough to judge (open error or fewer than the four +// discriminating bytes). When true, the format is "xlsx", "xls", or "" (bytes +// matching neither container). +func sniffWorkbookContainer(fio fileio.FileIO, filePath string) (string, bool) { + f, err := fio.Open(filePath) + if err != nil { + return "", false + } + defer f.Close() + + var head [8]byte + n, _ := io.ReadFull(f, head[:]) + if n < 4 { + return "", false + } + switch { + case head[0] == 0x50 && head[1] == 0x4B: // "PK" -> ZIP, i.e. OOXML .xlsx + return "xlsx", true + case head[0] == 0xD0 && head[1] == 0xCF && head[2] == 0x11 && head[3] == 0xE0: // OLE2 compound doc -> legacy .xls + return "xls", true + } + return "", true +} + +// workbookImportMislabelNote returns a user-facing note when content sniffing +// overrode the declared extension, or "" when no correction was applied. +func workbookImportMislabelNote(params drive.ImportParams) string { + declared := strings.TrimPrefix(strings.ToLower(filepath.Ext(params.File)), ".") + if params.FileExtension == "" || params.FileExtension == declared { + return "" + } + return fmt.Sprintf("Note: %s has a mislabeled .%s extension but is actually a .%s workbook; importing it as .%s.", + filepath.Base(params.File), declared, params.FileExtension, params.FileExtension) } diff --git a/shortcuts/sheets/lark_sheet_workbook_import_test.go b/shortcuts/sheets/lark_sheet_workbook_import_test.go index bdfebada7..345cb34d4 100644 --- a/shortcuts/sheets/lark_sheet_workbook_import_test.go +++ b/shortcuts/sheets/lark_sheet_workbook_import_test.go @@ -10,7 +10,7 @@ import ( "testing" "github.com/larksuite/cli/internal/httpmock" - _ "github.com/larksuite/cli/internal/vfs/localfileio" + "github.com/larksuite/cli/internal/vfs/localfileio" ) // chdirTemp switches into a fresh temp dir for the duration of the test and @@ -32,7 +32,7 @@ func chdirTemp(t *testing.T) { // shared drive import core and hard-codes the import target type to "sheet". func TestWorkbookImport_DryRunPinsSheetType(t *testing.T) { chdirTemp(t) - if err := os.WriteFile("data.xlsx", []byte("fake-xlsx"), 0o644); err != nil { + if err := os.WriteFile("data.xlsx", []byte("PK\x03\x04fake-xlsx"), 0o644); err != nil { t.Fatalf("write file: %v", err) } @@ -70,6 +70,89 @@ func TestWorkbookImport_RejectsNonSheetFile(t *testing.T) { requireValidation(t, err, "can only be imported") } +// TestCorrectedWorkbookExtension covers the content-sniffing that corrects (or +// rejects) a mislabeled Excel file before its extension reaches the backend. +func TestCorrectedWorkbookExtension(t *testing.T) { + ooxml := []byte("PK\x03\x04rest") // zip -> .xlsx + ole2 := []byte("\xD0\xCF\x11\xE0\xA1\xB1\x1A\xE1rest") // compound doc -> .xls + + tests := []struct { + name string + fileName string + content []byte + wantExt string // expected override ("" == leave the declared extension) + wantErrSub string // non-empty == expect a validation error containing this + }{ + {name: "xlsx content mislabeled as xls corrects to xlsx", fileName: "book.xls", content: ooxml, wantExt: "xlsx"}, + {name: "xls content mislabeled as xlsx corrects to xls", fileName: "book.xlsx", content: ole2, wantExt: "xls"}, + {name: "genuine xlsx left untouched", fileName: "book.xlsx", content: ooxml, wantExt: ""}, + {name: "genuine xls left untouched", fileName: "book.xls", content: ole2, wantExt: ""}, + {name: "unrecognized content on .xls rejected", fileName: "book.xls", content: []byte("
"), wantErrSub: "neither an OOXML"}, + {name: "non-excel extension never sniffed", fileName: "notes.csv", content: []byte("a,b\n1,2\n"), wantExt: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + chdirTemp(t) + if err := os.WriteFile(tt.fileName, tt.content, 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + + ext, err := correctedWorkbookExtension(&localfileio.LocalFileIO{}, "./"+tt.fileName) + if tt.wantErrSub != "" { + if err == nil || !strings.Contains(err.Error(), tt.wantErrSub) { + t.Fatalf("error = %v, want substring %q", err, tt.wantErrSub) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ext != tt.wantExt { + t.Fatalf("override ext = %q, want %q", ext, tt.wantExt) + } + }) + } +} + +// TestWorkbookImport_DryRunCorrectsMislabeledXls verifies an .xls file whose +// bytes are actually OOXML is imported with file_extension=xlsx end to end. +func TestWorkbookImport_DryRunCorrectsMislabeledXls(t *testing.T) { + chdirTemp(t) + if err := os.WriteFile("book.xls", []byte("PK\x03\x04zip-body"), 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + + calls := parseDryRunAPI(t, WorkbookImport, []string{"--file", "./book.xls"}) + + var createBody map[string]interface{} + for _, c := range calls { + cm, _ := c.(map[string]interface{}) + if u, _ := cm["url"].(string); u == "/open-apis/drive/v1/import_tasks" { + createBody, _ = cm["body"].(map[string]interface{}) + } + } + if createBody == nil { + t.Fatalf("no import_tasks create call in dry-run: %#v", calls) + } + if createBody["file_extension"] != "xlsx" { + t.Errorf("file_extension = %v, want xlsx (mislabeled .xls must be corrected)", createBody["file_extension"]) + } +} + +// TestWorkbookImport_RejectsUnrecognizedExcel ensures a file whose .xls/.xlsx +// name matches neither Excel container is rejected locally with a prescriptive +// error rather than deferring to the backend's opaque failure. +func TestWorkbookImport_RejectsUnrecognizedExcel(t *testing.T) { + chdirTemp(t) + if err := os.WriteFile("bogus.xls", []byte("not excel"), 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + + _, _, err := runShortcutCapturingErr(t, WorkbookImport, []string{"--file", "./bogus.xls", "--dry-run"}) + requireValidation(t, err, "neither an OOXML") +} + // TestWorkbookImport_ExecuteCreatesSheet runs the full upload → create → poll // flow against stubs and asserts the resulting URL is a /sheets/ link. func TestWorkbookImport_ExecuteCreatesSheet(t *testing.T) {