You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@santuario.apache.org by GitBox <gi...@apache.org> on 2023/01/06 18:09:16 UTC
[GitHub] [santuario-xml-security-java] dmatej opened a new pull request, #101: Small boost forward - aggregated changes from one stable stage to another
dmatej opened a new pull request, #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101
In the beginning was this innocent issue: https://github.com/eclipse-ee4j/glassfish/issues/24138
Then we have found that the XmlSec is to blame.
Then I have found that I don't like it's state - I had a suspiction about memory leaks, etc.
So I did this ...
### Changes in Artifact
- Updated dependencies
- JDK11 instead of JDK8
- that's why I changed the major version.
- If anyone wants to continue with JDK8 version, he can continue with maintenance in a new branch. But we need bigger progress.
- System.Logger instead of SLF4J
- Allows to use SLF4J2/LOG4J2/JUL/GJULE/Any Custom logging framework.
- No mandatory dependency required
- Added module-info.java
- That's why I tested it in action with the GlassFish and TCK10
- AutoCloseable
- Just for several types
- I have found some old discussion about it, but based on obsoleted opinions. Currently not having it was contraproductive and was asking for leaks (I was able to produce some, but not with GF, just in xmlsec unit tests).
- Split XmlSignatureInput to several standalone classes based on the type of input data.
- I still don't think it is alright, but it is still much better than original "god object"
- XmlSignatureStreamInput IS AutoCloseable. And still can cause leaks. But changing it would mean much larger changes. At least this type is standalone and specific. Implementation also allows to use custom closeable objects.
- Added some XmlUtil methods for simple work with File+Path+Stream, buffers
- Javadocs, formatting
### Changes in Tests
- PMD now tests the tests too
- All issues resolved
- Especially many resource leaks (200?)
- Performance tests moved to maven failsafe plugin and refactored to JMH+JUnit5
- Originally excluded
- Disabled by default, use -DskipIT=false to enable them
- Locally passed, however someone should analyze decryptDOM, after it ends there's still heap used, but I couldn't find why.
- With more changes the performance improved, especially with bigger buffers and NIO.
- Usage of Shenandoah GC - it is more aggressive, stats should be real or close to real at least
- Proven that StAX is much faster than DOM.
- Tests results: watch statistics, if the repeating is slower and slower, there's probably some leak (now it probably isn't). Junit tests just basic criteria.
- Fixed JaCoCo configuration.
### Consequences
- It would be good to release at least milestone after some time, so we could use it in MetroWSIT and Glassfish. Required changes are already prepared, it is quite easy and even better, WSIT will not need to repackage xmlsec, GF will not need SLF4J1 in dependencies, everything is more simple and clean.
- Jetty doesn't complain any more in xmlsec tests
- If some company would like to invest to xmlsec refactoring, it would be great ;-)
- TCK tests results are here: https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck/job/10.0.x/29/testReport/
If I would remember yet something, I will add it here later ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "coheigea (via GitHub)" <gi...@apache.org>.
coheigea commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1477537005
Any update @arjantijms ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] arjantijms commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by GitBox <gi...@apache.org>.
arjantijms commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1383885141
@coheigea technically speaking the PR is already split, as David used many commits instead of one big commit.
Maybe it's convenient for you to review on a commit basis and then (if needed) create small PRs containing just that commit from this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] arjantijms commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "arjantijms (via GitHub)" <gi...@apache.org>.
arjantijms commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1408981796
@coheigea I'm sure 4/5 PRs would be doable indeed. I had just created an internal TODO issue for myself to look at exactly that ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by GitBox <gi...@apache.org>.
coheigea commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1397307741
@arjantijms Yes a separate PR by commit would be much easier to review if that would be possible?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1600688384
And yet I should transform probably also all those loops. I tried to avoid that, because for i mostly doesn't fail on concurrent changes, but it can still cause errors. With enhanced loops they will become more visible. That might be painful, but we have to go through that once ... I believe there is no such error in xmlsec, so let's try that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on a diff in pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on code in PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#discussion_r1223069814
##########
src/main/java/org/apache/xml/security/stax/impl/processor/input/AbstractDecryptInputProcessor.java:
##########
@@ -344,7 +343,8 @@
XMLStreamReader xmlStreamReader =
inputProcessorChain.getSecurityContext().<XMLInputFactory>get(
XMLSecurityConstants.XMLINPUTFACTORY).createXMLStreamReader(
- new MultiInputStream(prologInputStream, decryptInputStream, epilogInputStream), StandardCharsets.UTF_8.name());
+ new MultiInputStream(prologInputStream, decryptInputStream, epilogInputStream),
Review Comment:
True, but not changed by this PR at this moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] arjantijms commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by GitBox <gi...@apache.org>.
arjantijms commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1377052318
@coheigea what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by GitBox <gi...@apache.org>.
coheigea commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1383561077
@dmatej There are way too many changes in this PR. Can you split them out for a single change in a single PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1600476669
I wanted to override further inflating of the PR, but you are right, it should be done. I will create a new PR for that in few minutes and rebase this after the merge. So this one will be yet bit smaller.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "coheigea (via GitHub)" <gi...@apache.org>.
coheigea commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1408962089
Hello @dmatej @arjantijms , can we try to find a way forward on this please? Forking the project would be a mistake IMO, as someone else would have to maintain the burden of finding and porting security fixes from this project. I'm not asking for a PR per-commit, but just grouping related commits into 4/5 PRs would be workable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1584590357
> Yep I'm OK with moving to Java 11 as the minimum version for a new 4.0.0 release. I will start going through this PR in more detail shortly
Ok, then I can create yet several standalone PRs to reduce differences.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by GitBox <gi...@apache.org>.
coheigea commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1397317710
Alternatively @dmatej , if you can bundle all changes which don't break backwards compatibility into a single PR, I wlll review all those changes at once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1584381717
@coheigea I have a question: Are you OK with change of the minimal Java from 8 to 11? Because if not, I can save time, close the PR and fork XmlSec as we need to keep it updated.
I could rename the PR to "Minimal Java upgraded to 11" too, because many changes depend on that:
1) Module-info
2) System.Logger (the original target was to remove SLF4J from mandatory dependencies)
3) Java9+ deatures like Path.of
4) `--add-exports`
5) Jakarta EE 10 dependencies
It is visible that XmlSec doesn't hurry anywhere and it is close to be really obsoleted. I think it would be better to create at least a prototype of the next XmlSec major version based on this branch and hurry to the year 2023.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "coheigea (via GitHub)" <gi...@apache.org>.
coheigea commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1584513756
Yep I'm OK with moving to Java 11 as the minimum version for a new 4.0.0 release. I will start going through this PR in more detail shortly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1636871141
Eh, I deleted the branch by mistake (typo in push -> delete branch with typo -> push to the correct branch ... without source branch, so I deleted it instead). Unfortunately pushing it back did not reopen this PR ... so I will recreate the PR after merging those fragments of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1598938003
@coheigea Hi, I tried to reorder commits and separate yet something, but what remained here is mostly SLF4J -> System.Logger, Java11 features, enabled PMD for tests, and refactoring to avoid resource leaks (changed API). Finally I even did not push the reordering, because it could make the commit history less consistent than now.
So this is probably the final state which has to be reviewed, and then we need to choose the right strategy for the release the `4.0.0-M1` at least.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by GitBox <gi...@apache.org>.
dmatej commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1383845499
This is not a bug fix but rather a refactoring with tested result. How could you do that? Yeah, I could avoid some updates and reordering, but trivial diffs can do gitbot and if I would have to wait for every review for weeks, I would never finish.
So this is something like a transaction, a change from one consistent state to another.
The last push is just a rebase, I should have done probably rather a merge than this, that's true.
Cutting that to small pieces doesn't bring anything, just more work. Yes, reviewing large pr by reading is not easy, especially with github browser.
This project would need several large refactoring iterations... next generation.
Another option is to fork the project and move quickly forward with own reviews in hours/days, but cooperation is better, even when sometimes somebody comes with scary radical changes (tested!). Another option is to replace this dependency with something in a better shape.
So, simply said, if you refuse this pr, make a decision. But honestly, reviewing it still takes much less time and effort than implementing and testing it. Someone has to do that... both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on a diff in pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on code in PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#discussion_r1223070523
##########
src/main/java/org/apache/xml/security/stax/impl/util/DigestOutputStream.java:
##########
@@ -67,9 +66,9 @@
public byte[] getDigestValue() {
if (isDebugEnabled) {
- LOG.debug("Pre Digest: ");
- LOG.debug(stringBuilder.toString());
- LOG.debug("End pre Digest ");
+ LOG.log(Level.DEBUG, "Pre Digest: ");
+ LOG.log(Level.DEBUG, stringBuilder.toString());
Review Comment:
True, but not changed by this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on a diff in pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "coheigea (via GitHub)" <gi...@apache.org>.
coheigea commented on code in PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#discussion_r1241595720
##########
src/main/java/org/apache/jcp/xml/dsig/internal/dom/ApacheNodeSetData.java:
##########
@@ -35,9 +35,9 @@
import org.apache.xml.security.utils.XMLUtils;
import org.w3c.dom.Node;
-public class ApacheNodeSetData implements ApacheData, NodeSetData {
+public class ApacheNodeSetData implements ApacheData, NodeSetData<Node> {
- private XMLSignatureInput xi;
+ private final XMLSignatureInput xi;
Review Comment:
Could all changes in this class be backported in a separate PR?
##########
src/main/java/org/apache/jcp/xml/dsig/internal/dom/ApacheOctetStreamData.java:
##########
@@ -27,15 +27,12 @@
import org.apache.xml.security.signature.XMLSignatureInput;
-public class ApacheOctetStreamData extends OctetStreamData
- implements ApacheData {
+public class ApacheOctetStreamData extends OctetStreamData implements ApacheData {
- private XMLSignatureInput xi;
Review Comment:
Could all changes in this class be backported in a separate PR?
##########
src/main/java/org/apache/xml/security/c14n/implementations/Canonicalizer20010315.java:
##########
@@ -303,10 +303,10 @@ protected void circumventBugIfNeeded(XMLSignatureInput input)
return;
}
Document doc = null;
Review Comment:
All changes in class could be backported?
##########
src/main/java/org/apache/xml/security/parser/XMLParser.java:
##########
@@ -27,6 +27,15 @@
*/
public interface XMLParser {
+ /**
+ * Parses a document from the input stream.
+ * Caller is responsible for closing the stream.
+ *
+ * @param inputStream
+ * @param disallowDocTypeDeclarations
+ * @return {@link Document}
+ * @throws XMLParserException
+ */
Review Comment:
Backport
##########
src/main/java/org/apache/xml/security/signature/XMLSignature.java:
##########
@@ -225,7 +226,7 @@ public final class XMLSignature extends SignatureElementProxy {
*/
private boolean followManifestsDuringValidation = false;
- private Element signatureValueElement;
+ private final Element signatureValueElement;
Review Comment:
Backport
##########
src/main/java/org/apache/xml/security/stax/impl/OutputProcessorChainImpl.java:
##########
@@ -31,18 +33,16 @@
import org.apache.xml.security.stax.ext.OutputProcessorChain;
import org.apache.xml.security.stax.ext.stax.XMLSecEvent;
import org.apache.xml.security.stax.ext.stax.XMLSecStartElement;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* Implementation of a OutputProcessorChain
*
*/
public class OutputProcessorChainImpl implements OutputProcessorChain {
- protected static final transient Logger LOG = LoggerFactory.getLogger(OutputProcessorChainImpl.class);
+ private static final Logger LOG = System.getLogger(OutputProcessorChainImpl.class.getName());
- private List<OutputProcessor> outputProcessors;
+ private final List<OutputProcessor> outputProcessors;
Review Comment:
Backport
##########
src/main/java/org/apache/xml/security/utils/resolver/implementations/ResolverFragment.java:
##########
@@ -83,20 +86,19 @@ public XMLSignatureInput engineResolveURI(ResourceResolverContext context)
);
}
}
- LOG.debug(
- "Try to catch an Element with ID {} and Element was {}", id, selectedElem
+ LOG.log(Level.DEBUG,
+ "Try to catch an Element with ID {0} and Element was {1}", id, selectedElem
);
}
- XMLSignatureInput result = new XMLSignatureInput(selectedElem);
+ XMLSignatureInput result = new XMLSignatureNodeInput(selectedElem);
result.setSecureValidation(context.secureValidation);
result.setExcludeComments(true);
-
result.setMIMEType("text/xml");
- if (context.baseUri != null && context.baseUri.length() > 0) {
- result.setSourceURI(context.baseUri.concat(context.uriToResolve));
- } else {
+ if (context.baseUri == null || context.baseUri.isEmpty()) {
result.setSourceURI(context.uriToResolve);
+ } else {
+ result.setSourceURI(context.baseUri.concat(context.uriToResolve));
Review Comment:
backport
##########
src/main/java/org/apache/xml/security/utils/resolver/implementations/ResolverLocalFilesystem.java:
##########
@@ -74,24 +70,24 @@ public boolean engineCanResolveURI(ResourceResolverContext context) {
}
try {
- LOG.debug("I was asked whether I can resolve {}", context.uriToResolve);
+ LOG.log(Level.DEBUG, "I was asked whether I can resolve {0}", context.uriToResolve);
if (context.uriToResolve.startsWith("file:") || context.baseUri.startsWith("file:")) {
- LOG.debug("I state that I can resolve {}", context.uriToResolve);
+ LOG.log(Level.DEBUG, "I state that I can resolve {0}", context.uriToResolve);
return true;
}
} catch (Exception e) {
- LOG.debug(e.getMessage(), e);
+ LOG.log(Level.DEBUG, e.getMessage(), e);
}
- LOG.debug("But I can't");
+ LOG.log(Level.DEBUG, "But I can't");
return false;
}
private static URI getNewURI(String uri, String baseURI) throws URISyntaxException {
- URI newUri = null;
- if (baseURI == null || baseURI.length() == 0) {
+ final URI newUri;
+ if (baseURI == null || baseURI.isEmpty()) {
Review Comment:
backport
##########
src/main/java/org/apache/jcp/xml/dsig/internal/dom/DOMReference.java:
##########
@@ -118,7 +123,7 @@ public final class DOMReference extends DOMStructure
private Data derefData;
private InputStream dis;
private MessageDigest md;
- private Provider provider;
+ private final Provider provider;
Review Comment:
Could be backported?
##########
.github/codeql/santuario.qls:
##########
@@ -1,5 +1,3 @@
- import: codeql-suites/java-security-and-quality.qls
from: codeql-java
-- exclude:
- id: java/missing-override-annotation
Review Comment:
Could be backported in a separate PR?
##########
src/main/java/org/apache/jcp/xml/dsig/internal/dom/Utils.java:
##########
@@ -35,24 +35,23 @@
/**
* Miscellaneous static utility methods for use in JSR 105 RI.
- *
*/
public final class Utils {
- private Utils() {}
+ private Utils() {
Review Comment:
Could be backported?
##########
src/main/java/org/apache/xml/security/encryption/XMLCipher.java:
##########
@@ -3462,7 +3464,7 @@ public String getBaseNamespace() {
private class ReferenceListImpl implements ReferenceList {
private Class<?> sentry;
- private List<Reference> references;
+ private final List<Reference> references;
Review Comment:
The final changes in this class could be backported
##########
src/main/java/org/apache/xml/security/c14n/implementations/Canonicalizer20010315Excl.java:
##########
@@ -342,11 +342,11 @@ protected void circumventBugIfNeeded(XMLSignatureInput input)
if (!input.isNeedsToBeExpanded() || inclusiveNSSet.isEmpty()) {
return;
}
- Document doc = null;
- if (input.getSubNode() != null) {
- doc = XMLUtils.getOwnerDocument(input.getSubNode());
- } else {
+ Document doc;
Review Comment:
All changes in class could be backported?
##########
src/main/java/org/apache/xml/security/keys/KeyInfo.java:
##########
@@ -113,7 +114,7 @@ public class KeyInfo extends SignatureElementProxy {
/**
* Stores the individual (per-KeyInfo) {@link KeyResolverSpi}s
*/
- private List<KeyResolverSpi> internalKeyResolvers = new ArrayList<>();
+ private final List<KeyResolverSpi> internalKeyResolvers = new ArrayList<>();
Review Comment:
Backport
##########
src/main/java/org/apache/xml/security/signature/Manifest.java:
##########
@@ -55,16 +57,15 @@ public class Manifest extends SignatureElementProxy {
*/
public static final int MAXIMUM_REFERENCE_COUNT = 30;
- private static final org.slf4j.Logger LOG =
- org.slf4j.LoggerFactory.getLogger(Manifest.class);
+ private static final Logger LOG = System.getLogger(Manifest.class.getName());
private static Integer referenceCount =
AccessController.doPrivileged(
(PrivilegedAction<Integer>) () -> Integer.parseInt(System.getProperty("org.apache.xml.security.maxReferences",
Integer.toString(MAXIMUM_REFERENCE_COUNT))));
/** Field references */
- private List<Reference> references;
+ private final List<Reference> references;
Review Comment:
Backport
##########
src/main/java/org/apache/xml/security/stax/ext/ComparableType.java:
##########
@@ -21,9 +21,9 @@
/**
*/
-public abstract class ComparableType<T extends ComparableType> implements Comparable<T> {
+public abstract class ComparableType<T extends ComparableType<?>> implements Comparable<T> {
Review Comment:
Backport all class changes
##########
src/main/java/org/apache/xml/security/encryption/XMLCipherInput.java:
##########
@@ -44,11 +46,10 @@
*/
public class XMLCipherInput {
- private static final org.slf4j.Logger LOG =
- org.slf4j.LoggerFactory.getLogger(XMLCipherInput.class);
+ private static final Logger LOG = System.getLogger(XMLCipherInput.class.getName());
/** The data we are working with */
- private CipherData cipherData;
+ private final CipherData cipherData;
Review Comment:
Backport
##########
src/main/java/org/apache/xml/security/c14n/CanonicalizerSpi.java:
##########
@@ -42,13 +43,13 @@ public abstract class CanonicalizerSpi {
*
* @throws XMLParserException
* @throws java.io.IOException
- * @throws javax.xml.parsers.ParserConfigurationException
+ * @throws CanonicalizationException
Review Comment:
All changes in class could be backported?
##########
src/main/java/org/apache/xml/security/stax/impl/InputProcessorChainImpl.java:
##########
@@ -30,18 +32,16 @@
import org.apache.xml.security.stax.ext.InputProcessorChain;
import org.apache.xml.security.stax.ext.XMLSecurityConstants;
import org.apache.xml.security.stax.ext.stax.XMLSecEvent;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* Implementation of a InputProcessorChain
*
*/
public class InputProcessorChainImpl implements InputProcessorChain {
- protected static final transient Logger LOG = LoggerFactory.getLogger(InputProcessorChainImpl.class);
+ private static final Logger LOG = System.getLogger(InputProcessorChainImpl.class.getName());
- private List<InputProcessor> inputProcessors;
+ private final List<InputProcessor> inputProcessors;
Review Comment:
Backport
##########
src/main/java/org/apache/xml/security/stax/impl/processor/input/AbstractDecryptInputProcessor.java:
##########
@@ -620,7 +620,7 @@ public abstract class AbstractDecryptedEventReaderInputProcessor
private boolean encryptedHeader = false;
private final InboundSecurityToken inboundSecurityToken;
private boolean rootElementProcessed;
- private EncryptedDataType encryptedDataType;
+ private final EncryptedDataType encryptedDataType;
Review Comment:
Backport
##########
src/main/java/org/apache/xml/security/transforms/implementations/TransformXPath.java:
##########
@@ -88,9 +88,7 @@ protected XMLSignatureInput enginePerformTransform(
}
Node xpathnode = xpathElement.getFirstChild();
if (xpathnode == null) {
- throw new DOMException(
- DOMException.HIERARCHY_REQUEST_ERR, "Text must be in ds:Xpath"
- );
+ throw new DOMException(DOMException.HIERARCHY_REQUEST_ERR, "Text must be in ds:Xpath");
Review Comment:
backport
##########
src/main/java/org/apache/xml/security/stax/impl/processor/output/AbstractSignatureEndingOutputProcessor.java:
##########
@@ -56,18 +58,16 @@
import org.apache.xml.security.stax.securityToken.SecurityTokenProvider;
import org.apache.xml.security.utils.UnsyncBufferedOutputStream;
import org.apache.xml.security.utils.XMLUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import static org.apache.xml.security.algorithms.implementations.SignatureBaseRSA.SignatureRSASSAPSS.DigestAlgorithm.fromDigestAlgorithm;
/**
*/
public abstract class AbstractSignatureEndingOutputProcessor extends AbstractBufferingOutputProcessor {
- private static final transient Logger LOG = LoggerFactory.getLogger(AbstractSignatureEndingOutputProcessor.class);
+ private static final transient Logger LOG = System.getLogger(AbstractSignatureEndingOutputProcessor.class.getName());
- private List<SignaturePartDef> signaturePartDefList;
+ private final List<SignaturePartDef> signaturePartDefList;
Review Comment:
backport
##########
src/main/java/org/apache/xml/security/stax/impl/processor/output/AbstractSignatureEndingOutputProcessor.java:
##########
@@ -287,9 +287,9 @@ protected static class SignedInfoProcessor extends AbstractOutputProcessor {
private Transformer transformer;
private byte[] signatureValue;
private String inclusiveNamespacePrefixes;
- private SignatureAlgorithm signatureAlgorithm;
- private XMLSecStartElement xmlSecStartElement;
- private String signatureId;
+ private final SignatureAlgorithm signatureAlgorithm;
+ private final XMLSecStartElement xmlSecStartElement;
+ private final String signatureId;
Review Comment:
backport
##########
src/main/java/org/apache/xml/security/transforms/implementations/TransformXPath2Filter.java:
##########
@@ -85,10 +85,10 @@ protected XMLSignatureInput enginePerformTransform(
}
Document inputDoc = null;
Review Comment:
Backport
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1409033465
> @coheigea I'm sure 4/5 PRs would be doable indeed. I had just created an internal TODO issue for myself to look at exactly that ;)
If you can do that, I welcome it, otherwise I would have to come to that probably much later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1483075892
> Any update @arjantijms ?
It seems I can get there next week, Arjan has too much work on Faces now ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] github-code-scanning[bot] commented on a diff in pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#discussion_r1221887613
##########
src/main/java/org/apache/xml/security/stax/impl/util/DigestOutputStream.java:
##########
@@ -67,9 +66,9 @@
public byte[] getDigestValue() {
if (isDebugEnabled) {
- LOG.debug("Pre Digest: ");
- LOG.debug(stringBuilder.toString());
- LOG.debug("End pre Digest ");
+ LOG.log(Level.DEBUG, "Pre Digest: ");
+ LOG.log(Level.DEBUG, stringBuilder.toString());
Review Comment:
## Log Injection
This log entry depends on a [user-provided value](1).
[Show more details](https://github.com/apache/santuario-xml-security-java/security/code-scanning/1125)
##########
src/main/java/org/apache/xml/security/stax/impl/processor/input/AbstractDecryptInputProcessor.java:
##########
@@ -344,7 +343,8 @@
XMLStreamReader xmlStreamReader =
inputProcessorChain.getSecurityContext().<XMLInputFactory>get(
XMLSecurityConstants.XMLINPUTFACTORY).createXMLStreamReader(
- new MultiInputStream(prologInputStream, decryptInputStream, epilogInputStream), StandardCharsets.UTF_8.name());
+ new MultiInputStream(prologInputStream, decryptInputStream, epilogInputStream),
Review Comment:
## Potential input resource leak
This MultiInputStream is not always closed on method exit.
[Show more details](https://github.com/apache/santuario-xml-security-java/security/code-scanning/1121)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] dmatej closed pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "dmatej (via GitHub)" <gi...@apache.org>.
dmatej closed pull request #101: Small boost forward - aggregated changes from one stable stage to another
URL: https://github.com/apache/santuario-xml-security-java/pull/101
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on pull request #101: Small boost forward - aggregated changes from one stable stage to another
Posted by "coheigea (via GitHub)" <gi...@apache.org>.
coheigea commented on PR #101:
URL: https://github.com/apache/santuario-xml-security-java/pull/101#issuecomment-1600087208
@dmatej Thanks for all your work so far. Looking at the existing PR it would be useful still to extract all the "@Override" changes into a separate PR that could be backported. Or if you prefer I could do this change and you rebase the PR afterwards?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org