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(core): log censoring is slow if response has many URLs #524

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 15 additions & 3 deletions packages/core/src/tools/create-logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ const {
findSensitiveValues,
recurseExtract,
} = require('@zapier/secret-scrubber');
// not really a public function, but it came from here originally
const { isUrlWithSecrets } = require('@zapier/secret-scrubber/lib/convenience');
// not really public functions, but they came from here originally
const {
isSensitiveKey,
isUrlWithSecrets,
} = require('@zapier/secret-scrubber/lib/convenience');

// The payload size per request to stream logs. This should be slighly lower
// than the limit (16 MB) on the server side.
Expand Down Expand Up @@ -111,6 +114,7 @@ const toStdout = (event, msg, data) => {
const buildSensitiveValues = (event, data) => {
const bundle = event.bundle || {};
const authData = bundle.authData || {};

// for the most part, we should censor all the values from authData
// the exception is safe urls, which should be filtered out - we want those to be logged
const sensitiveAuthData = recurseExtract(authData, (key, value) => {
Expand All @@ -119,10 +123,18 @@ const buildSensitiveValues = (event, data) => {
}
return true;
});

// For log extra data, we only want to censor with sensitive keys. URLs in log
// extra should be safe.
const sensitiveLogData = recurseExtract(
data,
(key, value) => _.isString(value) && isSensitiveKey(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

sensitive key -> numeric value should also be caught. eg {secretNumber: 123456789}

);

return [
...sensitiveAuthData,
...sensitiveLogData,
...findSensitiveValues(process.env),
...findSensitiveValues(data),
];
};

Expand Down
52 changes: 52 additions & 0 deletions packages/core/test/logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,58 @@ describe('logger', () => {
]);
});

it('should not replace urls from log extra', async () => {
const bundle = {
authData: {
password: 'hunter2',
},
};
const logger = createlogger({ bundle }, options);

const data = {
input: {
response: {
json: {
data: [
// An app could have thousands of URLs in a response. We don't
// want these to be in the sensitive vallues and make secret
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo vallues

// scrubbing too slow.
'https://example.com/?q=1',
'https://example.com/?q=2',
'https://example.com/?q=3',
],
token: 'something',
},
},
},
};

logger('Called something by hunter2', data);
const response = await logger.end();
response.status.should.eql(200);
response.content.token.should.eql(options.token);
response.content.logs.should.deepEqual([
{
message: 'Called :censored:9:a468e1d9fc: by :censored:7:850233b460:',
data: {
log_type: 'console',
input: {
response: {
json: {
data: [
'https://example.com/?q=1',
'https://example.com/?q=2',
'https://example.com/?q=3',
],
token: ':censored:9:a468e1d9fc:',
},
},
},
},
},
]);
});

it('should handle nullish values', async () => {
const bundle = {
authData: {
Expand Down