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

Adds trust parameter with deprecation for default #409

Merged
merged 4 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 = "jason@jbecker.co"),
person("Chung-hong", "Chan", role = c("aut", "cre"), email = "chainsawtiney@gmail.com",
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.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a minor formatting point (4 SPCs). I can fix it later.

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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used this from review example provided, but curious if an explicit NULL return is necessary/best practice. This would be a bit new to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can choose

is.null((function() { NULL })())
#> [1] TRUE
is.null((function() {  })())
#> [1] TRUE
is.null((function() { return() })())
#> [1] TRUE
is.null((function() { invisible() })())
#> [1] TRUE

Created on 2024-05-12 with reprex v2.1.0

}

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))
})
})

Loading