Add Fortify scanner THREAT_MODEL.md to third_party/react-draggable repo Bug: b/548454386 Change-Id: Id29cb6f67491170135b6305ecfb46fc36b410aa3 Reviewed-on: https://gnocchi-internal-review.git.corp.google.com/c/third_party/react-draggable/+/316650 Reviewed-by: Trevor Ryland <tryland@google.com>
diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md new file mode 100644 index 0000000..ce3c149 --- /dev/null +++ b/THREAT_MODEL.md
@@ -0,0 +1,317 @@ +# Security Threat Model: react-draggable (AppSheet Fork) + +## Asset Definition & Scope + +- **Component:** `react-draggable` — a React component (`<Draggable>` and the + lower-level `<DraggableCore>`) that makes a single child element draggable + via mouse and touch events, applying a CSS `transform: translate(...)` (or + an SVG `transform` attribute) to reposition it. +- **Repository:** Git-on-Borg + (`https://gnocchi-internal.googlesource.com/third_party/react-draggable`, + branch `main`). Mirrored at + `https://appsheet-third-party.googlesource.com/react-draggable`, which is the + URL AppSheet's `package.json` actually resolves. +- **Upstream:** `react-grid-layout/react-draggable` v`4.4.5` (2022-04-26). + `METADATA` records + `version: 44a8c6ed103ec6c0a4dda5faf7f8ebca16f9b325`, + `last_upgrade_date 2022-04-26`. +- **Scope:** Flow-typed sources under `lib/` (`Draggable.js`, + `DraggableCore.js`, `utils/domFns.js`, `utils/positionFns.js`, + `utils/getPrefix.js`, `utils/shims.js`, `utils/types.js`, `utils/log.js`, + `cjs.js`) **and** the checked-in transpiled output under `build/` + (`build/cjs/**`, `build/web/react-draggable.min.js`). Runtime dependencies + are `clsx` and `prop-types`. +- **Deployment context:** Browser-side only. Bundled into the AppSheet app + runtime and served to end users over the public Internet. AppSheet does + **not** import `react-draggable` directly anywhere in + `Nirvana/Content`; it is consumed **transitively**, pinned via + `package.json` `overrides` for two packages: + - `react-grid-layout@0.16.6` — used by + `Nirvana/Content/components/app/primary/AdjustableDash.tsx` (end-user + dashboard layout editing) and + `Nirvana/Content/components/app/ImageGallery.tsx` (responsive image + gallery). + - `react-resizable` — resize handles within the same grid surfaces. + + Both consumers render **customer-authored app content** (dashboard view + names, image assets, layout definitions) inside the AppSheet runtime, so the + surrounding DOM is attacker-influenceable by whoever authored the app. + +### AppSheet fork deltas vs. upstream + +| Commit | Change | +| --- | --- | +| `d625fe2` "Initial import" | Imports upstream `4.4.5` with a **Trusted Types compliance patch already applied** to `lib/utils/domFns.js:addUserSelectStyles`: upstream assigns the drag-time user-select CSS with `styleEl.innerHTML = ...` / `+=`, which violates a `require-trusted-types-for 'script'`/`TrustedHTML` policy and trips DOM-XSS sinks; the fork uses `styleEl.textContent` instead. Functionally equivalent for a `<style>` element, but it removes an `innerHTML` sink. | +| `f3d2c0f` "[nwd] Pushing the build folder with the trusted types patched built files" | Un-gitignores and checks in `build/cjs/**` and `build/web/react-draggable.min.js(.map)` so that AppSheet consumes the patched transpiled output directly from the Git dependency rather than rebuilding it. | +| `c67f829` "[NWD] Remove scripts from package.json" (HEAD) | Deletes the entire `scripts` block (`test`, `build`, `lint`, `flow`, …) so that `npm ci` does not attempt to rebuild the package on every install. | + +The net effect of `f3d2c0f` + `c67f829` is that **`build/` is the shipped +artifact and there is no longer any in-repo path that regenerates it from +`lib/`**. Sources and build output can silently diverge; see *Priority Review +Areas*. + +## Prioritization Signals + +- **1P OSS:** No (third-party OSS library maintained as an internal customized + fork for Google AppSheet). +- **1P Proprietary Shipped Software:** Yes (the checked-in `build/cjs/**` is + bundled into the minified JavaScript AppSheet ships to end-user browsers). +- **High-Risk Code Surface:** **No.** This is a UI interaction library. It has + no network I/O, no filesystem or process access, no serialization/ + deserialization of untrusted formats, no cryptography, and no authentication + or authorization logic. Its entire footprint is DOM measurement, pointer/ + touch event handling, and arithmetic on coordinates. The realistic + worst-case impact is a client-side DOM-manipulation or CSS-injection defect, + or a UI-redressing (clickjacking-adjacent) primitive — not remote code + execution or data exfiltration. Severity should be assessed accordingly and + **not** inflated to match the perimeter-exposure signal. +- **Perimeter Exposure:** Yes (AppSheet is a 1P service exposed to the public + Internet and customers; this code executes in end-user browsers on + customer-authored app content). +- **Data Sensitivity:** Low. The library handles only pointer coordinates, + touch identifiers, CSS class names, and element geometry. It never reads + form values, cookies, storage, or credentials. Coordinate data is passed to + caller callbacks and, in AppSheet's case, persisted as dashboard layout + geometry. +- **Untrusted Input Handling:** Yes, but low-severity. Inputs are + (a) browser-generated `MouseEvent`/`TouchEvent` objects, (b) caller-supplied + props including two **CSS selector strings** (`handle`, `cancel`) that are + fed to `querySelector`-family APIs, (c) `bounds` which may itself be a CSS + selector string, (d) `positionOffset` values which may be **arbitrary + strings interpolated into a CSS `transform` value**, and (e) DOM nodes + supplied via `nodeRef` / `offsetParent`. +- **Business Value:** Underpins AppSheet's dashboard layout editor and image + gallery. A defect here degrades UI correctness or availability of those + views. It is included in Fortify scope primarily because it is a + Google-modified fork that ships to the browser, not because it is a + high-value attack target. + +## Scanning Harness Prompts + +Direct the agentic code scanner as follows. **Calibrate severity to a UI +library**: the absence of a server, of network calls, and of any secret material +means most findings will be correctness or DoS issues rather than exploitable +vulnerabilities. Report inflated-severity findings as informational. + +1. **Verify the Google Trusted Types patch has not regressed.** In + `lib/utils/domFns.js:addUserSelectStyles` **and** its transpiled twin + `build/cjs/utils/domFns.js`, confirm the CSS text is assigned via + `textContent` and never `innerHTML`, `insertAdjacentHTML`, + `outerHTML`, or `CSSStyleSheet.insertRule` with interpolated data. This is + the single Google-authored security change in the repo. +2. **Diff `lib/` against `build/`.** The build is checked in and the build + scripts have been removed, so the two can drift. Flag any semantic + divergence — especially any place where `build/` retains an `innerHTML` + assignment that `lib/` has fixed, or vice versa. Also scan + `build/web/react-draggable.min.js` as shipped minified 1P JavaScript. +3. **CSS-injection / transform-string construction in + `lib/utils/domFns.js:getTranslation` (and its callers + `createCSSTransform`, `createSVGTransform`).** `positionOffset.x` and + `positionOffset.y` are interpolated **as raw strings** into + `translate(${defaultX}, ${defaultY})` when the caller passes strings rather + than numbers. Determine whether a caller-controlled `positionOffset` string + can break out of the `translate()` function and inject additional CSS + declarations or a `url(...)` reference into the element's inline `transform` + style. React's inline-`style` handling provides some protection, but the + value reaches the style object pre-assembled. Confirm AppSheet's consumers + (`react-grid-layout`, `react-resizable`) never route customer data into + `positionOffset`. +4. **Prototype pollution.** Audit every object-spread and dynamic key write for + unguarded `__proto__` / `constructor` / `prototype` keys. Specific sites: + `createCSSTransform` builds an object with a **computed key** + (`{[browserPrefixToKey('transform', browserPrefix)]: translation}`); + `getPrefix.js:browserPrefixToKey` / `kebabToTitleCase` derive that key from a + string; `Draggable.render` performs + `style: {...children.props.style, ...style}` and + `{...draggableCoreProps}`; `domFns.js:addEvent`/`removeEvent` write + `el['on' + event] = handler` on a legacy fallback path. Note that + `browserPrefix` is derived from `window.document.documentElement.style`, not + from user input, so the computed-key path is not currently + attacker-reachable — verify that remains true. +5. **Selector handling in `lib/utils/domFns.js:matchesSelector` / + `matchesSelectorAndParentsTo` and + `lib/utils/positionFns.js:getBoundPosition`.** The `handle`, `cancel`, and + string-valued `bounds` props are passed unvalidated to + `el.matches(selector)` and `ownerDocument.querySelector(bounds)`. Check for + (a) unhandled `SyntaxError` on a malformed selector causing a render/drag + crash, and (b) unbounded tree walks in `matchesSelectorAndParentsTo`, which + loops to `document` when `baseNode` is not an ancestor of `el`. +6. **DoS / unbounded loops and NaN propagation.** `positionFns.js:snapToGrid` + divides by `grid[0]`/`grid[1]` (a zero grid yields `Infinity`/`NaN`); + `createDraggableData` divides deltas by `props.scale` (`scale: 0` yields + `Infinity`). Trace whether `NaN`/`Infinity` coordinates reach the CSS + transform and wedge layout. Also check the + `while (node)` loop in `matchesSelectorAndParentsTo` and the retry loop in + `DraggableCore.handleDrag` (which synthesizes a `new MouseEvent('mouseup')` + to self-terminate) for termination guarantees. +7. **Event-listener lifecycle and memory safety in `lib/DraggableCore.js`.** + `handleDragStart` attaches `mousemove`/`mouseup` (and touch equivalents) + listeners to `ownerDocument` with `{capture: true}`. Verify every path + through `handleDragStop` and `componentWillUnmount` removes them + symmetrically, including the early-return paths and the `mounted === false` + race. Leaked document-level capture listeners are both a memory leak and a + latent input-hijacking surface. +8. **`ReactDOM.findDOMNode` usage.** Both `Draggable.findDOMNode` and + `DraggableCore.findDOMNode` fall back to the deprecated + `ReactDOM.findDOMNode(this)` when no `nodeRef` is supplied, reaching outside + the component's own subtree. Confirm this cannot be steered to a node the + component does not own. +9. **Debug logging.** `lib/utils/log.js` gates `console.log` on + `process.env.DRAGGABLE_DEBUG`, which + `babel-plugin-transform-inline-environment-variables` inlines at build time. + Confirm the shipped `build/` output has this compiled out and does not log + `%j`-serialized DOM nodes and coordinates to end-user consoles. + +## Entry Points and Untrusted Inputs + +| Entry Point | Type | Trusted? | Validation | +|---|---|---|---| +| `<Draggable>` / `<DraggableCore>` props (`axis`, `bounds`, `grid`, `scale`, `position`, `defaultPosition`, `positionOffset`, `handle`, `cancel`, `disabled`, `allowAnyClick`, `enableUserSelectHack`, `nodeRef`, `offsetParent`) | React props from calling application code (`react-grid-layout`, `react-resizable`) | Partially — callers are npm packages driven by AppSheet code and customer-authored app layout data | `PropTypes` runtime shape checks only, including a custom `DraggableCore.propTypes.children` validator. `PropTypes` are stripped in production builds and are **not** a security control. No sanitisation of selector strings or `positionOffset` strings. | +| `mousedown`/`mousemove`/`mouseup`, `touchstart`/`touchmove`/`touchend` events → `DraggableCore.handleDragStart`, `handleDrag`, `handleDragStop` | Browser-generated DOM events on the document and on the draggable node | No — user/attacker controls timing, target, button, and touch identifiers | `allowAnyClick` gates on `e.button !== 0`; `e.target instanceof ownerDocument.defaultView.Node` type check; `handle`/`cancel` selector matching; touch-identifier matching in `positionFns.getControlPosition` / `domFns.getTouch` | +| `handle`, `cancel`, and string-valued `bounds` props | CSS selector strings | No | Passed unvalidated to `el.matches(...)` (`domFns.matchesSelector`) and `ownerDocument.querySelector(...)` (`positionFns.getBoundPosition`). A malformed selector throws; `getBoundPosition` throws an explicit `Error` when the selector matches nothing. | +| `positionOffset: {x, y}` | Numbers **or arbitrary strings** | No | Strings are interpolated verbatim into the CSS `transform` value by `domFns.getTranslation`. **No escaping.** | +| `nodeRef`, `offsetParent` props | Caller-supplied DOM node references | Partially | `DraggableCore.propTypes.offsetParent` is a custom validator that throws `"Draggable's offsetParent must be a DOM Node."` when `nodeType !== 1`, but `propTypes` are stripped in production builds, so at runtime the node is used unchecked in `positionFns.getControlPosition` → `domFns.offsetXYFromParent` | +| Ambient DOM/CSSOM state (`getComputedStyle`, `clientHeight`, `offsetLeft`, `scrollLeft`, `getBoundingClientRect`) | Read from the host page | No — customer-authored app content can influence layout and computed styles | Coerced through `shims.int()` (`parseInt(..., 10)`), which yields `NaN` on non-numeric input rather than throwing | +| `window.document.documentElement.style` | Browser feature detection | Yes | `getPrefix.js` guards for `typeof window === 'undefined'` and optional-chains `window.document?.documentElement?.style` | + +## Trust Boundaries and Auth Assumptions + +- **Authentication**: None, and none is applicable. This library performs no + network I/O and makes no identity assertions. +- **Authorization**: None at the library level. Whether a given AppSheet user + may rearrange a dashboard is decided entirely by AppSheet's own permission + model; `disabled` here is a UI affordance, **not** an access control. +- **Implicit trust**: + - Trusts that its caller supplies well-formed props — in particular that + selector strings are valid CSS and that `positionOffset` strings are not + attacker-controlled. + - Trusts the host page's DOM: it reads geometry from `offsetParent`, + `ownerDocument`, and `getComputedStyle`, all of which customer-authored + app content can influence. + - Trusts that `React.Children.only(children)` receives a single element + that tolerates having `className`, `style`, and `transform` overwritten + via `React.cloneElement`. + - Explicitly handles the iframe case (`doc` is threaded through + `addUserSelectStyles`/`removeUserSelectStyles`, and `ownerDocument` is + used rather than the global `document`), so it may operate across + same-origin document boundaries. +- **Boundary crossings**: The only meaningful one is + **untrusted user gesture → DOM mutation in the AppSheet origin**. There is + no client→server boundary inside this library; any persistence of the + resulting coordinates is performed by AppSheet's own code, which must + re-validate that data server-side. + +## Sensitive Data Paths + +| Data Type | Source | Destination | Protection | +|---|---|---|---| +| Pointer / touch coordinates (`clientX`, `clientY`, `identifier`) | Browser input events | `DraggableCore` component state → `positionFns.createCoreData` / `createDraggableData` → caller `onStart`/`onDrag`/`onStop` callbacks | Transient in-memory only; never persisted or transmitted by this library | +| Element geometry (`offsetLeft`, `clientWidth`, computed padding/border/margin) | Host DOM via `getComputedStyle` and layout properties | `positionFns.getBoundPosition` bounds arithmetic | Read-only measurement; coerced via `shims.int()` | +| Layout position (`x`, `y`) | Drag interaction | Inline CSS `transform` on the child element, and caller callbacks (AppSheet persists these as dashboard layout geometry) | Values are numeric unless the caller injects `positionOffset` strings; server-side layout validation is AppSheet's responsibility | +| CSS class names | `defaultClassName`, `defaultClassNameDragging`, `defaultClassNameDragged` props and `children.props.className` | `clsx(...)` → child element `className` | Assigned through React's `className` prop (attribute-escaped), or via `el.className +=` / regex replace in the legacy `domFns.addClassName`/`removeClassName` fallback for browsers lacking `classList` | +| Drag-time user-select stylesheet | Hard-coded constant string | `<style id="react-draggable-style-el">` appended to `document.head` by `domFns.addUserSelectStyles` | Assigned via `textContent` (Google patch); content is a compile-time constant with no interpolation | +| **No** secrets, credentials, tokens, cookies, or PII | — | — | The library reads none | + +## Privileged Actions + +| Action | Location | Guard | +|---|---|---| +| Inject a `<style>` element into `document.head` | `lib/utils/domFns.js:addUserSelectStyles` | Idempotent on `#react-draggable-style-el`; content is a hard-coded constant assigned via `textContent` (Google Trusted Types patch); gated on `props.enableUserSelectHack` | +| Mutate `document.body` class list | `domFns.addClassName` / `removeClassName`, invoked from `addUserSelectStyles` / `removeUserSelectStyles` | Fixed class name `react-draggable-transparent-selection` | +| Attach document-level capturing event listeners | `DraggableCore.handleDragStart` → `domFns.addEvent(ownerDocument, ..., {capture: true})` | Removed in `handleDragStop` and `componentWillUnmount`; legacy fallback assigns `el['on' + event]` directly | +| Clear the user's text selection | `domFns.removeUserSelectStyles` (`selection.removeAllRanges()`) | Skipped when `selection.type === 'Caret'`; wrapped in `try/catch` | +| Write inline CSS `transform` / SVG `transform` on the child element | `Draggable.render` → `domFns.createCSSTransform` / `createSVGTransform` → `domFns.getTranslation` | None on string-valued `positionOffset`; numeric values get a `px` suffix | +| Overwrite the child's `className`, `style`, and `transform` props | `Draggable.render` (`React.cloneElement(React.Children.only(children), {...})`) | `React.Children.only` enforces exactly one child; existing `style` is spread-merged and can be shadowed | +| Reach outside the component subtree for a DOM node | `Draggable.findDOMNode` / `DraggableCore.findDOMNode` (`ReactDOM.findDOMNode(this)`) | Bypassed when the caller supplies `nodeRef` | +| Synthesize and dispatch a drag-stop | `DraggableCore.handleDrag` (`this.handleDragStop(new MouseEvent('mouseup'))`) | Triggered only when bound-position math detects an aborted drag | +| Write to the browser console | `lib/utils/log.js:log` | Gated on `process.env.DRAGGABLE_DEBUG`, inlined at build time | + +## Priority Review Areas + +1. **Source/build divergence — the highest-value review target.** `f3d2c0f` + checked `build/` into the repo and `c67f829` removed every `scripts` entry + from `package.json`, so the artifact AppSheet actually loads + (`build/cjs/cjs.js`, per the `main` field) is **never regenerated from + `lib/`**. A reviewer or a scanner that only reads `lib/` will not be + auditing the shipped code. Confirm `build/cjs/**` is a faithful transpilation + of the current `lib/**` — in particular that + `build/cjs/utils/domFns.js:addUserSelectStyles` carries the same + `textContent` (not `innerHTML`) assignment — and treat + `build/web/react-draggable.min.js` as shipped 1P minified JavaScript in its + own right. + +2. **Regression-proofing the Trusted Types patch + (`lib/utils/domFns.js:addUserSelectStyles`).** This is the only + Google-authored security modification in the fork: it replaces upstream's + `styleEl.innerHTML = ...` with `styleEl.textContent = ...` to satisfy + Trusted Types and to remove an `innerHTML` sink. Because the patch lives in + the "Initial import" commit rather than in a labelled follow-up, a naive + upstream re-import would silently revert it. Any future version bump must + re-apply and re-verify this change in both `lib/` and `build/`. + +3. **String interpolation into CSS `transform` + (`lib/utils/domFns.js:getTranslation`).** When `positionOffset.x`/`.y` are + strings they are concatenated unescaped into + `translate(${defaultX}, ${defaultY})`. This is the library's only + string-building-into-a-DOM-sink path. Establish whether the resulting value + can carry additional CSS declarations, and confirm that AppSheet's + consumers (`react-grid-layout@0.16.6`, `react-resizable`) never derive + `positionOffset` from customer-authored app definitions. Realistic impact is + CSS injection / UI redressing, not script execution — React assigns the + value through the inline `style` object, which does not evaluate JavaScript. + +4. **Prototype pollution via computed keys and object spreads.** Review + `createCSSTransform`'s computed-key object literal, the + `browserPrefixToKey`/`kebabToTitleCase` string derivation that produces that + key, the `{...children.props.style, ...style}` and `{...draggableCoreProps}` + spreads in `Draggable.render`, and the + `el['on' + event] = handler` legacy assignment in `domFns.addEvent`. Today + the key is derived from browser feature detection rather than user input, so + no attacker-reachable pollution path is apparent — the review objective is + to confirm that invariant and to catch any future change that makes the + prefix or event name caller-controlled. + +5. **Unvalidated CSS selectors and unbounded DOM traversal + (`domFns.matchesSelector`, `matchesSelectorAndParentsTo`, + `positionFns.getBoundPosition`).** `handle`, `cancel`, and string `bounds` + reach `matches()`/`querySelector()` with no validation; malformed input + throws a `SyntaxError` mid-drag, and `getBoundPosition` throws an explicit + `Error` when a bounds selector matches nothing. Neither is caught. Assess + the availability impact on the AppSheet dashboard editor and whether the + `while (node)` ancestor walk can be made to traverse the full document on + every pointer event. + +6. **Event-listener symmetry and unmount races in `lib/DraggableCore.js`.** + Document-level capturing `mousemove`/`mouseup`/`touchmove`/`touchend` + listeners are added in `handleDragStart` and removed in `handleDragStop` / + `componentWillUnmount`. Verify no early-return path (the `shouldUpdate === + false` / `this.mounted === false` checks, the synthesized + `new MouseEvent('mouseup')` self-stop, the `nodeRef`-is-null case) can leave + a listener attached to the document after unmount. Leaked capture-phase + listeners on `document` intercept input site-wide and are the most plausible + genuine security-relevant defect in this component. + +7. **Numeric edge cases producing `NaN`/`Infinity` + (`positionFns.snapToGrid`, `createDraggableData`, `shims.int`).** A `grid` + containing `0`, or `scale: 0`, produces non-finite coordinates that flow + into the CSS transform. Low severity (client-side layout corruption / + hang), but cheap to confirm. + +8. **Stale upstream baseline.** The fork tracks `4.4.5` (April 2022). Review + upstream releases since then for correctness and security fixes, and weigh + the cost of re-importing against the risk of losing the Trusted Types patch + (see item 2). + +## Out of Scope + +- `react-grid-layout`, `react-resizable`, `clsx`, and `prop-types` — resolved + from the public npm registry and covered by dependency scanning rather than + by this source target. +- Server-side validation of persisted dashboard layout geometry, which lives + in the `jeenee` repository. +- `example/`, `specs/`, `karma*.conf.js`, `webpack.config.js`, `Makefile`, + `.travis.yml`, and `appveyor.yml` — development and test scaffolding that is + not shipped (`package.json` `files` restricts the published surface to + `/build`, `/typings`, and the `web/` bundles). +- Teams can extend this section with any known filed vulnerability bugs that + they have determined should be out of scope from future Fortify bug filing.