Repository navigation
Conversation
The length limit is meant to protect against expansion of placeables,
but the resolver also applied it to text written directly in a message
and to the selected variant of a select expression, so any long piece
of text combined with a placeable was formatted as {???}.
Also report the actual limit in the error message, which previously
showed MAX_PARTS (1000) instead of MAX_PART_LENGTH (2500).
Fixes projectfluent#170
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MAX_PART_LENGTHis there to stop placeable expansion bombs, but the resolver also applied it to text written directly in a message. Any message with more than 2500 characters of plain text plus a placeable came out as{???}. The same happened when a select expression picked a long variant. As noted in #170, fluent.js and fluent-rs have no such limit.Changes in
fluent.runtime/fluent/runtime/resolver.py:Patternno longer runs the length check on literal elements (text and string literals) or on placeables. Placeables already resolve and check their own value.MAX_PART_LENGTH, 2500). Before, it showedPattern.MAX_PARTS(1000), which is where the "1000 characters" in the issue title came from.Message and term references, variables and function results are still checked, so the billion-laughs protection works the same as before.
test_bomb.pypasses unchanged, and one assertion was added for the corrected limit in the message.Fixes #170
Before / after
long = xxx…(3000 chars) { $arg }{???},[ValueError('Too many characters in placeable (3001, max allowed is 1000)')]Testing
New tests are in
fluent.runtime/tests/test_bomb.py, underTestLongText. They cover long text before a placeable (isolating and non-isolating), a long string literal, a long select variant, and a long select variant after text. All 6 of the new and updated tests fail without the fix and pass with it.