Skip to content
This repository was archived by the owner on May 14, 2020. It is now read-only.

Conversation

@lifeforms
Copy link
Contributor

@lifeforms lifeforms commented Sep 15, 2017

We are devving on v3.1/dev, but we are missing a large number of commits which were made on the v3.0/dev branch in the meantime. For instance, the regression tests and also some rule fixes were in v3.0/dev. I need to add tests so I made a PR that merges all stragglers that remained on v3.0/dev in one go.

After this merge, we should be very careful to not accept a change to v3.0/dev again (unless it's a backported fix). I hope the bot or some Github setting can help us keep that rule.

Reviewers

I hope this can be integrated quickly. But it's a huge merge. I'm not a Git wizard and I'm notoriously bad at resolving conflicts, although I did my best.

Please look extra closely at the commits I marked as cp,conflict cause those involved manual work, or already-done as they seemed to be in v3.1 already so I skipped them.

Extra point for @csanders-git: Could you verify that the regressions code was ported correctly? Maybe you can do a diff on the whole directory, cause the files should be the same in v3.1/dev. In case of discrepancies or things that need to be changed for v3.1/dev, you should be able to edit this PR.

What I did

A git merge v3.0/dev failed catastrophically and gave an enormous list of conflicts in almost every source line, likely because of the indentation changes already done in v3.1/dev. (Should have done this before the indent changes, oh well.) It seemed enormously error-prone to go resolve all those conflicts manually or mass-ignore them. So I cherry picked commits from v3.0/dev to minimize and localize the conflict resolutions, at the cost of a bit messier Git history.

I used git log v3.0/dev ^v3.1/dev --no-merges --reverse to see which commits were supposedly only in v3.0/dev and not in v3.1/dev. Then I did git show to investigate the old commit and git cherry-pick -x to pick only the needed commits to v3.1/dev. It turns out that some of these commits were already there anyway, and some caused conflicts which I had to resolve.

My log (cp = cherry-picked, cp,conflict = cherry-picked and resolved conflicts, already-done = commit appeared already to be present in v3.1/dev, moot = commit is no longer necessary)

cp 62f96ef Fix for issue #789
cp,conflict 0679184 Fix minor bug in the CHANGES from 3.0.2 push
cp 5056885 Updating to reflect OWASP Status
already-done 3eb61e2 Apply transformations to the correct rule
already-done 2e7c132 Add missing msg
moot cf03da2 Update to CONTRIBUTORS file
already-done d444d64 Updating libinjection rule to remove use of capture which isn't supported
cp 0b8ab44 Adding Docker support for CRS
cp a4998ff initial travis deployment
cp 2e6575a inital commit of regression tests into main branch
cp 5f61b4b Getting rid of weird formatting
cp 78e90af updating FTW link
cp efe9d44 changing up da name
cp d8442ca Change default to look for linux
cp,conflict f1616dc inital commit of regression tests into main branch
cp cbfc1a1 Set ruledir to correct location
cp,conflict e3c6f85 figure out where this settings.ini file is
cp 6b55d51 Fix spelling
cp 4b8c060 Use config.py
cp 5856c18 Add init and config
cp 8cd8103 Delete settings
cp 7e894fa Adding local mountpoint for travis.
cp 8bda0ca updating logging location so that docker works!
already-done bb310ea Spaces before continuation
cp e14d747 Adding support for paranoia mode to docker
cp 249878a adding entrypoint so user can pass paranoia
cp b2ec5ca Updated README and travis to reflect user entrypoint usage
cp 07bcc8f fixing whitespace issue and repo
cp 05d139e minor readme update to test travis
cp 31de142 Updating Travis with support for new types of tests
cp 6730214 adding README update to see travis run
cp 91ccb12 Updating regression tests because some rules aren't fireable ... yet
cp 9c78843 Updating Travis for new tests
cp 855d292 remove 921170 because it won't ever fire
cp,conflict ed9f133 Updating minor incorrectness in asp.net regex
cp 8c62463 Updating tests for session fixation so they pass
cp,conflict 401561e Updating XSS rules, travis, and updating 920430 because it'll never fire in phase 2
cp 6d0b0ab Updating quotes to fix 941190
cp 779dca3 Fixing FN check in 941160
cp 5f9eeb2 Add notification for builds against #modsecurity on freenode
cp a081e06 (origin/contributors) CONTRIBUTORS: add all past code contributors, convert to markdown
cp,conflict 4a2cc78 (origin/upload-phps) Block uploads of files with .phps extension
already-done 528b84d Update README.md
already-done 1944343 Fixing whitespace according to standard
already-done 7794a02 Fixing whitespace for checks to pass
already-done c967c4f Changing indentation for each rule in the chain
already-done 8387712 fix more different spacing
already-done cd9c4b0 Last erasing of 8 spaces in rules
already-done bd97f82 Increasing range header
already-done b399727 update known_bugs
already-done b5a6541 Update KNOWN_BUGS
already-done 9bdf686 Update KNOWN_BUGS

skidoosh and others added 30 commits September 15, 2017 21:38
(cherry picked from commit 62f96ef)
(cherry picked from commit 5056885)
(cherry picked from commit 0b8ab44)
(cherry picked from commit a4998ff)
(cherry picked from commit 5f61b4b)
(cherry picked from commit 78e90af)
(cherry picked from commit efe9d44)
this is what docker uses

(cherry picked from commit d8442ca)
(cherry picked from commit cbfc1a1)
(cherry picked from commit 6b55d51)
(cherry picked from commit 4b8c060)
(cherry picked from commit 5856c18)
(cherry picked from commit 8cd8103)
(cherry picked from commit 7e894fa)
(cherry picked from commit 07bcc8f)
(cherry picked from commit 05d139e)
(cherry picked from commit 9c78843)
@dune73
Copy link
Contributor

dune73 commented Sep 21, 2017

Wow. Thank you for this huge change set @lifeforms. You warned us of this growing gap and I am very happy you not only warned us, but you also stepped in to fix it for us.

The change set was a bit less terrifying once I realized that most of it is regression testing.

I have looked over the whole change set and then did a 2nd round looking at the conflicts. I am not totally sure I get it right, but I see diffs at the following two commits:

cp,conflict e3c6f85 figure out where this settings.ini file is
cp,conflict 4a2cc78 (origin/upload-phps) Block uploads of files with .phps extension

Everything else looks good to me.

@lifeforms
Copy link
Contributor Author

Lol, v3.1/dev has now drifted already and it's again conflicted ;(

e3c6f85: Had to adjust here for code that was also changed in the other branch, I think I got it right.

4a2cc78: To me, 496d313 and 4a2cc78 seem the same though?

@dune73 dune73 self-assigned this Oct 2, 2017
@fzipi fzipi mentioned this pull request Oct 3, 2017
@fzipi
Copy link
Contributor

fzipi commented Oct 3, 2017

Can these conflicts be solved and incorporated to this PR?

@dune73
Copy link
Contributor

dune73 commented Oct 13, 2017

I agree on the identical nature of the commits mentioned above.

I am open to merge this at last, but the conflicts should be resolved first. Or should I try and resolve that?

@dune73
Copy link
Contributor

dune73 commented Oct 20, 2017

Fixing the conflicts; directly in the web editor.

.travis.yml

trivial

rules/REQUEST-933-APPLICATION-ATTACK-PHP.conf

<<<<<<< merge-v3.0-commits
SecRule FILES|REQUEST_HEADERS:X-Filename|REQUEST_HEADERS:X_Filename|REQUEST_HEADERS:X-File-Name "@rx .*\.(?:php\d*|phps|phtml)\.*$" \
    "msg:'PHP Injection Attack: PHP Script File Upload Found',\
=======
SecRule FILES|REQUEST_HEADERS:X-Filename|REQUEST_HEADERS:X_Filename|REQUEST_HEADERS:X-File-Name "@rx .*\.(?:php\d*|phtml)\.*$" \
    "id:933110,\
>>>>>>> v3.1/dev

This is a change (phps) that is neither in v3.0/master nor in v3.0/dev and also not in v3.1/dev.
Removing it. Please issue a new PR for this individually if this really should go in, which is probably a good idea.

rules/REQUEST-943-APPLICATION-ATTACK-SESSION-FIXATION.conf

Escaping a dot.

@dune73
Copy link
Contributor

dune73 commented Oct 20, 2017

OK. I think we are ready to merge. Conflicts resolved. I do not think any additional drift in v3.1/dev matters much as there are no more conflicts. If I am misunderstanding this, then another PR should fix it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants