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

(SECURITY) Allow use of "integrity" and "crossorigin" attributes when specifying script dependencies #187

Closed
wants to merge 17 commits into from

Conversation

strazto
Copy link
Contributor

@strazto strazto commented Oct 16, 2020

Resolves #101 - Use of such attributes are considered best-practise for security when referencing scripts stored at remote servers:

https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity

R/html_dependency.R Outdated Show resolved Hide resolved
@wch
Copy link
Collaborator

wch commented Oct 16, 2020

For htmlDependency(), the value of the script can currently be a vector of multiple JS files, as in:

htmlDependency(
  name = "lib2",
  version = "1.0",
  src = list(href = "https://cdn.com/libs"), 
  script = c(
    "js/lib2.js",
    "js/lib2-extra.js"
  )
)

Here's a set of cases that would need to work:

renderDependencies(
  list(
    # Single .js file with integrity and crossorigin
    htmlDependency(
      name = "lib1",
      version = "1.0",
      src = list(href = "https://cdn.com/libs"), 
      script = c(
        "js/lib1.js",
        integrity = "asdf",
        crossorigin = "anonymous"
      )
    ),

    # Multiple .js files
    htmlDependency(
      name = "lib2",
      version = "1.0",
      src = list(href = "https://cdn.com/libs"), 
      script = c(
        "js/lib2.js",
        "js/lib2-extra.js"
      )
    ),

    # Multiple .js files, one with integrity, crossorigin
    htmlDependency(
      name = "lib3",
      version = "1.0",
      src = list(href = "https://cdn.com/libs"), 
      script = list(
        c(
          "js/lib3.js",
          integrity = "asdf",
          crossorigin = "anonymous"
        ),
        "js/lib3-extra.js"
      )
    ),
    
    # Multiple .js files, all with integrity, crossorigin;
    # one in list, one in character vector
    htmlDependency(
      name = "lib4",
      version = "1.0",
      src = list(href = "https://cdn.com/libs"), 
      script = list(
        list(
          "js/lib4.js",
          integrity = "asdf",
          crossorigin = "anonymous"
        ),
        c(
          "js/lib4-extra.js",
          integrity = "asdf",
          crossorigin = "anonymous"
        )
      )
    )
  )
)

It would also be helpful to have examples in the docs that illustrate how to provide integrity and crossorigin. (The docs for htmlDependency don't currently have any examples, but it would benefit from having some.)

R/html_dependency.R Outdated Show resolved Hide resolved
R/html_dependency.R Outdated Show resolved Hide resolved
@strazto
Copy link
Contributor Author

strazto commented Oct 16, 2020

Thanks for the quick response, & for the feedback - I already discovered that the script can be a character vector the hard way, when I was trying to use my fork w/ kableExtra & got errors on its bootstrap dep.

@strazto
Copy link
Contributor Author

strazto commented Oct 16, 2020

It sounds like the way forward is as follows:

if script is a named list with fields: src, integrity, crossorigin, turn those into a list, and nest it in script

For each element item of script:
if item is a list
check item for names & transform into attributes
else
put item directly into script tags

I'll modify the test cases to support the character vector of scripts you mentioned, as well as the flavours of script tags we've discused

@strazto strazto marked this pull request as draft October 17, 2020 06:45
@strazto strazto marked this pull request as ready for review October 17, 2020 07:25
@strazto
Copy link
Contributor Author

strazto commented Oct 17, 2020

For my documentation of the script tag- I don't have time to look up the Rd spec on writing lists, and I usually use markdown for my docs, so the lists will need to be reformated, but this is ready to pull besides that @wch

@strazto strazto requested a review from wch October 17, 2020 07:28
Copy link
Collaborator

@wch wch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the other comments, I think there also be some tests for non-list input, to ensure that it's handled correctly.

R/html_dependency.R Outdated Show resolved Hide resolved
R/html_dependency.R Outdated Show resolved Hide resolved
R/html_dependency.R Outdated Show resolved Hide resolved
@@ -505,6 +518,68 @@ renderDependencies <- function(dependencies,
HTML(paste(html, collapse = "\n"))
}



renderScript <- function(script, srcpath, encodeFunc, hrefFilter) {
Copy link
Collaborator

@cpsievert cpsievert Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this change safer and easier to reason about, consider adding an early exit here, something like:

if (is.character(script)) {
  return(renderCharacterScript(script, srcpath, encodeFunc, hrefFilter))
}

With renderCharacterScript() defined as:

renderCharacterScript <- function(script, path, encodeFunc, hrefFilter) {
  paste0(
    '<script src="',
    htmlEscape(hrefFilter(file.path(path, encodeFunc(script)))),
    "'></script>"
  )
}

You could then also reuse this function when you go to render the more complex list case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see how that could be used for the more complex list case.

The llist case aims to support instances such as:

renderScript(list("s1.js", list(src = "s2.js"), list(src = "s3.js", integrity = "hash", crossorigin = "anonymous")))
<script src="s1.js"></script>
<script src="s2.js"></script>
<script src="s3.js" integrity="hash" crossorigin="anonymous"></script>

or:

renderScript(list(src = "s3.js", integrity = "hash", crossorigin = "anonymous"))
<script src="s3.js" integrity="hash" crossorigin="anonymous"></script>

These accept mixed use of character vectors, nested lists, & pure lists.

Unless I'm mistaken, the change you're proposing would mean I'd have to duplicate the actual templating pattern for the case of unnamed character vectors, and named lists.

Remove name checks from renderScript, now any attributes may be given
comment on logic for initial transformations in renderScript

add more general helper function, renderTag, that renders any tag.
(without a body for now).
@strazto
Copy link
Contributor Author

strazto commented Oct 23, 2020

@wch @cpsievert the requested changes have been addressed, except for #187 (comment) , on which I disagree (or perhaps misunderstood) the requested change.

Should be ready for pull, let me know if you'd like me to squash some of the history down.

@strazto
Copy link
Contributor Author

strazto commented Oct 23, 2020

Cleaned & reopened this in #188

@strazto strazto closed this Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for <script> "integrity" and "crossorigin" attributes
3 participants