Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typescript ignores prototype of object literals assigned to index signatures #37963

Closed
mike-marcacci opened this issue Apr 14, 2020 · 8 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@mike-marcacci
Copy link

mike-marcacci commented Apr 14, 2020

TypeScript Version: 3.8.3 & nightly 2020-04-14

Search Terms:

  • Object.create
  • index signatures
  • prototype

Code

// This should not be possible but is:
const unsafeStringMap: { [key: string]: string } = {};
const notActuallyAString: string | undefined = unsafeStringMap['constructor'];
console.log(
    `Typescript thinks this is a string (or undefined), but it's actually a ${typeof notActuallyAString}.`
);

// This is a correct implementation of the same:
const safeStringMap: { [key: string]: string } = Object.create(null);
const aString: string | undefined = safeStringMap['constructor'];
console.log(
    `Typescript thinks this is a string (or undefined), and indeed it's ${typeof aString}.`
);

// Typescript seems to correctly handle this when instantiating the `Object` constructor.
const correctlyHandledStringMap: { [key: string]: string } = new Object();

// Typescript also seems to be aware of an object literal's prototype chain for other purposes.
const foo: { constructor: typeof Object.prototype.constructor } = {}

Expected behavior:
Assigning {} to { [key: string]: string } should fail, because it does not have a compatible signature. In javascript, an "object literal" {} is equivalent to new Object() and TS should treat it accordingly.

Actual behavior:
TS allows the assigning of {} to { [key: string]: string }, leading to runtime crashes and potential security issues (including prototype pollution) when a key with the same name as a member of Object.prototype (constructor, __proto__, etc) is accessed or set.

Playground Link:
Link

Related Issues:
#13778

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Apr 17, 2020
@RyanCavanaugh
Copy link
Member

This is a potential problem, but the type system was designed prior to the existence of Object.create(null) and many correct programs use { } in places where the keys, by construction, don't overlap with any built-in names (or are always set as own properties first).

@mike-marcacci
Copy link
Author

mike-marcacci commented Apr 17, 2020

Thanks for the note!

Ya you're definitely right that there will be use-cases that happen to be safe (for example where we know from some external guarantee that key names are UUIDs), but it isn't really provable via the type system, making it painfully easy to introduce type errors or worse when that assumption gets broken.

I've personally messed this up enough to suspect that there are very real safety issues with the vast majority of cases that would become invalid with this fix.

From an ergonomics perspective this fix would ideally be accompanied by my suggestion over here, to support __proto__ in object literal declarations only, since object spreads have gained considerable popularity and the rules here are quite straightforward.

You mention that you suspect there is a design limitation here, but the last two examples in my code block suggest that this information is being tracked, and just not enforced in the cases of indexed objects (although I certainly don't know how it's implemented). I'm curious to know what the limitation might be?


EDIT: I have turned the suggestion referenced above into a more thoroughly constructed proposal in #38385.

@RyanCavanaugh
Copy link
Member

The "limitation" is "we don't want to break a ton of existing correct code" 😉

@mike-marcacci
Copy link
Author

mike-marcacci commented Apr 17, 2020

Ah haha, makes sense :) I suppose we could make this behavior suppressible via an option (maybe supressIndexedObjectPrototypeErrors), or possibly "opt-in" via a new "strict" option (maybe strictIntexedObjects). (Although this probably needs to be weighed against adding more complexity to the configuration.)

@mike-marcacci
Copy link
Author

mike-marcacci commented May 8, 2020

While working on #38385 I discovered that Object.create(null) was part of the 2009 ES5.0 spec, so I'd argue that this behavior has never been correct...

The problem is quite prevalent.
I quickly searched github for indexed object signatures in typescript files, and found an astonishing number of potentially serious issues that would have been caught by correct type checking here.

I also took a moment to look through the documentation and external resources (blog posts, etc) that describe this feature, and I have yet to see this implemented safely, with either Object.create(null) or { __proto__: null }.

Unless I missed a whole contingent of the internet, this means that the VAST majority of cases where this feature is being used, it is giving a false sense of safety...


EDIT: Below is part of my original comment here, which I'm removing from the scope of this issue, per @RyanCavanaugh's comment below.

This appears to be part of a broader problem with index signatures.

Further experimentation has brought me to the opinion that unconstrained indexed object signatures are severely safe in TypeScript at the moment. (I'm ignoring scenarios where index signatures are constrained by another type, like {[k in keyof Foo}: null | F[k]}.) For example:

function expectsNumberValues(arg: {[key: string]: number}) {
  return Object.keys(arg).map(key => arg[key]).reduce((acc, arg) => acc + arg, 0);
}

function acceptsLiteral(arg: { prop1: number }): void {
  // This should fail, since arg does NOT define an exhaustive property signature.
  expectsNumberValues(arg)
}

// The "extra" prop2 property here is definitely not a number...
const someObject = { prop1: 5000, prop2: { foo: "bar"} };

acceptsLiteral(someObject);

The problem isn't just that the prototype of object literals is ignored, but that more broadly, index signatures describe an exhaustive set of properties. Accordingly, it's really never safe to "refine" one from a type that only defines a partial set of properties.

Both problems should probably be addressed together.

It's possible that these are two different issues, but I'd like to treat them as one, since the initial pushback was against introducing errors into codebases that currently (although incorrectly) compile. Based on my initial reasoning, both fixes expose the same kind of safety issue, and should be released together to avoid unnecessary developer toil.

@mike-marcacci
Copy link
Author

Property exhaustiveness problem relates to #12936.

@RyanCavanaugh
Copy link
Member

@mike-marcacci this is a known soundness hole; optional properties have the same problem. While it's sometimes possible to construct an adversarial repro to cause an issue to occur, in practice this doesn't cause problems and it's an intentional usability trade-off that we allow it.

@mike-marcacci
Copy link
Author

@RyanCavanaugh thanks for the reply. That makes sense. I've updated my comment to retract the "exhaustiveness" problem from the scope of this issue, which can continue to focus on the original problem.

Is there a place that lists these soundness caveats? Having them is certainly an acceptable call, but it would be nice to know the intended limits of type safety that TS can provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

2 participants