Skip to content

Remove support for R expression in log messages #21

Closed

Description

@jcheng5

The docs for the otel_<severity> functions say you can include R expressions in braces. This is dangerous. Here is a line from Shiny's feat/otel branch:

otel::log_info(paste0("Setting reactive values: ", key, " = ", value))

The key and value are both values sent by the client; a malicious visitor to this Shiny app could drop into the browser's JavaScript console and call Shiny.setInputValue("bad", "{unlink("~", recursive=TRUE)}").

I suggest removing the R expression interpolation from the basic versions of otel_<severity> and add otel_<severity>_glue versions if you must. And even there, I suggest erroring if the first argument to otel_<severity>_glue is not a string literal.

Activity

  1. gaborcsardi commented on Aug 11, 2025

    @gaborcsardi
    Member

    A safe way to use interpolated strings (here, in glue, in cli, etc.) is to always use a string literal, i.e.

    otel::log_info("Setting reactive values: {key} = {value}")

    It definitely could error if the first argument is not a literal string.

  2. jcheng5 commented on Aug 11, 2025

    @jcheng5
    MemberAuthor

    I'm calling otel::log_info() through a wrapper function, so I can't use a literal string. I could use rlang::inject() to fake a literal string, but that would be unsafe as the string includes user input. I really just need a way to disable the glue behavior. (Currently I'm doing it by gsub("{", "{{", msg) but that feels fragile)

  3. gaborcsardi commented on Aug 11, 2025

    @gaborcsardi
    Member

    Sure, but you can use the general pattern of

    wrapper <- function(msg, ..., .envir = parent.frame()) {
      otel::log_info(msg, ..., .envir = .envir)
    }
    
    key <- "K"
    value <- "V"
    wrapper("Setting reactive values: {key} = {value}")

    Or, if you really want to disable all interpolation, then:

    safewrapper <- function(msg, ...) {
      otel::log_info("{msg}", ...)
    }
    safewrapper(paste0("Setting reactive values: ", key, " = ", value))
  4. gaborcsardi commented on Aug 11, 2025

    @gaborcsardi
    Member

    Btw. the advantage of the interpolation is that it'll use structured logging, whereas safewrapper() will only log a text message. Unless, you also supply key, value, etc. in an additional attributes argument.

  5. jcheng5 commented on Aug 11, 2025

    @jcheng5
    MemberAuthor

    Oh, I didn't know about the structured logging side effect. Given the context in which otel is useful, I'm much more worried about potential security vulnerabilities than maximum convenience/conciseness.

    With the wrapper approach, is it still possible to detect and disallow non-string-literal msg arguments?

    Would you be open to using (the equivalent of) glue_safe, so at least no arbitrary code execution is possible?

  6. gaborcsardi commented on Aug 13, 2025

    @gaborcsardi
    Member

    Using something like glue_safe is a great idea, I can certainly change that.

    But if you are still not comfortable with this, I can also remove support for R expressions. You can still do structured logging without R expressions, by explicitly supplying the attributes argument.

  7. jcheng5 commented on Aug 13, 2025

    @jcheng5
    MemberAuthor

    I would argue for log_info not supporting any R expressions or implicit attributes, and log_info_glue supporting safe expressions and warning if the template is not a string literal. And noting in the docs that interpolation also has a side effect of adding an attribute.

    I totally appreciate why the glue approach is cool, but I think safety and predictability should take precedence in this context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions