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

refactor: remove is-svg #1034

Merged
merged 2 commits into from Apr 2, 2021
Merged

refactor: remove is-svg #1034

merged 2 commits into from Apr 2, 2021

Conversation

@ludofischer
Copy link
Collaborator

@ludofischer ludofischer commented Mar 31, 2021

Hopefully this will stop further alarmed comments.

Copy link
Member

@alexander-akait alexander-akait left a comment

Need fix CI

@ludofischer
Copy link
Collaborator Author

@ludofischer ludofischer commented Apr 1, 2021

Those look like legit test failures. I'l need to investigate what is happening.

@ludofischer
Copy link
Collaborator Author

@ludofischer ludofischer commented Apr 1, 2021

Cannot update past is-svg 4.2.2, which luckily contains the CVE fix.

Interestingly there's a regression in is-svg 4.3.x, it fails to reject strings that contain random text after the svg.

See the is-svg test: https://github.com/sindresorhus/is-svg/blob/034967d4053dd6ca907a6bfeb047cd61b4c12f0a/test.js#L39
and the corresponding issue in the library that is-svg uses: NaturalIntelligence/fast-xml-parser#327

@ludofischer ludofischer force-pushed the silence-cve-alerts branch from c1224f5 to 8f9f3f4 Apr 1, 2021
@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Apr 2, 2021

We should wait fix in this case

@ludofischer
Copy link
Collaborator Author

@ludofischer ludofischer commented Apr 2, 2021

We could maybe skip the isSvg step completely. Since is-svg was never 100% accurate, some non-SVGs things are caught at the isSvg step and some in the svgo.optimize() step. I wonder if we could just try svgo.optimize() and catch the error. That way we would also stop depending on two different XML parsers (the one that comes from is-svg and the one from svgo). The problem is that we would feed a lot of the non-svg things inside url() to SVGO, but in fact both is-svg and SVGO start by trying to parse the input as XML, they just offer a different interface for reporting errors.

@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Apr 2, 2021

Sounds good

As there is no practical difference between an
invalid SVG and an SVG that SVGO cannot parse,
just try to parse input with SVGO directly.

Also remove dependency on extra XML parsers.
@ludofischer ludofischer force-pushed the silence-cve-alerts branch from 8f9f3f4 to 22d2a18 Apr 2, 2021
@ludofischer ludofischer changed the title chore: bump is-svg and svgo minimum versions refactor: remove is-svg Apr 2, 2021
@@ -203,19 +209,6 @@ test(
)
);

Copy link
Collaborator Author

@ludofischer ludofischer Apr 2, 2021

I have changed these tests because I 've realized there is no meaningful difference between these cases. They're all invalid SVGs. They were handled separately because some failed at the isSvg stage and some at the svgo stage.

let result;
try {
result = optimize(svg, opts);
if (result.error) {
if (result.error.startsWith('Error in parsing SVG')) {
Copy link
Collaborator Author

@ludofischer ludofischer Apr 2, 2021

I could not find a better way to detect the cause of the error. Maybe @TrySound has got a better suggestion? Anyway I wonder whether it makes sense to throw an error just because we cannot process sommething, instead of just leaving it alone. See this comment #910 (comment)

Copy link
Member

@alexander-akait alexander-akait Apr 2, 2021

I think we don't need throw the error in this cases (and all cases, for example postcss-calc, if we can't parse it just log warning, so developers can report about it and we still continue work), let's use console.warn and continue work

Copy link
Collaborator

@TrySound TrySound Apr 2, 2021

Doesn't postcss have warn method?

Copy link
Member

@alexander-akait alexander-akait Apr 2, 2021

oh, yep, forgot about it

@TrySound
Copy link
Collaborator

@TrySound TrySound commented Apr 2, 2021

I don't get why do we even need to check something? data:image/svg+xml implies we deal with svg. So we run svgo with it and if this svg is invalid throw error. Am I missing something?

@ludofischer
Copy link
Collaborator Author

@ludofischer ludofischer commented Apr 2, 2021

I don't get why do we even need to check something? data:image/svg+xml implies we deal with svg. So we run svgo with it and if this svg is invalid throw error. Am I missing something?

I 99% agree with you. Except in the context of cssnano I would not throw on invalid SVG , as it might be preferable to skip minification of that particular section rather than crashing completely.
After all, cssnano is not a linter.

@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Apr 2, 2021

So let's avoid using throw new Error and just use postcss warn method, in future we should avoid using throw new Error everything

Handle all SVGO failures in the same way, as there
is no practical difference between being unable to parse
and being unable to optimise.
@ludofischer ludofischer merged commit 53a5d66 into master Apr 2, 2021
11 checks passed
@ludofischer ludofischer deleted the silence-cve-alerts branch Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants