Skip to content

Do not stringify log messages in the browser console #40265

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

Merged
gzdunek merged 7 commits into from
Apr 9, 2024
Merged

Do not stringify log messages in the browser console #40265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3cb69a2
Extract current console transport to a separate function
gzdunek Apr 5, 2024
3e304f6
Add separate transport for the browser console
gzdunek Apr 5, 2024
0de5b88
Check exact key names when filtering out sensitive properties
gzdunek Apr 5, 2024
e6c427b
Remove `passw`
gzdunek Apr 8, 2024
e0d0faf
Use `getLoggerName()` in `getRegularConsoleTransport()`
gzdunek Apr 8, 2024
dbdf75f
Improve comment
gzdunek Apr 8, 2024
8e32f19
Fix tests
gzdunek Apr 8, 2024
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
67 changes: 51 additions & 16 deletions web/packages/teleterm/src/services/logger/loggerService.ts
View file
Open in desktop
Copy link
Copy Markdown
Member

@ravicious ravicious Apr 8, 2024

Choose a reason for hiding this comment

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

The only thing that worries me is that there's no way to expand all collapsed objects at once. This means that:

  1. You won't see the output as it comes, e.g. when the app continuously receives some kind of messages from tshd.
  2. You won't be able to filter by parts of the output that have been collapsed.

The workaround for both of those things would be to simply look at the log file instead. Also, you can use Opt + click to recursively expand any given object, but that still works on only a single object.

I think we should be able to live with that, what do you think?

Copy link
Copy Markdown
Contributor Author

@gzdunek gzdunek Apr 8, 2024

Choose a reason for hiding this comment

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

  1. Correct, but if the response is more complex, looking for a particular part of it is hard when the message is unformatted.
  2. I think I've never had a need to look for something in more than one response, so expanding it wouldn't be too much work.

Overall, your points are valid, but the pros still outweigh the cons to me.
You also provided a good workaround, so I think we don't loose anything here.

Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import split2 from 'split2';
import { Logger, LoggerService, NodeLoggerService } from './types';
import { KeepLastChunks } from './keepLastChunks';

import type { Logform } from 'winston';

import type { ChildProcess } from 'node:child_process';

/**
Expand Down Expand Up @@ -95,22 +97,12 @@ export function createFileLoggerService(
});

if (opts.dev) {
instance.add(
new transports.Console({
format: format.printf(({ level, message, context }) => {
const loggerName =
opts.loggerNameColor &&
`\x1b[${opts.loggerNameColor}m${opts.name.toUpperCase()}\x1b[0m`;

const text = stringifier(message as unknown as unknown[]);
const logMessage = opts.passThroughMode
? text
: `[${context}] ${level}: ${text}`;

return [loggerName, logMessage].filter(Boolean).join(' ');
}),
})
);
// Browser environment.
if (typeof window !== 'undefined') {
instance.add(getBrowserConsoleTransport(opts));
} else {
instance.add(getRegularConsoleTransport(opts));
}
}

return {
Expand Down Expand Up @@ -217,3 +209,46 @@ type FileLoggerOptions = {
* */
omitTimestamp?: boolean;
};

/** Does not stringify messages and logs directly using `console.*` functions. */
function getBrowserConsoleTransport(opts: FileLoggerOptions) {
Comment thread
gzdunek marked this conversation as resolved.
return new transports.Console({
log({ level, message, context }: Logform.TransformableInfo, next) {
const loggerName = getLoggerName(opts);

const logMessage = opts.passThroughMode
? message
: [`[${context}] ${level}:`, ...message];

const toLog = [loggerName, logMessage].filter(Boolean).flat();
// We allow level to be only info, warn and error (createLoggerFromWinston).
console[level](...toLog);
next();
},
});
}

/** Stringifies log messages and logs with winston's console transport. */
function getRegularConsoleTransport(opts: FileLoggerOptions) {
return new transports.Console({
format: format.printf(({ level, message, context }) => {
const loggerName = getLoggerName(opts);

const text = stringifier(message as unknown as unknown[]);
const logMessage = opts.passThroughMode
? text
: `[${context}] ${level}: ${text}`;

return [loggerName, logMessage].filter(Boolean).join(' ');
}),
});
}

function getLoggerName(
Comment thread
gzdunek marked this conversation as resolved.
opts: Pick<FileLoggerOptions, 'loggerNameColor' | 'name'>
) {
return (
opts.loggerNameColor &&
`\x1b[${opts.loggerNameColor}m${opts.name.toUpperCase()}\x1b[0m`
);
}
4 changes: 2 additions & 2 deletions web/packages/teleterm/src/services/tshd/interceptors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ it('do not log sensitive info like password', () => {
service: { typeName: 'FooService' } as ServiceInfo,
} as MethodInfo,
{
passw: {},
password: {},
userData: {
login: 'admin',
password: 'admin',
Expand All @@ -50,7 +50,7 @@ it('do not log sensitive info like password', () => {
);

expect(infoLogger).toHaveBeenCalledWith(expect.any(String), {
passw: '~FILTERED~',
password: '~FILTERED~',
userData: { login: 'admin', password: '~FILTERED~' },
});
});
Expand Down
4 changes: 2 additions & 2 deletions web/packages/teleterm/src/services/tshd/interceptors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { isObject } from 'shared/utils/highbar';

import Logger from 'teleterm/logger';

const SENSITIVE_PROPERTIES = ['passw', 'authClusterId', 'pin'];
const SENSITIVE_PROPERTIES = ['password', 'authClusterId', 'pin'];

export function loggingInterceptor(logger: Logger): RpcInterceptor {
return {
Expand Down Expand Up @@ -109,7 +109,7 @@ export function filterSensitiveProperties(toFilter: object): object {
const transformer = (result: object, value: any, key: any) => {
if (
SENSITIVE_PROPERTIES.some(
sensitiveProp => typeof key === 'string' && key.includes(sensitiveProp)
sensitiveProp => typeof key === 'string' && key === sensitiveProp
)
) {
result[key] = '~FILTERED~';
Expand Down