Fix DoS via getNthKey

[JeremyMoeglich]

Previously getNthKey might be called many times each time iterating through a full user provided collection.
By providing a large collection and a large referentialEqualities array you could cause quadratic compute.
For a 1mb payload (a common webserver limit) this can block the event loop for 20 - 40 seconds

Benchmarks (run on a Ryzen 7 5700X, Arch Linux)
Previous

NodeJS:
baseline: 37.97 ms
small attack: 439.44 ms
double inputs: 1695.84 ms (3.85x)
optimized: 30912.67 ms

Bun:
baseline: 28.13 ms
small attack: 524.92 ms
double inputs: 2063.38 ms (3.93x)
optimized: 45527.36 ms

After this change:

NodeJS:
baseline: 62.29 ms
small attack: 14.02 ms
double inputs: 17.99 ms (1.28x)
optimized: 101.53 ms

Bun:
baseline: 52.97 ms
small attack: 17.19 ms
double inputs: 27.44 ms (1.60x)
optimized: 94.58 ms

Greptile Overview

Greptile Summary

Fixes DoS vulnerability by replacing O(n²) getNthKey iteration with O(1) cached array lookups using a WeakMap context. Previously, deserializing large payloads with many referential equalities could block the event loop for 20-40 seconds; now completes in ~100ms. The fix introduces a caching layer that stores Map/Set keys in arrays, initialized during untransformation and maintained through mutations.

Confidence Score: 4/5

• Effective DoS fix with comprehensive regression tests, but cache mutation logic has edge cases
• Solves a critical security vulnerability with significant performance improvements (30s → 0.1s). Comprehensive test coverage for circular references and edge cases. Cache mutation logic maintains consistency but relies on specific ordering assumptions that could be fragile.
• Pay close attention to src/accessDeep.ts cache mutation logic (lines 135-185)

Important Files Changed

Filename	Overview
src/accessDeep.ts	Replaces O(n) getNthKey iteration with O(1) cached array lookups, adds cache mutation logic to maintain consistency when Set/Map elements are updated
src/transformer.ts	Initializes cache when untransforming Sets and Maps by storing key arrays in context WeakMap
src/plainer.ts	Threads AccessDeepContext through value and referential equality annotation application

Sequence Diagram

sequenceDiagram
    participant Client
    participant deserialize
    participant applyValueAnnotations
    participant transformer
    participant applyReferentialEqualities
    participant accessDeep
    participant WeakMap as Context (WeakMap)

    Client->>deserialize: deserialize(json, meta)
    deserialize->>WeakMap: new WeakMap()
    
    alt has value annotations
        deserialize->>applyValueAnnotations: (result, meta.values, context)
        applyValueAnnotations->>transformer: untransformValue() for Set/Map
        transformer->>WeakMap: set(Set/Map, keys[])
        Note over transformer,WeakMap: O(n) - cache keys once
        transformer-->>applyValueAnnotations: return untransformed Set/Map
    end
    
    alt has referential equalities
        deserialize->>applyReferentialEqualities: (result, meta.referentialEqualities, context)
        loop for each reference
            applyReferentialEqualities->>accessDeep: getDeep/setDeep
            accessDeep->>WeakMap: get(Set/Map)
            WeakMap-->>accessDeep: return cached keys[]
            Note over accessDeep,WeakMap: O(1) lookup (was O(n))
            accessDeep->>accessDeep: access by index
            alt mutation needed
                accessDeep->>accessDeep: update Set/Map
                accessDeep->>WeakMap: update cached keys[]
            end
        end
    end
    
    deserialize-->>Client: return deserialized object

Loading

[Skn0tt]

Hi Jeremy, thanks for your contribution. I don‘t like how this minor fix significantly increases complexity. Let‘s discuss the problem in an issue and see what are possible alternatives.

[JeremyMoeglich]

This PR also seems to have a regression I'll have to fix if we want to take this approach over yours.
Though I would still prefer to take your approach as that reduces code quite a bit.

import superjson from 'superjson';

const set = new Set();
const root = { back: set };

set.add(null);
set.add(root);

try {
  const json = superjson.stringify(root);
  const _ = superjson.parse(json);
  console.log("✅ Success");
} catch (e) {
  console.error("❌ Crash detected:", e.message);
}

[Skn0tt]

Looks good. I tried finding an alternative solution in #347, but failed. Let's go with this approach. Trading some memory for reduced attack vector should be sane, especially since the used memory will be linear to the input size. cc @flybayer can you give it a second look to double-check?

[JeremyMoeglich]

I'll also read over this once more today then

This PR also seems to have a regression I'll have to fix if we want to take this approach over yours. Though I would still prefer to take your approach as that reduces code quite a bit.

import superjson from 'superjson';

const set = new Set();
const root = { back: set };

set.add(null);
set.add(root);

try {
  const json = superjson.stringify(root);
  const _ = superjson.parse(json);
  console.log("✅ Success");
} catch (e) {
  console.error("❌ Crash detected:", e.message);
}

I'll have to check if this issue I found back then to see if it's actually present

[Skn0tt]

While you're at it, maybe you could also take the tests I added in #347 and add them.

[flybayer]

@greptile

[greptile-apps]

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[JeremyMoeglich]

My tests still show some places where this implementation is incorrect, but these were issues that were present in the previous version of superjson too. To avoid this PR becoming even larger I think fixing all of these isn't a good idea.

[Skn0tt]

What an involved fix! Looking good though. I left some remarks about performance.

About the failing test cases you found, could you commit them as failing tests? It's always better to know the bugs than not to :)

[Skn0tt]

i might be missing something - why can't this be getNthKey?

[JeremyMoeglich]

We mutate indexed afterwards, we can replace this with getNthKey but then we would later have to write into context directly