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

Use vm.runInThisContext instead of eval #18

Merged
merged 1 commit into from
Jan 12, 2015
Merged

Use vm.runInThisContext instead of eval #18

merged 1 commit into from
Jan 12, 2015

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Jan 11, 2015

Fixes #17

While this gets rid of eval in node and in "mixed" environments like node-webkit or Atom Shell apps, it still resolves to eval when using browserify.

For a solution that is compatible with CSP policies we'd need actual parsing of the literals (wish coffee-script would do it).

See:

Fixes #17

While this gets rid of `eval` in node or in "mixed" environments
like node-webkit or Atom Shell apps, it still resolves to eval
when using browserify.

For a solution that is compatible with CSP policies we'd need
actual parsing of the literals (wish coffee-script would do it).

See:
* https://github.com/substack/vm-browserify/blob/bfd7c5f59edec856dc7efe0b77a4f6b2fa20f226/index.js#L105
@ghost
Copy link

ghost commented Jan 12, 2015

:shipit:

jkrems added a commit that referenced this pull request Jan 12, 2015
Use `vm.runInThisContext` instead of eval
@jkrems jkrems merged commit a47e308 into master Jan 12, 2015
@jkrems jkrems deleted the jk/no-eval branch January 12, 2015 17:27
@jkrems
Copy link
Contributor Author

jkrems commented Jan 12, 2015

v1.0.4

@kevinsawicki
Copy link

💚

@johan
Copy link
Contributor

johan commented Jan 12, 2015

One way of achieving that without writing a lot of code is to convert the ' string case to the " case using the known-safe JSON.parse. It's a fairly easy replace of \' to \", taking care to not trip up backslashes that are part of a \\ token.

Unless I'm misthinking something.

@jkrems
Copy link
Contributor Author

jkrems commented Jan 12, 2015

@johan I wouldn't want to bet that all escaping rules would be preserved with a solution like that. Falling back to JSON.parse is already a hack for double-quoted strings (since it breaks "\," which is now a parse error but resolved to , before). But I'd be happy to accept a PR that goes down that road.

@johan
Copy link
Contributor

johan commented Jan 12, 2015

I also missed that " of course needs to become \", but I might try a PR for this. Maybe even escaping \, to resolve the right result after JSON.parse. :-)

@johan
Copy link
Contributor

johan commented Jan 12, 2015

It's a bit too late in the evening for a PR, but I think this sketch might be good, both for ' and " strings:

quotes = /\\['"\\,]|['"]/g
quoted = {
  "'":  "'"
  '"':  '"'
  '\\': '\\'
  ',':  '\\,'
}

parseStringLiteral = (literal) ->
  json = literal.replace quotes, (ch) ->
    '\\' + quoted[ch.charAt ch.length-1]
  JSON.parse '"' + json + '"'

@jkrems
Copy link
Contributor Author

jkrems commented Jan 13, 2015

@johan Well, JSON.parse treats unnecessary backslashes in strings as syntax errors. So you're solution will give:

parseStringLiteral("won't") // SyntaxError: Unexpected token '
parseStringLiteral("say \\\"what\\\"") = "say \"what\""
parseStringLiteral("lines end with \\n") = "lines end with \n"
parseStringLiteral("say \\u0022what\\u0022") = "say \"what\""
parseStringLiteral("escape using \\\\") = "escape using \\"
parseStringLiteral("col\\u000Bcol") = "col\u000bcol"

Code used:

strings = [
  "won't"
  "say \\\"what\\\""
  "lines end with \\n"
  "say \\u0022what\\u0022"
  "escape using \\\\"
  "col\\u000Bcol"
]

strings.forEach (str) ->
  try
    parsed = parseStringLiteral str
    console.log 'parseStringLiteral(%j) = %j', str, parsed
  catch err
    console.log 'parseStringLiteral(%j) // %s: %s', str, err.name, err.message

EDIT: Fixed the tests. I think they are now escaping correctly.

@johan
Copy link
Contributor

johan commented Jan 13, 2015

Your array doesn't actually have legal string tokens in it after compilation; ITYM something more like:

strings = [
  '"won\'t"'
  '"say \\"what\\""'
  '"lines end with \\n"'
  '"say \\u0022what\\u0022"'
  '"escape using \\\\"'
  '"col\\u000Bcol"'
]

...but the examples are still good proof my quick hack needs additional work before it can do business. :-)

@jkrems
Copy link
Contributor Author

jkrems commented Jan 13, 2015

Actually, you add the wrapping quotes in your parsing code.* Anyhow, those are the basic "happy path" test cases I could think of. :)

[*] Quote: JSON.parse '"' + json + '"'

@johan
Copy link
Contributor

johan commented Jan 13, 2015

Ah, my hack was meant to read literal.slice(1, -1).replace to get rid of the wrap apostrophes/quotes, but thanks for the test data; these types of hacks are best done TDD style. And then perhaps subjected to a fuzzer that benchmarks it a zillion times against what eval would have done for random strings. :-)

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.

Use vm.runInThisContext instead of eval?
3 participants