| # Security Threat Model: @google/markerclusterer (AppSheet Fork) |
| |
| ## Asset Definition & Scope |
| |
| `@google/markerclusterer` is a single-file (~1,290 line) browser JavaScript |
| library that groups large numbers of Google Maps JS API v3 markers into |
| per-zoom-level clusters, drawing each cluster as a custom `OverlayView` `<div>` |
| positioned over the map. Repository: |
| `sso://gnocchi-internal/third_party/@google/markerclusterer`, branch `main` |
| (mirrored at `appsheet-third-party/@google/markerclusterer`). |
| |
| - **Upstream:** `@google/markerclusterer` version `1.0.3` — the legacy |
| `MarkerClusterer` from `googlemaps/v3-utility-library` |
| (`last_upgrade_date` 2023-06-07, per `METADATA`). |
| |
| > [!NOTE] |
| > The `third_party.url` field in `METADATA` points at |
| > `https://github.com/googlemaps/js-markerclusterer`, which is the |
| > *successor* project. The vendored code and the pinned `1.0.3` version are |
| > from the older `v3-utility-library` `markerclusterer` (matching |
| > `package.json`'s `repository.url`). The METADATA URL is inaccurate and |
| > should not be used to diff against upstream. |
| |
| - **Scope:** `src/markerclusterer.js` (the entire library; `package.json` |
| declares it as both `main` and the sole entry in `files`). |
| - **Deployment context:** Executes in the end user's browser as part of the |
| AppSheet web frontend. Not used server-side. |
| |
| ## Consumption in AppSheet |
| |
| A **direct, first-class runtime dependency** of the AppSheet frontend: |
| |
| - `jeenee/Nirvana/Content/package.json`: |
| `"@google/markerclusterer": "git+https://appsheet-third-party.googlesource.com/@google/markerclusterer"` |
| - `package-lock.json` resolves it to commit |
| `7b91a86be0a313447f55813c81ee801ac9c9bd4d`, version `1.0.3`. |
| - `Nirvana/Content/scripts/_shared/lib/GoogleMapInstance.ts` — |
| `import MarkerClusterer from '@google/markerclusterer'`; `initMarkerClusterer` |
| constructs `new MarkerClusterer(this.map, [], { maxZoom, zoomOnClick, |
| minimumClusterSize })`, caches one clusterer per theme colour in |
| `_markerClusterers`, and calls `markerClusterer.setStyles(...)`. |
| `clearMarkerClusterers` invokes `clearMarkers()` on unmount. |
| - `Nirvana/Content/components/app/ClusterMapPane.tsx` — drives it from the |
| customer-facing map view (`initMarkerClusterer`, |
| `updateMarkerClustererOptions`). |
| |
| Cluster icon images are generated locally by `clusterIconSVG(color, diameter)` |
| in `GoogleMapInstance.ts`, which interpolates an app theme colour into an inline |
| SVG, base64-encodes it, and passes the resulting |
| `data:image/svg+xml;base64,…` URI as `styles[].url`. `textColor` is the |
| hardcoded string `'white'`. |
| |
| The library therefore runs on every AppSheet map view rendering |
| customer-authored location rows. It is genuinely used. |
| |
| ## The AppSheet Customization |
| |
| Four commits. Two are Trusted Types remediation, one is a follow-up fix to a |
| regression introduced by that remediation, and one is a Maps API compatibility |
| fix. |
| |
| | Commit | Change | |
| |---|---| |
| | `392fd44` "Inital import" | Upstream `@google/markerclusterer` 1.0.3, unmodified. | |
| | `c386355` "[nwd] Patching innerHTML violations" (`go/trusted-types-appsheet`) | In `src/markerclusterer.js`, replaced `this.div_.innerHTML = this.sums_.text` in `ClusterIcon.prototype.onAdd` and `this.div_.innerHTML = sums.text` in `ClusterIcon.prototype.setSums` with `this.div_.text = …`. | |
| | `57c6edf` "Fix a bug" | Corrected both to `this.div_.textContent = …`. The previous patch used `div.text`, which is not a rendering property on `HTMLDivElement`, so cluster count labels silently stopped rendering. This is the commit that makes the Trusted Types fix actually correct. | |
| | `7b91a86` (HEAD) "Fix for map cluster size icons not disappearing after zoom" (b/493541353) | Renamed `ClusterIcon.prototype.remove` to `ClusterIcon.prototype.removeClusterIcon` (and its caller in `Cluster.prototype.remove`) to avoid a name collision with a newly added `google.maps.OverlayView.remove` API that was preventing cluster icons from being torn down on zoom. | |
| |
| **Security-relevant net effect:** the cluster label sink is now `textContent`, |
| so cluster label text — including anything a custom `setCalculator()` might |
| return — is inserted as text, not markup. Upstream's `innerHTML` XSS sink is |
| closed in this fork. Everything else in the file is upstream code, including the |
| unescaped CSS string construction described below, which the patch did **not** |
| address. |
| |
| ## Prioritization Signals |
| |
| - **1P OSS:** No (Third-party OSS maintained as an internal customized fork |
| for Google AppSheet; nominally Google-authored upstream, but consumed here |
| as an unmaintained vendored 3P dependency) |
| - **1P Proprietary Shipped Software:** Yes (Bundled and minified into the |
| AppSheet frontend JavaScript shipped to browsers) |
| - **High-Risk Code Surface:** Yes — the library creates DOM overlay elements |
| per cluster, writes bulk inline CSS via unescaped string concatenation into |
| `style.cssText`, builds image URLs by string concatenation, renders |
| caller-supplied label text, and performs floating-point coordinate and |
| pixel-projection arithmetic on customer-supplied latitude/longitude values. |
| - **Perimeter Exposure:** Yes (AppSheet is a 1P service exposed to the public |
| Internet and customers; map views are rendered for end users of |
| customer-authored apps) |
| - **Data Sensitivity:** Medium — the library processes customer-authored |
| location data: geographic coordinates from app data rows, which frequently |
| represent real addresses, asset locations, or individuals' positions, and |
| can constitute PII. It also renders aggregate counts derived from that data. |
| Data stays client-side and in-memory; nothing is persisted or transmitted by |
| the library itself. |
| - **Untrusted Input Handling:** Yes — marker positions originate from |
| customer-authored app data rows, and clusterer styling/behaviour options |
| (`imagePath`, `imageExtension`, `styles[]`, `gridSize`, `maxZoom`, |
| `minimumClusterSize`, and any custom `calculator`) originate from |
| app-author-controlled configuration surfaced through AppSheet's map view |
| settings. |
| - **Business Value:** Renders the clustered map view, a core AppSheet |
| presentation type. Realistic security impact is bounded — the historic |
| `innerHTML` XSS is patched — but the remaining unescaped `style.cssText` |
| construction is a live CSS-injection sink whose exploitability depends |
| entirely on how much of `styles[]` AppSheet ever lets an app author control. |
| |
| ## Scanning Harness Prompts |
| |
| 1. **Unescaped CSS string construction in `ClusterIcon.prototype.createCss` |
| (primary sink).** This function builds a complete inline style string by raw |
| concatenation and the result is assigned to `this.div_.style.cssText` in |
| both `ClusterIcon.prototype.onAdd` and `ClusterIcon.prototype.show`. The |
| interpolated values are `this.url_`, `this.backgroundPosition_`, |
| `this.textColor_`, `this.textSize_`, `this.height_`, `this.width_`, |
| `this.anchor_[0]`/`[1]`, and `pos.x`/`pos.y`. **None of the string-typed |
| values are escaped or type-checked** (only `anchor_` members get |
| `typeof === 'number'` guards). Concretely: |
| - `style.push('background-image:url(' + this.url_ + ');')` — a `url_` |
| containing `)` plus additional declarations breaks out of the `url()` |
| token and injects arbitrary CSS. |
| - `'color:' + txtColor + ';'` where `txtColor = this.textColor_` — the |
| cleanest injection point; a value such as |
| `red; background-image: url(https://attacker.example/leak)` appends |
| attacker-chosen declarations. |
| - `'background-position:' + backgroundPosition + ';'`, and |
| `height`/`line-height`/`width`/`font-size` built from `height_`, |
| `width_`, `textSize_` with no numeric coercion. |
| Trace all of these back to `MarkerClusterer.prototype.setStyles` / |
| `ClusterIcon.prototype.useStyle` and determine whether any element of the |
| `styles[]` array can be influenced by app-author configuration. In the |
| current AppSheet integration they are supplied by `GoogleMapInstance.ts` |
| as generated data URIs and the literal `'white'`, which bounds the |
| severity — but `setStyles` is a public API and this is the highest-value |
| area to review. |
| 2. **Image URL construction in `MarkerClusterer.prototype.setupStyles_`.** |
| `url: this.imagePath_ + (i + 1) + '.' + this.imageExtension_` concatenates |
| two constructor options into a URL that is then interpolated into |
| `background-image:url(...)`. Check whether `imagePath`/`imageExtension` can |
| be set from app configuration; if so this is both a CSS-injection vector and |
| an outbound-request / SSRF-by-browser primitive (arbitrary third-party |
| resource load, and a `javascript:`/`data:` URL attempt). |
| 3. **Label rendering path (verify the patch, and that it stays).** |
| `ClusterIcon.prototype.setSums` and `ClusterIcon.prototype.onAdd` write |
| `sums.text` via `textContent`. `sums` comes from |
| `MarkerClusterer.prototype.calculator_`, which is replaceable through the |
| public `MarkerClusterer.prototype.setCalculator`. Confirm no `innerHTML`, |
| `insertAdjacentHTML`, `outerHTML`, or `document.write` has been reintroduced |
| anywhere in `src/markerclusterer.js`, and that any AppSheet-supplied custom |
| calculator is treated as returning untrusted text. Reversion here |
| re-opens stored XSS reachable from customer-authored app data. |
| 4. **Numeric and coordinate handling.** Customer app data can yield missing, |
| non-numeric, out-of-range, `NaN`, or `Infinity` latitude/longitude values. |
| Review `MarkerClusterer.prototype.getExtendedBounds` (pixel projection and |
| `gridSize_` offsets), `MarkerClusterer.prototype.distanceBetweenPoints_` |
| (haversine using `Math.sin`/`Math.cos`/`Math.atan2`), |
| `MarkerClusterer.prototype.isMarkerInBounds_`, |
| `MarkerClusterer.prototype.createClusters_`, |
| `Cluster.prototype.isMarkerInClusterBounds`, and |
| `ClusterIcon.prototype.getPosFromLatLng_` (which uses |
| `parseInt(this.width_ / 2, 10)`). Look for: `NaN` propagating into |
| `pos.x`/`pos.y` and thence into the `cssText` string; division by zero or a |
| zero/negative `gridSize_` producing non-terminating clustering loops; |
| quadratic blow-up in the marker-to-cluster matching loops for large marker |
| counts (client-side DoS on a map view with many rows); and |
| `parseInt(dv / 10, 10)` in `calculator_` behaving unexpectedly for |
| non-integer counts. |
| 5. **Event listener and overlay lifecycle (memory leak / DoS).** The |
| `MarkerClusterer` constructor registers `zoom_changed` and `idle` listeners |
| on the map via `google.maps.event.addListener` and **never removes them**; |
| `MarkerClusterer.prototype.clearMarkers` only tears down clusters, not |
| listeners. Each marker additionally gets a `dragend` listener, and |
| `ClusterIcon.prototype.onAdd` attaches a `click` listener via |
| `google.maps.event.addDomListener` on every re-add. Verify against |
| `ClusterIcon.prototype.onRemove`, `ClusterIcon.prototype.removeClusterIcon`, |
| and `Cluster.prototype.remove` (which does `delete this.markers_`) that |
| repeated mount/unmount cycles in `GoogleMapInstance.ts` do not accumulate |
| listeners and detached DOM across a long AppSheet session. |
| 6. **Global scope pollution.** The Closure export block at the bottom of |
| `src/markerclusterer.js` executes `var window = window || {};` and then |
| assigns `window['MarkerClusterer']`. Review this for unintended global |
| creation or shadowing under the AppSheet bundler and strict-mode settings, |
| and for whether exposing `MarkerClusterer` on `window` gives page scripts an |
| unnecessary handle on the clustering internals. |
| 7. **Fork drift.** Upstream `@google/markerclusterer` 1.0.3 is effectively |
| unmaintained (superseded by `js-markerclusterer`). Ensure any future |
| re-sync or upgrade preserves the `textContent` patch (commits `c386355` + |
| `57c6edf`) and the `removeClusterIcon` rename (`7b91a86`); both are |
| load-bearing. |
| |
| ## Entry Points and Untrusted Inputs |
| |
| | Entry Point | Type | Trusted? | Validation | |
| |---|---|---|---| |
| | `new MarkerClusterer(map, markers, options)` (`src/markerclusterer.js`) | Constructor; options object | No — `options` originate from AppSheet map view configuration | Defaults applied with `\|\|` (`gridSize_` → 60, `maxZoom_`, `minClusterSize_`, `imagePath_`, `imageExtension_`, `styles_`); **no type or range checking** on any value | |
| | `MarkerClusterer.prototype.addMarker` / `addMarkers` / `pushMarkerTo_` | Marker ingestion | No — marker positions derive from customer-authored app data rows | Checks `marker.getPosition()` presence and `isMarkerInBounds_`; no validation that coordinates are finite numbers | |
| | `MarkerClusterer.prototype.setStyles(styles)` / `ClusterIcon.prototype.useStyle` | Rendering configuration (`url`, `height`, `width`, `textColor`, `textSize`, `anchor`, `backgroundPosition`) | No — public API; in AppSheet currently fed generated data URIs and literals from `GoogleMapInstance.ts` | **None.** Values are interpolated directly into `createCss` output. `anchor_` members alone get `typeof === 'number'` and range checks. | |
| | `MarkerClusterer.prototype.setCalculator(calculator)` | Caller-supplied function returning `{ text, index }` | No — public API | None. Returned `index` is clamped in `useStyle` via `Math.max`/`Math.min`; returned `text` is rendered via `textContent` (safe as text) | |
| | `MarkerClusterer.prototype.setGridSize` / `setMaxZoom` / `setMinimumClusterSize` / `setImagePath` / `setImageExtension` | Runtime configuration setters | No | None | |
| | Google Maps `zoom_changed` / `idle` map events; marker `dragend`; cluster `<div>` `click` | Browser / Maps API event stream | Yes (Maps API and user gestures) | Handlers read map zoom and bounds; `click` fires the `clusterclick` event and optionally `fitBounds` | |
| | `google.maps.LatLng` positions and projection results | Maps API | Yes (API), but seeded with untrusted coordinates | `getProjection().fromLatLngToDivPixel` output used arithmetically without finiteness checks | |
| |
| ## Trust Boundaries and Auth Assumptions |
| |
| - **Authentication**: None at the library level. All authentication and |
| session management is handled by AppSheet before the map view is rendered. |
| - **Authorization**: None at the library level. Which rows — and therefore |
| which coordinates — reach the browser is decided by AppSheet's security |
| filters and row-level access control upstream. The clusterer renders |
| whatever it is handed. |
| - **Implicit trust**: (a) `styles[]` entries, `imagePath`, and |
| `imageExtension` are trusted to be safe CSS/URL fragments — the library |
| performs no escaping and this is the single most important assumption in the |
| model. (b) Marker positions are assumed to be valid, finite |
| `google.maps.LatLng` values. (c) A custom `calculator` is assumed to return |
| a well-formed `{ text, index }`. (d) The Google Maps API `OverlayView` |
| contract is assumed stable — an assumption that already broke once, per |
| b/493541353. |
| - **Boundary crossings**: Customer-authored app data (location rows, labels, |
| map view configuration) → AppSheet backend → AppSheet frontend bundle → |
| `MarkerClusterer` → live DOM in the end user's browser, inside the |
| application's origin. The library sits at the final, rendering end of that |
| chain, which is why its DOM and CSS sinks matter. |
| |
| ## Sensitive Data Paths |
| |
| | Data Type | Source | Destination | Protection | |
| |---|---|---|---| |
| | Customer location coordinates (latitude/longitude, potentially PII) | Customer app data rows → `google.maps.Marker` positions → `addMarkers` | In-memory `markers_` / `clusters_` arrays; projected to pixel offsets in `getPosFromLatLng_`; aggregated into cluster bounds | Client-side and in-memory only; never serialised or transmitted by the library. Row visibility is enforced upstream by AppSheet security filters. | |
| | Cluster label text (aggregate marker counts) | `MarkerClusterer.prototype.calculator_` (or a caller-supplied calculator) | `ClusterIcon.prototype.onAdd` / `setSums` → `div_.textContent` | Rendered as **text**, not markup, via the AppSheet Trusted Types patch — the fork's primary security control | |
| | Cluster icon image URL | `styles[].url` (AppSheet: base64 `data:image/svg+xml` URI built by `clusterIconSVG` from an app theme colour) | `ClusterIcon.prototype.createCss` → `background-image:url(...)` → `div_.style.cssText` | **Unescaped concatenation.** In AppSheet the value is base64-encoded before it reaches the library, which prevents CSS token breakout; note however that `clusterIconSVG` interpolates the theme `color` into the SVG `style` attribute unescaped before encoding, so an unvalidated theme colour is an SVG-content injection (rendered as a non-scripting `background-image`). | |
| | Styling parameters (`textColor`, `textSize`, `height`, `width`, `backgroundPosition`, `anchor`) | `setStyles` caller | `createCss` → `div_.style.cssText` | **Unescaped concatenation**, no numeric coercion (except `anchor`) | |
| |
| ## Privileged Actions |
| |
| | Action | Location | Guard | |
| |---|---|---| |
| | Bulk inline style assignment from a concatenated string | `ClusterIcon.prototype.onAdd`, `ClusterIcon.prototype.show` (`this.div_.style.cssText = this.createCss(pos)`) | **None** — `createCss` performs no escaping or type coercion on `url_`, `backgroundPosition_`, `textColor_`, `textSize_`, `height_`, `width_` | |
| | CSS text construction | `ClusterIcon.prototype.createCss` | `typeof === 'number'` plus range checks on `anchor_[0]`/`anchor_[1]` only; defaults `'0 0'` for background position, `'black'` for colour, `11` for size | |
| | Cluster label rendering | `ClusterIcon.prototype.onAdd`, `ClusterIcon.prototype.setSums` (`div_.textContent = …`) | Trusted Types patch: `textContent` is a text sink and cannot introduce markup | |
| | Overlay DOM element creation and insertion | `ClusterIcon.prototype.onAdd` (`document.createElement('DIV')`, `panes.overlayMouseTarget.appendChild`) | Maps `OverlayView` pane lifecycle | |
| | Image URL construction | `MarkerClusterer.prototype.setupStyles_` (`this.imagePath_ + (i + 1) + '.' + this.imageExtension_`) | None | |
| | DOM event listener registration | `ClusterIcon.prototype.onAdd` (`google.maps.event.addDomListener(this.div_, 'click', …)`) | Removed implicitly when `onRemove` detaches the node; no explicit `removeListener` | |
| | Map event listener registration | `MarkerClusterer` constructor (`google.maps.event.addListener(this.map_, 'zoom_changed' / 'idle', …)`) | **Never removed** — not cleaned up by `clearMarkers` | |
| | Marker event listener registration | `MarkerClusterer.prototype.pushMarkerTo_` (`google.maps.event.addListener(marker, 'dragend', …)`) | Not explicitly removed on marker removal | |
| | Map viewport mutation | `ClusterIcon.prototype.triggerClusterClick` (`this.map_.fitBounds`), `MarkerClusterer.prototype.fitMapToMarkers` | Gated on `isZoomOnClick()`; bounds derived from clustered marker positions | |
| | Global object assignment | Closure export block, end of `src/markerclusterer.js` (`var window = window || {}; window['MarkerClusterer'] = MarkerClusterer;`) | None | |
| |
| ## Priority Review Areas |
| |
| 1. **`ClusterIcon.prototype.createCss` → `style.cssText` (highest value).** |
| This is the library's one genuinely unescaped sink and the only remaining |
| injection primitive after the Trusted Types patch. `textColor_`, `url_`, and |
| `backgroundPosition_` are concatenated into a live inline style string with |
| no escaping; `height_`, `width_`, and `textSize_` are concatenated with no |
| numeric coercion. Severity today is bounded because AppSheet supplies these |
| from `GoogleMapInstance.ts` as generated data URIs and the literal |
| `'white'` — the review question is whether any app-author-controlled value |
| can reach `setStyles`, now or after a future map-styling feature. If one |
| can, this is CSS injection (data exfiltration via attacker-chosen resource |
| URLs, UI redress, style-based content inference) in a public-facing surface. |
| 2. **Persistence of the `textContent` patch on the label path.** Commits |
| `c386355` and `57c6edf` together are the reason this fork exists: they |
| convert upstream's `innerHTML` cluster-label sink — reachable from |
| customer-authored data via a custom calculator — into a text sink. Any |
| reintroduction of an HTML sink in `ClusterIcon.prototype.onAdd` or |
| `setSums`, or an AppSheet calculator that builds markup, restores a stored |
| XSS. This should be a standing regression check, not a one-time review. |
| 3. **Coordinate and numeric robustness for customer-supplied locations.** |
| Malformed, missing, or extreme latitude/longitude values from app data rows |
| flow unvalidated into `getExtendedBounds`, `distanceBetweenPoints_`, |
| `isMarkerInBounds_`, `createClusters_`, and `getPosFromLatLng_`. Review for |
| `NaN`/`Infinity` propagation into the generated CSS position values, for |
| non-terminating or quadratic clustering loops (client-side DoS on map views |
| backed by large or hostile datasets), and for the unguarded `gridSize_` used |
| as a pixel offset and divisor-adjacent value. |
| 4. **`imagePath` / `imageExtension` URL concatenation in `setupStyles_`.** |
| Determine whether these constructor/setter options are reachable from app |
| configuration. If they are, they allow an app author to make every end |
| user's browser fetch an arbitrary URL from within the AppSheet origin's page |
| context, and feed an unescaped string into `background-image:url(...)`. |
| 5. **Listener and overlay lifecycle leaks.** The map-level `zoom_changed` and |
| `idle` listeners are registered in the constructor and never removed; |
| `clearMarkers()` — the only teardown AppSheet calls, from |
| `GoogleMapInstance.clearMarkerClusterers` — does not remove them. Combined |
| with per-marker `dragend` listeners and per-`onAdd` `click` listeners, this |
| is a plausible accumulation path across repeated map mount/unmount cycles in |
| a long-lived AppSheet session. Note the related history: b/493541353 showed |
| cluster icons failing to tear down at all, so the teardown path here has |
| already proven fragile against Maps API changes. |
| 6. **Maps API surface collisions.** `7b91a86` renamed `ClusterIcon.remove` to |
| `removeClusterIcon` because a new `google.maps.OverlayView.remove` API |
| silently shadowed it. Audit the remaining `OverlayView` subclass members on |
| `ClusterIcon` (`onAdd`, `draw`, `onRemove`, `show`, `hide`, `setMap`, |
| `getPanes`, `getProjection`) for further collision risk; a shadowed |
| lifecycle method is a correctness bug that manifests as stale overlays |
| obscuring interactive map content. |
| |
| ## Out of Scope |
| |
| Teams can fill out this section with any known filed vulnerability bugs that |
| they've determined should be out of scope from future Fortify bug filing. |