feat: show match context #396 #436
Conversation
Screencast.from.14.11.2024.12.18.49.webm |
|
New modals are draggable and index themselves properly in front of everything else. New data structure allows to present data like in attached photo: |
|
Looks good! (just looking at screenshots, didn't review the code yet) Can you align the byte and text views? (such that text and byte lines contain the same data, just encoded differently). Can you separate bytes with spaces? I mean Nitpick: it's customary to show bytes first, so could you reorder the "bytes" and "text" columns? Finally, a question just in case - did you test this with non-printable bytes? When displaying, you can probably replace every character < 32 and >= 127 with a dot |
|
Current styling and implementation with new API: |
mickol34
force-pushed
the
feat/show-match-context-396
branch
from
f84d210 to
8dd4305
Compare
mickol34
changed the title
|
Looks good! Some small comments: Consider changing the close button to: (change class to
Text still doesn't align properly:
This may be an issue with my browser, but since people use various browers it's better to be resilient. I think the easiest solution would be to wrap this manually (use |
| } | ||
| return "."; | ||
| }) | ||
| .join(" ") + " " |
There was a problem hiding this comment.
maybe remove the whitespace between letters? So (" "). I understand that now spacing depends on this, but this should be solved anyway.
There was a problem hiding this comment.
Done, now there's no spaces in between bytes or chars.
There was a problem hiding this comment.
This time I was probably unclear. The usual hexdump format, that it would be nice to have:
The important parts are:
- (usually) spaces between bytes
- no spaces between characters (to make plaintext easy to read)
- 16 characters per line
- optionally hexadecimal addresses on the left (we don't have that)
So spaces between bytes are OK (they make bytes easier to read). And the really important thing is 16 bytes per line. It may sound like I'm nitpicking, but I'm not ;)
As a POC:
There was a problem hiding this comment.
To I think this is the required change, but please verify:
❯ git diff --unified=1
diff --git a/src/mqueryfront/src/components/ActionShowMatchContext.js b/src/mqueryfront/src/components/ActionShowMatchContext.js
index 68e8955..096d0a5 100644
--- a/src/mqueryfront/src/components/ActionShowMatchContext.js
+++ b/src/mqueryfront/src/components/ActionShowMatchContext.js
@@ -33,4 +33,4 @@ function base64ToHex(str64) {
})
- .join("")
- .toUpperCase();
+ .join(" ")
+ .toUpperCase() + " ";
}
@@ -103,3 +103,3 @@ const ActionShowMatchContext = (props) => {
<>
- <td scope="row" style={{ width: "10%" }}>
+ <td scope="row">
<span className="badge rounded-pill bg-info ms-1 mt-1">
@@ -110,3 +110,3 @@ const ActionShowMatchContext = (props) => {
{ReactHtmlParser(
- cellHTML(foundSample, 20, base64ToHex)
+ cellHTML(foundSample, 48, base64ToHex)
)}
@@ -115,3 +115,3 @@ const ActionShowMatchContext = (props) => {
{ReactHtmlParser(
- cellHTML(foundSample, 10, base64ToSanitizedUtf8)
+ cellHTML(foundSample, 16, base64ToSanitizedUtf8)
)}
@@ -129,3 +129,2 @@ const ActionShowMatchContext = (props) => {
rowSpan={Object.keys(props.context[rulename]).length}
- style={{ width: "15%" }}
>
@@ -178,3 +177,3 @@ const ActionShowMatchContext = (props) => {
>
- <div className="modal-dialog modal-lg">
+ <div className="modal-dialog modal-xl">
<div className="modal-content">| ); | ||
| } | ||
|
|
||
| function base64ToUtf8(str64) { |
There was a problem hiding this comment.
same goes for this function. I suggest sanitize and
sanitize(atob(str)) instead of
base64ToUtf8(str)
There was a problem hiding this comment.
As mentioned above, I think that sanitize is too vague. If there will be another use case for such decryption in the project, it would be easier to get to know what was it's original purpose.
There was a problem hiding this comment.
On the other hand, base64ToUtf8 is wrong:
- atob already returns an unicode string
- the string has no particular encoding (the underlying interpreter must encode the string to bytes somehow, and may or may not use utf8, but for JS this is just a unicode string, without any encoding assigned)
I suggested sanitize because it's short, but it's true that it's a bit vogue . Consider:
safeText(text),toSafeText(text),toPrintableString(text),replaceUnprintableCharacters(text), as an alternative tosanitizebase64ToSafeText(text),base64ToSafeText(text),base64ToPrintableString(text),base64DecodeAndReplaceUnprintableCharacters(text)as an alternative tobase64ToUtf8
(or another variation that describes "replacing unprintable characters in a string with dots" succintly)
There was a problem hiding this comment.
I just noticed in the source that the name is already base64ToSanitizedUtf8. That's much better (though utf8 is still technically incorrect - maybe text or string?)
| ); | ||
| } | ||
|
|
||
| function base64ToUtf8(str64) { |
There was a problem hiding this comment.
On the other hand, base64ToUtf8 is wrong:
- atob already returns an unicode string
- the string has no particular encoding (the underlying interpreter must encode the string to bytes somehow, and may or may not use utf8, but for JS this is just a unicode string, without any encoding assigned)
I suggested sanitize because it's short, but it's true that it's a bit vogue . Consider:
safeText(text),toSafeText(text),toPrintableString(text),replaceUnprintableCharacters(text), as an alternative tosanitizebase64ToSafeText(text),base64ToSafeText(text),base64ToPrintableString(text),base64DecodeAndReplaceUnprintableCharacters(text)as an alternative tobase64ToUtf8
(or another variation that describes "replacing unprintable characters in a string with dots" succintly)
| } | ||
| return "."; | ||
| }) | ||
| .join(" ") + " " |
There was a problem hiding this comment.
This time I was probably unclear. The usual hexdump format, that it would be nice to have:
The important parts are:
- (usually) spaces between bytes
- no spaces between characters (to make plaintext easy to read)
- 16 characters per line
- optionally hexadecimal addresses on the left (we don't have that)
So spaces between bytes are OK (they make bytes easier to read). And the really important thing is 16 bytes per line. It may sound like I'm nitpicking, but I'm not ;)
As a POC:
| } | ||
| return "."; | ||
| }) | ||
| .join(" ") + " " |
There was a problem hiding this comment.
To I think this is the required change, but please verify:
❯ git diff --unified=1
diff --git a/src/mqueryfront/src/components/ActionShowMatchContext.js b/src/mqueryfront/src/components/ActionShowMatchContext.js
index 68e8955..096d0a5 100644
--- a/src/mqueryfront/src/components/ActionShowMatchContext.js
+++ b/src/mqueryfront/src/components/ActionShowMatchContext.js
@@ -33,4 +33,4 @@ function base64ToHex(str64) {
})
- .join("")
- .toUpperCase();
+ .join(" ")
+ .toUpperCase() + " ";
}
@@ -103,3 +103,3 @@ const ActionShowMatchContext = (props) => {
<>
- <td scope="row" style={{ width: "10%" }}>
+ <td scope="row">
<span className="badge rounded-pill bg-info ms-1 mt-1">
@@ -110,3 +110,3 @@ const ActionShowMatchContext = (props) => {
{ReactHtmlParser(
- cellHTML(foundSample, 20, base64ToHex)
+ cellHTML(foundSample, 48, base64ToHex)
)}
@@ -115,3 +115,3 @@ const ActionShowMatchContext = (props) => {
{ReactHtmlParser(
- cellHTML(foundSample, 10, base64ToSanitizedUtf8)
+ cellHTML(foundSample, 16, base64ToSanitizedUtf8)
)}
@@ -129,3 +129,2 @@ const ActionShowMatchContext = (props) => {
rowSpan={Object.keys(props.context[rulename]).length}
- style={{ width: "15%" }}
>
@@ -178,3 +177,3 @@ const ActionShowMatchContext = (props) => {
>
- <div className="modal-dialog modal-lg">
+ <div className="modal-dialog modal-xl">
<div className="modal-content">
Your checklist for this pull request
What is the current behaviour?
Currently there's no feature to show match context in search results.
What is the new behaviour?
This PR introduces "Show match context" action to search results using newly introduced backend Match field.
Test plan
Query some YARA, look at results, toggle modal with match context and check styling in various cases.
Closing issues
fixes #396