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

wordpress: gutenberg support #1298

Merged
merged 4 commits into from Feb 12, 2019

Conversation

Projects
None yet
4 participants
@siric1
Copy link
Contributor

siric1 commented Jan 29, 2019

This adds support for the new Gutenberg editor in Wordpress. #1232

@siric1 siric1 force-pushed the siric1:v3.2/dev branch from 1b205bb to 0ec79d3 Jan 29, 2019

@lifeforms lifeforms self-assigned this Jan 29, 2019

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jan 29, 2019

Thanks a lot for your contribution! I'll give it a spin ASAP :)

@siric1 siric1 force-pushed the siric1:v3.2/dev branch from 0ec79d3 to 380181c Jan 29, 2019

@siric1 siric1 force-pushed the siric1:v3.2/dev branch from 380181c to f86f2ad Jan 29, 2019

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Feb 7, 2019

Hi @siric1 , I've reviewed your PR, it was almost perfect so I made some very small improvements :)

  1. I've excluded all CRS rules from the content field, instead of just the rule IDs you mentioned. When posting free-form content to a CMS page, users will always trigger new rules. If we exclude the whole CRS by tag, we are done for the future.
  2. In ModSecurity 3.x, a parsed JSON value under key "content" will appear as ARGS:json.content. I'm assuming you are using ModSecurity 3. However, in ModSecurity 2.x, the content appears as ARGS:content without the json prefix. So, I've added the prefix-less variant as well, to support both ModSec versions.
  3. Made the regexp a little more efficient by removing the outside group, and using a non-capturing group.

This PR is ready to be merged AFAIK, however, I would appreciate a look from somebody else to point 2 in my message - Is this ModSec 2/3 discrepancy also your experience? Should we add to our contributions guide to include both variants with and without json.?

(Also, why are unrelated tests failing again? 😞)

@lifeforms lifeforms removed their assignment Feb 9, 2019

@siric1

This comment has been minimized.

Copy link
Contributor Author

siric1 commented Feb 12, 2019

Great, thanks @lifeforms. I'm indeed using modsec 3.x. Can someone using 2.x confirm that Gutenberg's post data appears as ARGS:content? It would be good to include in the docs if we want to maintain backwards compat.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Feb 12, 2019

Yes, I'm using it on 2.x fine with this adaptation @siric1, it's ready in my opinion. Thanks a lot! :)

The test failures on rule 933210 seem to be because our dev branche has already shifted in the meantime (rule 933210 is not yet in this PR), so I'm merging this. @csanders-git feel free to cherrypick this for 3.1.1 release.

@lifeforms lifeforms merged commit 804be9d into SpiderLabs:v3.2/dev Feb 12, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@theMiddleBlue

This comment has been minimized.

Copy link
Contributor

theMiddleBlue commented Feb 16, 2019

referring to #1309 it seems that the same kind of request could happen on index.php as querystring: POST /index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F50%2Fautosaves&_locale=user. Maybe REQUEST_URI should include both versions. What do you think about something like that? (need to be tested):

SecRule REQUEST_URI "@rx ^/wp/v[0-9]+/(?:posts|pages)" \
    "id: 9002140,\
    phase:1,\
    pass,\
    t:none,t:urlDecode,t:lowercase,t:normalizePath,\
    nolog,\
    ctl:ruleRemoveTargetByTag=CRS;ARGS:content,\
    ctl:ruleRemoveTargetByTag=CRS;ARGS:json.content"
@JeffCleverley

This comment has been minimized.

Copy link

JeffCleverley commented Feb 16, 2019

referring to #1309 it seems that the same kind of request could happen on index.php as querystring: POST /index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F50%2Fautosaves&_locale=user. Maybe REQUEST_URI should include both versions. What do you think about something like that? (need to be tested):

SecRule REQUEST_URI "@rx ^/wp/v[0-9]+/(?:posts|pages)" \
    "id: 9002140,\
    phase:1,\
    pass,\
    t:none,t:urlDecode,t:lowercase,t:normalizePath,\
    nolog,\
    ctl:ruleRemoveTargetByTag=CRS;ARGS:content,\
    ctl:ruleRemoveTargetByTag=CRS;ARGS:json.content"

I tried this with

ctl:ruleRemoveById=941180"

For my issue, but got no joy...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.