react-R / reactR

React for R
https://react-R.github.io/reactR
Other
320 stars 33 forks source link

update core-js version in html_dependency_corejs() #85

Open jhk0530 opened 3 months ago

jhk0530 commented 3 months ago

Hi, thanks for awesome work.

When reactR used in Quarto HTML page and commited to github.

This will cause security problem like below.

스크린샷 2024-05-24 오후 9 13 56

*note, above image says that issue closed (since I changed to not use reactR in that code)

To reproduce this, use below as contents of index.qmd and render with quarto. (Which is example from readme)

```{r}
library(reactR)
library(htmltools)

browsable(tagList(
  tags$div(id = "app"),
  tags$script(
  "
    ReactDOM.render(
      React.createElement(
        'h1',
        null,
        'Powered by React'
      ),
      document.getElementById('app')
    )
  "
  ),
  #add core-js first to work in RStudio Viewer
  html_dependency_corejs(),
  html_dependency_react()
))

Actually, used the `core-js-2.5.3` version of the javascript library will cause this problem.

and the code
``` r
html_dependency_corejs()

which is actually works as below

htmltools::htmlDependency(name = "core-js", version = "2.5.3", 
        src = c(file = system.file("www/core-js/", package = "reactR")), 
        script = "shim.min.js")

cause this.

to solve this. updating version from 2.5.3 to further version which is not use grunt-karma as <=4.0.1 or latest(3.37.1) can be considered.

[!NOTE] I don't think core-js is required any more o to work in Rstudio viewer at now (2024)

Thanks.

timelyportfolio commented 2 months ago

@jhk0530 Thanks so much. You are correct that core-js is no longer required, but I do feel like I should continue to include for anyone on legacy setups. I plan to push 0.6.0 to CRAN this week, but I am worried this might require testing that would delay this release. Over the next couple of weeks, I'll try to

  1. update core-js to core-js-bundle@3.37.1 which unfortunately is 229kb versus previous 85.9kb
  2. remove core-js from the default dependencies in the templates but any widgets and inputs built with prior templates will still by default include core-js. Updated core-js in step 1 should mean though that everything works.

@glin any thoughts or concerns?

glin commented 2 months ago

@timelyportfolio No concerns, I doubt core-js is still necessary in >99% of cases. I had also wanted to remove core-js from reactable a few years ago during the IE11 EOL because of its added size, and that it was getting flagged for vulnerabilities (https://github.com/glin/reactable/issues/245#issuecomment-1166363344)

Removing it by default but leaving it in the package to opt into sounds like a good idea.