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

[Logger] Add redact step #1610

Merged
merged 2 commits into from
Sep 6, 2023
Merged

[Logger] Add redact step #1610

merged 2 commits into from
Sep 6, 2023

Conversation

dayo09
Copy link
Contributor

@dayo09 dayo09 commented Jul 20, 2023

This commit adds redact step to a Logger.

ONE-vscode-DCO-1.0-Signed-off-by: Dayoung Lee [email protected]


For #1609

This commit adds redact step to a Logger.

ONE-vscode-DCO-1.0-Signed-off-by: Dayoung Lee <[email protected]>
@dayo09 dayo09 mentioned this pull request Jul 20, 2023
@dayo09 dayo09 requested a review from a team July 20, 2023 09:34
@dayo09
Copy link
Contributor Author

dayo09 commented Jul 20, 2023

Redacted log example

When running configuration, docker runs with a github token (in-office network only, though)

[2023. 7. 20. 오후 6:25:32][JobRunner][info] Run tool: onecc-docker args: 
ToolArgs: ["-t","********","-C","/home/dayo/git/ONE-vscode/res/modelDir/truediv/model.cfg"] 
                   ↖ HERE

@dayo09 dayo09 added the 2 approvals 2 approvals required to be merged label Jul 20, 2023
jyoungyun
jyoungyun previously approved these changes Jul 28, 2023
Copy link
Collaborator

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

@jyoungyun jyoungyun requested a review from a team July 28, 2023 00:14
@dayo09
Copy link
Contributor Author

dayo09 commented Jul 31, 2023

@Samsung/one-vscode_committers PTAL :-D

const redact = (msg: string) => {
// Replace github tokens with ********
const prefix = "ghp_";
const regex = new RegExp(`${prefix}[a-zA-Z0-9]+`, "g");
Copy link

Choose a reason for hiding this comment

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

How about to add the regular expression for 'Fine Grained Personal Acess Token' either?
I'm not sure whether RegExp() supports 'ORed' expression, if so we can add it as OR-ed option as like,

const prefix_classic = "ghp_";
const prefix_fgp = "github_pat_";
`^(${prefix_classic}[a-zA-Z0-9]+|${prefix_fgp}_[a-zA-Z0-9]{22}_[a-zA-Z0-9]{59})`

Copy link
Contributor Author

@dayo09 dayo09 Aug 1, 2023

Choose a reason for hiding this comment

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

@batcheu Look like a good idea to add the fine grained tokens :-D

BTW I didn't find the string length of classic token so I made it to redact all the strings starting with 'ghp_'.
And I cannot find the string length of fine grained token either, nowhere in github docs.

If you have found some, could you let me know the link?

If not, let me simply add find-grained Regex as github_pat_[_a-zA-Z0-9]+.

Copy link
Contributor Author

@dayo09 dayo09 Aug 2, 2023

Choose a reason for hiding this comment

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

@batcheu PTAL:-D

Copy link

@batcheu batcheu Aug 3, 2023

Choose a reason for hiding this comment

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

Oops, I didn't notice that you added comments. 😮
Sorry for late reply 😢

I checked the regular expression of new type of access token in below link.

But it's personal blog, so it would not support up-to-date version of token format.

And today, I also found below GITHUB's official annoucement related with access token.

The length of our tokens is remaining the same for now.
However, GitHub tokens will likely increase in length in future updates, so integrators should plan to support tokens up to 255 characters after June 1, 2021.

So, I think that your suggestion is more proper than fixed length expression.

Copy link

@batcheu batcheu left a comment

Choose a reason for hiding this comment

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

LGTM

@dayo09 dayo09 requested review from jyoungyun and removed request for a team August 7, 2023 05:17
Copy link
Collaborator

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

@dayo09
Copy link
Contributor Author

dayo09 commented Aug 11, 2023

@Samsung/one-vscode_committers PTAL :-D

@seanshpark seanshpark merged commit a2a6d3d into Samsung:main Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 approvals 2 approvals required to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants