theKashey / used-styles

πŸ“All the critical styles you've used to render a page.
MIT License
137 stars 9 forks source link

fixes style injections nested inside style, select, script #57

Closed hnrchrdl closed 7 months ago

hnrchrdl commented 7 months ago

tackles #56

theKashey commented 7 months ago

πŸ‘ a good start

hnrchrdl commented 7 months ago

pushed a commit. let me know what you think.

hnrchrdl commented 7 months ago

not sure why the tests fail in the install phase due to error Error: https://packages.atlassian.com/api/npm/npm-remote/postcss/-/postcss-8.4.6.tgz: Request failed "401 Unauthorized"

hnrchrdl commented 7 months ago

pushed a commit for a fix (forgot to call back in one specific case).

I tested the code in my huge production app, and it seems to work fine.

would be interested in your input on this.

theKashey commented 7 months ago

https://packages.atlassian.com/api/npm/npm-remote/postcss

🀦 another company setting lured into lock file. That is internal repository you dont have access to

theKashey commented 7 months ago

I tested the code in my huge production app, and it seems to work fine.

The solution seems to work right as it should πŸ‘

I am not 100% sure that hasOpenedPureTagMatch is generated properly if all "PURE" tags are found in one chunk - it should pick first, handle it until it's closed and then try to pick another one because it's possible to have select and style, with select being closed and style being not. In this case logic will wait for select close tag, but will never find it.

Cannot prove this to be a problem, or not to be a problem without creating corresponding tests mixing different possible situations.

In any case it's an ease fix I can apply on top of your solution.

hnrchrdl commented 7 months ago

🀦 another company setting lured into lock file. That is internal repository you dont have access to

then let's merge this first https://github.com/theKashey/used-styles/pull/58

hnrchrdl commented 7 months ago

I am not 100% sure that hasOpenedPureTagMatch is generated properly if all "PURE" tags are found in one chunk

i cannot think of a chunk that opens two or more "pure" tags at the same time, since they can't be nested. ok maybe something like this:

<select>
  <script>
    ...
  // end of chunk

is this what you mean? in any case, having failing tests for these egde cases would be helpful.

hnrchrdl commented 7 months ago

some other problematic cases i could think of:

<script>
  console.log('i really like <style> tags');
</script>
<label>enter your favorite tag</label>
<input value="<style>" />

and so on. in those cases, really the flaws of parsing HTML with RegExp come into play. I don't know how critical they are, i would hope they are more like extreme edge cases.

theKashey commented 7 months ago
<script>
  console.log('i really like <style> tags');
</script>

is not a problem as style tag is "skipped" while it is inside the script - tag which happened first

<input value="<style>" />

is a problem, but I think this is a defect we can accept. Also, we can measure open/closed PURE tags and emit errors if numbers don't match.

theKashey commented 7 months ago

cannot think of a chunk that opens two or more "pure" tags at the same time

<select></select>
<style>
// end of chunk

Current logic will capture select and ignore that it has been ended and now one should await for style tag

hnrchrdl commented 7 months ago

cannot think of a chunk that opens two or more "pure" tags at the same time

<select></select>
<style>
// end of chunk

Current logic will capture select and ignore that it has been ended and now one should await for style tag

no, it will actually capture <style>, since <select> was closed properly.

image
theKashey commented 7 months ago

πŸ€” nice, missed that part of RegExp. Look like you've covered everything and the build is green.

So lets move forward as this change is solving your problem and we haven't found any real edge cases for it.

Thank you so much for another generous contribution.


As a safety net we can introduce a parallel job that would collect result HTML and verify it using some proper parser. In case something went wrong the developer will be informed.

hnrchrdl commented 7 months ago

thanks for merging. happy to help. could you also please release a new version from it?

theKashey commented 7 months ago

v2.6.3 has been just released