Rzasm info page enhancement#3595
Conversation
| const QString tooltip = | ||
| QString("<html><div style='font-family:%1;font-size:%2pt;'>%3</div></html>") |
There was a problem hiding this comment.
i don't like HTML injected into QT. this needs to change.
There was a problem hiding this comment.
@wargio Hi, is there any particular reason why is it not good? Because there are some tooltips in the codebase that already do it this way, for example this one
bool DisassemblyPreview::showDisasPreviewAt(QWidget *parent, const QPoint &pointOfEvent, const RVA offset
{
QStringList disasmPreview = Core()->getDisassemblyPreview(offset, 10);
if (!disasmPreview.isEmpty()) {
const QFont &fnt = Config()->getFont();
QString tooltip = QString { "<html><div style=\"font-family: %1; font-size: %2pt; "
"white-space: nowrap;\"><div style=\"margin-bottom: "
"10px;\"><strong>Disassembly Preview</strong>:<br>%3<div>" }
.arg(fnt.family())
.arg(qMax(8, fnt.pointSize() - 1))
.arg(disasmPreview.join("<br>"));
QToolTip::showText(pointOfEvent, tooltip, parent, QRect {}, 3500);
return true;
}
return false;
}
There was a problem hiding this comment.
these needs to be changed. you need to use native elements, because when injecting HTML, you can introduce XSS vulnerabilities
There was a problem hiding this comment.
@wargio Would it be possible for me to keep the tooltip here as is, make a separate issue about removing all the injected HTML and work on it separately? I have been working on it for some days now and there are a lot of changes that are beyond the scope of this PR. I would love to work on that issue too though since I've already started doing it.
There was a problem hiding this comment.
From my point of view - it's okay to do it in a separate PR. Not sure what others think.
notxvilka
added
the
Waiting-For: Author
|
@tsogp hi, what is the status of this one? |
Hi @notxvilka! I'm still working on the HTML tooltips rewrite, it turned out to be much harder to do than I thought since it involves a lot of rich text parsing that was previously handled by HTML. I believe that I will be able to open a draft PR in 2-4 days. To not leave this PR stale for now and proceed with it, would it be possible for me to temporarily comment out tooltip logic and then uncomment it in the tooltip rewrite PR (or leave it as it is right now)? |
|
@wargio @notxvilka Hi! I have escaped all the data that we pass to html (usually it's the font name) and made sure that all the html structures is correct, so there should be no threats of XSS attacks. The disassemblyPreview data is escaped with CutterCore::ansiEscapeToHtml(const QString &text). Can you please have a look? |
|
update the screenshots. |
|
@wargio Done! I've put them in the PR description |
|
way better |
notxvilka
left a comment
notxvilka
left a comment
There was a problem hiding this comment.
Please rebase on top of the latest dev. This should fix the CI
tsogp
force-pushed
the
rzasm-enhancement
branch
from
7c19fb1 to
63b6766
Compare
|
@notxvilka Done! |
3 participants
Your checklist for this pull request
Detailed description
Lowered the width of the
CPU'scolumn, added a tooltip with the style fromDisassemblyPreview::getToolTipStyleSheet()to be able to see the whole value when hovering.Extended
CutterCore::getRAsmPluginDescriptions()function to return the number of bits and capabilities. The implementation is based on Rizin'sconst char *has_esil(RzCore *core, const char *name)andvoid bits_to_string(int bits, char output[32])fromcasm.c. I have also added aconst V *Find(K k, bool *found = nullptr)method toCutterHt##XXto search for analysis plugins.Before:
Screencast_20260329_183914.mov
CPU's columns is very long and no 'Capabilities' and 'Bits' columns
After:
Screencast_20260329_184251.mov
Test plan (required)
CPU'scolumn has smaller width, when hovering you can see values in tooltipsClosing issues
closes #2385