Prevent users from passing arbitrary parser options to Jint #1831
Conversation
…pt/ModuleParseOptions in the public API)
…re created using Engine.PrepareScript/PrepareModule
adams85
changed the title
| using Jint.Runtime; | ||
| using Jint.Runtime.Modules; | ||
|
|
||
| namespace Jint.Tests.Runtime |
There was a problem hiding this comment.
nit: for new files can use file-scoped namespaces
Jint/ParseOptions.cs
Outdated
| using System.Text.RegularExpressions; | ||
|
|
||
| namespace Jint | ||
| { |
There was a problem hiding this comment.
nit: could use file-scoped namespace
Jint/ParseOptions.cs
Outdated
|
|
||
| namespace Jint | ||
| { | ||
| public interface IParseOptions |
There was a problem hiding this comment.
I'm not a native speaker, but would IParsingOptions be more natura, also ScriptParsingOptions - just a thought
There was a problem hiding this comment.
Neither am I... :) The problem with ScriptParsingOptions is that then we should also have ScriptPreparingOptions. Sounds a bit weird. How about ParseScriptOptions and PrepareScriptOptions maybe?
There was a problem hiding this comment.
Ah, I just saw #1831 (comment). I'm fine with IParsingOptions/IPreparationOptions as well.
There was a problem hiding this comment.
I'm fine with either way, I guess we need an Oxford scholar for this 😆
There was a problem hiding this comment.
Naming things and cache invalidation, the two hard problems in software engineering :D
I ran a quick search on the ASP.NET Core repo. Although there are some counter-examples but it's mostly [...]ingOptions or [...]tionOptions, so I'm convinced. :D
Jint/ParseOptions.cs
Outdated
| bool Tolerant { get; init; } | ||
| } | ||
|
|
||
| public record class ScriptParseOptions : IParseOptions |
Jint/ParseOptions.cs
Outdated
| /// Defaults to <see langword="true"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Please note that there is a plan to change the default to <see langword="false"/> in the next major version. |
There was a problem hiding this comment.
next major of Acornima? so should this just be true?
There was a problem hiding this comment.
I meant the next major version of Jint.
Or we could just make this BC in v3.1 as the related APIs will be broken anyway. What do you think?
There was a problem hiding this comment.
I'm not just quite sure if it's best approach on interpreter side to be strict-by-default. For parsing it makes sense, but for anyone just "having a good time with JS" do we need to restrict parsing and allowed constructs? But again seems that NodeJS throws error on console from using return so maybe it makes sense.
There was a problem hiding this comment.
I wrote about this in the other PR: "[...] I can only speak for myself, but I'd be surprised if I fed some script to Jint which is invalid per spec and get some results instead of SyntaxError. I mean I think relaxing the rules should be a deliberate choice, not the default. [...]"
But again seems that NodeJS throws error on console from using return so maybe it makes sense.
Disabling tolerant mode doesn't affect top level return because that's controlled by another switch than tolerant mode. That option would be on for scripts by default: https://github.com/sebastienros/jint/pull/1831/files#diff-54aceb67398ce1901835b606789cff7f0ac2abb5136a75480469b5c732d19b11R40
There was a problem hiding this comment.
Let's not break this behavior in a minor version, so I'd say let's leave it as is for now and reconsider it when v4 is on the table.
Jint/ParseOptions.cs
Outdated
| /// <remarks> | ||
| /// Please note that there is a plan to change the default to <see langword="false"/> in the next major version. | ||
| /// </remarks> | ||
| bool Tolerant { get; init; } |
There was a problem hiding this comment.
live below (came back to this), should we just initialize with true?
There was a problem hiding this comment.
I'm not sure I get this. This is an interface, we can't initialize the property here, can't we? It's initialized here: https://github.com/sebastienros/jint/pull/1831/files#diff-5196d539da48e2bc4acd76456adfff1ca098ad422ce71e90849e98258c066633R62
Jint/PrepareOptions.cs
Outdated
| @@ -0,0 +1,52 @@ | |||
| namespace Jint | |||
| { | |||
| public interface IPrepareOptions<out TParseOptions> | |||
There was a problem hiding this comment.
not my grammar, but IPreparationOptions?
|
Thank you once again! |
|
Sure thing! |
As suggested here, this PR extracts some changes from #1820 to make them happen earlier.
This means some breaking changes to the public API which prevent users from passing arbitrary parser options to Jint. Only a subset of the parser options should be adjustable by end users, while the rest should be reserved for Jint to achieve the desired parser behavior when it needs to parse code dynamically (
eval,Functionctor, shadow realms, etc.)The PR also...
ExecutionContextfor dynamic code parsing - e.g. when the script is parsed in non-tolerant mode, dynamic code should be as well. (In the case ofevalthis doesn't work currently as Esprima doesn't provide the necessary settings, so tolerant mode must be enabled no matter what. This will be solved though once the migration to Acornima happens.)