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
Hotfix for possible SVG spearsphishing attack vector #3394
Conversation
PRO-1971 Implement solution to SVG XSS attack risk
See PRO-1859 for the background on this.
|
@@ -707,7 +734,10 @@ module.exports = { | |||
const batchSize = 100; | |||
let lastId = ''; | |||
while (true) { | |||
const docs = await self.db.find({ _id: { $gt: lastId } }).limit(batchSize).sort({ _id: 1 }).toArray(); | |||
const docs = await self.db.find({ | |||
...(criteria || {}), |
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.
A bug that got in the way: the each
method for attachment migrations was ignoring its criteria
argument.
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.
Also an eslint error
@@ -402,6 +405,16 @@ module.exports = { | |||
} | |||
info.length = await self.apos.util.fileLength(file.path); | |||
info.md5 = await self.apos.util.md5File(file.path); | |||
if (info.extension === 'svg') { | |||
try { | |||
await self.sanitizeSvg(file, info); |
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 info
parameter isn't used in the method.
This is low probability, but possible. This initial PR is against a base branch that matches 3.3.1. I like this procedure better than starting out with a PR against main, because this way no error-prone cherry-picking is required, just a merge to main after release.