Use a running count of resolved placeables to protect against denial of service attacks

[stasm]

Remove the arbitrary limit imposed on the length of resolved placeables. Instead, keep a running count of how many placeables there have been so far in the current call to formatPattern. This includes all nested placeables, preventing deeply nested hierarchies of references from eating up the memory and the CPU during resolution.

[Pike]

This looks OK, but we'll need to update the parent scope when we return from term references, too. I want to take a look at that one again.

In the generator I have in Python, I'm kinda making the same mistake, but I catch that in the outer loop still.

When we talk about standardization, we'll need to have a second look at whether it's parts or placeables that are better to restrict.

Also, the Python impl uses MAX_PARTS = 1000, which is a bit more lenient than MAX_PLACEABLES = 100. I wish we had good reasons to choose one number over the other ;-).

[stasm]

This looks OK, but we'll need to update the parent scope when we return from term references, too. I want to take a look at that one again.

Thanks, great feedback. Since we need to clean up after resolving a TermReference, I went with a mutable Scope instead of creating a new local one for each term.

I also changed how the resolver detects that it's inside a TermReference. Rather than use the insideTermReference flag and override Scope.args, I added a new nullable field, Scope.params which serves both purposes.

When we talk about standardization, we'll need to have a second look at whether it's parts or placeables that are better to restrict.

+1

Also, the Python impl uses MAX_PARTS = 1000, which is a bit more lenient than MAX_PLACEABLES = 100. I wish we had good reasons to choose one number over the other ;-).

Does it sound like a good idea to start with the lower number and see if it's enough? In the future, we could also consider making this an option to the FluentBundle constructor, if there are good reasons to do so.

[Pike]

I wonder if using a Map for named parameters would be better than an Object, but we're not off worse here, just slightly different if someone goes for __proto__ as variable name.

[stasm]

Thanks for the review!

I wonder if using a Map for named parameters would be better than an Object,

That's what I did in the reference resolver (WIP). The semantics of Map are really what we want here. However, in terms of API ergonomics as well as performance, switching to Map should be carefully considered, and we might want to still keep the object literal.

but we're not off worse here, just slightly different if someone goes for __proto__ as variable name.

I've had a change of heart this morning :) Let me revert to using hasOwnProperty and fix this properly in #428.