potiuk commented on PR #36469: URL: https://github.com/apache/airflow/pull/36469#issuecomment-1871381217
> > NIT: can you please add a unit test showing the failure scenario? > > Seems the test failed because of using `re` module instead of `re2`. Yes. We require re2 globally in order to protect against other security issues (recursive denial of service for example). So you will need to switch to re2 . But my comment was to add a new unit test case not about failing static check. A unit test that passes the bad value and have the code reject it. Generally we almost never accept PR that add a functionality when there is no corresponding unit test. Because - for example it makes it impossible to verify if it still works after cherry-picking it to another branch. If we would like to merge it to 2.8 branch we need to have unit test that should fail before and work after the change - this way when we cherry-pick the change we can see that the fix works also in the bugfix branch. And yes - we do not have really a "security" issue as explained in the security report. And there is no known security attack surface, that's why we asked to just add validation as a regular PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
