refactor: use primordials in extensions/web

[bartlomieju]

Ref #11224

[SimonRask]

08_text_encoding.js - line 389 TypedArrayPrototypeSlice

[SimonRask]

.slice() if you forgot :)

[bartlomieju]

.slice() if you forgot :)

Thanks, fixed

[lucacasonato]

Update Uint8Array.from on line 65 of base64.js

[ghost]

should use TextEncoderPrototypeDecode

[ghost]

ditto in text_encoding:

deno/extensions/web/08_text_encoding.js

Line 391 in 8bab7d4

return new TextDecoder(encoding).decode(

[littledivy]

TextEncoderPrototypeDecode does not exist. Primordials are only for JS builtins

[ghost]

TextEncoderPrototypeDecode does not exist. Primordials are only for JS builtins

It's prone to prototype manipulation, and that is the purpose of the existance of the primordials in general. Or are they more narrow in their scope/goal?

I see these as things that should be created, because, although I haven't checked the relevant spec for this algorithm, I doubt it specifies performing an ECMAScript Get operation on a TextDecoder instance.

[littledivy]

Primordials are limited to JS builtins not external APIs (analogous to its name). Node.js doesn't do TextEncoderPrototypeDecode either

[ghost]

If that is so, then all of my comments here besides the ones about DataView and TypedArray can be discarded/ignored.

Although, why should Deno aim to only do as little as Node.js had?

[ghost]

These property accesses should directly use prototype getters: ArrayBufferPrototypeGetByteLength, DataViewPrototypeGetByteLength, TypedArrayPrototypeGetLength. I'm iffy about this line because it accesses a dynamic prototype get accessor, I'd suggest maybe doing something more like

let getByteOffset;
if (value instanceof DataView) {
  length = DataViewPrototypeGetByteLength(value);
  getByteOffset = DataViewPrototypeGetByteOffset;
} else {
  length = TypedArrayPrototypeGetLength(value)
  getByteOffset = TypedArrayPrototypeGetByteOffset;
}

return new (value.constructor)(
  clonedBuffer,
  getByteOffset(value),
  length,
);

also iffy about line 56's buffer access. You'll need an extra branch to get the correct get accessor for the buffer.

This u8 array construction requires an extra branch and usage of primordial accessors.

[ghost]

(Unrelated to this PR, but the abort_signal file seem to be missing WebIDL assertions on getters and such, is that handled in configurePrototype?)

Line 107 should use AbortSignalPrototypeGetAborted. I also believe that EventTargetPrototypeDispatchEvent should be used here?

[ghost]

Apologies for being as pedantic as I already was, but lastly, the location file needs to use URLPrototypeGet... accessors. :)

[littledivy]

These property accesses should directly use prototype getters: ArrayBufferPrototypeGetByteLength, DataViewPrototypeGetByteLength, TypedArrayPrototypeGetLength. I'm iffy about this line because it accesses a dynamic prototype get accessor, I'd suggest maybe doing something more like

let getByteOffset;
if (value instanceof DataView) {
  length = DataViewPrototypeGetByteLength(value);
  getByteOffset = DataViewPrototypeGetByteOffset;
} else {
  length = TypedArrayPrototypeGetLength(value)
  getByteOffset = TypedArrayPrototypeGetByteOffset;
}

return new (value.constructor)(
  clonedBuffer,
  getByteOffset(value),
  length,
);

also iffy about line 56's buffer access. You'll need an extra branch to get the correct get accessor for the buffer.

This u8 array construction requires an extra branch and usage of primordial accessors.

@crimsoncodes0 get accessors of internal primordial objects (eg window.__bootstrap.Dataview) are not exposed to userland; hence they are not vulnerable to prototype pollution

[ghost]

These property accesses should directly use prototype getters: ArrayBufferPrototypeGetByteLength, DataViewPrototypeGetByteLength, TypedArrayPrototypeGetLength. I'm iffy about this line because it accesses a dynamic prototype get accessor, I'd suggest maybe doing something more like

let getByteOffset;
if (value instanceof DataView) {
  length = DataViewPrototypeGetByteLength(value);
  getByteOffset = DataViewPrototypeGetByteOffset;
} else {
  length = TypedArrayPrototypeGetLength(value)
  getByteOffset = TypedArrayPrototypeGetByteOffset;
}

return new (value.constructor)(
  clonedBuffer,
  getByteOffset(value),
  length,
);

also iffy about line 56's buffer access. You'll need an extra branch to get the correct get accessor for the buffer.
This u8 array construction requires an extra branch and usage of primordial accessors.

@crimsoncodes0 get accessors of internal primordial objects (eg window.__bootstrap.Dataview) are not exposed to userland; hence they are not vulnerable to prototype pollution

Can you explain what you mean by that, and how it pertains to the code in question?
Was your comment referring to when I'd said

because it accesses a dynamic prototype get accessor

in particular? If not, can you specify which part of my comment you were replying to?

[littledivy]

Can you explain what you mean by that, and how it pertains to the code in question?

@crimsoncodes0 The code you are referring to (structuredClone) deals with for example globalThis.DataView and window.__bootstrap.primordials.DataView. So we now have 2 situations:

a. value is passed from userland i.e. globalThis.DataView - Users are allowed to manipulate value's dynamic accessors. (they own the value and how it behaves)

b. value is created internally i.e. window.__bootstrap.primordials.DataView - The getters for value are not prone to prototype pollution at all since window.__bootstrap.primordials.DataView was never exposed to userland.

[ghost]

Can you explain what you mean by that, and how it pertains to the code in question?

@crimsoncodes0 The code you are referring to (structuredClone) deals with for example globalThis.DataView and window.__bootstrap.primordials.DataView. So we now have 2 situations:

a. value is passed from userland i.e. globalThis.DataView - Users are allowed to manipulate value's dynamic accessors. (they own the value and how it behaves)

b. value is created internally i.e. window.__bootstrap.primordials.DataView - The getters for value are not prone to prototype pollution at all since window.__bootstrap.primordials.DataView was never exposed to userland.

In situation "a," we obviously need to have the primordials in use to ensure correctness.

As for situation "b"... I don't think I understand the primordial code; how does one have both, a prototype copy, yet also retain correct semantics for comparing the user-provided object against the primordial prototype object via the instanceof operator?

[lucacasonato]

@crimsoncodes0 You are correct we will need primordials for these too eventually. These primordials do not yet exist however, so there is nothing to change for this PR.

[ghost]

@crimsoncodes0 You are correct we will need primordials for these too eventually. These primordials do not yet exist however, so there is nothing to change for this PR.

From my read of the code, a deep primordial copy is made of both, ArrayBuffer, and TypedArray, so those changes are very possible already, unless I've misunderstood the setup there?