fix(import): enforce geometry budgets and queued navigation

src/data/geoFileImport.js

@@ -154,18 +154,24 @@ export function parseGeoJsonText(text) {
  * Count features and vertices in a parsed GeoJSON object without recursion
  * limits or array spreading, stopping as soon as a limit is passed.
  * @param {object} geojson
- * @param {{maxFeatures?: number, maxPositions?: number}} [limits]
- * @returns {{features: number, positions: number, over: ''|'features'|'positions'}}
+ * @param {{maxFeatures?: number, maxPositions?: number, maxEntities?: number}} [limits]
+ * @returns {{features: number, positions: number, entities: number,
+ *   over: ''|'features'|'positions'|'entities'}}
  */
 export function countGeoJsonGeometry(
   geojson,
   {
     maxFeatures = MAX_IMPORT_ENTITIES,
     maxPositions = MAX_IMPORT_POSITIONS,
+    maxEntities = MAX_IMPORT_ENTITIES,
   } = {},
 ) {
-  let features = 0;
+  let features = ['Feature', 'FeatureCollection'].includes(geojson?.type)
+    ? 0
+    : 1;
   let positions = 0;
+  let entities = 0;
+  const result = (over = '') => ({ features, positions, entities, over });
   const stack = [geojson];
   while (stack.length) {
     const node = stack.pop();
@@ -177,8 +183,11 @@ export function countGeoJsonGeometry(
     }
     if (node.type === 'Feature') {
       features += 1;
-      if (features > maxFeatures)
-        return { features, positions, over: 'features' };
+      if (features > maxFeatures) return result('features');
+      if (node.geometry == null) {
+        entities += 1;
+        if (entities > maxEntities) return result('entities');
+      }
       stack.push(node.geometry);
       continue;
     }
@@ -188,8 +197,14 @@ export function countGeoJsonGeometry(
       continue;
     }
     if (GEOJSON_TYPES.has(node.type)) {
-      // A bare geometry is one feature: Cesium makes one entity for it.
-      if (node === geojson) features += 1;
+      // Multipart geometries create one Cesium entity per component, even
+      // when the document wraps them in just one Feature.
+      entities += node.type.startsWith('Multi')
+        ? Array.isArray(node.coordinates)
+          ? node.coordinates.length
+          : 0
+        : 1;
+      if (entities > maxEntities) return result('entities');
       // Walk the coordinate arrays iteratively: a position is an array whose
       // first item is a number.
       const coords = [node.coordinates];
@@ -198,17 +213,20 @@ export function countGeoJsonGeometry(
         if (!Array.isArray(c)) continue;
         if (typeof c[0] === 'number') {
           positions += 1;
-          if (positions > maxPositions)
-            return { features, positions, over: 'positions' };
+          if (positions > maxPositions) return result('positions');
         } else for (let i = 0; i < c.length; i++) coords.push(c[i]);
       }
     }
   }
-  return { features, positions, over: '' };
+  return result();
 }
 
 /** The refusal for a file over a size limit, in the person's terms. */
 function overLimitError({ over, features, positions }) {
+  if (over === 'entities')
+    return new GeoImportError(
+      `That file draws more than ${MAX_IMPORT_ENTITIES.toLocaleString()} objects (the limit).`,
+    );
   if (over === 'links')
     return new GeoImportError(
       `That file has more than ${MAX_KML_LINKS.toLocaleString()} styles and icon links (the limit).`,
@@ -485,8 +503,36 @@ export function sanitizeKmlDocument(doc, resolveLocal = () => null) {
   }
   // Placemark balloons are HTML; the app shows no info box, and keeping their
   // <img>/<iframe> sources out means no future surface can load them either.
-  for (const name of ['description', 'BalloonStyle', 'Snippet'])
+  for (const name of ['description', 'BalloonStyle', 'Snippet', 'ExtendedData'])
     elementsNamed(doc, name).forEach(drop);
+  // Draw recorded KML paths as their original static vertices. Letting the
+  // loader build time-dynamic tracks would expand each into thousands of
+  // interpolated points later and exceed the checked geometry budget.
+  for (const track of elementsNamed(doc, 'Track')) {
+    const line = doc.createElementNS(
+      'http://www.opengis.net/kml/2.2',
+      'LineString',
+    );
+    const coordinates = doc.createElementNS(
+      'http://www.opengis.net/kml/2.2',
+      'coordinates',
+    );
+    const recordedCoordinates = childrenNamed(track, 'coord').map((coord) =>
+      coord.textContent.trim().split(/\s+/).join(','),
+    );
+    coordinates.textContent = recordedCoordinates.join(' ');
+    line.appendChild(coordinates);
+    track.parentNode?.replaceChild(line, track);
+  }
+  for (const tracks of elementsNamed(doc, 'MultiTrack')) {
+    const geometry = doc.createElementNS(
+      'http://www.opengis.net/kml/2.2',
+      'MultiGeometry',
+    );
+    for (const line of childrenNamed(tracks, 'LineString'))
+      geometry.appendChild(line);
+    tracks.parentNode?.replaceChild(geometry, tracks);
+  }
   return { removed };
 }
 
@@ -508,23 +554,46 @@ const childText = (el, localName) =>
  * and `<Style>` elements (`links`), each of which can make Cesium load and
  * decode an image.
  * @param {Document} doc
- * @returns {{features: number, positions: number, links: number,
- *   over: ''|'features'|'positions'|'links'}}
+ * @returns {{features: number, entities: number, positions: number, links: number,
+ *   over: ''|'features'|'entities'|'positions'|'links'}}
  */
 export function countKmlGeometry(
   doc,
   {
     maxFeatures = MAX_IMPORT_ENTITIES,
+    maxEntities = MAX_IMPORT_ENTITIES,
     maxPositions = MAX_IMPORT_POSITIONS,
     maxLinks = MAX_KML_LINKS,
   } = {},
 ) {
   let features = 0;
+  let entities = 0;
   let positions = 0;
   let links = 0;
-  const result = (over) => ({ features, positions, links, over });
+  const result = (over) => ({ features, entities, positions, links, over });
+  const geometryNames = new Set([
+    'Point',
+    'LineString',
+    'LinearRing',
+    'Polygon',
+    'Track',
+    'MultiTrack',
+    'MultiGeometry',
+    'Model',
+  ]);
   for (const el of doc.getElementsByTagName('*')) {
     const name = el.localName;
+    // Cesium creates a parent for every placemark and a child entity for
+    // each geometry in a MultiGeometry, including nested containers.
+    if (
+      name === 'Placemark' ||
+      name === 'GroundOverlay' ||
+      (geometryNames.has(name) &&
+        ['MultiGeometry', 'MultiTrack'].includes(el.parentNode?.localName))
+    ) {
+      entities += 1;
+      if (entities > maxEntities) return result('entities');
+    }
     if (name === 'href' || name === 'Style') {
       links += 1;
       if (links > maxLinks) return result('links');
@@ -574,8 +643,12 @@ export function gpxToGeoJson(
   const features = [];
   let positions = 0;
   const point = (el) => {
-    const lat = Number(el.getAttribute('lat'));
-    const lon = Number(el.getAttribute('lon'));
+    const rawLat = el.getAttribute('lat');
+    const rawLon = el.getAttribute('lon');
+    if (rawLat == null || rawLon == null || !rawLat.trim() || !rawLon.trim())
+      return null;
+    const lat = Number(rawLat);
+    const lon = Number(rawLon);
     if (
       !Number.isFinite(lat) ||
       !Number.isFinite(lon) ||
@@ -789,10 +862,10 @@ export async function loadGeoFile(file, { viewer, color = '#39d0ff' }) {
         );
       } else {
         geojson = parseGeoJsonText(new TextDecoder().decode(bytes));
-        const size = countGeoJsonGeometry(geojson);
-        if (size.over) throw overLimitError(size);
         stripGeoJsonStyleProperties(geojson);
       }
+      const size = countGeoJsonGeometry(geojson);
+      if (size.over) throw overLimitError(size);
       dataSource = await Cesium.GeoJsonDataSource.load(geojson, {
         clampToGround: true,
         stroke,

src/data/geoFileImport.test.mjs

@@ -5,12 +5,14 @@ import * as Cesium from 'cesium';
 import {
   GeoImportError,
   MAX_IMPORT_POSITIONS,
+  MAX_IMPORT_ENTITIES,
   countGeoJsonGeometry,
   countKmlGeometry,
   detachImportedTime,
   gpxToGeoJson,
   importFeatureName,
   scrubImportedEntities,
+  sanitizeKmlDocument,
   stripGeoJsonStyleProperties,
   detectGeoFileFormat,
   importDisplayName,
@@ -231,8 +233,15 @@ function xmlEl(localName, attrs = {}, kids = []) {
     localName,
     childNodes: [],
     getAttribute: (n) => (n in attrs ? String(attrs[n]) : null),
+    removeChild(child) {
+      this.childNodes.splice(this.childNodes.indexOf(child), 1);
+      child.parentNode = null;
+    },
   };
-  for (const k of typeof kids === 'string' ? [] : kids) el.childNodes.push(k);
+  for (const k of typeof kids === 'string' ? [] : kids) {
+    k.parentNode = el;
+    el.childNodes.push(k);
+  }
   const text = typeof kids === 'string' ? kids : null;
   Object.defineProperty(el, 'textContent', {
     get: () => text ?? el.childNodes.map((c) => c.textContent).join(''),
@@ -252,6 +261,17 @@ const xmlDoc = (root) => ({
     return out;
   },
 });
+test('KML extended metadata is removed before the document reaches the loader', () => {
+  const point = xmlEl('Point', {}, [xmlEl('coordinates', {}, '1,2,0')]);
+  const metadata = xmlEl('ExtendedData', {}, [
+    xmlEl('Data', { name: 'note' }, [xmlEl('value', {}, 'ordinary metadata')]),
+  ]);
+  const placemark = xmlEl('Placemark', {}, [metadata, point]);
+  const doc = xmlDoc(xmlEl('kml', {}, [placemark]));
+  assert.equal(sanitizeKmlDocument(doc).removed, 1);
+  assert.deepEqual(placemark.childNodes, [point]);
+  assert.equal(metadata.parentNode, null);
+});
 const trkpt = (lat, lon, time) =>
   xmlEl('trkpt', { lat, lon }, time ? [xmlEl('time', {}, time)] : []);
 
@@ -363,6 +383,7 @@ test('GeoJSON features and vertices are counted before loading, without recursio
   assert.deepEqual(countGeoJsonGeometry(fc), {
     features: 3,
     positions: 55,
+    entities: 4,
     over: '',
   });
   assert.equal(
@@ -375,6 +396,7 @@ test('GeoJSON features and vertices are counted before loading, without recursio
     {
       features: 1,
       positions: 1,
+      entities: 1,
       over: '',
     },
   );
@@ -386,6 +408,35 @@ test('GeoJSON features and vertices are counted before loading, without recursio
   assert.equal(countGeoJsonGeometry(huge).over, 'positions');
 });
 
+test('multipart GeoJSON is refused before loading when it expands past the entity cap', () => {
+  const parts = Array.from({ length: MAX_IMPORT_ENTITIES + 1 }, () => [1, 2]);
+  for (const geometry of [
+    { type: 'MultiPoint', coordinates: parts },
+    {
+      type: 'GeometryCollection',
+      geometries: parts.map((coordinates) => ({ type: 'Point', coordinates })),
+    },
+    { type: 'MultiLineString', coordinates: parts.map((p) => [p, [2, 3]]) },
+  ]) {
+    const size = countGeoJsonGeometry({ type: 'Feature', geometry });
+    assert.equal(size.over, 'entities');
+  }
+});
+
+test('GPX points with missing or blank coordinates are skipped rather than placed at zero', () => {
+  const doc = xmlDoc(
+    xmlEl('gpx', {}, [
+      xmlEl('wpt', { lon: 2 }),
+      xmlEl('wpt', { lat: 1 }),
+      xmlEl('wpt', { lat: '', lon: 2 }),
+      xmlEl('wpt', { lat: 1, lon: ' ' }),
+      xmlEl('wpt', { lat: 0, lon: 0 }),
+    ]),
+  );
+  assert.equal(gpxToGeoJson(doc).features.length, 1);
+  assert.deepEqual(gpxToGeoJson(doc).features[0].geometry.coordinates, [0, 0]);
+});
+
 test('KML placemarks and coordinate tuples are counted before loading', () => {
   const coords = (n) =>
     xmlEl(
@@ -406,6 +457,7 @@ test('KML placemarks and coordinate tuples are counted before loading', () => {
   );
   assert.deepEqual(countKmlGeometry(doc), {
     features: 2,
+    entities: 2,
     positions: 42,
     links: 0,
     over: '',
@@ -414,6 +466,35 @@ test('KML placemarks and coordinate tuples are counted before loading', () => {
   assert.equal(countKmlGeometry(doc, { maxFeatures: 1 }).over, 'features');
 });
 
+test('KML multipart geometries count every Cesium entity before loading', () => {
+  const doc = xmlDoc(
+    xmlEl('kml', {}, [
+      xmlEl('Placemark', {}, [
+        xmlEl('MultiGeometry', {}, [
+          xmlEl('Point'),
+          xmlEl('MultiGeometry', {}, [xmlEl('Point'), xmlEl('LineString')]),
+          xmlEl('MultiTrack', {}, [xmlEl('Track'), xmlEl('Track')]),
+        ]),
+      ]),
+      xmlEl('GroundOverlay'),
+    ]),
+  );
+  assert.equal(countKmlGeometry(doc).entities, 9);
+  assert.equal(countKmlGeometry(doc, { maxEntities: 8 }).over, 'entities');
+  const oversized = xmlDoc(
+    xmlEl('kml', {}, [
+      xmlEl('Placemark', {}, [
+        xmlEl(
+          'MultiGeometry',
+          {},
+          Array.from({ length: MAX_IMPORT_ENTITIES }, () => xmlEl('Point')),
+        ),
+      ]),
+    ]),
+  );
+  assert.equal(countKmlGeometry(oversized).over, 'entities');
+});
+
 test('KML styles and icon links are counted against their own limit', () => {
   const style = () =>
     xmlEl('Style', {}, [

src/ui/geoFileImport.js

@@ -212,17 +212,18 @@ export function initGeoFileImport({
    */
   function importFiles(files) {
     const list = [...(files || [])];
-    const run = queue.then(() => importBatch(list));
+    if (destroyed || !list.length) return Promise.resolve([]);
+    // Capture the user's intent when the files are chosen, including time
+    // spent waiting for an earlier batch. Later navigation must still win.
+    const generation = beginAutoFly();
+    const run = queue.then(() => importBatch(list, generation));
     queue = run.catch(() => {});
     return run;
   }
 
-  async function importBatch(list) {
+  async function importBatch(list, generation) {
     const outcomes = [];
     if (destroyed || !list.length) return outcomes;
-    // Stamped now, re-asserted before the fly: whatever navigation happens
-    // while the files load (a drag, a search, Cockpit) supersedes this one.
-    const generation = beginAutoFly();
     for (const file of list) {
       if (destroyed) break;
       const fileName = String(file?.name || 'file');

src/ui/geoFileImport.test.mjs

@@ -529,6 +529,42 @@ test('a slow import does not fly once something newer took the camera', async ()
   assert.equal(flown.length, 0);
 });
 
+test('a queued import cannot fly over navigation that happened while it waited', async () => {
+  let release;
+  const gate = new Promise((resolve) => {
+    release = resolve;
+  });
+  let generation = 0;
+  const flown = [];
+  const nav = {
+    beginDeferredLocationNavigation: () => ++generation,
+    reassertDeferredLocationNavigation: (stamp) => stamp === generation,
+  };
+  const t = setup({
+    navigation: nav,
+    flyToSource: (v, ds) => flown.push(ds),
+    load: async (f) => {
+      if (f.name === 'first.geojson') await gate;
+      return {
+        dataSource: {},
+        name: f.name,
+        format: 'geojson',
+        entityCount: 1,
+        revoke() {},
+      };
+    },
+  });
+  const first = t.tool.importFiles([file('first.geojson')]);
+  await new Promise((resolve) => setImmediate(resolve));
+  const queued = t.tool.importFiles([file('queued.geojson')]);
+  generation++; // A newer user camera action while the second batch is waiting.
+  release();
+  const results = await Promise.all([first, queued]);
+  assert.ok(results.every(([outcome]) => outcome.ok));
+  assert.equal(t.tool.list().length, 2);
+  assert.equal(flown.length, 0);
+});
+
 test('a refused deferred stamp (Cockpit) imports without flying', async () => {
   const flown = [];
   const nav = navigationDouble();