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.

Sign up for GitHub

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

Jump to bottom

Use primordials everywhere #11224

Closed
66 of 67 tasks
lucacasonato opened this issue Jul 2, 2021 · 17 comments
Closed
66 of 67 tasks

Use primordials everywhere #11224

lucacasonato opened this issue Jul 2, 2021 · 17 comments
Labels
refactor

Comments

@lucacasonato
Copy link
Member

lucacasonato commented Jul 2, 2021 •
edited by bartlomieju
Loading

In #10939 we landed primordials into deno_core. They are frozen intrinsics that are not vulnerable to prototype pollution. We now need to undergo the tedious task to refactor all of our JS code to use these primordials. Here is the TODO list:

I will open some first PRs shortly that show how a migration of a file to primordials should look.

If you want to convert some files, please comment on this issue before starting so we don't duplicate work.
@lucacasonato lucacasonato pinned this issue Jul 2, 2021
This was referenced Jul 2, 2021
Merged
Merged
Merged
This was referenced Jul 2, 2021
Merged
Merged
@SimonRask
Copy link
Contributor

I'll do:

  • extensions/timers/01_timers.js
  • extensions/timers/02_performance.js

@SimonRask SimonRask mentioned this issue Jul 2, 2021
Closed
@bartlomieju
Copy link
Member

bartlomieju commented Jul 2, 2021 •
edited
Loading

Claiming:

  • runtime/js/01_build.js
  • runtime/js/01_errors.js
  • runtime/js/01_version.js
  • runtime/js/01_web_util.js
  • runtime/js/06_util.js
  • runtime/js/11_workers.js
  • runtime/js/12_io.js

@lucacasonato
Copy link
Member Author

@SimonRask Please wait with extensions/timers/02_performance.js until #10887 is landed.

@littledivy
Copy link
Member

littledivy commented Jul 3, 2021 •
edited
Loading

Claiming:

  • extensions/websocket/01_websocket.js
  • extensions/webstorage/01_webstorage.js

@littledivy littledivy mentioned this issue Jul 3, 2021
Merged
@lucacasonato
Copy link
Member Author

@littledivy: @crowlKats is already working on web sockets

@littledivy littledivy mentioned this issue Jul 3, 2021
Merged
@littledivy
Copy link
Member

Ah i'll close it then!

@bartlomieju bartlomieju mentioned this issue Jul 3, 2021
Merged
@SimonRask
Copy link
Contributor

Claiming:

  • runtime/js/13_buffer.js
  • runtime/js/30_fs.js
  • runtime/js/30_metrics.js

@bartlomieju
Copy link
Member

bartlomieju commented Jul 3, 2021 •
edited
Loading

Claiming:

  • runtime/js/30_os.js
  • runtime/js/40_compiler_api.js
  • runtime/js/40_diagnostics.js
  • runtime/js/40_error_stack.js
  • runtime/js/40_files.js
  • runtime/js/40_fs_events.js
  • runtime/js/40_permissions.js
  • runtime/js/40_plugins.js
  • runtime/js/40_process.js
  • runtime/js/40_read_file.js
  • runtime/js/40_signals.js
  • runtime/js/40_testing.js
  • runtime/js/40_tty.js
  • runtime/js/40_write_file.js
  • runtime/js/41_prompt.js
  • runtime/js/90_deno_ns.js
  • runtime/js/99_main.js

@satyarohith
Copy link
Member

Claiming:

  • extensions/fetch/01_fetch_util.js
  • extensions/fetch/20_headers.js
  • extensions/fetch/21_formdata.js
  • extensions/fetch/22_body.js
  • extensions/fetch/22_http_client.js
  • extensions/fetch/23_request.js
  • extensions/fetch/23_response.js
  • extensions/fetch/26_fetch.js

@crowlKats
Copy link
Member

@littledivy: @crowlKats is already working on web sockets

I am handing it over to divy

@littledivy
Copy link
Member

Claiming:

  • extensions/console/01_colors.js
  • extensions/console/02_console.js

@SimonRask SimonRask mentioned this issue Jul 3, 2021
Merged
@bartlomieju bartlomieju mentioned this issue Jul 3, 2021
Merged
@SimonRask
Copy link
Contributor

There is no need for primordials in runtime/js/30_metrics.js

@littledivy littledivy mentioned this issue Jul 3, 2021
Merged
@bartlomieju
Copy link
Member

bartlomieju commented Jul 3, 2021 •
edited
Loading

@lucacasonato

All the snippets of code that are executed from rust via deno_core / deno_runtime / deno at runtime

These only use window.dispatchEvent(...) so I don't think we need to change anything.

I presume you skipped 99_compiler_main.js as it is completely internal and not exposed to user code in any way?

@lucacasonato
Copy link
Member Author

These only use window.dispatchEvent(...) so I don't think we need to change anything.

That should really be using an internal bound dispatch method. I'll circle back to this once everything else is done. I am working on a rewrite of the event target code right now anyway to make it more compliant and clean it up.

I presume you skipped 99_compiler_main.js as it is completely internal and not exposed to user code in any way?

Yup.

@lucacasonato lucacasonato mentioned this issue Jul 3, 2021
Merged
@SimonRask SimonRask mentioned this issue Jul 3, 2021
Merged
@bartlomieju
Copy link
Member

bartlomieju commented Jul 4, 2021 •
edited
Loading

Claiming extensions/web/02_event.js

EDIT: I will probably regret it but I'm also claiming (that's a lot of JS code...)

  • extensions/webgpu/01_webgpu.js
  • extensions/webgpu/02_idl_types.js

@bartlomieju bartlomieju mentioned this issue Jul 4, 2021
Merged
@satyarohith satyarohith mentioned this issue Jul 4, 2021
Merged
8 tasks
@bartlomieju bartlomieju mentioned this issue Jul 4, 2021
Merged
@ghost
Copy link

ghost commented Jul 6, 2021 •
edited by ghost
Loading

I mentioned that the WebGPU PR left a few dynamic object accesses. Ref #11265 (comment)

While looking through it, the changes are hard to do as they primarily accept WebIDL BufferSource objects, which themselves are very dynamic objects. Comparing the prototype against via the instanceof operator is a very fallible operation in that it doesn't genuinely give the true type of the object, therefore checking the prototype and then using an accessor such as TypedArrayPrototypeGetBuffer or DataViewPrototypeGetBuffer can still throw. In those cases, there's not much more that can be done, besides possibly trying the opposite accessor, but then it may have just thrown due to other reasons anyways. Do you have any suggestions as to how this should be handled?

webidl.js is used across the entire codebase, yet it also suffers from this vulnerability as it doesn't use the prototype primordials for these.

@bartlomieju bartlomieju mentioned this issue Jul 6, 2021
Merged
@bartlomieju
Copy link
Member

Gonna close this issue as we migrated all the files listed. Huge thanks to @littledivy, @SimonRask, @crowlKats and @crimsoncodes0 who helped it push forward in just a week.

@crimsoncodes0 let's address your comments regarding getters in a follow up issue.

@lucacasonato lucacasonato unpinned this issue Jul 10, 2021
@ghost ghost mentioned this issue Jul 10, 2021
Closed
@magurotuna magurotuna mentioned this issue Jul 23, 2021
Merged
@lucacasonato lucacasonato mentioned this issue Jul 23, 2021
Closed
@cmorten cmorten mentioned this issue Sep 9, 2021
Closed
@aduh95 aduh95 mentioned this issue Dec 12, 2021
Closed
This was referenced Sep 14, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
refactor
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@lucacasonato @crowlKats @bartlomieju @satyarohith @SimonRask @littledivy