Skip to content
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

adjust for border syntax check #73

Open
wants to merge 5 commits into
base: development
Choose a base branch
from

Conversation

Ghesselink
Copy link
Contributor

Same as #72, but for the syntax checks instead of schema.

I've also looked at the gherkin checks, but here were no such issues.

Before, the same issue:
example_old

After:
example_new

@aothms
Copy link
Member

aothms commented Jun 21, 2023

One thing that needs to be accounted for is the ^ character that is used to indicate where the error is in the line. We have this approach because the syntax checker can also be used in a terminal setting.

afbeelding

(see how the ^ is exactly underneath the 2nd comma).

I think the simplest option is replace this approach client-side by transforming

00008 | #1=IFCPERSON($,$,'',,$,$,$,$);
                            ^

to

00008 | #1=IFCPERSON($,$,'',<span style='text-decoration:underline'>,</span>$,$,$,$);

Counting the number of space characters before the ^ will tell you where to insert the html element on the 1st line.

Alternative is that we adapt the syntax checker to have some sort of --html output mode. But this requires more steps and also does not fix the situation for existing reports.

@Ghesselink
Copy link
Contributor Author

Ghesselink commented Jun 21, 2023

Ah yes, I see how splitting the strings into multiple lines causes confusion.

It's modified it as suggested. I've also made the text bold and with a slight background color to improve visibility as solely underscoring sometimes is unclear (I think), for example when underscoring the ; sign.
image

Lastly,
<br /> {}

is added in between

<span class='pre'>{item.msg.split('\n').slice(0, -2).join('\n')}</span>
 <span class='pre mono' dangerouslySetInnerHTML={{ __html: modifiedStr }}></span>

To make sure each line indeed starts on a new line (this was sometimes not the case)

const msg_parts = item.msg.split('\n')
const whitespaces = msg_parts[msg_parts.length -1].match(/\s*/)[0].length;
const error_instance = msg_parts[msg_parts.length - 2]
const modifiedStr = `${error_instance.substring(0, whitespaces)}<span style='text-decoration:underline; font-weight:bold; background-color:#ddd;'>${error_instance[whitespaces]}</span>${error_instance.substring(whitespaces + 1)}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The token length is not necessarily 1. Try one with a duplicate numeric instance id. It will highlight the entire definition I think. (Not sure actually abt other cases of actual tokens if length can be more than one. )

#1=IFCPERSON(...
#1=IFCWALL(...

But don't assume +1, but count the number of ^ characters.

It would also make the regex a little bit more robust (making sure there is actually a ^ in there) by combining those:

Something like const [numWhitespace, numHighlight] = .match(/(\s*)(\^+)).map(s => s.length)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also missing ;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also try with a border:solid 1px red for the span style to make it even clearer.

Copy link
Contributor Author

@Ghesselink Ghesselink Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done Ghesselink@2634fe1

This worked
const [_, numWhitespace, numHighlight] = msg_parts[msg_parts.length -1].match(/(\s*)(\^+)/).map(s => s.length);

border:solid 1px red
Is also added

The result. Clearer indeed :)
image

In case of a duplicate ID
example

@@ -61,7 +69,8 @@ export default function SyntaxResult({ content, status }) {
<td>{item.column}</td>
<td>
<span class='pre'>{item.msg.split('\n').slice(0, -2).join('\n')}</span>
<span class='pre mono'>{item.msg.split('\n').slice(-2).join('\n')}</span>
<br /> {}
<span class='pre mono' dangerouslySetInnerHTML={{ __html: modifiedStr }}></span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think dangerouslySetInnerHTML is needed here? It's actually risky in this case.

Because the IFC could be

#1=IFCPERSON($, $, '<script>alert("haha")</script>'

And in that case the text that was embedded in the IFC content will be executed as actual HTML. This vulnerability is called XSS for cross site scripting and could potentially result in an attacker having access to your account.

Copy link
Contributor Author

@Ghesselink Ghesselink Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified in Ghesselink@2634fe1

Right, it doesn't even seem that difficult to embed some malicious html.

I think the following is safer. The message is split into three parts (in which the caret ^is the highlighted part) and rendered seperately as a react (Fragment) component.

  function renderMessage(error_instance, numWhitespace, numHighlight) {
    const beforeHighlight = error_instance.substring(0, numWhitespace);
    const highlightedChar = error_instance.substring(numWhitespace, numWhitespace + numHighlight);
    const afterHighlight = error_instance.substring(numWhitespace + numHighlight);
    return (
      <React.Fragment>
        {beforeHighlight}
        <span
          style={{
            textDecoration: 'underline',
            fontWeight: 'bold',
            backgroundColor: '#ddd',
            border: 'solid 1px red'
          }}
        >
          {highlightedChar}
        </span>
        {afterHighlight}
      </React.Fragment>
    );
  };

Define const

              const errorMessage = renderMessage(
                error_instance,
                numWhitespace,
                numHighlight
              );

Display table
<span class='pre mono'>{errorMessage}</span>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants