From bcda76003c1c2390a2325ce2fc6b86e977fd87bd Mon Sep 17 00:00:00 2001 From: Bakhtiyar Issakhmetov Date: Sat, 5 Sep 2026 14:44:21 +0500 Subject: [PATCH] fix(datasets): accept BigTIFF headers and sanitise object keys F1: ValidateFileContent rejected the pipeline's own output. GDAL writes a BigTIFF header ("II+\0" / "MM\0+") whenever BIGTIFF=IF_SAFER kicks in for a large raster, so accept both alongside the classic TIFF magic and reword the error to "file is not a valid TIFF/BigTIFF GeoTIFF". GodalIO.Create now delegates to an unexported createGTiff so a test can force BIGTIFF=YES and assert the produced header passes the check. F3: a user-supplied filename went straight into the S3 object key for dataset uploads and for both dataset and dataset-group SLD uploads, so "../../x.sld" escaped the dataset's own prefix. Reduce every such name to its base with a shared safeObjectName helper, rejecting names with no usable base. F9: rename the shadowing inner "raw" in the PATCH dg_id case to "s". Co-Authored-By: Claude Fable 5.1 --- internal/domain/dataset.go | 20 +++++- internal/domain/dataset_test.go | 3 + internal/raster/godal.go | 9 ++- internal/raster/godal_test.go | 41 +++++++++++ internal/service/dataset.go | 34 +++++++-- internal/service/dataset_group.go | 6 +- internal/service/dataset_group_test.go | 26 +++++++ internal/service/dataset_test.go | 83 ++++++++++++++++++++++ internal/transport/http/dataset_handler.go | 6 +- 9 files changed, 215 insertions(+), 13 deletions(-) diff --git a/internal/domain/dataset.go b/internal/domain/dataset.go index 0c14f75..d63542d 100644 --- a/internal/domain/dataset.go +++ b/internal/domain/dataset.go @@ -127,9 +127,23 @@ func ExtensionAllowedFor(ft FileType, ext string) bool { func ValidateFileContent(ext string, head []byte) error { switch ext { case ".tif", ".tiff": - // TIFF: little-endian "II*\0" or big-endian "MM\0*". - if !bytes.HasPrefix(head, []byte("II*\x00")) && !bytes.HasPrefix(head, []byte("MM\x00*")) { - return fmt.Errorf("file is not a valid TIFF/GeoTIFF") + // Classic TIFF: little-endian "II*\0" or big-endian "MM\0*". + // BigTIFF (version 43) uses "II+\0" / "MM\0+"; GDAL writes it for + // large rasters (BIGTIFF=IF_SAFER), so the pipeline's own COG output + // must pass this check too. + tiffMagic := [][]byte{ + []byte("II*\x00"), []byte("MM\x00*"), + []byte("II+\x00"), []byte("MM\x00+"), + } + ok := false + for _, m := range tiffMagic { + if bytes.HasPrefix(head, m) { + ok = true + break + } + } + if !ok { + return fmt.Errorf("file is not a valid TIFF/BigTIFF GeoTIFF") } case ".zip": // ZIP local-file or empty-archive signature. diff --git a/internal/domain/dataset_test.go b/internal/domain/dataset_test.go index 43d74c1..964401f 100644 --- a/internal/domain/dataset_test.go +++ b/internal/domain/dataset_test.go @@ -30,7 +30,10 @@ func TestValidateFileContent(t *testing.T) { }{ {"valid tiff LE", ".tif", []byte("II*\x00rest"), true}, {"valid tiff BE", ".tiff", []byte("MM\x00*rest"), true}, + {"valid bigtiff LE", ".tif", []byte("II+\x00rest"), true}, + {"valid bigtiff BE", ".tiff", []byte("MM\x00+rest"), true}, {"bad tiff", ".tif", []byte("not a tiff"), false}, + {"bad tiff near-miss prefix", ".tif", []byte("II-\x00rest"), false}, {"valid zip", ".zip", []byte("PK\x03\x04rest"), true}, {"bad zip", ".zip", []byte("RARrest"), false}, {"valid gpkg", ".gpkg", []byte("SQLite format 3\x00rest"), true}, diff --git a/internal/raster/godal.go b/internal/raster/godal.go index 904f44d..905bac9 100644 --- a/internal/raster/godal.go +++ b/internal/raster/godal.go @@ -66,10 +66,17 @@ func (r *godalReader) Close() error { return r.ds.Close() } type godalWriter struct{ ds *godal.Dataset } func (g *GodalIO) Create(_ context.Context, path string, grid Grid, o WriterOptions) (Writer, error) { + return createGTiff(path, grid, o, "IF_SAFER") +} + +// createGTiff writes a tiled Int16 GeoTIFF. bigTIFF is the value of the +// BIGTIFF creation option ("IF_SAFER" in production); it is a parameter so +// tests can force a BigTIFF header. +func createGTiff(path string, grid Grid, o WriterOptions, bigTIFF string) (Writer, error) { tile := fmt.Sprint(o.TileSize) ds, err := godal.Create(godal.GTiff, path, 1, godal.Int16, grid.Width, grid.Height, godal.CreationOption("TILED=YES", "BLOCKXSIZE="+tile, "BLOCKYSIZE="+tile, - "COMPRESS="+o.Compression, "PREDICTOR=2", "BIGTIFF=IF_SAFER")) + "COMPRESS="+o.Compression, "PREDICTOR=2", "BIGTIFF="+bigTIFF)) if err != nil { return nil, fmt.Errorf("create raster %s: %w", path, err) } diff --git a/internal/raster/godal_test.go b/internal/raster/godal_test.go index 783d826..a17cf1b 100644 --- a/internal/raster/godal_test.go +++ b/internal/raster/godal_test.go @@ -1,9 +1,14 @@ package raster import ( + "bytes" "context" + "io" + "os" "path/filepath" "testing" + + "gis/internal/domain" ) func TestGodalIO_RoundTrip(t *testing.T) { @@ -60,3 +65,39 @@ func TestGodalIO_RoundTrip(t *testing.T) { t.Fatal("SameGrid should be false for shifted origin") } } + +// TestGodalIO_BigTIFFPassesContentValidation guards the upload magic-byte +// check against the pipeline's own output: GDAL writes a BigTIFF header +// (version 43) whenever BIGTIFF is YES or IF_SAFER kicks in for a large +// raster, and domain.ValidateFileContent must accept it. +func TestGodalIO_BigTIFFPassesContentValidation(t *testing.T) { + path := filepath.Join(t.TempDir(), "big.tif") + grid := Grid{Width: 8, Height: 8, GeoTransform: [6]float64{100, 30, 0, 200, 0, -30}, ProjectionWKT: "EPSG:32642"} + + w, err := createGTiff(path, grid, DefaultWriterOptions(), "YES") + if err != nil { + t.Fatal(err) + } + if err := w.WriteInt16(Window{0, 0, 8, 8}, make([]int16, 8*8)); err != nil { + t.Fatal(err) + } + if err := w.Close(); err != nil { + t.Fatal(err) + } + + f, err := os.Open(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + head := make([]byte, 4) + if _, err := io.ReadFull(f, head); err != nil { + t.Fatal(err) + } + if !bytes.HasPrefix(head, []byte("II+\x00")) && !bytes.HasPrefix(head, []byte("MM\x00+")) { + t.Fatalf("BIGTIFF=YES did not produce a BigTIFF header: % x", head) + } + if err := domain.ValidateFileContent(".tif", head); err != nil { + t.Fatalf("ValidateFileContent rejected our own BigTIFF output: %v", err) + } +} diff --git a/internal/service/dataset.go b/internal/service/dataset.go index 640c6b7..d98c530 100644 --- a/internal/service/dataset.go +++ b/internal/service/dataset.go @@ -226,7 +226,11 @@ func (s *DatasetService) Upload(ctx context.Context, in UploadInput) (domain.Dat status = domain.DatasetStatusParsing } - storageKey := fmt.Sprintf("%s/%s", uuid.New().String(), in.Filename) + objectName, err := safeObjectName(in.Filename) + if err != nil { + return domain.Dataset{}, err + } + storageKey := fmt.Sprintf("%s/%s", uuid.New().String(), objectName) if err := s.store.Put(ctx, storageKey, content, in.Size, in.ContentType); err != nil { return domain.Dataset{}, err } @@ -297,7 +301,10 @@ func (s *DatasetService) UploadSLD(ctx context.Context, id uuid.UUID, in UploadS } content := io.MultiReader(bytes.NewReader(head), in.Reader) - key := deriveSLDKey(dataset.StorageKey, in.Filename) + key, err := deriveSLDKey(dataset.StorageKey, in.Filename) + if err != nil { + return domain.Dataset{}, err + } if err := s.store.Put(ctx, key, content, in.Size, in.ContentType); err != nil { return domain.Dataset{}, err } @@ -391,10 +398,27 @@ func (s *DatasetService) CreateRasterFromFile(ctx context.Context, in CreateRast }) } +// safeObjectName reduces a user-supplied filename to a single path element so +// it can never escape the object-key prefix it is appended to. Rejects names +// that carry no usable base ("", ".", "..", "/"). +func safeObjectName(name string) (string, error) { + base := path.Base(filepath.ToSlash(strings.TrimSpace(name))) + switch base { + case "", ".", "..", "/": + return "", fmt.Errorf("%w: invalid filename", domain.ErrValidation) + } + return base, nil +} + // deriveSLDKey places a dataset's SLD style file alongside the original under -// an sld/ prefix, named after the uploaded SLD file. -func deriveSLDKey(storageKey, filename string) string { - return path.Join(path.Dir(storageKey), "sld", filename) +// an sld/ prefix, named after the uploaded SLD file. The filename is +// sanitised, so the key always stays under the dataset's own prefix. +func deriveSLDKey(storageKey, filename string) (string, error) { + name, err := safeObjectName(filename) + if err != nil { + return "", err + } + return path.Join(path.Dir(storageKey), "sld", name), nil } // enqueueProcessing schedules the appropriate async job for a dataset based on diff --git a/internal/service/dataset_group.go b/internal/service/dataset_group.go index b833757..157d72c 100644 --- a/internal/service/dataset_group.go +++ b/internal/service/dataset_group.go @@ -151,7 +151,11 @@ func (s *DatasetGroupService) UploadSLD(ctx context.Context, id uuid.UUID, in Up } content := io.MultiReader(bytes.NewReader(head), in.Reader) - key := path.Join("groups", g.ID.String(), "sld", in.Filename) + name, err := safeObjectName(in.Filename) + if err != nil { + return domain.DatasetGroup{}, err + } + key := path.Join("groups", g.ID.String(), "sld", name) if err := s.store.Put(ctx, key, content, in.Size, in.ContentType); err != nil { return domain.DatasetGroup{}, err } diff --git a/internal/service/dataset_group_test.go b/internal/service/dataset_group_test.go index 373e3b1..7b0eceb 100644 --- a/internal/service/dataset_group_test.go +++ b/internal/service/dataset_group_test.go @@ -255,3 +255,29 @@ func TestDatasetGroupService_Update_RequiresSlug(t *testing.T) { t.Fatalf("want ErrValidation, got %v", err) } } + +func TestDatasetGroupService_UploadSLD_RejectsPathTraversal(t *testing.T) { + ctx := context.Background() + groups := newStubGroupRepo() + store := newStubStore() + svc := NewDatasetGroupService(groups, newStubDatasetRepo(), store) + g, err := svc.Create(ctx, DatasetGroupInput{Code: "pop", Name: "Population"}) + if err != nil { + t.Fatal(err) + } + + updated, err := svc.UploadSLD(ctx, g.ID, UploadSLDInput{ + Filename: "../../other/sld/evil.sld", ContentType: "application/xml", Size: 3, Reader: strings.NewReader(""), + }) + if err != nil { + t.Fatal(err) + } + wantPrefix := "groups/" + g.ID.String() + "/sld/" + if updated.SldStorageKey == nil || !strings.HasPrefix(*updated.SldStorageKey, wantPrefix) || + !strings.HasSuffix(*updated.SldStorageKey, "evil.sld") { + t.Fatalf("sld key escaped the group prefix: %v", updated.SldStorageKey) + } + if len(store.put) != 1 || !strings.HasPrefix(store.put[0], wantPrefix) { + t.Fatalf("object stored outside the group sld prefix: %v", store.put) + } +} diff --git a/internal/service/dataset_test.go b/internal/service/dataset_test.go index b6f9dde..431886d 100644 --- a/internal/service/dataset_test.go +++ b/internal/service/dataset_test.go @@ -1926,3 +1926,86 @@ func TestDatasetService_CreateRasterFromFile_MissingFile(t *testing.T) { t.Fatal("want an error for a missing file") } } + +func TestSafeObjectName(t *testing.T) { + cases := []struct { + name string + in string + want string + wantErr bool + }{ + {"plain name", "style.sld", "style.sld", false}, + {"trims space", " style.sld ", "style.sld", false}, + {"nested path reduced to base", "a/b/style.sld", "style.sld", false}, + {"traversal reduced to base", "../../x.sld", "x.sld", false}, + {"absolute path reduced to base", "/etc/passwd", "passwd", false}, + {"dotdot alone", "..", "", true}, + {"dot alone", ".", "", true}, + {"root alone", "/", "", true}, + {"empty", "", "", true}, + {"blank", " ", "", true}, + {"trailing slash", "a/b/", "b", false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := safeObjectName(c.in) + if c.wantErr { + if err == nil { + t.Fatalf("want error, got %q", got) + } + if !errors.Is(err, domain.ErrValidation) { + t.Fatalf("want ErrValidation, got %v", err) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != c.want { + t.Fatalf("safeObjectName(%q) = %q, want %q", c.in, got, c.want) + } + }) + } +} + +func TestDatasetService_UploadSLD_RejectsPathTraversal(t *testing.T) { + id := uuid.New() + repo := newStubDatasetRepo() + repo.store[id] = domain.Dataset{ID: id, FileType: domain.FileTypeRaster, StorageKey: "uid/dem.tif"} + store := &stubStore{} + svc := newDatasetService(repo, store, true) + + in := validSLDUpload() + in.Filename = "../../other/sld/evil.sld" + got, err := svc.UploadSLD(context.Background(), id, in) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.SldStorageKey == nil || *got.SldStorageKey != "uid/sld/evil.sld" { + t.Fatalf("sld key escaped the dataset prefix: %v", got.SldStorageKey) + } + if len(store.put) != 1 || !strings.HasPrefix(store.put[0], "uid/sld/") || !strings.HasSuffix(store.put[0], "evil.sld") { + t.Fatalf("object stored outside the dataset sld prefix: %v", store.put) + } +} + +func TestDatasetService_Upload_SanitisesTraversalFilename(t *testing.T) { + repo := newStubDatasetRepo() + store := &stubStore{} + svc := newDatasetService(repo, store, true) + + in := validUpload() + in.Filename = "../../evil.geojson" + got, err := svc.Upload(context.Background(), in) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // The key is "/": exactly two segments, no traversal. + parts := strings.Split(got.StorageKey, "/") + if len(parts) != 2 || parts[1] != "evil.geojson" { + t.Fatalf("storage key escaped its uuid prefix: %q", got.StorageKey) + } + if len(store.put) != 1 || store.put[0] != got.StorageKey { + t.Fatalf("object stored under an unexpected key: %v", store.put) + } +} diff --git a/internal/transport/http/dataset_handler.go b/internal/transport/http/dataset_handler.go index 0387c58..402a210 100644 --- a/internal/transport/http/dataset_handler.go +++ b/internal/transport/http/dataset_handler.go @@ -481,13 +481,13 @@ func (h *DatasetHandler) patch(w http.ResponseWriter, r *http.Request) { } in.Unit, in.UnitSet = unit, true case "dg_id": - raw, err := decodeOptionalPatchString(val) + s, err := decodeOptionalPatchString(val) if err != nil { httputil.WriteError(w, http.StatusUnprocessableEntity, "dg_id must be a valid UUID or null") return } - if raw != nil { - gid, err := uuid.Parse(*raw) + if s != nil { + gid, err := uuid.Parse(*s) if err != nil { httputil.WriteError(w, http.StatusUnprocessableEntity, "dg_id must be a valid UUID or null") return