-
Notifications
You must be signed in to change notification settings - Fork 353
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
Remove string-replace-webpack-plugin
to fix critical security warning.
#7127
base: main
Are you sure you want to change the base?
Conversation
The plugin was mainly used for replacing babel generated getters with `superGet` in mobx 4. But after mobx upgrade this is no longer required. It is currently only used for upadting the credit string in Ion.js that is shown when using the default Ion token. I have dropped that in favour of keeping the build script simpler.
config.module.rules.push({ | ||
test: /\.js?$/, | ||
include: path.resolve(cesiumDir, "Source", "ThirdParty"), | ||
loader: StringReplacePlugin.replace({ | ||
replacements: [ | ||
{ | ||
pattern: /\/\*[\S\s]*?\*\//g, // find multi-line comments | ||
replacement: function (match) { | ||
// replace http:// and https:// with a spelling-out of it. | ||
return match.replace(/(https?):\/\//g, "$1-colon-slashslash "); | ||
} | ||
} | ||
] | ||
}) | ||
}); | ||
|
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.
The sprintf
credits this was trying to replace is no longer a dependency of Cesium.
{ | ||
pattern: /Please assign <i>Cesium.Ion.defaultAccessToken<\/i>/g, | ||
replacement: function () { | ||
return 'Please set "cesiumIonAccessToken" in config.json'; | ||
} | ||
}, | ||
{ | ||
pattern: / before making any Cesium API calls/g, | ||
replacement: function () { | ||
return ""; | ||
} | ||
} |
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.
Do we really need this?
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 think it's pretty important. The default message is flat out wrong.
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.
Ok, I'll work out a way to include it - I thought it was too small to read.
I think substituting the Cesium Ion key default message is important to us |
What this PR does
Removes string-replace-webpack-plugin which generates a security warning due to one of its stale dependency.
The plugin was mainly used for replacing babel generated getters with
superGet
in mobx 4. But after mobx upgrade this is no longer required.It is currently only used for updating the credit string in Ion.js that is shown when using the default Ion token.
It looks like this:
I'm not sure if it is a very useful customization to have, so I have dropped it in favor of keeping the build script simpler. However if the consensus is to add it back, I can use a more updated webpack plugin like string-replace-loader or see if we can customize it run time instead of build time.
Test me
Shouldn't affect the app except it will now show Cesium's default credit line when a default access token is used.
Checklist
doc/
.