Validate incoming GPS data (#951)

* Validate incoming GPS data and throw an error if it is incorrect, storing Null values

* Extracted GPS data processing to function in external parser; optimized IF in internal parser; removed unnecessary comments; set exact values for positive test

* Add the migration for removing existing invalid GPS data and its test; added better errors to asserts in the GPS validation test

* Install FFmpeg and ExifTool on the API unit-test environment

* Fix 'stripped.jpg', 'IncorrectGPS.jpg', and 'CorrectGPS.jpg' tests for the external parser

* Optimized data validation in the external parser, returned error by the internal parser for invalid data, updated test to expect errors and handle them

* Switched from error to log entry in case of incorrect GPS data, as error handling is not so transparent in the internal parser

---------

Co-authored-by: Konstantin Koval

.github/workflows/tests.yml

@@ -72,12 +72,12 @@ jobs:
           restore-keys: |
             ${{ runner.os }}-go-
 
-      - name: Get C dependencies
+      - name: Get C dependencies and 3rd-party tools
         run: |
           sudo add-apt-repository ppa:strukturag/libheif
           sudo add-apt-repository ppa:strukturag/libde265
           sudo apt-get update
-          sudo apt-get install -y libdlib-dev libblas-dev libatlas-base-dev liblapack-dev libjpeg-turbo8-dev libheif-dev
+          sudo apt-get install -y libdlib-dev libblas-dev libatlas-base-dev liblapack-dev libjpeg-turbo8-dev libheif-dev ffmpeg exiftool
 
       - name: Get GO dependencies
         run: |

.gitignore

@@ -13,8 +13,9 @@ testing.env
 # docker
 docker-compose.yml
 /Makefile
-storage
+/storage
-database
+/database
+/tmp
 
 # dependencies
 node_modules/

api/database/database.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/photoview/photoview/api/database/drivers"
+	"github.com/photoview/photoview/api/database/migrations"
 	"github.com/photoview/photoview/api/graphql/models"
 	"github.com/photoview/photoview/api/utils"
 	"github.com/pkg/errors"
@@ -194,6 +195,11 @@ func MigrateDatabase(db *gorm.DB) error {
 		log.Printf("Failed to run exif fields migration: %v\n", err)
 	}
 
+	// Remove invalid GPS data from DB
+	if err := migrations.MigrateForExifGPSCorrection(db); err != nil {
+		log.Printf("Failed to run exif GPS correction migration: %v\n", err)
+	}
+
 	return nil
 }
 

api/database/migrations/exif_invalid_gps.go

@@ -0,0 +1,23 @@
+package migrations
+
+import (
+	"github.com/photoview/photoview/api/graphql/models"
+	"github.com/pkg/errors"
+	"gorm.io/gorm"
+)
+
+// MigrateForExifGPSCorrection finds and removes invalid GPS data from media_exif table
+func MigrateForExifGPSCorrection(db *gorm.DB) error {
+	return db.Transaction(func(tx *gorm.DB) error {
+		if err := tx.Model(&models.MediaEXIF{}).
+			Where("ABS(gps_longitude) > ?", 90).
+			Or("ABS(gps_latitude) > ?", 90).
+			Updates(map[string]interface{}{
+				"gps_latitude":  nil,
+				"gps_longitude": nil,
+			}).Error; err != nil {
+			return errors.Wrap(err, "failed to remove invalid GPS data from media_exif table")
+		}
+		return nil
+	})
+}

api/database/migrations/exif_invalid_gps_test.go

@@ -0,0 +1,74 @@
+package migrations_test
+
+import (
+	"bufio"
+	"math"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/photoview/photoview/api/database/migrations"
+	"github.com/photoview/photoview/api/graphql/models"
+	"github.com/photoview/photoview/api/test_utils"
+)
+
+func TestExifMigration(t *testing.T) {
+	envFile, err := os.Open("/home/runner/work/photoview/photoview/api/testing.env")
+	if err != nil {
+		t.Fatalf("Failed to open environment file: %v", err)
+	}
+	defer envFile.Close()
+
+	scanner := bufio.NewScanner(envFile)
+	for scanner.Scan() {
+		line := scanner.Text()
+		parts := strings.SplitN(line, "=", 2)
+		if len(parts) != 2 {
+			t.Fatalf("Invalid line in environment file: %s", line)
+		}
+		key, value := parts[0], strings.Trim(parts[1], "'")
+		os.Setenv(key, value)
+	}
+
+	db := test_utils.DatabaseTest(t)
+	defer db.Exec("DELETE FROM media_exif") // Clean up after test
+
+	// Create test data
+	exifEntries := []models.MediaEXIF{
+		{GPSLatitude: floatPtr(90.1), GPSLongitude: floatPtr(90.0)},   // Invalid GPSLatitude
+		{GPSLatitude: floatPtr(-90.1), GPSLongitude: floatPtr(-90.0)}, // Invalid GPSLatitude
+		{GPSLatitude: floatPtr(90.0), GPSLongitude: floatPtr(90.1)},   // Invalid GPSLongitude
+		{GPSLatitude: floatPtr(-90.0), GPSLongitude: floatPtr(-90.1)}, // Invalid GPSLongitude
+		{GPSLatitude: floatPtr(90.0), GPSLongitude: floatPtr(90.0)},   // Valid GPS data
+		{GPSLatitude: floatPtr(-90.0), GPSLongitude: floatPtr(-90.0)}, // Valid GPS data
+		{GPSLatitude: floatPtr(90.1), GPSLongitude: floatPtr(90.1)},   // Invalid GPSLatitude and GPSLongitude
+		{GPSLatitude: floatPtr(-90.1), GPSLongitude: floatPtr(-90.1)}, // Invalid GPSLatitude and GPSLongitude
+	}
+
+	// Insert test data
+	for _, entry := range exifEntries {
+		assert.NoError(t, db.Create(&entry).Error)
+	}
+
+	// Run migration
+	assert.NoError(t, migrations.MigrateForExifGPSCorrection(db))
+
+	// Validate the results
+	var results []models.MediaEXIF
+	assert.NoError(t, db.Find(&results).Error)
+
+	for _, entry := range results {
+		if entry.GPSLatitude != nil {
+			assert.LessOrEqual(t, math.Abs(*entry.GPSLatitude), 90.0, "GPSLatitude should be within [-90, 90]: %+v", entry)
+		}
+		if entry.GPSLongitude != nil {
+			assert.LessOrEqual(t, math.Abs(*entry.GPSLongitude), 90.0, "GPSLongitude should be within [-90, 90]: %+v", entry)
+		}
+	}
+}
+
+func floatPtr(f float64) *float64 {
+	return &f
+}

api/scanner/exif/exif_parser_external.go

@@ -63,6 +63,31 @@ func sanitizeEXIF(exif *models.MediaEXIF) {
 	}
 }
 
+func extractValidGpsData(fileInfo *exiftool.FileMetadata, media_path string) (*float64, *float64) {
+	var GPSLat, GPSLong *float64
+
+	// GPS coordinates - longitude
+	longitudeRaw, err := fileInfo.GetFloat("GPSLongitude")
+	if err == nil {
+		GPSLong = &longitudeRaw
+	}
+
+	// GPS coordinates - latitude
+	latitudeRaw, err := fileInfo.GetFloat("GPSLatitude")
+	if err == nil {
+		GPSLat = &latitudeRaw
+	}
+
+	// GPS data validation
+	if (GPSLat != nil && math.Abs(*GPSLat) > 90) || (GPSLong != nil && math.Abs(*GPSLong) > 90) {
+		log.Printf(
+			"Incorrect GPS data in the %s Exif data: %f, %f, while expected values between '-90' and '90'. Ignoring GPS data.",
+			media_path, *GPSLat, *GPSLong)
+		return nil, nil
+	}
+	return GPSLat, GPSLong
+}
+
 func (p *externalExifParser) ParseExif(media_path string) (returnExif *models.MediaEXIF, returnErr error) {
 	// ExifTool - No print conversion mode
 	if p.et == nil {
@@ -182,18 +207,10 @@ func (p *externalExifParser) ParseExif(media_path string) (returnExif *models.Me
 		newExif.ExposureProgram = &expProgram
 	}
 
-	// GPS coordinates - longitude
+	// Get GPS data
-	longitudeRaw, err := fileInfo.GetFloat("GPSLongitude")
+	newExif.GPSLatitude, newExif.GPSLongitude = extractValidGpsData(&fileInfo, media_path)
-	if err == nil {
+	if (newExif.GPSLatitude != nil) && (newExif.GPSLongitude != nil) {
 		found_exif = true
-		newExif.GPSLongitude = &longitudeRaw
-	}
-
-	// GPS coordinates - latitude
-	latitudeRaw, err := fileInfo.GetFloat("GPSLatitude")
-	if err == nil {
-		found_exif = true
-		newExif.GPSLatitude = &latitudeRaw
 	}
 
 	if !found_exif {

api/scanner/exif/exif_parser_internal.go

@@ -3,6 +3,7 @@ package exif
 import (
 	"fmt"
 	"log"
+	"math"
 	"math/big"
 	"os"
 	"time"
@@ -142,9 +143,17 @@ func (p internalExifParser) ParseExif(media_path string) (returnExif *models.Med
 
 	lat, long, err := exifTags.LatLong()
 	if err == nil {
+		if math.Abs(lat) > 90 || math.Abs(long) > 90 {
+			returnExif = &newExif
+			log.Printf(
+				"Incorrect GPS data in the %s Exif data: %f, %f, while expected values between '-90' and '90'. Ignoring GPS data.",
+				media_path, long, lat)
+			return
+		} else {
 			newExif.GPSLatitude = &lat
 			newExif.GPSLongitude = &long
 		}
+	}
 
 	returnExif = &newExif
 	return

api/scanner/exif/exif_test.go

@@ -43,11 +43,12 @@ func TestExifParsers(t *testing.T) {
 
 	images := []struct {
 		path   string
-		assert func(t *testing.T, exif *models.MediaEXIF)
+		assert func(t *testing.T, exif *models.MediaEXIF, err error)
 	}{
 		{
 			path: "./test_data/bird.jpg",
-			assert: func(t *testing.T, exif *models.MediaEXIF) {
+			assert: func(t *testing.T, exif *models.MediaEXIF, err error) {
+				assert.NoError(t, err)
 				assert.EqualValues(t, *exif.Description, "Photo of a Bird")
 				assert.WithinDuration(t, *exif.DateShot, time.Unix(1336318784, 0).UTC(), time.Minute)
 				assert.EqualValues(t, *exif.Camera, "Canon EOS 600D")
@@ -65,16 +66,61 @@ func TestExifParsers(t *testing.T) {
 		},
 		{
 			path: "./test_data/stripped.jpg",
-			assert: func(t *testing.T, exif *models.MediaEXIF) {
+			assert: func(t *testing.T, exif *models.MediaEXIF, err error) {
+				assert.NoError(t, err)
+				if exif == nil {
 					assert.Nil(t, exif)
+				} else {
+					assert.Equal(t, 0, exif.ID)
+					assert.True(t, exif.CreatedAt.IsZero())
+					assert.True(t, exif.UpdatedAt.IsZero())
+					assert.Nil(t, exif.Description)
+					assert.Nil(t, exif.Camera)
+					assert.Nil(t, exif.Maker)
+					assert.Nil(t, exif.Lens)
+					assert.Nil(t, exif.Exposure)
+					assert.Nil(t, exif.Aperture)
+					assert.Nil(t, exif.Iso)
+					assert.Nil(t, exif.FocalLength)
+					assert.Nil(t, exif.Flash)
+					assert.Nil(t, exif.Orientation)
+					assert.Nil(t, exif.ExposureProgram)
+					assert.Nil(t, exif.GPSLatitude)
+					assert.Nil(t, exif.GPSLongitude)
+				}
 			},
 		},
 		{
 			path: "./test_data/bad-exif.jpg",
-			assert: func(t *testing.T, exif *models.MediaEXIF) {
+			assert: func(t *testing.T, exif *models.MediaEXIF, err error) {
+				assert.NoError(t, err)
 				assert.Nil(t, exif.Exposure)
 			},
 		},
+		{
+			path: "./test_data/IncorrectGPS.jpg",
+			assert: func(t *testing.T, exif *models.MediaEXIF, err error) {
+				assert.Nil(t, exif.GPSLatitude,
+					"GPSLatitude expected to be NULL for an incorrect input data: %+v", exif.GPSLatitude)
+				assert.Nil(t, exif.GPSLongitude,
+					"GPSLongitude expected to be NULL for an incorrect input data: %+v", exif.GPSLongitude)
+			},
+		},
+		{
+			path: "./test_data/CorrectGPS.jpg",
+			assert: func(t *testing.T, exif *models.MediaEXIF, err error) {
+				const precision = 1e-7
+				assert.NoError(t, err)
+				assert.NotNil(t, exif.GPSLatitude,
+					"GPSLatitude expected to be Not-NULL for a correct input data: %+v", exif.GPSLatitude)
+				assert.NotNil(t, exif.GPSLongitude,
+					"GPSLongitude expected to be Not-NULL for a correct input data: %+v", exif.GPSLongitude)
+				assert.InDelta(t, *exif.GPSLatitude, 44.478997222222226, precision,
+					"The exact value from input data is expected: %+v", exif.GPSLatitude)
+				assert.InDelta(t, *exif.GPSLongitude, 11.297922222222223, precision,
+					"The exact value from input data is expected: %+v", exif.GPSLongitude)
+			},
+		},
 	}
 
 	for _, p := range parsers {
@@ -90,9 +136,7 @@ func TestExifParsers(t *testing.T) {
 
 				exif, err := p.parser.ParseExif(img.path)
 
-				if assert.NoError(t, err) {
+				img.assert(t, exif, err)
-					img.assert(t, exif)
-				}
 			})
 		}
 	}