Skip to content

Conversation

Xhoenix
Copy link
Member

@Xhoenix Xhoenix commented Jul 14, 2025

Reference: Link

Copy link
Contributor

github-actions bot commented Jul 14, 2025

📊 Quantitative test results for language: eng, year: 2023, size: 10K, paranoia level: 1:
🚀 Quantitative testing did not detect new false positives

@Xhoenix Xhoenix added the release:new-detection In this PR we introduce a new detection label Jul 14, 2025
@Xhoenix Xhoenix requested a review from a team July 14, 2025 07:00
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice addition!

@Xhoenix Xhoenix added this pull request to the merge queue Jul 14, 2025
Merged via the queue into coreruleset:main with commit 55bbf2e Jul 14, 2025
7 checks passed
@Xhoenix Xhoenix deleted the detect-latex-injection branch July 14, 2025 12:43
@azurit
Copy link
Member

azurit commented Jul 14, 2025

Your regex is too wide, it is matching, for example:

\anything-here-unlimited-lenght{
\anything-here-unlimited-lenght=
\anything-here-unlimited-lenght|

I suggest to match only specitifc latex commands, which we consider dangerous, for example \openin\file=.

@azurit
Copy link
Member

azurit commented Jul 14, 2025

Should not be merged yet.

@Xhoenix Xhoenix restored the detect-latex-injection branch July 14, 2025 12:55
@Xhoenix Xhoenix deleted the detect-latex-injection branch July 14, 2025 13:08
@Xhoenix
Copy link
Member Author

Xhoenix commented Jul 14, 2025

Already merged, revert not possible without creating another PR. Will create another PR with suggested changes.

@azurit
Copy link
Member

azurit commented Jul 14, 2025

Thanks.

@fzipi fzipi mentioned this pull request Jul 31, 2025
@S0obi
Copy link
Contributor

S0obi commented Jul 31, 2025

@Xhoenix @azurit seems like this rule sneaked in release v4.17.0 without the proposed change. I am actually worried about the potential false positives it may create. Should we act on it ? Thanks in advance

@theseion
Copy link
Contributor

theseion commented Aug 1, 2025

Thanks @S0obi. @Xhoenix could you open that PR? We can create a patch release.

@theseion
Copy link
Contributor

theseion commented Aug 1, 2025

FYI @fzipi.

@fzipi
Copy link
Member

fzipi commented Aug 1, 2025

Ugh.

@azurit
Copy link
Member

azurit commented Aug 2, 2025

@Xhoenix You promised to send a PR, do you have time for it or should we handle it?

@Xhoenix
Copy link
Member Author

Xhoenix commented Aug 2, 2025

I wanted to implement the changes, but decided not due to the discussion in #4210. Let's get that sorted out.

@azurit
Copy link
Member

azurit commented Aug 2, 2025

It MUST be changed somehow, no matter what, we cannot match strings like \................whole web page containing anything with any lenght................= on PL1.

@Xhoenix
Copy link
Member Author

Xhoenix commented Aug 2, 2025

I'll try to modify #4210.

@Xhoenix
Copy link
Member Author

Xhoenix commented Aug 4, 2025

@azurit I've suggested some changes in #4210 , either you can implement those, or close that PR and come up with your own changes, otherwise we'll have to revert this PR, until a fix is provided, since we can't accept a clear FP at PL 1 in production.

@fzipi
Copy link
Member

fzipi commented Aug 4, 2025

For what is worth, we are reverting this one then until someone comes with a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:new-detection In this PR we introduce a new detection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants