Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XenForo exclusion profile #1403

Merged
merged 5 commits into from Jul 10, 2019

Conversation

@lifeforms
Copy link
Collaborator

commented May 6, 2019

XenForo is a self-hosted forum package. This PR adds support for it.

@dune73

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

Thank you for the PR @lifeforms. It's good to include additional rule exclusion packages.

Some minor issues:

Phases

Most rules run in phase 2. I've seen 2-3 running in phase 1. Any particular reason?

If you put everything in the same phase, you are likely to be able to axe one of the initial skipAfter rules.

Rule formatting 9006800

The rule has multiple actions on a single line and the indentation is not consistent.

I do not have a XenForo installation ready, so I can not test. But the PR looks clean and proper to me.

@lifeforms

This comment has been minimized.

Copy link
Collaborator Author

commented May 7, 2019

Most rules run in phase 2. I've seen 2-3 running in phase 1. Any particular reason?

Good question!

Most exclusions pertain to POST requests, so I've put those in phase 2.

(Could theoretically improve performance by skipping over those rules if we don't have a POST request?)

Only exceptions are the ones which need to run in phase 1:

  • 9006100 is for a GET endpoint
  • 9006220 needs to disable rule 200003 (which may be a bit debatable by the way because it's outside the CRS). As I do the exclusion dynamically, it needs to go before rule 200003, so my only option was to do this in phase:1
  • 9006600 is for a GET endpoint
  • 9006800 is for cookies so must also cover GET requests

I'll edit the format of 9006800, thanks.

@dune73

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

I realize most of our rule exclusions run in phase 2. But technically, there is no need to wait for phase 2 to exclude a parameter from being processed in phase 2. Or am I wrong?

In my understanding, you manipulating the list of targets of the rule by adding parameters to the ignorelist of said existing rule for the transaction. The existence of the parameter does not matter at all.

With that being said, we could shift all rule exclusions to phase 1 and I do not know why this did not occur to me before.

But there is no need to change your PR more a general idea.

I do not think it is worth to skip over some of the rules if it is not a POST. It complicates the rule set and does not add that much (given there are still 150 CRS rules to execute).

@dune73

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

There are now 3 errors in the build:

  • 920490-1 (my new rule)
  • 920470-9 (your change from yesterday)
  • Syntax invalid: {'Error': u'Syntax error in line 137 col 42: Expected '"' at position rules/REQUEST-903.9006-XENFORO-EXCLUSION-RULES.conf:(137, 42) => '/index.php*?editor/to'.'}

We maybe overlook the latter one as it is a non-breaking error.

Rebasing should solve the first two. The third one is in your rules.

@lifeforms lifeforms self-assigned this May 10, 2019

@dune73 dune73 added the Needs action label Jun 3, 2019

@lifeforms

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

Fixed and ready for review!

@lifeforms lifeforms removed their assignment Jul 4, 2019

@lifeforms lifeforms removed the Needs action label Jul 4, 2019

@dune73

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

Thank you for the PR. This is very welcome. Merging this.

@dune73 dune73 merged commit b11fc80 into SpiderLabs:v3.2/dev Jul 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.