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

feat: add base64 encode transformation #1257

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tty2
Copy link

@tty2 tty2 commented Dec 19, 2024

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!

This PR brings base64Encode method.
Issue 1252

This changes rely on encoding/base std library base64.StdEncoding.EncodeToString
I spent a little bit of time to investigate if it's necessary to implement custom base64Encode. If you have any doubts, real cases where std doesn't work or not appropriate for this project, please let me know here.

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

  • My code includes positive and negative tests.
    As long as this implementation is technically a one liner that relies on std lib there is no real reason make unit test for it, otherwise we'll test std lib.
    Plus as I can see there is a ready test data for base64encode in transformation/testdata/base64encode.json which is used in TestTransformations.
    Plus I see there is no test.go files for the other one liners (ex. hexEncode), as I understood just to avoid bloating the codebase.
  • I have an appropriate description with correct grammar.
    This code follows the code style in the project: has a specific signature with 3 returning parameters (string, bool, error) as I understand for the future flexibility and compatibility.
  • I have read Contribution guidelines, maintainers note and Quality standard.
  • My code is properly linted and passes pre-commit tests.
    I ran CGO_ENABLED=1 go run mage.go check on my local machine and didn't get any fail.

Thanks for your contribution ❤️

@tty2 tty2 requested a review from a team as a code owner December 19, 2024 10:33
@tty2 tty2 force-pushed the transfomations_base64Encode branch from 2453eec to c248752 Compare December 19, 2024 10:35
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.

Can you please add tests?

Take a look at the original tests and try to replicate those :)

@fzipi fzipi changed the title Transfomations base64 encode feat: add base64 encode transformation Dec 19, 2024
@tty2
Copy link
Author

tty2 commented Dec 19, 2024

@fzipi

Can you please add tests?

Take a look at the original tests and try to replicate those :)

Could you clarify please what should be done?
I currently run tests and these cases run as well.
Quick prove with println in a screenshot
Screenshot From 2024-12-19 14-59-47

Do you mean add test cases to this json?

@fzipi
Copy link
Member

fzipi commented Dec 19, 2024

Ah, good catch! They are already there! Don't need to add them 😄

@fzipi fzipi self-requested a review December 19, 2024 14:39
fzipi
fzipi previously approved these changes Dec 19, 2024
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.

LGTM! Thanks for your contribution!

@fzipi fzipi self-requested a review December 19, 2024 14:41
@fzipi
Copy link
Member

fzipi commented Dec 19, 2024

I wonder why tests didn't run :/

@fzipi
Copy link
Member

fzipi commented Dec 19, 2024

Ok, I forced a small update to the date to force running local tests :)

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.69%. Comparing base (dcdbc28) to head (fdaf31f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1257   +/-   ##
=======================================
  Coverage   81.68%   81.69%           
=======================================
  Files         168      169    +1     
  Lines        9762     9767    +5     
=======================================
+ Hits         7974     7979    +5     
  Misses       1537     1537           
  Partials      251      251           
Flag Coverage Δ
coraza.rule.case_sensitive_args_keys 81.65% <100.00%> (+<0.01%) ⬆️
coraza.rule.multiphase_valuation 81.69% <100.00%> (+<0.01%) ⬆️
coraza.rule.no_regex_multiline 81.63% <100.00%> (+<0.01%) ⬆️
default 81.69% <100.00%> (+<0.01%) ⬆️
examples+ 16.55% <20.00%> (+<0.01%) ⬆️
examples+coraza.rule.case_sensitive_args_keys 81.65% <100.00%> (+<0.01%) ⬆️
examples+coraza.rule.multiphase_valuation 81.52% <100.00%> (+<0.01%) ⬆️
examples+coraza.rule.no_regex_multiline 81.55% <100.00%> (+<0.01%) ⬆️
examples+memoize_builders 81.65% <100.00%> (+<0.01%) ⬆️
examples+no_fs_access 80.97% <100.00%> (+<0.01%) ⬆️
ftw 81.69% <100.00%> (+<0.01%) ⬆️
memoize_builders 81.78% <100.00%> (+<0.01%) ⬆️
no_fs_access 81.14% <100.00%> (+<0.01%) ⬆️
tinygo 81.66% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

2 participants