superjson throws on constructor property

[Janpot]

superjson recently started throwing on values that contain a constructor property:

superjson.stringify({ constructor: { name: 'hello' } })
// throws Error: Detected property constructor. This is a prototype pollution risk, please remove it from your object.

We're transferring values that contain jsonschemas that describe user generated typescript interfaces. Therefore we don't have control over which property names are in our objects, but we do know they are plain javascript objects.

Any way this restriction can be relaxed again?

[Skn0tt]

This error comes from here:

https://github.com/blitz-js/superjson/blob/0130a809f87df142b5e190e1b519be0c0280fb16/src/plainer.ts#L219-L227

In your case, there's no transform happening so I don't think there's a true risk for prototype pollution. Let me see if we can relax this ...

[Janpot]

Does this mean that the parsing side uses these properties to "guess" the original constructor without any other marker in the metadata? That feels very brittle to me.

[Skn0tt]

No, the parsing side reads the metadata to find the original prototype / constructor. The exception you're seeing was added in response to #267 - we're trying to catch invalid inputs on parsing, not on serialisation. Otherwise you'll store something in your database, only to realise that you can't retrieve it anymore when parsing.

[Janpot]

Otherwise you'll store something in your database, only to realise that you can't retrieve it anymore when parsing.

It's a bit counterintuitive, because I can perfectly parse superjson which it's unable to serialize again.

const parsed = superjson.parse(
    '{"json":{"schema":{"constructor":{"type":"string"}},"createdAt":"2023-12-06T10:49:28.911Z"},"meta":{"values":{"createdAt":["Date"]}}}'
  )

console.log(typeof parsed);
// 'object'
console.log(superjson.stringify(parsed));
// Error: Detected property constructor. This is a prototype pollution risk, please remove it from your object.

[Skn0tt]

This specific value should be stringifiable, I agree. I believe the open PR already addressed that, will add a test case 👍

[Skn0tt]

Test case is over here, and it's green: 5fa55b7

The important thing here is that constructor doesn't appear in meta, so it's not a prototype pollution risk. If you added e.g. a regex to the constructor object, it would appear in meta, and would become a risk.

[Janpot]

I may still be misunderstanding the attack vector fully but I'm reading this section. Do I understand that when assigning to nested properties of referentialEqualities, superjson doesn't discriminate between own properties and prototype properties? Wouldn't it then be possible to effectively eliminate the attack vector, while traversing the property path, to test whether each nested one is an own property before assigning to it. And error out in case it's not an "own property". Which would only happen on specially crafted serialized form, and never with something that was serialized with superjson.

In pseudo code

function getSafeNested(obj, [next, ...rest]) {
  if (!Object.prototype.hasOwnProperty.call(obj, next)) {
    throw new Error('checkmate hackers')
  }
  return rest.length === 0 ? obj[next] : getSafeNested(obj[next], rest)
}

function setSafeNested(obj, [next, ...rest], value) {
  if (!Object.prototype.hasOwnProperty.call(obj, next)) {
    throw new Error('checkmate hackers')
  }
  if (rest.length > 0) {
    setSafeNested(obj[next], rest, value)
  } else {
    obj[next] = value
  }
}

for (const [src, tgts] of Object.entries(serialized.meta.referentialEqualities)) {
  const srcValue = getSafeNested(serialized.json, src.split('.'))
  for (const tgt of tgts) {
    setSafeNested(serialized.json, tgt.split('.'), srcValue)
  }
}

Assuming you already considered this, what am I missing?

[Skn0tt]

I have not considered this yet, actually. If we can prevent prototype pollution with this, i'd be happy to! @Janpot would you be willing to work on a PR with this?

[Janpot]

@Janpot would you be willing to work on a PR with this?

I won't in the near future, but I may pick this up at some point if it doesn't get implemented.

[Janpot]

@Skn0tt To be honest, I'm having a bit of trouble replicating the original prototype pollution. Did they ever provide an actual payload that triggers it?

[Skn0tt]

Yes, there were some test cases added for it: 0d68cd5#diff-258035e5968f6bf645400d417f310218d7d9a9a10606a3c34e7f55db193f58f3