Skip to content

Decreased the 'phase' values where variable(s) allow(s) - #1941

Merged
dune73 merged 3 commits into
coreruleset:v3.4/devfrom
airween:v3.4/move_phases
Dec 9, 2020
Merged

Decreased the 'phase' values where variable(s) allow(s)#1941
dune73 merged 3 commits into
coreruleset:v3.4/devfrom
airween:v3.4/move_phases

Conversation

@airween

@airween airween commented Dec 3, 2020

Copy link
Copy Markdown
Contributor

Decreased the phase values where variable activated the previous phase - eg. REQUEST_FILENAME available in phase:1, not necessary to wait to 2nd phase (body processing). All changes are from phase:2 to phase:1, except in case of rule 950100, which is a response rule - there the old phase value is 4, now it's 3.

@dune73 dune73 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have looked through all the changes in this PR by hand and I have not found a problem.

It looks like all the changes are valid and we do not lose anything by shifting to phase:1 in these situations. This looks very good.

I did not check the remaining phase:2 rules so I can not tell whether some of them could still be moved to phase:1, but I presume this is not the case.

@airween

airween commented Dec 4, 2020

Copy link
Copy Markdown
Contributor Author

I did not check the remaining phase:2 rules so I can not tell whether some of them could still be moved to phase:1, but I presume this is not the case.

I've collected the variables by a script, and checked the variables and the phases where they waked. The "target" phase became the highest value - except the collection variables, like TX or IP - I skipped them.

The untouched rules are in right phases I guess.

@dune73

dune73 commented Dec 4, 2020

Copy link
Copy Markdown
Member

I am positive this is the case, not the least because you did work by script.

Where I am a bit weary is skipAfter happening in the wrong phase now. But I checked all the rule-exclusion packages and they are correct in this regard. So I think we are OK.

@fzipi

fzipi commented Dec 7, 2020

Copy link
Copy Markdown
Member

Did an additional review also, and haven't found a problem. This is a very good PR!

@dune73

dune73 commented Dec 7, 2020

Copy link
Copy Markdown
Member

Thank you, @fzipi.

We're ready to merge, but since this is a significant change, we'll keep it on the agenda for tonight. There are also other questions around the underlying issue. And we should discuss those.

@dune73

dune73 commented Dec 8, 2020

Copy link
Copy Markdown
Member

Meeting decision (#1944 (comment)): This will be merged once the new conflict is resolved.

@airween

airween commented Dec 8, 2020

Copy link
Copy Markdown
Contributor Author

Conflict had resolved.

@dune73

dune73 commented Dec 9, 2020

Copy link
Copy Markdown
Member

Thank you for your quick reaction and the patch. Merging now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants