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

Fix for issue #193 #199

Closed
wants to merge 19 commits into from
Closed

Conversation

ShiMing-Q
Copy link
Contributor

@ShiMing-Q ShiMing-Q commented Mar 13, 2022

Thank you for contributing to Coraza WAF, your effort is greatly appreciated
Before submitting check if what you want to add to coraza list meets quality standards before sending pull request. Thanks!

Note: that go.mod and go.sum can only be modified for tested dependency updates or justified new features.

Make sure that you've checked the boxes below before you submit PR:

Thanks for your PR ❤️

@ShiMing-Q
Copy link
Contributor Author

@jptosso Please review this

@syinwu
Copy link
Member

syinwu commented Mar 15, 2022

@jptosso Please review this

Thank you for your contribution. Could you please fix the code to pass the github action?

@ShiMing-Q
Copy link
Contributor Author

@Bxlxx Why we got this for compatibility/test?

go: downloading go.uber.org/multierr v1.8.0
go get: github.com/jptosso/coraza-waf/[email protected]: parsing go.mod:
	module declares its path as: github.com/corazawaf/coraza/v2
	        but was required as: github.com/jptosso/coraza-waf/v2
Error: Process completed with exit code 1.

@syinwu
Copy link
Member

syinwu commented Mar 16, 2022

@Bxlxx Why we got this for compatibility/test?

go: downloading go.uber.org/multierr v1.8.0
go get: github.com/jptosso/coraza-waf/[email protected]: parsing go.mod:
	module declares its path as: github.com/corazawaf/coraza/v2
	        but was required as: github.com/jptosso/coraza-waf/v2
Error: Process completed with exit code 1.

I'm trying to fix the problem. Please wait a moment.

@ShiMing-Q
Copy link
Contributor Author

Not sure if it's due to this file, https://github.com/jptosso/coraza-pcre/blob/master/go.mod

# go get -u github.com/jptosso/coraza-pcre@1bea0f0
go: downloading github.com/jptosso/coraza-waf/v2 v2.0.0-rc.3
go: downloading go.uber.org/multierr v1.8.0
go get: github.com/jptosso/coraza-waf/[email protected]: parsing go.mod:
	module declares its path as: github.com/corazawaf/coraza/v2
	        but was required as: github.com/jptosso/coraza-waf/v2

@jptosso
Copy link
Member

jptosso commented Mar 16, 2022

Not sure if it's due to this file, https://github.com/jptosso/coraza-pcre/blob/master/go.mod

# go get -u github.com/jptosso/coraza-pcre@1bea0f0
go: downloading github.com/jptosso/coraza-waf/v2 v2.0.0-rc.3
go: downloading go.uber.org/multierr v1.8.0
go get: github.com/jptosso/coraza-waf/[email protected]: parsing go.mod:
	module declares its path as: github.com/corazawaf/coraza/v2
	        but was required as: github.com/jptosso/coraza-waf/v2

It seems you are right, that's because I have created a tag for the renamed project. I will fix it

@fzipi
Copy link
Member

fzipi commented Mar 17, 2022

What needs to be done here? Do we need to rebase also, right?

@jptosso
Copy link
Member

jptosso commented Mar 18, 2022

It is a valid solution and it's passing tests but I think we should discuss this one.

Maybe adding the parent ID as a parameter is too much and we could add the parent rule id to MatchedData

@ShiMing-Q
Copy link
Contributor Author

It is a valid solution and it's passing tests but I think we should discuss this one.

Maybe adding the parent ID as a parameter is too much and we could add the parent rule id to MatchedData

Sounds good, let me check the code again.

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.

Looking good now.

@jptosso
Copy link
Member

jptosso commented Mar 21, 2022

It seems it's not enough to fix the issue: #193 (comment) We will do some additional research on this one.

@fzipi
Copy link
Member

fzipi commented Apr 11, 2022

What is needed here now? Can we document what's the actual state?

@jptosso
Copy link
Member

jptosso commented Apr 11, 2022

So the issue reporter said it's wrong but we haven't received the detailed feedback. I will try to get it today.

@jptosso
Copy link
Member

jptosso commented Apr 11, 2022

Please review #216

@jptosso
Copy link
Member

jptosso commented Apr 14, 2022

Closed in favor of #220

Thank you Shiming, your PR was technically right but exporting new fields is a sensitive topic and we should avoid it if possible. Also tests were required.

@jptosso jptosso closed this Apr 14, 2022
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.

4 participants