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

Binary output and Request filtering #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pabloko
Copy link

@pabloko pabloko commented Jul 27, 2017

Here you go, #18
Cleaned a bit and added settings for binary dumping and filtering urls.

Copy link
Owner

@danghvu danghvu left a comment

Choose a reason for hiding this comment

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

Some comments to start with

@@ -42,7 +42,8 @@ static void dumpit(request_rec *r, apr_bucket *b, char *buf, apr_size_t *current
if (nbytes) {
DEBUG(r, "%ld bytes read from bucket for request %s", nbytes, r->the_request);
nbytes = min(nbytes, cfg->max_size - *current_size);
strncpy(buf, ibuf, nbytes);
//strncpy(buf, ibuf, nbytes);
Copy link
Owner

Choose a reason for hiding this comment

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

remove this commented out code, we know from the history.

@@ -75,23 +87,58 @@ apr_status_t logit(ap_filter_t *f) {
apr_ctime(time, r->request_time);

// condition taken from mod_security
#if AP_SERVER_MAJORVERSION_NUMBER > 1 && AP_SERVER_MINORVERSION_NUMBER > 2
#if AP_SERVER_MAJORVERSION_NUMBER > 1 && AP_SERVER_MINORVERSION_NUMBER > 2
Copy link
Owner

Choose a reason for hiding this comment

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

Why changing the alignment here ? and below..


//Test buffer to search non ASCII characters
int not_bin=1;
for (int i = 0; i < state->log_size; i++)
Copy link
Owner

Choose a reason for hiding this comment

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

can you align similar to other code ? 2 spaces, and {
} style ?

if (state->buffer[i]<0x20 || state->buffer[i]>0x7E) {
if (state->buffer[i]=='\n') continue;
if (state->buffer[i]=='\r') continue;
not_bin=0;
Copy link
Owner

Choose a reason for hiding this comment

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

So "not_bin" == 0 mean it is binary ? this is a weird name, how about initialize it at 0 and not_bin = 1 here then ?
But honestly, I think you should drop this check -- if the user specify "I want to get hex" then we should just hex it, the data doesn't matter..

@@ -102,9 +149,33 @@ apr_status_t dumpost_input_filter (ap_filter_t *f, apr_bucket_brigade *bb,
(dumpost_cfg_t *) ap_get_module_config(f->r->per_dir_config, &dumpost_module);

apr_bucket *b;
apr_status_t ret;
/* restoring state */
apr_status_t ret;
Copy link
Owner

Choose a reason for hiding this comment

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

align properly this and below.

@@ -193,6 +264,8 @@ static void *dumpost_create_dconfig(apr_pool_t *mp, char *path) {
cfg->headers = apr_array_make(mp, 0, sizeof(char *));
cfg->pool = mp;
cfg->file = 0;
cfg->log_bin = 0;
cfg->filters = apr_array_make(mp, 0, sizeof(char *));
Copy link
Owner

Choose a reason for hiding this comment

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

alignment

@pabloko
Copy link
Author

pabloko commented Jul 28, 2017

Sorry about alignment stuff maybe i have some misconfiguration because alignment is good on my editor (sublime text)

About the binary dumping, in my case i prefer to display ascii ones plain and the binary ones in hex string, because in my workflow i have some plain data requests before getting a binary blob with data, for me is easier to identify this data quickly instead having all the log in hex and having to decode all of them...

Dont get it wrong DumpPostLogBinary isnt about hex the data, its just to log or not binary blobs that cant be logged plain to prevent data loss. I could also add a config value to hex everything, that could make sense...

As you can see im doing development around very specific tasks, and adapting the module to what i need, so fell free to close this PR and use the chunks of my commit if you want on future updates. Note that i didn't properly tested this yet.

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