Filter secrets from stderr and redact them + Add log message for log collection #1563

chinmay-browserstack · 2024-04-01T05:37:23Z

This PR adds the support of sending build logs to API and include following changes.

Adding the warning that we are collecting logs and someone can opt using PERCY_CLIENT_ERROR_LOGS env variable.
Sending build logs to API in case of success as well as failure.
Separating out CI logs and CLI logs, CLI Logs will be collected everytime but CI logs will be collected based on PERCY_CLIENT_ERROR_LOGS.
Measuring asset-discovery time per snapshot.
Redaction of secrets from logs.

Amit3200 · 2024-04-03T09:47:46Z

packages/core/src/percy.js

+
+  sendBuildEvents() {
+    // Only send cli error when PERCY_CLIENT_ERROR_LOGS is set to true
+    if (process.env.PERCY_CLIENT_ERROR_LOGS !== 'false') {


PERCY_CLIENT_ERROR_LOGS This is for the CI errors of users?

Amit3200 · 2024-04-03T09:49:07Z

packages/core/src/percy.js

+          content: content
+        };
+        // console.log(content)
+        this.client.sendBuildEvents(this.build?.id, eventObject);


sendBuildEvents Not sure the function name of above function should change or this function should change in my opinion.

This will change post I implement logs controller

Amit3200 · 2024-04-03T09:49:33Z

packages/core/src/secretPatterns.yml

+  - pattern:
+      name: Bitcoin Address
+      regex: '^[13][a-km-zA-HJ-NP-Z0-9]{26,33}$'
+      confidence: high


endl is missing.

Amit3200 · 2024-04-03T09:51:25Z

packages/core/src/percy.js

+      // mark instance as stopped
+      this.readyState = 3;
+    } catch(err) {
+      this.log.error(err)


In discovery we are only collecting error. Shouldn't we capture the time taken and all for the same?

Adding timing for asset discovery using performance api

ninadbstack · 2024-04-03T11:46:33Z

packages/cli-exec/src/exec.js

@@ -85,6 +85,7 @@ export const exec = command('exec', {
  env.PERCY_BUILD_URL = percy?.build?.url;
  env.PERCY_LOGLEVEL = log.loglevel();

+  log.debug('Notice: Percy collects CI logs for service improvement, stored for 14 days. Opt-out anytime with export PERCY_CLIENT_ERROR_LOGS=false');


This should not be debug, we collect logs regardless.

ninadbstack · 2024-04-03T11:49:25Z

packages/core/src/utils.js

@@ -343,6 +347,23 @@ export async function withRetries(fn, { count, onRetry, signal, throwOn }) {
  }
 }

+export function redactSecrets(message) {


we are calling this function multiple times, it doesnt make sense to read patterns from file everytime

Also why is this function sync ? this will block execution of other parts of cli [ if file read is not part of it it can be sync ]

Now executing this method at very last so should be fine as till that time processing would be over.

ninadbstack · 2024-04-03T11:55:11Z

packages/cli-exec/src/exec.js

@@ -120,7 +120,9 @@ async function* spawn(cmd, args, percy) {

    if (proc.stderr) {
      proc.stderr.on('data', (data) => {
-        errorMessage += data.toString();
+        const message = redactSecrets(data.toString())


I would recommend to run this separately later before sending. Here the data comes in chunks so its not necessary that the chunk with succeed regex match when you have a secret split across multiple chunks. I am not sure if thats possible or not, but we need a confirmation.

ninadbstack · 2024-04-03T11:56:00Z

packages/core/src/percy.js

+
+  sendBuildEvents() {
+    // Only send cli error when PERCY_CLIENT_ERROR_LOGS is set to true
+    if (process.env.PERCY_CLIENT_ERROR_LOGS !== 'false') {


convert to a guard condition

ninadbstack · 2024-04-03T11:56:05Z

packages/core/src/percy.js

+    // Only send cli error when PERCY_CLIENT_ERROR_LOGS is set to true
+    if (process.env.PERCY_CLIENT_ERROR_LOGS !== 'false') {
+      // percy token is present
+      if (process.env.PERCY_TOKEN) {


ninadbstack · 2024-04-03T11:57:34Z

packages/logger/src/logger.js

@@ -46,6 +46,15 @@ export class PercyLogger {
    return this.level;
  }

+  // get all log messages stored for the session
+  messageDetail(message) {


why are we adding this ? we already have query function for the same. lets not repeat unless required.
Also this is not consistent with other functions. the message seems like a string arg but you seem to be expecting an object

ninadbstack · 2024-04-03T11:59:29Z

packages/core/src/percy.js

+    if (process.env.PERCY_CLIENT_ERROR_LOGS !== 'false') {
+      // percy token is present
+      if (process.env.PERCY_TOKEN) {
+        const logs = [...this.log.messageDetail()].map((item) => {return JSON.stringify(item)}).join('\n')


we should keep user ci messages and cli messages separate

ninadbstack · 2024-04-03T12:00:54Z

packages/core/src/percy.js

+          content: content
+        };
+        // console.log(content)
+        this.client.sendBuildEvents(this.build?.id, eventObject);


this is an async function, you are not awaiting it, events may not be sent in that case

ninadbstack · 2024-04-03T12:07:32Z

packages/core/src/secretPatterns.yml

Have we checked terms of the license for usage of this file ?

creative commons attribution share alike 4.0 license is there, I checked it allows to use it without any issues and can be modified as well

That requires attribution, make sure we have all necessary attribution steps followed.

prklm10 · 2024-04-12T05:39:04Z

packages/cli-exec/src/exec.js

-        errorMessage += data.toString();
+        const message = data.toString();
+        let entry = { message, timestamp: Date.now(), type: 'ci' };
+        percy.log.error(entry, null, true);


when percy is disabled then it will throw an error.
Add test case for this case as well.

prklm10 · 2024-04-12T05:48:07Z

packages/core/src/utils.js

+  }
+  return data;
+}
+


do we know the performance for this?
Can we check with the debug logs in percy-web buildkite build

We are not going to run on all CLI logs only CI error logs only. Although I have run in several time on 500KB file it is taking around 60ms

chinmay-browserstack requested a review from a team as a code owner April 1, 2024 05:37

chinmay-browserstack requested review from prklm10 and pankaj443 April 1, 2024 05:37

rishigupta1599 force-pushed the redact_secrets_logging branch from 9274db5 to f7aea7c Compare April 3, 2024 04:47

rishigupta1599 marked this pull request as draft April 3, 2024 05:18

Amit3200 reviewed Apr 3, 2024

View reviewed changes

ninadbstack reviewed Apr 3, 2024

View reviewed changes

rishigupta1599 force-pushed the redact_secrets_logging branch from 56774e1 to 3fdc517 Compare April 10, 2024 10:05

rishigupta1599 marked this pull request as ready for review April 12, 2024 05:33

prklm10 reviewed Apr 12, 2024

View reviewed changes

chinmay-browserstack and others added 8 commits April 12, 2024 20:59

Filter secerts and redact them + Add log message for log collection

0460e37

Add test improve coverage

cfc5502

redaction + log collection

3f65346

Refactoring code

4656fb5

removing extra method

73f78c9

Sending build logs to API

419ee6b

Lint fixes

468604e

Adding some specs

c96e8df

rishigupta1599 added 6 commits April 12, 2024 20:59

Fixing specs

bddfdf0

Fixing more specs

68b4489

Adding temp code to know spec issue

3d964e1

Adding timing.js and fixing specs

021940e

Updating reference_id pattern

e49de83

Resolving comments + semgrep ignore

8478005

rishigupta1599 force-pushed the redact_secrets_logging branch from 76543ab to 8478005 Compare April 12, 2024 15:30

rishigupta1599 added 2 commits April 12, 2024 21:10

Adding new line

fa15261

Adding attribution for secret-database

e0924bd

prklm10 approved these changes Apr 15, 2024

View reviewed changes

Amit3200 approved these changes Apr 17, 2024

View reviewed changes

rishigupta1599 merged commit ae0a1f8 into master Apr 17, 2024
36 checks passed

rishigupta1599 deleted the redact_secrets_logging branch April 17, 2024 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter secrets from stderr and redact them + Add log message for log collection #1563

Filter secrets from stderr and redact them + Add log message for log collection #1563

chinmay-browserstack commented Apr 1, 2024 •

edited by rishigupta1599

Amit3200 Apr 3, 2024

rishigupta1599 Apr 4, 2024

Amit3200 Apr 3, 2024

rishigupta1599 Apr 4, 2024

Amit3200 Apr 3, 2024

Amit3200 Apr 3, 2024

rishigupta1599 Apr 4, 2024

ninadbstack Apr 3, 2024

ninadbstack Apr 3, 2024

ninadbstack Apr 3, 2024 •

edited

rishigupta1599 Apr 4, 2024

ninadbstack Apr 3, 2024

ninadbstack Apr 3, 2024

ninadbstack Apr 3, 2024

ninadbstack Apr 3, 2024 •

edited

ninadbstack Apr 3, 2024

ninadbstack Apr 3, 2024 •

edited

ninadbstack Apr 3, 2024

rishigupta1599 Apr 4, 2024

ninadbstack Apr 4, 2024

prklm10 Apr 12, 2024

prklm10 Apr 12, 2024

rishigupta1599 Apr 15, 2024

Filter secrets from stderr and redact them + Add log message for log collection #1563

Filter secrets from stderr and redact them + Add log message for log collection #1563

Conversation

chinmay-browserstack commented Apr 1, 2024 • edited by rishigupta1599

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninadbstack Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninadbstack Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninadbstack Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmay-browserstack commented Apr 1, 2024 •

edited by rishigupta1599

ninadbstack Apr 3, 2024 •

edited

ninadbstack Apr 3, 2024 •

edited

ninadbstack Apr 3, 2024 •

edited