-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
allow to use libxml2-wasm
for XML validation
#1184
base: main
Are you sure you want to change the base?
Changes from 4 commits
1603609
a4c45c1
818b1e5
caa8cd2
d329805
dc6530f
0e59ff4
c5f922f
452d572
ef3bd62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,53 @@ | ||||||
/*! | ||||||
This file is part of CycloneDX JavaScript Library. | ||||||
|
||||||
Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
you may not use this file except in compliance with the License. | ||||||
You may obtain a copy of the License at | ||||||
|
||||||
http://www.apache.org/licenses/LICENSE-2.0 | ||||||
|
||||||
Unless required by applicable law or agreed to in writing, software | ||||||
distributed under the License is distributed on an "AS IS" BASIS, | ||||||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
See the License for the specific language governing permissions and | ||||||
limitations under the License. | ||||||
|
||||||
SPDX-License-Identifier: Apache-2.0 | ||||||
Copyright (c) OWASP Foundation. All Rights Reserved. | ||||||
*/ | ||||||
|
||||||
import { readFile } from 'fs/promises'; | ||||||
import { ParseOption, XmlDocument, XsdValidator } from 'libxml2-wasm'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did not look into details of untested code for #1184 (comment)
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or, we could see if we can get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or you could ask the library authors for help. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This isn't possible with module set to CommonJS in the tsconfig: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it be possible to move the export default (async function (schemaPath: string): Promise<Validator> {
const { ParseOption, XmlDocument, XsdValidator } = await import('libxml2-wasm');
// ...
}) satisfies Functionality There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also tried that, but it always gets transpiled into a require, which triggers the error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. So we might require dropping node14 support, This will cause breaking-changes - which is no blocker, just a remark. I would be happy working with you to make this happen. 👍 You have carte blanche - change whatever is needed to make this feature work. Just don't rush, good things may take a while. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can give it a shot, I've done some tests locally and it seems the most work is in the Mocha tests, since they're written in Javascript with requires. They all need to be rewritten to imports. |
||||||
import { pathToFileURL } from 'url'; | ||||||
|
||||||
import type { ValidationError } from '../../validation/types'; | ||||||
import type { Functionality, Validator } from '../xmlValidator'; | ||||||
|
||||||
/** @internal */ | ||||||
export default (async function (schemaPath: string): Promise<Validator> { | ||||||
const schema = XmlDocument.fromString( | ||||||
await readFile(schemaPath, 'utf-8'), | ||||||
{ | ||||||
option: ParseOption.XML_PARSE_NONET | ParseOption.XML_PARSE_COMPACT, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this XEE safe? see https://owasp.org/www-community/vulnerabilities/XML_External_Entity_(XXE)_Processing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The interface for this wrapper is somewhat different, building the parse options is combining the flags you want on. In the other implementation it's an object where they could be turned on and off explicitely. So this should result in the same options. I also added this implementation to the xmlValidator tests, and that includes an XXE test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets see how the tests turn out. |
||||||
url: pathToFileURL(schemaPath).toString() | ||||||
}); | ||||||
const validator = XsdValidator.fromDoc(schema); | ||||||
|
||||||
return function (data: string): null | ValidationError { | ||||||
const doc = XmlDocument.fromString(data, { option: ParseOption.XML_PARSE_NONET | ParseOption.XML_PARSE_COMPACT }); | ||||||
jkowalleck marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
let errors = null; | ||||||
try { | ||||||
validator.validate(doc); | ||||||
} | ||||||
catch (validationErrors) { | ||||||
errors = validationErrors; | ||||||
} | ||||||
|
||||||
doc.dispose(); | ||||||
validator.dispose(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. really free/dispose the validator and schema here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, this will go wrong the second call. I could just not dispose the validator and the schema. But the library emphasises proper disposing (https://jameslan.github.io/libxml2-wasm/v0.4/documents/Memory_Management.html). Here I'm really lacking in Typescript knowledge on how to solve this, could I use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. regarding regarding manually disposing/freeing: maybe just try it out. in the end, it all is javascript - just see what you can do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SierraNL , could you see if changing this makes the tests pass? |
||||||
schema.dispose(); | ||||||
|
||||||
return errors; | ||||||
} | ||||||
}) satisfies Functionality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since a release of 7.0.0in the meantime, and a forward-merge,
this needs to be updated to be in the
unreleased
category.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the line (and a header) to unreleased.