Add Fortify scanner THREAT_MODEL.md to third_party/@google/markerclusterer repo Bug: b/548455202 Change-Id: I18b93391ffa304908cf9332398c85f96a70f6a28 Reviewed-on: https://gnocchi-internal-review.git.corp.google.com/c/third_party/@google/markerclusterer/+/316630 Reviewed-by: Dmitry Riegle <riegle@google.com> Autosubmit: Hughes Hilton <hugheshilton@google.com>
diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md new file mode 100644 index 0000000..67bbc5f --- /dev/null +++ b/THREAT_MODEL.md
@@ -0,0 +1,303 @@ +# 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.