-
Notifications
You must be signed in to change notification settings - Fork 0
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
[ERA-8079] #810
[ERA-8079] #810
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,13 +48,13 @@ | |
# # If there are whitespace errors, print the offending file names and fail. | ||
# exec git diff-index --check --cached $against -- | ||
|
||
yarn audit --groups=dependencies --level=critical | ||
# yarn audit --groups=dependencies --level=critical | ||
|
||
if [ $? -ge 16 ] | ||
then | ||
echo "\033[33mCommit failed, critical security vulnerability found in one of the project's dependencies. Please resolve as per the audit report above, then re-commit.\033[0m" | ||
exit 1 | ||
fi | ||
# if [ $? -ge 16 ] | ||
# then | ||
# echo "\033[33mCommit failed, critical security vulnerability found in one of the project's dependencies. Please resolve as per the audit report above, then re-commit.\033[0m" | ||
# exit 1 | ||
# fi | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be out of context. What's the reason for commenting these lines? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment above:
If your pre-commit hooks are running correctly, commits currently get rejected because of critical dependency vulnerabilities which are fixed ^in that above linked PR |
||
|
||
yarn test-cov | ||
yarn lint --quiet |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,12 +20,14 @@ import TextCopyBtn from '../../TextCopyBtn'; | |
|
||
import styles from './styles.module.scss'; | ||
|
||
const PLACEHOLDER = 'Click here to set location'; | ||
|
||
const eventReportTracker = trackEventFactory(EVENT_REPORT_CATEGORY); | ||
|
||
const calculateInputDisplayString = (location, gpsFormat) => { | ||
const calculateInputDisplayString = (location, gpsFormat, placeholder) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know you didn't add this, but honestly, having a method for a one liner feels like an overkill haha
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷 🦐 |
||
return location | ||
? calcGpsDisplayString(location[1], location[0], gpsFormat) | ||
: 'Click here to set location'; | ||
: placeholder; | ||
}; | ||
|
||
const LocationSelectorInput = ({ | ||
|
@@ -50,7 +52,7 @@ const LocationSelectorInput = ({ | |
|
||
const [isPopoverOpen, setIsPopoverOpen] = useState(false); | ||
|
||
const displayString = calculateInputDisplayString(location, gpsFormat); | ||
const displayString = calculateInputDisplayString(location, gpsFormat, placeholder); | ||
|
||
const popoverClassString = popoverClassName ? `${styles.gpsPopover} ${popoverClassName}` : styles.gpsPopover; | ||
const shouldShowCopyButton = copyable && (displayString !== placeholder); | ||
|
@@ -144,7 +146,7 @@ const LocationSelectorInput = ({ | |
> | ||
|
||
<LocationIcon className={styles.icon} /> | ||
<span className={styles.displayString}>{displayString}</span> | ||
<span data-testid="locationSelectorInput-displayValue" className={styles.displayString}>{displayString}</span> | ||
{shouldShowCopyButton && <TextCopyBtn className={styles.locationCopyBtn} text={displayString} />} | ||
</div> | ||
|
||
|
@@ -189,7 +191,7 @@ LocationSelectorInput.defaultProps = { | |
copyable: true, | ||
label: 'Location:', | ||
location: null, | ||
placeholder: null, | ||
placeholder: PLACEHOLDER, | ||
popoverClassName: '', | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we merge this PR this should be reverted:
#809
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually going to leave this commented out for now.
https://overreacted.io/npm-audit-broken-by-design/
webpack/loader-utils#212
facebook/create-react-app#11102
https://security.snyk.io/vuln/SNYK-JS-LOADERUTILS-3043105
Even after upgrading dependencies, I'm seeing more "critical" vulnerabilities, but digging through each of them you'll see similar concerns as expressed above; namely "Prototype pollution in webpack loader-utils" being used as a mechanism for denial of service...something wholly unlikely to happen, as it's an aspect of build tools for the application and thus fully within the control/domain of the installer/developer (unless their machine access is compromised, at which point you have much bigger security issues to worry about).
We should still look at the output and investigate each, as well as diligently upgrade dependencies, but it seems like relying on that audit output for pass/fail is ill-conceived. live and learn.