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

feat(core): incorporate secret-scrubber into core #393

Merged
merged 2 commits into from
Jul 1, 2021
Merged

feat(core): incorporate secret-scrubber into core #393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9ecba7
incorporate secret-scrubber into core
xavdid Jun 29, 2021
0d889f0
only scrub data when we have secrets to scrub
xavdid Jun 29, 2021
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
1 change: 1 addition & 0 deletions packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
},
"engineStrict": true,
"dependencies": {
"@zapier/secret-scrubber": "^1.0.1",
"bluebird": "3.7.2",
"content-disposition": "0.5.3",
"dotenv": "9.0.2",
Expand Down
12 changes: 0 additions & 12 deletions packages/core/src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,6 @@ const REQUEST_OBJECT_SHORTHAND_OPTIONS = { isShorthand: true, replace: true };
const DEFAULT_LOGGING_HTTP_ENDPOINT = 'https://httplogger.zapier.com/input';
const DEFAULT_LOGGING_HTTP_API_KEY = 'R24hzu86v3jntwtX2DtYECeWAB'; // It's ok, this isn't PROD

const SENSITIVE_KEYS = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this logic was moved to the package

'api_key',
'apikey',
'auth',
'passwd',
'password',
'secret',
'signature',
'token',
];

const SAFE_LOG_KEYS = [
'account_id',
'api_title',
Expand Down Expand Up @@ -76,6 +65,5 @@ module.exports = {
REQUEST_OBJECT_SHORTHAND_OPTIONS,
RESPONSE_SIZE_LIMIT,
SAFE_LOG_KEYS,
SENSITIVE_KEYS,
STATUSES,
};
100 changes: 32 additions & 68 deletions packages/core/src/tools/create-logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,29 @@
const _ = require('lodash');

const request = require('./request-client-internal');
const cleaner = require('./cleaner');
const dataTools = require('./data');
const hashing = require('./hashing');
const { simpleTruncate, recurseReplace } = require('./data');
const ZapierPromise = require('./promise');
const {
DEFAULT_LOGGING_HTTP_API_KEY,
DEFAULT_LOGGING_HTTP_ENDPOINT,
SAFE_LOG_KEYS,
SENSITIVE_KEYS,
} = require('../constants');
const { unheader } = require('./http');
const { scrub, findSensitiveValues } = require('@zapier/secret-scrubber');
// not really a public function, but it came from here originally
const { isUrlWithSecrets } = require('@zapier/secret-scrubber/lib/convenience');

const truncate = (str) => dataTools.simpleTruncate(str, 3500, ' [...]');
const isUrl = (url) => {
try {
// eslint-disable-next-line no-new
new URL(url);
return true;
} catch (e) {
return false;
}
};

const truncate = (str) => simpleTruncate(str, 3500, ' [...]');

const formatHeaders = (headers = {}) => {
if (_.isEmpty(headers)) {
Expand Down Expand Up @@ -89,66 +99,19 @@ const toStdout = (event, msg, data) => {
}
};

const isSafeUrl = (value) => {
let url;
try {
url = new URL(value);
} catch (err) {
return false;
}
return !(url.username || url.password || url.search);
};

const makeSensitiveBank = (event, data) => {
const buildSensitiveValues = (event, data) => {
const bundle = event.bundle || {};

const bank = { ...bundle.authData, ...process.env };
const sensitiveValues = Object.entries(bank).reduce(
(values, [key, value]) => {
if (SENSITIVE_KEYS.includes(key) || !isSafeUrl(value)) {
return [...values, value];
}
// Allow uncensored if the value is a safe URL and the key is not in
// SENSITIVE_KEYS
return [...values];
},
[]
);

const matcher = (key, value) => {
if (_.isString(value)) {
const lowerKey = key.toLowerCase();
return _.some(SENSITIVE_KEYS, (k) => lowerKey.indexOf(k) >= 0);
}
return false;
};

dataTools.recurseExtract(data, matcher).forEach((value) => {
sensitiveValues.push(value);
});

return _.reduce(
sensitiveValues,
(bank, val) => {
// keeps short values from spamming censor strings in logs, < 6 chars is not a proper secret
// see https://github.com/zapier/zapier-platform-core/issues/4#issuecomment-277855071
if (val && String(val).length > 5) {
const censored = hashing.snipify(val);
bank[val] = censored;
bank[encodeURIComponent(val)] = censored;
try {
bank[Buffer.from(String(val)).toString('base64')] = censored;
} catch (e) {
if (e.name !== 'TypeError') {
throw e;
}
// ignore; Buffer is semi-selective about what types it takes
}
}
return bank;
},
{}
const authData = Object.values(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 = authData.filter(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic keeps "safe urls" around. there's code in scrubber to "intelligently" pull sensitive data out of urls which we could use here too, but we're also fine to censor all non-safe urls.

If we did that, it would pull my-secret out of https://www.example.com/whatever?token=my-secret&foo=bar. Then instead of censoring the whole url, it would be https://www.example.com/whatever?token=:censored:3:asdf:&foo=bar. But, it's also fine as is.

(item) => !isUrl(item) || isUrlWithSecrets(item)
);
return [
...sensitiveAuthData,
...findSensitiveValues(process.env),
...findSensitiveValues(data),
];
};

const sendLog = (options, event, message, data) => {
Expand All @@ -158,15 +121,16 @@ const sendLog = (options, event, message, data) => {
data.request_headers = unheader(data.request_headers);
data.response_headers = unheader(data.response_headers);

const sensitiveBank = makeSensitiveBank(event, data);
const sensitiveValues = buildSensitiveValues(event, data);
// scrub throws an error if there are no secrets
const safeMessage = truncate(
cleaner.recurseReplaceBank(message, sensitiveBank)
sensitiveValues.length ? scrub(message, sensitiveValues) : message
);
const safeData = dataTools.recurseReplace(
cleaner.recurseReplaceBank(data, sensitiveBank),
const safeData = recurseReplace(
sensitiveValues.length ? scrub(data, sensitiveValues) : data,
truncate
);
const unsafeData = dataTools.recurseReplace(data, truncate);
const unsafeData = recurseReplace(data, truncate);

// Keep safe log keys uncensored
Object.keys(safeData).forEach((key) => {
Expand Down
17 changes: 0 additions & 17 deletions packages/core/src/tools/data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,22 +123,6 @@ const recurseReplace = (obj, replacer, options = {}) => {
return obj;
};

// Recursively extract values from a nested object based on the matcher function.
const recurseExtract = (obj, matcher) => {
const values = [];
Object.keys(obj).forEach((key) => {
const value = obj[key];
if (matcher(key, value)) {
values.push(value);
} else if (isPlainObj(value)) {
recurseExtract(value, matcher).forEach((v) => {
values.push(v);
});
}
});
return values;
};

const _IGNORE = {};

// Flatten a nested object.
Expand Down Expand Up @@ -200,7 +184,6 @@ module.exports = {
isPlainObj,
jsonCopy,
memoizedFindMapDeep,
recurseExtract,
recurseReplace,
simpleTruncate,
};
40 changes: 20 additions & 20 deletions packages/core/test/logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ describe('logger', () => {
token: options.token,
message: 'test',
data: {
password: ':censored:6:a5023f748d:',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests are all the same, but I changed the default salt for the censor function, so the hashes had to be updated.

password: ':censored:6:57a71b6062:',
log_type: 'console',
key: ':censored:6:8f63f9ff57:',
key: ':censored:6:e3b0ee5182:',
},
});
});
Expand All @@ -101,10 +101,10 @@ describe('logger', () => {
response.status.should.eql(200);
const j = response.content.json;
j.data.request_headers.should.eql(
'authorization: basic :censored:10:d98440830f:'
'authorization: basic :censored:10:cf265ec679:'
);
j.data.response_headers.should.eql(
'Authorization: :censored:30:f914b1b0d1:'
'Authorization: :censored:30:2a1f21f809:'
);
}
);
Expand Down Expand Up @@ -135,11 +135,11 @@ describe('logger', () => {
response.status.should.eql(200);
const j = response.content.json;
j.data.request_headers.should.eql(
'authorization: basic :censored:10:d98440830f:'
'authorization: basic :censored:10:cf265ec679:'
);
// Headers class downcases everything
j.data.response_headers.should.eql(
'authorization: :censored:30:f914b1b0d1:'
'authorization: :censored:30:2a1f21f809:'
);
}
);
Expand Down Expand Up @@ -198,10 +198,10 @@ describe('logger', () => {
message: 'test',
data: {
response_content: `{
"something": ":censored:6:a5023f748d:",
"somethingElse": ":censored:6:8f63f9ff57:",
"something": ":censored:6:57a71b6062:",
"somethingElse": ":censored:6:e3b0ee5182:",
}`,
request_url: 'https://test.com/?api_key=:censored:8:f274744218:',
request_url: 'https://test.com/?api_key=:censored:8:89250e9365:',
log_type: 'console',
},
});
Expand Down Expand Up @@ -236,13 +236,13 @@ describe('logger', () => {
message: 'test',
data: {
response_json: {
access_token: ':censored:12:8e4a58294b:',
PASSWORD: ':censored:10:b0c55acfea:',
access_token: ':censored:12:94a59e640f:',
PASSWORD: ':censored:10:0c2fe1350e:',
name: 'not so secret',
},
response_content: `{
"access_token": ":censored:12:8e4a58294b:",
"PASSWORD": ":censored:10:b0c55acfea:",
"access_token": ":censored:12:94a59e640f:",
"PASSWORD": ":censored:10:0c2fe1350e:",
"name": "not so secret"
}`,
log_type: 'console',
Expand Down Expand Up @@ -275,10 +275,10 @@ describe('logger', () => {
message: 'test',
data: {
response_json: {
hello: ':censored:9:9cb84e8ccc:',
hello: ':censored:9:0caea7fafe:',
},
response_content: `{
"hello": :censored:9:9cb84e8ccc:
"hello": :censored:9:0caea7fafe:
}`,
log_type: 'console',
},
Expand Down Expand Up @@ -345,9 +345,9 @@ describe('logger', () => {
token: options.token,
message: 'test',
data: {
password: ':censored:6:a5023f748d:',
password: ':censored:6:57a71b6062:',
log_type: 'console',
key: ':censored:9:699f352527:',
key: ':censored:9:f0d5b7b789:',
customuser_id: logExtra.customuser_id,
},
});
Expand All @@ -374,12 +374,12 @@ describe('logger', () => {
message: '200 GET https://example.com/test',
data: {
log_type: 'console',
password: ':censored:31:68af4cbe15:',
password: ':censored:31:0dbc81268a:',
// Only safe_url (no basic auth or query params) should be left
// uncensored
safe_url: 'https://example.com',
basic_auth_url: ':censored:27:5eaaee9fc2:',
param_url: ':censored:27:c1e27a03b8:',
basic_auth_url: ':censored:27:bad5875ee0:',
param_url: ':censored:27:9d59e27abe:',
},
});
});
Expand Down
24 changes: 0 additions & 24 deletions packages/core/test/misc-tools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,30 +219,6 @@ describe('Tools', () => {
});
});

describe('recurseExtract', () => {
it('should extract values that match', () => {
const obj = {
secret_key: '1234',
another: {
secret_key: '5678',
deep: {
secret: 'abcd',
},
},
name: 'dont_care',
id: 88,
};
const matcher = (key, value) => {
if (typeof value === 'string') {
return key.indexOf('secret') >= 0;
}
return false;
};
const values = dataTools.recurseExtract(obj, matcher);
values.should.eql(['1234', '5678', 'abcd']);
});
});

describe('recurseCleanFuncs', () => {
it('should handle objects, arrays and function->str', () => {
const output = cleaner.recurseCleanFuncs({
Expand Down
7 changes: 7 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1589,6 +1589,13 @@
resolved "https://registry.yarnpkg.com/@ungap/promise-all-settled/-/promise-all-settled-1.1.2.tgz#aa58042711d6e3275dd37dc597e5d31e8c290a44"
integrity sha512-sL/cEvJWAnClXw0wHk85/2L0G6Sj8UB0Ctc1TEMbKSsmpRosqhwj9gWgFRZSrBr2f9tiXISwNhCPmlfqUqyb9Q==

"@zapier/secret-scrubber@^1.0.1":
version "1.0.1"
resolved "https://registry.yarnpkg.com/@zapier/secret-scrubber/-/secret-scrubber-1.0.1.tgz#201491301c2d096ee9989ccbe1bbed47a7a86b79"
integrity sha512-9SfiIOUjEfNFNpIGsYHPbzMmkytWLQm1qD7Y4niRVEfL8L9wVaIlWWY6VcHm2T0XaL/97a9oj7vOV/eWB2yfrA==
dependencies:
lodash.isplainobject "^4.0.6"

JSONStream@^1.0.3, JSONStream@^1.0.4:
version "1.3.5"
resolved "https://registry.yarnpkg.com/JSONStream/-/JSONStream-1.3.5.tgz#3208c1f08d3a4d99261ab64f92302bc15e111ca0"
Expand Down