Skip to content

Commit

Permalink
Adds trust parameter with deprecation for default (#409)
Browse files Browse the repository at this point in the history
* Adds trust parameter with deprecation for default

* Adds tests

* Moves to using .check_trust and early exit to reduce duplication and nested conditionals

* Documentation updates
  • Loading branch information
jsonbecker authored May 13, 2024
1 parent 09b4560 commit d63ccb5
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 15 deletions.
12 changes: 6 additions & 6 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
Contributions to **rio** are welcome from anyone and are best sent as pull requests on [the GitHub repository](https://github.com/leeper/rio/). This page provides some instructions to potential contributors about how to add to the package.
Contributions to **rio** are welcome from anyone and are best sent as pull requests on [the GitHub repository](https://github.com/gesistsa/rio/). This page provides some instructions to potential contributors about how to add to the package.

1. Contributions can be submitted as [a pull request](https://help.github.com/articles/creating-a-pull-request/) on GitHub by forking or cloning the [repo](https://github.com/leeper/rio/), making changes and submitting the pull request.
1. Contributions can be submitted as [a pull request](https://help.github.com/articles/creating-a-pull-request/) on GitHub by forking or cloning the [repo](https://github.com/gesistsa/rio/), making changes and submitting the pull request.

2. Pull requests should involve only one commit per substantive change. This means if you change multiple files (e.g., code and documentation), these changes should be committed together. If you don't know how to do this (e.g., you are making changes in the GitHub web interface) just submit anyway and the maintainer will clean things up.

3. All contributions must be submitted consistent with the package license ([GPL-2](http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html)).

4. All contributions need to be noted in the `Authors@R` field in the [DESCRIPTION](https://github.com/leeper/rio/blob/master/DESCRIPTION). Just follow the format of the existing entries to add your name (and, optionally, email address). Substantial contributions should also be noted in [`inst/CITATION`](https://github.com/leeper/rio/blob/master/inst/CITATION).
4. All contributions need to be noted in the `Authors@R` field in the [DESCRIPTION](https://github.com/gesistsa/rio/blob/master/DESCRIPTION). Just follow the format of the existing entries to add your name (and, optionally, email address). Substantial contributions should also be noted in [`inst/CITATION`](https://github.com/gesistsa/rio/blob/master/inst/CITATION).

5. This package uses royxgen code and documentation markup, so changes should be made to roxygen comments in the source code `.R` files. If changes are made, roxygen needs to be run. The easiest way to do this is a command line call to: `Rscript -e devtools::document()`. Please resolve any roxygen errors before submitting a pull request.

Expand All @@ -20,10 +20,10 @@ Some specific types of changes that you might make are:

- Import is based on S3 dispatch to functions of the form `.import.rio_FORMAT()`. Export works the same, but with `.export.rio_FORMAT()`. New import/export methods should take this form. There's no need to change the body of the `import()` or `export()` functions; S3 will take care of dispatch. All `.import()` methods must accept a `file` and `which` argument: `file` represents the path to the file and `which` can be used to extract sheets or files from multi-object files (e.g., zip, Excel workbooks, etc.). `.export()` methods take two arguments: `file` and `x`, where `file` is the path to the file and `x` is the data frame being exported. Most of the work of import and export methods involves mapping these arguments to their corresponding argument names in the various underlying packages. See the Vignette: `remap`.

- The S3 methods should be documented in [NAMESPACE](https://github.com/leeper/rio/blob/master/NAMESPACE) using `S3method()`, which is handled automatically by roxygen markup in the source code.
- Any new format support needs to be documented in each of the following places: [README.Rmd](https://github.com/leeper/rio/blob/master/README.Rmd), [the vignette](https://github.com/leeper/rio/blob/master/vignettes/rio.Rmd), and the appropriate Rd file in [`/man`](https://github.com/leeper/rio/tree/master/man).
- The S3 methods should be documented in [NAMESPACE](https://github.com/gesistsa/rio/blob/master/NAMESPACE) using `S3method()`, which is handled automatically by roxygen markup in the source code.
- Any new format support needs to be documented in each of the following places: [README.Rmd](https://github.com/gesistsa/rio/blob/master/README.Rmd), [the vignette](https://github.com/gesistsa/rio/blob/master/vignettes/rio.Rmd), and the appropriate Rd file in [`/man`](https://github.com/gesistsa/rio/tree/master/man).

- New formats or new options for handling formats should have a test added in [`/tests/testthat`](https://github.com/leeper/rio/tree/master/tests/testthat) called `test_format_FORMAT.R` that completely covers the function's behavior. This may require adding an example file to [`inst/examples`](https://github.com/leeper/rio/tree/master/inst/examples) (e.g., for testing `import()`).
- New formats or new options for handling formats should have a test added in [`/tests/testthat`](https://github.com/gesistsa/rio/tree/master/tests/testthat) called `test_format_FORMAT.R` that completely covers the function's behavior. This may require adding an example file to [`inst/examples`](https://github.com/gesistsa/rio/tree/master/inst/examples) (e.g., for testing `import()`).

3. Changes requiring a new package dependency should be discussed on the GitHub issues page before submitting a pull request.

Expand Down
2 changes: 1 addition & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Package: rio
Type: Package
Title: A Swiss-Army Knife for Data I/O
Version: 1.0.2
Version: 1.0.3
Authors@R: c(person("Jason", "Becker", role = "aut", email = "[email protected]"),
person("Chung-hong", "Chan", role = c("aut", "cre"), email = "[email protected]",
comment = c(ORCID = "0000-0002-6232-7530")),
Expand Down
4 changes: 4 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# rio 1.0.3

* Adds `trust` parameter to functions that are used to load various R environment formats (`.R`, `.Rds`, `.Rdata`, etc). This parameter is defaulted to `TRUE` today to ensure backwards compatibility. A deprecation notice warns this will default to `FALSE` in `rio` 2.0. We are informing users that these data types should only be loaded from trusted sources, which should be affirmatively attested to.

# rio 1.0.2

* For missing files in `import_list` it gives more informative warnings fix #389
Expand Down
20 changes: 12 additions & 8 deletions R/import_methods.R
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,14 @@ import_delim <- function(file, which = 1, sep = "auto", header = "auto", strings
}

#' @export
.import.rio_r <- function(file, which = 1, ...) {
.import.rio_r <- function(file, which = 1, trust = TRUE, ...) {
.check_trust(trust, format = ".R")
.docall(dget, ..., args = list(file = file))
}

#' @export
.import.rio_dump <- function(file, which = 1, envir = new.env(), ...) {
.import.rio_dump <- function(file, which = 1, envir = new.env(), trust = TRUE, ...) {
.check_trust(trust, format = "dump")
source(file = file, local = envir)
if (missing(which)) {
if (length(ls(envir)) > 1) {
Expand All @@ -145,23 +147,25 @@ import_delim <- function(file, which = 1, sep = "auto", header = "auto", strings
}

#' @export
.import.rio_rds <- function(file, which = 1, ...) {
.import.rio_rds <- function(file, which = 1, trust = TRUE, ...) {
.check_trust(trust, format = "Rds")
readRDS(file = file)
}

#' @export
.import.rio_rdata <- function(file, which = 1, envir = new.env(), ...) {
.import.rio_rdata <- function(file, which = 1, envir = new.env(), trust = TRUE, ...) {
.check_trust(trust, format = "RData")
load(file = file, envir = envir)
if (missing(which)) {
if (length(ls(envir)) > 1) {
warning("Rdata file contains multiple objects. Returning first object.")
warning("Rdata file contains multiple objects. Returning first object.")
}
which <- 1
which <- 1
}
if (is.numeric(which)) {
get(ls(envir)[which], envir)
get(ls(envir)[which], envir)
} else {
get(ls(envir)[grep(which, ls(envir))[1]], envir)
get(ls(envir)[grep(which, ls(envir))[1]], envir)
}
}

Expand Down
13 changes: 13 additions & 0 deletions R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -136,3 +136,16 @@ escape_xml <- function(x, replacement = c("&amp;", "&quot;", "&lt;", "&gt;", "&a
}
return(file)
}

.check_trust <- function(trust, format) {
lifecycle::deprecate_warn(
when = "2.0.0",
what = "import(trust)",
details = paste0("Trust will be set to FALSE by default for ", format, ".")
)
if (isFALSE(trust)) {
stop(format, "files may execute arbitary code. Only load", format, "files that you personally generated or can trust the origin.", call. = FALSE)
}
NULL
}

10 changes: 10 additions & 0 deletions tests/testthat/test_format_R.R
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,13 @@ test_that("Export / Import to .R dump file", {
expect_true(is.data.frame(import(iris_file)))
})
})

test_that("Deprecation of untrusted dump", {
withr::with_tempfile("iris_file", fileext = ".dump", code = {
export(iris, iris_file)
## expect deprecation to work
lifecycle::expect_deprecated(import(iris_file), regexp = "set to FALSE by default")
## expect false to error
expect_error(import(iris_file, trust = FALSE))
})
})
7 changes: 7 additions & 0 deletions tests/testthat/test_format_rdata.R
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ test_that("Export to and import from Rdata", {
## expect error otherwise
expect_error(export(iris$Species, iris_file))
})
withr::with_tempfile("iris_file", fileext = ".Rdata", code = {
export(iris, iris_file)
## expect deprecation to work
lifecycle::expect_deprecated(import(iris_file), regexp = "set to FALSE by default")
## expect false to error
expect_error(import(iris_file, trust = FALSE))
})
})

test_that("Export to and import from rda", {
Expand Down
11 changes: 11 additions & 0 deletions tests/testthat/test_format_rds.R
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,14 @@ test_that("Export to rds (non-data frame)", {
expect_true(length(import(list_file)) == 2L)
})
})

test_that("Deprecation of untrusted rds", {
withr::with_tempfile("iris_file", fileext = ".rds", code = {
export(iris, iris_file)
## expect deprecation to work
lifecycle::expect_deprecated(import(iris_file), regexp = "set to FALSE by default")
## expect false to error
expect_error(import(iris_file, trust = FALSE))
})
})

0 comments on commit d63ccb5

Please sign in to comment.