From c33dd62afdbe2fc593ac6746c4852565be3e0f94 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Fri, 17 Jan 2025 15:44:48 -0800 Subject: [PATCH] Restrict CSSDataTypeParser function and simple block parsing to block scope (#48768) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/48768 Right now, if something like `CSSColor` accepted a function notation block (e.g. for `rgb()` syntax), it is given a syntax parser which may extend beyond the scope of the current function block. This is confusing, but also problematic for the `CSSSyntaxParser` block visiting logic to validate a correct scope exit. This change makes it so that the `CSSSyntaxParser` passsed to `CSSDataTypeParser` for function and simple blocks is limited to the syntax within the given block. This prevents extra visiblity beyond the block we are trying to parse, and allows the function/simple block visitor to reliably fail parsing if the block is not correctly terminated, or there are unconsumed component values within the block not handled by the data type parser. Changelog: [Internal] Reviewed By: joevilches Differential Revision: D68340127 fbshipit-source-id: 402f2eabdf6df7bce99223c966f22c2158103be5 --- .../react/renderer/css/CSSSyntaxParser.h | 71 +++++---- .../react/renderer/css/CSSValueParser.h | 32 +++-- .../css/tests/CSSSyntaxParserTest.cpp | 136 ++++++++++++++---- 3 files changed, 170 insertions(+), 69 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h b/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h index 2b5eaf78020b17..d7d4f4c0cabad3 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h @@ -15,6 +15,8 @@ namespace facebook::react { +class CSSSyntaxParser; + /** * Describes context for a CSS function component value. */ @@ -41,9 +43,10 @@ struct CSSSimpleBlock { * of the function block. */ template -concept CSSFunctionVisitor = requires(T visitor, CSSFunctionBlock func) { - { visitor(func) } -> std::convertible_to; -}; +concept CSSFunctionVisitor = + requires(T visitor, CSSFunctionBlock func, CSSSyntaxParser& blockParser) { + { visitor(func, blockParser) } -> std::convertible_to; + }; /** * A CSSPreservedTokenVisitor is called after parsing a preserved token @@ -61,9 +64,10 @@ concept CSSPreservedTokenVisitor = * of the block. */ template -concept CSSSimpleBlockVisitor = requires(T visitor, CSSSimpleBlock block) { - { visitor(block) } -> std::convertible_to; -}; +concept CSSSimpleBlockVisitor = + requires(T visitor, CSSSimpleBlock block, CSSSyntaxParser& blockParser) { + { visitor(block, blockParser) } -> std::convertible_to; + }; /** * Any visitor for a component value. @@ -164,6 +168,11 @@ class CSSSyntaxParser { } private: + CSSSyntaxParser(CSSSyntaxParser& parser, CSSTokenType terminator) + : tokenizer_{parser.tokenizer_}, + currentToken_{parser.currentToken_}, + terminator_{terminator} {} + constexpr const CSSToken& peek() const { return currentToken_; } @@ -174,8 +183,14 @@ class CSSSyntaxParser { return prevToken; } + constexpr void advanceToBlockParser(CSSSyntaxParser& blockParser) { + currentToken_ = blockParser.currentToken_; + tokenizer_ = blockParser.tokenizer_; + } + CSSTokenizer tokenizer_; CSSToken currentToken_; + CSSTokenType terminator_{CSSTokenType::EndOfFile}; }; template ... VisitorsT> @@ -201,6 +216,10 @@ struct CSSComponentValueVisitorDispatcher { break; } + if (parser.peek().type() == parser.terminator_) { + return {}; + } + switch (parser.peek().type()) { case CSSTokenType::Function: if (auto ret = visitFunction(visitors...)) { @@ -235,28 +254,22 @@ struct CSSComponentValueVisitorDispatcher { return ReturnT{}; } - constexpr ReturnT consumeNextCommaDelimitedValue( - const VisitorsT&... visitors) { - parser.consumeWhitespace(); - if (parser.consumeToken().type() != CSSTokenType::Comma) { - return {}; - } - parser.consumeWhitespace(); - return consumeComponentValue(std::forward(visitors)...); - } - - constexpr ReturnT consumeNextWhitespaceDelimitedValue( - const VisitorsT&... visitors) { - parser.consumeWhitespace(); - return consumeComponentValue(std::forward(visitors)...); - } - constexpr std::optional visitFunction( const CSSComponentValueVisitor auto& visitor, const CSSComponentValueVisitor auto&... rest) { if constexpr (CSSFunctionVisitor) { - auto functionValue = - visitor({.name = parser.consumeToken().stringValue()}); + auto name = parser.consumeToken().stringValue(); + + // CSS syntax spec says whitespace is a preserved token, but CSS values + // spec allows whitespace around parens for all function notation, so we + // allow this to let the visitors not need to deal with leading/trailing + // whitespace. https://www.w3.org/TR/css-values-3/#functional-notations + parser.consumeWhitespace(); + + auto blockParser = + CSSSyntaxParser{parser, CSSTokenType::CloseParen /*terminator*/}; + auto functionValue = visitor({name}, blockParser); + parser.advanceToBlockParser(blockParser); parser.consumeWhitespace(); if (parser.peek().type() == CSSTokenType::CloseParen) { parser.consumeToken(); @@ -273,16 +286,16 @@ struct CSSComponentValueVisitorDispatcher { return {}; } - // Can be one of std::monostate (variant null-type), CSSWideKeyword, - // CSSLength, or CSSPercentage - constexpr std::optional visitSimpleBlock( CSSTokenType endToken, const CSSComponentValueVisitor auto& visitor, const CSSComponentValueVisitor auto&... rest) { if constexpr (CSSSimpleBlockVisitor) { - auto blockValue = - visitor({.openBracketType = parser.consumeToken().type()}); + auto openBracketType = parser.consumeToken().type(); + parser.consumeWhitespace(); + auto blockParser = CSSSyntaxParser{parser, endToken}; + auto blockValue = visitor({openBracketType}, blockParser); + parser.advanceToBlockParser(blockParser); parser.consumeWhitespace(); if (parser.peek().type() == endToken) { parser.consumeToken(); diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h b/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h index 953bceea5e932f..7acdbc72a364cb 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h @@ -42,15 +42,15 @@ class CSSValueParser { ReturnT, CSSDataTypeParser...>(token); }, - [&](const CSSSimpleBlock& block) { + [&](const CSSSimpleBlock& block, CSSSyntaxParser& blockParser) { return tryConsumeSimpleBlock< ReturnT, - CSSDataTypeParser...>(block); + CSSDataTypeParser...>(block, blockParser); }, - [&](const CSSFunctionBlock& func) { + [&](const CSSFunctionBlock& func, CSSSyntaxParser& blockParser) { return tryConsumeFunctionBlock< ReturnT, - CSSDataTypeParser...>(func); + CSSDataTypeParser...>(func, blockParser); }); } @@ -90,7 +90,9 @@ class CSSValueParser { } template - constexpr ReturnT tryConsumeSimpleBlock(const CSSSimpleBlock& /*token*/) { + constexpr ReturnT tryConsumeSimpleBlock( + const CSSSimpleBlock& /*token*/, + CSSSyntaxParser& /*blockParser*/) { return {}; } @@ -98,18 +100,22 @@ class CSSValueParser { typename ReturnT, CSSValidDataTypeParser ParserT, CSSValidDataTypeParser... RestParserT> - constexpr ReturnT tryConsumeSimpleBlock(const CSSSimpleBlock& block) { + constexpr ReturnT tryConsumeSimpleBlock( + const CSSSimpleBlock& block, + CSSSyntaxParser& blockParser) { if constexpr (CSSSimpleBlockSink) { - if (auto ret = ParserT::consumeSimpleBlock(block, parser_)) { + if (auto ret = ParserT::consumeSimpleBlock(block, blockParser)) { return *ret; } } - return tryConsumeSimpleBlock(block); + return tryConsumeSimpleBlock(block, blockParser); } template - constexpr ReturnT tryConsumeFunctionBlock(const CSSFunctionBlock& /*func*/) { + constexpr ReturnT tryConsumeFunctionBlock( + const CSSFunctionBlock& /*func*/, + CSSSyntaxParser& /*blockParser*/) { return {}; } @@ -117,14 +123,16 @@ class CSSValueParser { typename ReturnT, CSSValidDataTypeParser ParserT, CSSValidDataTypeParser... RestParserT> - constexpr ReturnT tryConsumeFunctionBlock(const CSSFunctionBlock& func) { + constexpr ReturnT tryConsumeFunctionBlock( + const CSSFunctionBlock& func, + CSSSyntaxParser& blockParser) { if constexpr (CSSFunctionBlockSink) { - if (auto ret = ParserT::consumeFunctionBlock(func, parser_)) { + if (auto ret = ParserT::consumeFunctionBlock(func, blockParser)) { return *ret; } } - return tryConsumeFunctionBlock(func); + return tryConsumeFunctionBlock(func, blockParser); } CSSSyntaxParser& parser_; diff --git a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp index d97178ab7e99e8..8dee9bc285a2f2 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp @@ -47,9 +47,15 @@ TEST(CSSSyntaxParser, single_function_no_args) { CSSSyntaxParser parser{"foo()"}; auto funcName = parser.consumeComponentValue( - [](const CSSFunctionBlock& function) { + [](const CSSFunctionBlock& function, CSSSyntaxParser& blockParser) { EXPECT_EQ(function.name, "foo"); return function.name; + + auto hasMoreTokens = blockParser.consumeComponentValue( + CSSComponentValueDelimiter::Whitespace, + [](const CSSPreservedToken& /*token*/) { return true; }); + + EXPECT_FALSE(hasMoreTokens); }); EXPECT_EQ(funcName, "foo"); } @@ -58,12 +64,12 @@ TEST(CSSSyntaxParser, single_function_with_whitespace_delimited_args) { CSSSyntaxParser parser{"foo( a b c)"}; auto funcArgs = parser.consumeComponentValue>( - [&](const CSSFunctionBlock& function) { + [&](const CSSFunctionBlock& function, CSSSyntaxParser& blockParser) { EXPECT_EQ(function.name, "foo"); std::vector args; - args.emplace_back(parser.consumeComponentValue( + args.emplace_back(blockParser.consumeComponentValue( CSSComponentValueDelimiter::Whitespace, [](const CSSPreservedToken& token) { @@ -72,7 +78,7 @@ TEST(CSSSyntaxParser, single_function_with_whitespace_delimited_args) { return token.stringValue(); })); - args.emplace_back(parser.consumeComponentValue( + args.emplace_back(blockParser.consumeComponentValue( CSSComponentValueDelimiter::Whitespace, [](const CSSPreservedToken& token) { @@ -81,7 +87,7 @@ TEST(CSSSyntaxParser, single_function_with_whitespace_delimited_args) { return token.stringValue(); })); - args.emplace_back(parser.consumeComponentValue( + args.emplace_back(blockParser.consumeComponentValue( CSSComponentValueDelimiter::Whitespace, [](const CSSPreservedToken& token) { @@ -90,6 +96,12 @@ TEST(CSSSyntaxParser, single_function_with_whitespace_delimited_args) { return token.stringValue(); })); + auto hasMoreTokens = blockParser.consumeComponentValue( + CSSComponentValueDelimiter::Whitespace, + [](const CSSPreservedToken& /*token*/) { return true; }); + + EXPECT_FALSE(hasMoreTokens); + return args; }); @@ -101,12 +113,12 @@ TEST(CSSSyntaxParser, single_function_with_comma_delimited_args) { CSSSyntaxParser parser{"rgb(100, 200, 50 )"}; auto funcArgs = parser.consumeComponentValue>( - [&](const CSSFunctionBlock& function) { + [&](const CSSFunctionBlock& function, CSSSyntaxParser& blockParser) { EXPECT_EQ(function.name, "rgb"); std::array rgb{}; - rgb[0] = parser.consumeComponentValue( + rgb[0] = blockParser.consumeComponentValue( CSSComponentValueDelimiter::Whitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Number); @@ -114,7 +126,7 @@ TEST(CSSSyntaxParser, single_function_with_comma_delimited_args) { return static_cast(token.numericValue()); }); - rgb[1] = parser.consumeComponentValue( + rgb[1] = blockParser.consumeComponentValue( CSSComponentValueDelimiter::Comma, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Number); @@ -122,7 +134,7 @@ TEST(CSSSyntaxParser, single_function_with_comma_delimited_args) { return static_cast(token.numericValue()); }); - rgb[2] = parser.consumeComponentValue( + rgb[2] = blockParser.consumeComponentValue( CSSComponentValueDelimiter::Comma, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Number); @@ -130,6 +142,12 @@ TEST(CSSSyntaxParser, single_function_with_comma_delimited_args) { return static_cast(token.numericValue()); }); + auto hasMoreTokens = blockParser.consumeComponentValue( + CSSComponentValueDelimiter::Whitespace, + [](const CSSPreservedToken& /*token*/) { return true; }); + + EXPECT_FALSE(hasMoreTokens); + return rgb; }); @@ -141,9 +159,9 @@ TEST(CSSSyntaxParser, complex) { CSSSyntaxParser parser{"foo(a bar())baz() 12px"}; auto fooFunc = parser.consumeComponentValue( - [&](const CSSFunctionBlock& function) { + [&](const CSSFunctionBlock& function, CSSSyntaxParser& blockParser) { EXPECT_EQ(function.name, "foo"); - auto identArg = parser.consumeComponentValue( + auto identArg = blockParser.consumeComponentValue( CSSComponentValueDelimiter::Whitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); @@ -152,20 +170,32 @@ TEST(CSSSyntaxParser, complex) { }); EXPECT_EQ(identArg, "a"); - auto barFunc = parser.consumeComponentValue( + auto barFunc = blockParser.consumeComponentValue( CSSComponentValueDelimiter::Whitespace, - [&](const CSSFunctionBlock& function) { + [&](const CSSFunctionBlock& function, + CSSSyntaxParser& nestedBlockParser) { EXPECT_EQ(function.name, "bar"); + auto hasMoreTokens = + nestedBlockParser.consumeComponentValue( + CSSComponentValueDelimiter::Whitespace, + [](const CSSPreservedToken& /*token*/) { return true; }); + EXPECT_FALSE(hasMoreTokens); + return function.name; }); EXPECT_EQ(barFunc, "bar"); + auto hasMoreTokens = blockParser.consumeComponentValue( + CSSComponentValueDelimiter::Whitespace, + [](const CSSPreservedToken& /*token*/) { return true; }); + EXPECT_FALSE(hasMoreTokens); + return function.name; }); EXPECT_EQ(fooFunc, "foo"); auto bazFunc = parser.consumeComponentValue( - [&](const CSSFunctionBlock& function) { + [&](const CSSFunctionBlock& function, CSSSyntaxParser& /*blockParser*/) { EXPECT_EQ(function.name, "baz"); return function.name; }); @@ -184,18 +214,22 @@ TEST(CSSSyntaxParser, complex) { TEST(CSSSyntaxParser, unterminated_functions) { EXPECT_FALSE(CSSSyntaxParser{"foo("}.consumeComponentValue( - [](const CSSFunctionBlock&) { return true; })); + [](const CSSFunctionBlock&, CSSSyntaxParser& /*blockParser*/) { + return true; + })); EXPECT_FALSE(CSSSyntaxParser{"foo(a bar()baz()"}.consumeComponentValue( - [](const CSSFunctionBlock&) { return true; })); + [](const CSSFunctionBlock&, CSSSyntaxParser& /*blockParser*/) { + return true; + })); } TEST(CSSSyntaxParser, simple_blocks) { CSSSyntaxParser parser1{"(a)"}; auto identValue = parser1.consumeComponentValue( - [&](const CSSSimpleBlock& block) { + [&](const CSSSimpleBlock& block, CSSSyntaxParser& blockParser) { EXPECT_EQ(block.openBracketType, CSSTokenType::OpenParen); - return parser1.consumeComponentValue( + return blockParser.consumeComponentValue( [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); return token.stringValue(); @@ -205,9 +239,9 @@ TEST(CSSSyntaxParser, simple_blocks) { CSSSyntaxParser parser2{"[b ]"}; auto identValue2 = parser2.consumeComponentValue( - [&](const CSSSimpleBlock& block) { + [&](const CSSSimpleBlock& block, CSSSyntaxParser& blockParser) { EXPECT_EQ(block.openBracketType, CSSTokenType::OpenSquare); - return parser2.consumeComponentValue( + return blockParser.consumeComponentValue( [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); return token.stringValue(); @@ -217,9 +251,9 @@ TEST(CSSSyntaxParser, simple_blocks) { CSSSyntaxParser parser3{"{c}"}; auto identValue3 = parser3.consumeComponentValue( - [&](const CSSSimpleBlock& block) { + [&](const CSSSimpleBlock& block, CSSSyntaxParser& blockParser) { EXPECT_EQ(block.openBracketType, CSSTokenType::OpenCurly); - return parser3.consumeComponentValue( + return blockParser.consumeComponentValue( [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); return token.stringValue(); @@ -231,9 +265,9 @@ TEST(CSSSyntaxParser, simple_blocks) { TEST(CSSSyntaxParser, unterminated_simple_blocks) { CSSSyntaxParser parser1{"(a"}; auto identValue = parser1.consumeComponentValue( - [&](const CSSSimpleBlock& block) { + [&](const CSSSimpleBlock& block, CSSSyntaxParser& blockParser) { EXPECT_EQ(block.openBracketType, CSSTokenType::OpenParen); - return parser1.consumeComponentValue( + return blockParser.consumeComponentValue( [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); return token.stringValue(); @@ -243,9 +277,9 @@ TEST(CSSSyntaxParser, unterminated_simple_blocks) { CSSSyntaxParser parser2{"[b "}; auto identValue2 = parser2.consumeComponentValue( - [&](const CSSSimpleBlock& block) { + [&](const CSSSimpleBlock& block, CSSSyntaxParser& blockParser) { EXPECT_EQ(block.openBracketType, CSSTokenType::OpenSquare); - return parser2.consumeComponentValue( + return blockParser.consumeComponentValue( [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); return token.stringValue(); @@ -255,9 +289,9 @@ TEST(CSSSyntaxParser, unterminated_simple_blocks) { CSSSyntaxParser parser3{"{c"}; auto identValue3 = parser3.consumeComponentValue( - [&](const CSSSimpleBlock& block) { + [&](const CSSSimpleBlock& block, CSSSyntaxParser& blockParser) { EXPECT_EQ(block.openBracketType, CSSTokenType::OpenCurly); - return parser3.consumeComponentValue( + return blockParser.consumeComponentValue( [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); return token.stringValue(); @@ -266,4 +300,50 @@ TEST(CSSSyntaxParser, unterminated_simple_blocks) { EXPECT_EQ(identValue3, ""); } +TEST(CSSSyntaxParser, unconsumed_function_args) { + CSSSyntaxParser parser{"foo(a)"}; + auto funcValue = + parser.consumeComponentValue>( + [&](const CSSFunctionBlock& function, + CSSSyntaxParser& /*blockParser*/) { + EXPECT_EQ(function.name, "foo"); + return function.name; + }); + + EXPECT_EQ(funcValue, std::nullopt); +} + +TEST(CSSSyntaxParser, whitespace_surrounding_function_args) { + CSSSyntaxParser parser{"foo( a )"}; + auto funcValue = parser.consumeComponentValue( + [&](const CSSFunctionBlock& function, CSSSyntaxParser& blockParser) { + EXPECT_EQ(function.name, "foo"); + + auto identArg = blockParser.consumeComponentValue( + CSSComponentValueDelimiter::None, + [](const CSSPreservedToken& token) { + EXPECT_EQ(token.type(), CSSTokenType::Ident); + EXPECT_EQ(token.stringValue(), "a"); + return token.stringValue(); + }); + + EXPECT_EQ(identArg, "a"); + + return function.name; + }); + + EXPECT_EQ(funcValue, "foo"); +} + +TEST(CSSSyntaxParser, unconsumed_simple_block_args) { + CSSSyntaxParser parser{"{a}"}; + auto funcValue = parser.consumeComponentValue>( + [&](const CSSSimpleBlock& block, CSSSyntaxParser& /*blockParser*/) { + EXPECT_EQ(block.openBracketType, CSSTokenType::OpenCurly); + return block.openBracketType; + }); + + EXPECT_EQ(funcValue, std::nullopt); +} + } // namespace facebook::react