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

Fail base64 validation if unrecognized characters are found #5

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

iglosiggio
Copy link

This should fix #4. I've changed the conformance tests accordingly but these aren't enabled by default during compilation (and some are failing).

@svanteschubert
Copy link
Collaborator

@iglosiggio A thousand thanks for this patch! Allow me to fix the enabling of the tests first, I was not aware!
I realized that when I move the non-java files from, for example
xsdlib/src/test/java
to
xsdlib/src/test/resources
they are automatically copied by Maven during build and in this case, the main method finds the test XML with its class loader!

PS: I beg your pardon, that I have overseen the patches in my email (they are not yet in my "primary" folder of GMail, but in "updates" with commercials) I added an item to my ToDo list to mark GitHub notifications as important!

@svanteschubert
Copy link
Collaborator

For instance, for XSDLIB Git would show the following file movements:
R java/com/sun/msv/datatype/xsd/conformance/DataTypeTest.xml -> resources/com/sun/msv/datatype/xsd/conformance/DataTypeTest.xml
R java/com/sun/msv/datatype/xsd/conformance/DerivationTest.xml -> resources/com/sun/msv/datatype/xsd/conformance/DerivationTest.xml
R java/com/sun/msv/datatype/xsd/conformance/package.html -> resources/com/sun/msv/datatype/xsd/conformance/package.html

@svanteschubert
Copy link
Collaborator

After moving the XML files from src/test/java to src/test/resources they will appear in target test class directory
xsdlib/target/test-classes and are accessible by the class loader:

Earlier you will find only class files:
find . -name *.class -type f
later you will find other files:
find . -not -name *.class -type f

@svanteschubert
Copy link
Collaborator

@iglosiggio Could you please try to update your branch/pull request?
I have moved the XML file that you have changed and GitHub is not automatically rebasing the changes.
Otherwise, I will try in the following days!

PS: When you test, the error message "CatalogManager.properties: catalogs not found." happened earlier as well, therefore I still think the *.properties belong to the src/main/resources directory.

@svanteschubert
Copy link
Collaborator

As the update of a pull request not by the owner is not intuitively documented (uncertain this is possible at all at GitHub), I have created a new rebased branch, where your issues for #4 are within:
https://github.com/xmlark/msv/tree/strict-base64-validation
Could you give the branch strict-base64-validation a try, which are your two commits rebased on top of the main branch!

What concerns me is that there are some errors thrown in the newly activated test, although it seems not related to your change, but regarding data types.

Can you or anyone dive there a bit deeper?

I would like to do a new MSV release soon, but the new errors do not look so good (even if the test had been disabled earlier) ;-)

Thank you in advance,
Svante

PS: I renamed the test class back "removing again its new suffix 'Test'", as I remembered that explicit names could be added to the pom.xml of Maven to be executed by the Surefire test plugin!

@svanteschubert
Copy link
Collaborator

@iglosiggio I just read beyond the box, that I simply would have cloned your repo and made a pull request there, to update this pull request! Now I know for the next time! :-)

@iglosiggio
Copy link
Author

Thanks for all the comments @svanteschubert! I'll look into this tomorrow 😅

@iglosiggio
Copy link
Author

@svanteschubert just checked on our sw and it worked perfectly! I've just reseted my branch personal so it points at your rebase (feel free to merge either one).

@svanteschubert
Copy link
Collaborator

@iglosiggio Thanks a lot, Ignacio!
The only thing that worries me is the old tests that is now running and providing some errors:
************* error *************
type name : gDay
tested instance : " ---00 "
supposed to be valid : false
verify method : true
createValue method : true
createJavaObj method : true
diagnose method : true
JavaObjectType : class java.util.GregorianCalendar
expected Type : class java.util.Calendar
serializeJavaObject : ---31
convertToLexical : ---00

see https://github.com/xmlark/msv/blob/strict-base64-validation/xsdlib/src/test/java/com/sun/msv/datatype/xsd/conformance/TestDriver.java#L70

Which I have not attached to failing a JUnit test (the build would break).
Likely those are "just" false positives, but I am uncertain and have little time to debug.
As you have adopted this test - at least the XML and by this awaken the sleeping docs ;-) - could you provide some feedback on this? Perhaps fix those?

Thanks in advance,
Svante

@svanteschubert
Copy link
Collaborator

I have added some JUnit Assert.fail(..) to the tests to make our problem a little more obvious:
strict-base64-validation...iglosiggio:msv:strict-base64-validation
and duplicated it on our branch here:
2f3c720

Earlier these tests were not executed, I was not aware of them until you fixed one.
Which is good! I enabled them now - we might disable them again, but I would feel safer if someone could take a look at it before a release... (I am sorry, I am drowning in deadlines...)

Several other implementations convert IllegalArgumentException to
returning null, so do the same here.

This fixes some DataTypeTest.
It's not very obvious how this should work, but it is obvious that this
can't ever have passed completely.

There is one definite bug there where negative gYear is decremented by 1
on roundtrip.

Eventually ran out of time so disabled about a third of the tests
altogether.
@mistmist
Copy link
Collaborator

i did try to fix the test, it's quite confusing, i found 2 bugs, fixed one of them, disabled lots of test inputs, ran out of time...

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.

Base64BinaryType validation is (maybe) not compliant.
3 participants