-
Notifications
You must be signed in to change notification settings - Fork 71
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
feat(ses): Allow import and eval methods #669
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
But I defer approval to @erights
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
under SES to specifically allow the use of `import.import()` or | ||
`evaluator.eval()` methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In both cases, the identifier to the left of the dot looks too specific. To avoid misunderstanding but without metasyntactic ceremony, I'd just say something like something.import()
or something.eval()
methods.
packages/ses/src/transforms.js
Outdated
// The importPattern incidentally captures an initial \n in | ||
// an attempt to reject a . prefix, so we need to offset | ||
// the line number in that case. | ||
const [p1] = stringMatch(src, pattern); | ||
const adjustment = stringStartsWith(p1, '\n') ? 1 : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems weird to do both a stringSearch
and a stringMatch
. Is the following equivalent?
// The importPattern incidentally captures an initial \n in | |
// an attempt to reject a . prefix, so we need to offset | |
// the line number in that case. | |
const [p1] = stringMatch(src, pattern); | |
const adjustment = stringStartsWith(p1, '\n') ? 1 : 0; | |
// The importPattern incidentally captures an initial \n in | |
// an attempt to reject a . prefix, so we need to offset | |
// the line number in that case. | |
const adjustment = src[index] === '\n' ? 1 : 0; |
@@ -90,7 +103,7 @@ export function evadeHtmlCommentTest(src) { | |||
|
|||
// ///////////////////////////////////////////////////////////////////////////// | |||
|
|||
const importPattern = new RegExp('\\bimport(\\s*(?:\\(|/[/*]))', 'g'); | |||
const importPattern = new RegExp('(^|[^.])\\bimport(\\s*(?:\\(|/[/*]))', 'g'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. I never knew a ^
for "beginning of match" could be used as a disjunct. Nice.
* Finally, if the original appears in code where it is not parsed as an | ||
* expression, for example `foo.import(path)`, then this evasion would rewrite | ||
* to `foo.__import__(path)` which has a surprisingly different meaning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment no longer applicable, or does it just need a different example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is no longer applicable. I don’t know of an example to replace this one. Cases where import(
is not an expression of dynamic import that I can readily recall are the case where it’s a comment, in which case there’s no change to the meaning at runtime, and when it’s a method, which should no longer apply.
Relaxes the censorship of
import
andeval
in programs evaluated under SES to specifically allow the use ofimport.import()
orevaluator.eval()
methods.Fixes #664