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.

Sign up for GitHub

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

Align "reference" field value in JSON transaction log with standard log. #3093

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

Align "reference" field value in JSON transaction log with standard log. #3093

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/transaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1806,7 +1806,7 @@ std::string Transaction::toJSON(int parts) {
strlen("details"));
yajl_gen_map_open(g);
LOGFY_ADD("match", a.m_match.c_str());
LOGFY_ADD("reference", a.m_reference.c_str());
LOGFY_ADD("reference", utils::string::limitTo(200, a.m_reference).c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the semantic (which might be ok) but it also might break automated parsing tools. I think when we cut of data, the remaining stuff should still be parsable (e.g. cut of in a way that all the remaining data is valid and follow the expected format)

Copy link
Member

Choose a reason for hiding this comment

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

I asked the example to check the behavior on mod_security2.

Unfortunately I can't find any documentation about the expected behavior, but as I know this is not a bug, but a feature - I mean the audit.log contains the whole log entry.

In fact, the limitTo() method is justified, because Nginx has a limit for the length of error.log line (NB: Apache has it too). But audit.log does not have any limit, so if we truncate the line in audit.log, we can lost relevant information.

In addition: I can imagine this PR can be a useful feature, but only with a configure option: if anyone wants to use this, then it can be optional in build time. Then we can be sure that user knows what he does.

Let me know how mod_security2 works.

LOGFY_ADD("ruleId", std::to_string(a.m_ruleId).c_str());
LOGFY_ADD("file", a.m_ruleFile->c_str());
LOGFY_ADD("lineNumber", std::to_string(a.m_ruleLine).c_str());
Expand Down
Loading