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

AppSec: resist missing ruleset file #1948

Merged
merged 4 commits into from
Apr 5, 2022
Merged

Conversation

lloeki
Copy link
Contributor

@lloeki lloeki commented Mar 25, 2022

Adjusts the behaviour from raising an exception to outputting an error and allow the app to continue running. This also generalises the error management beyond a ruleset file being missing and also takes into account various invalid ruleset cases or libddwaf unavailability.

In these cases, in addition to the error, a warning is produced that alerts the user that AppSec is subsequently disabled.

@lloeki lloeki changed the title Appsec resist missing file AppSec: resist missing ruleset file Mar 25, 2022
@lloeki lloeki force-pushed the appsec-resist-missing-file branch 6 times, most recently from 2ec2f05 to 33b4196 Compare March 25, 2022 22:37
@lloeki lloeki marked this pull request as ready for review March 25, 2022 22:47
@lloeki lloeki requested a review from a team March 25, 2022 22:47
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Seems reasonable! Left a bunch of nitpicks, and in particular there seems to be a bunch of code on request_middleware.rb that can be removed, and I also think there's quite a bit of repetition on processor_spec.rb that would be nice to avoid.

lib/datadog/appsec/processor.rb Outdated Show resolved Hide resolved
lib/datadog/appsec/contrib/rack/request_middleware.rb Outdated Show resolved Hide resolved
lib/datadog/appsec/processor.rb Outdated Show resolved Hide resolved
lib/datadog/appsec/processor.rb Outdated Show resolved Hide resolved
spec/datadog/appsec/processor_spec.rb Outdated Show resolved Hide resolved
spec/datadog/appsec/processor_spec.rb Show resolved Hide resolved
lib/datadog/appsec/processor.rb Show resolved Hide resolved
spec/datadog/appsec/processor_spec.rb Show resolved Hide resolved
spec/datadog/appsec/processor_spec.rb Outdated Show resolved Hide resolved
@lloeki
Copy link
Contributor Author

lloeki commented Apr 4, 2022

Addressed some (though not all yet) of the review comments.

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

I gave it another pass. I think the specs can still be simplified / reduced duplication a bit, but otherwise I think this is in good shape so here sir take my approval 😄 .

lib/datadog/appsec/processor.rb Show resolved Hide resolved
Adjusts the behaviour from raising an exception to outputting an error
and allow the app to continue running. This also generalises the error
management beyond a ruleset file being missing and also takes into
account various invalid ruleset cases or libddwaf unavailability.

In these cases, in addition to the error, a warning is produced that
alerts the user that AppSec is subsequently disabled.
@lloeki lloeki force-pushed the appsec-resist-missing-file branch from 5e09e4f to 42a0752 Compare April 5, 2022 12:44
@lloeki lloeki merged commit 60b2447 into master Apr 5, 2022
@lloeki lloeki deleted the appsec-resist-missing-file branch April 5, 2022 14:08
@github-actions github-actions bot added this to the 1.0.0.beta2 milestone Apr 5, 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.

None yet

2 participants