Skip to content

fix(logging): avoid logging sensitive param values#31522

Closed
sterlingwes wants to merge 1 commit into
facebook:mainfrom
sterlingwes:fix-logging
Closed

fix(logging): avoid logging sensitive param values#31522
sterlingwes wants to merge 1 commit into
facebook:mainfrom
sterlingwes:fix-logging

Conversation

@sterlingwes
Copy link
Copy Markdown

@sterlingwes sterlingwes commented May 13, 2021

Summary

We noticed that by default when the RootView / ReactView calls runApplication, we're logging at an info level any props ("params") passed to that component. In our case, one of these props was sensitive in nature, causing the value to leak out in logs for our release builds. This is especially problematic on Android where device logs can be accessed by any app which requests that permission.

This is probably more of a concern for brownfield react-native apps, but it seems worthwhile locking this down in non-dev builds.

Changelog

[General] [Security] - Avoiding logging root view params outside of dev / debug mode builds

Test Plan

  • build app in release mode on Android and verified I could not see: Running "my app" with { sensitive: 'thing' } in logcat in Android Studio with a tethered device

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 13, 2021
@pull-bot
Copy link
Copy Markdown

Messages
📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

DetailsCATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against d8407be

@analysis-bot
Copy link
Copy Markdown

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,273,015 +99
android hermes armeabi-v7a 8,789,594 +101
android hermes x86 9,734,517 +92
android hermes x86_64 9,700,586 +97
android jsc arm64-v8a 10,880,951 +4
android jsc armeabi-v7a 10,390,555 +2
android jsc x86 10,909,185 +17
android jsc x86_64 11,517,162 +10

Base commit: ca499a6

@analysis-bot
Copy link
Copy Markdown

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: ca499a6

@acoates-ms
Copy link
Copy Markdown
Contributor

Have you considered using https://www.npmjs.com/package/babel-plugin-transform-remove-console or something similar? That will cut down on your bundle size too.

@sterlingwes
Copy link
Copy Markdown
Author

Have you considered using https://www.npmjs.com/package/babel-plugin-transform-remove-console or something similar? That will cut down on your bundle size too.

I guess a better question is: are we OK with this verbose logging as default behaviour in all apps powered by React Native?

@charlesbdudley charlesbdudley self-assigned this Sep 20, 2021
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@charlesbdudley has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @sterlingwes in e612d3a.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Type: Security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants