fix: prevent html injection in cockpit#1381
Conversation
andremedeiros
force-pushed
the
fix/html-injection-in-cockpit
branch
from
96b6e4e to
5301c9b
Compare
| .replace(/</g, "<") | ||
| .replace(/>/g, ">") | ||
| .replace(/\"/g, """) | ||
| .replace(/\'/g, "'"); |
There was a problem hiding this comment.
There are npm packages that do that, but if we prefer our version of it, it's better to put it in a utils, because you use it at least twice.
| (ws, _req) => { | ||
| this.events.on('process-log-' + this.processName, function (log) { | ||
| log.msg = self.escapeMessage(log.msg); | ||
| log.msg_clear = self.escapeMessage(log.msg_clear); |
There was a problem hiding this comment.
Instead of introducing self , just replace function (log) { with (log) => {
andremedeiros
force-pushed
the
fix/html-injection-in-cockpit
branch
from
5301c9b to
e2b3a11
Compare
| let response = result; | ||
| if (typeof result !== "string") { | ||
| response = stringify(result, utils.jsonFunctionReplacer, 2); | ||
| this.logger.info(response); |
There was a problem hiding this comment.
Does stringify handle escaping HTML that could be produced from stringifying an object?
There was a problem hiding this comment.
It does not, but I'm pretty sure that this is the JSON printer, which we still want to be parsed as HTML.
The alternative is introducing plaintext / JSON response types that the cockpit could respond to, but that's a whole other bag of worms.
There was a problem hiding this comment.
Right, so when we stringify in this case, we return it in the response at the end. So I think maybe we should still escape it, no? For example,
if (typeof result !== "string") {
response = stringify(result, utils.jsonFunctionReplacer, 2);
this.logger.info(response);
} else {
// Avoid HTML injection in the Cockpit
this.logger.info(response);
}
response = escapeHtml(response);
return res.send({ result: response });
There was a problem hiding this comment.
Isn't the JSON tree explorer rendered in the backend? Or is it the frontend that renders it?
There was a problem hiding this comment.
I think that's a frontend thing, but could be wrong.
| if(typeof message !== "string") return message; | ||
|
|
||
| return message | ||
| .replace(/&/g, "&") |
There was a problem hiding this comment.
Is there a library that would handle this for us and possibly handle additional use cases that we haven't thought of?
There was a problem hiding this comment.
Probably tons of them, but this is a simple enough fix that I think we can handle it ourselves without introducing yet another dependency.
andremedeiros
changed the title
|
Yep, looks good! |
4 participants
No description provided.