Skip to content

fix: overlay content #3553

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

Merged
merged 7 commits into from
Jul 23, 2021
Merged

fix: overlay content #3553

merged 7 commits into from
Jul 23, 2021

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Jul 22, 2021

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

Fix #3552

Before

Screenshot 2021-07-22 at 4 44 46 PM

After

Screenshot 2021-07-22 at 4 51 23 PM

Breaking Changes

No

Additional Info

No

@@ -138,7 +137,7 @@ function show(messages, type) {

// Make it look similar to our terminal.
const errorMessage = message.message || messages[0];
const text = ansiHTML(encode(errorMessage));
Copy link
Member

Choose a reason for hiding this comment

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

It is not safe, here can be XSS

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we need decode(text, {level: 'html5'}) afterwards

Copy link
Member

Choose a reason for hiding this comment

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

hm, maybe, just try it

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Also we need test it

@snitin315
Copy link
Member Author

Also we need test it

WIP on tests

@codecov
Copy link

codecov bot commented Jul 22, 2021

Codecov Report

Merging #3553 (c7482d6) into master (9307755) will decrease coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3553      +/-   ##
==========================================
- Coverage   94.02%   93.55%   -0.48%     
==========================================
  Files          15       15              
  Lines        1256     1256              
  Branches      431      431              
==========================================
- Hits         1181     1175       -6     
- Misses         69       75       +6     
  Partials        6        6              
Impacted Files Coverage Δ
lib/servers/WebsocketServer.js 91.66% <0.00%> (-5.56%) ⬇️
lib/utils/DevServerPlugin.js 97.82% <0.00%> (-2.18%) ⬇️
lib/Server.js 93.61% <0.00%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9307755...c7482d6. Read the comment docs.

@snitin315 snitin315 marked this pull request as ready for review July 22, 2021 13:59
@snitin315 snitin315 changed the title [WIP] fix: overlay content fix: overlay content Jul 22, 2021
@snitin315
Copy link
Member Author

Ready for review. /cc @alexander-akait

@alexander-akait
Copy link
Member

Thanks, will look at near future

@alexander-akait
Copy link
Member

I was wrong about XSS due Creates a new Text node. This method can be used to escape HTML characters. https://developer.mozilla.org/en-US/docs/Web/API/Document/createTextNode

@alexander-akait
Copy link
Member

Fixed, anyway thanks

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Less deps ⭐

@joacub
Copy link

joacub commented Jul 23, 2021

the correct fix is this ====>

const errorMessage = message.message || messages[0];
const content = ansiHTML(errorMessage);
const messageContentNode = document.createElement('div');
messageContentNode.innerHTML = content;

html still showing bad, with this looks perfect, see the imagen bellow:

image

@alexander-akait
Copy link
Member

It is xss, no it is not good

@alexander-akait
Copy link
Member

Current solution has the same output

@alexander-akait alexander-akait merged commit bec34e7 into master Jul 23, 2021
@alexander-akait alexander-akait deleted the fix-overlay branch July 23, 2021 13:28
@joacub
Copy link

joacub commented Jul 23, 2021

Current solution has the same output

Impossible, html in a text node will be show up like entities…. 😓😓😓😓
this still wrong....

Copy link

@joacub joacub left a comment

Choose a reason for hiding this comment

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

Html in text node will appear like <span>raw</span>

@alexander-akait
Copy link
Member

alexander-akait commented Jul 23, 2021

Please provide screenshot, again, always use the issue template, our tests are fine

@joacub
Copy link

joacub commented Jul 23, 2021

Please provide screenshot, again, always use the issue template, our tests are fine

Did you test with html content ?

@alexander-akait
Copy link
Member

alexander-akait commented Jul 23, 2021

We should not accept HTML content from error/warning messages, otherwise you can inject <script> and do XSS

@joacub
Copy link

joacub commented Jul 23, 2021

Look is very simple, text node can no allocate html only text….

@alexander-akait
Copy link
Member

Because it is XSS

@joacub
Copy link

joacub commented Jul 23, 2021

Because it is XSS

Inject xss to my self 🤣

@alexander-akait
Copy link
Member

alexander-akait commented Jul 23, 2021

other plugin which you can be installed and emit HTML content and steel your environments, it is not joke

@joacub
Copy link

joacub commented Jul 23, 2021

Perfect, is a 1% of the time that can happen maybe less, there is other solution to show correctly this? cause is impossible to read anything

@alexander-akait
Copy link
Member

No one plugin should not emit HTML content in warning/error messages

@alexander-akait
Copy link
Member

In your screenshot only text content and it should be good in overlay

@joacub
Copy link

joacub commented Jul 23, 2021

No one plugin should not emit HTML content in warning/error messages

Eslint do and do nice and read is bestifully, so best way is disable this not useful overlay

@alexander-akait
Copy link
Member

alexander-akait commented Jul 23, 2021

Eslint do and do nice and read is bestifully

HTML content? don't talk nonsense, eslint uses only ansi colors (yes, you can write custom output, but I don't see it is good idea), I don't see HTML content

@joacub
Copy link

joacub commented Jul 23, 2021

Eslint do and do nice and read is bestifully

HTML content? don't talk nonsense, eslint uses only ansi colors (yes, you can write custom output, but I don't see it is good idea), I don't see HTML content

Eslint return html you check in the other issue the screenshot with html …..

@alexander-akait
Copy link
Member

No HTML in any place

@joacub
Copy link

joacub commented Jul 23, 2021

Loook this

@alexander-akait
Copy link
Member

Okay, please open a new issue with reproducible test repo and format this as issue template, original problem with html entries was fixed, what is why I ask ALWAYS use the issue template

@webpack webpack locked as resolved and limited conversation to collaborators Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overlay HTML
3 participants