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

Avoid logging AuthToken from transition links #5425

Merged
merged 1 commit into from
Sep 22, 2021

Conversation

TomatoToaster
Copy link
Contributor

@TomatoToaster TomatoToaster commented Sep 22, 2021

CC: @marcaaron

Details

Fixed Issues

Follow up to a problem identified here: #5396 (comment)

Tests

Same as Web QA done locally

QA Steps

Exact same as this PR: #5396
After you've navigated to NewDot successfully, open up the JS console and search for the log line: Navigating from transition link from OldDot using short lived authToken and verify you don't see any huge authToken in the logs like here: #5396 (comment)

image

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

N/A

@TomatoToaster TomatoToaster requested a review from a team as a code owner September 22, 2021 19:59
@TomatoToaster TomatoToaster self-assigned this Sep 22, 2021
@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@MelvinBot MelvinBot requested review from robertjchen and removed request for a team September 22, 2021 20:00
@TomatoToaster
Copy link
Contributor Author

Tested this locally and it works:
image

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

LGTM


// Don't log the route transitions from OldDot because they contain authTokens
if (path.includes('/transition/')) {
Log.info('Navigating from transition link from OldDot using short lived authToken');
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I don't think we need to log this immediately though so we can add a false like we are doing in the other call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually wait theres no need, it's false by default: https://github.com/Expensify/expensify-common/blob/master/lib/Logger.jsx#L82

@TomatoToaster TomatoToaster merged commit 64e2140 into main Sep 22, 2021
@TomatoToaster TomatoToaster deleted the amal-dont-log-token branch September 22, 2021 21:04
github-actions bot pushed a commit that referenced this pull request Sep 22, 2021
Avoid logging AuthToken from transition links

(cherry picked from commit 64e2140)
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @TomatoToaster in version: 1.1.1-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@TomatoToaster
Copy link
Contributor Author

My bad on the testing steps, it's not actually going to be on the dev console in staging, but in the real logs. @MitchExpensify tested it and the logs for the request being correctly logged are right here: https://www.expensify.com/_devportal/tools/logSearch/#sort=asc&size=2000&query=blob%3A%20%22Navigating%20from%22%20AND%20email%3A%22mitch%40frankchu.xyz%22%20AND%20timestamp%3A%5B2021-09-23T00%3A00%20TO%202021-09-24T23%3A59%5D

So i can say this passed QA on Staging 👍🏾

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.1-8 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @TomatoToaster in version: 1.1.1-9 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.2-7 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants