You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/08/19 11:55:49 UTC
[GitHub] [fineract] maektwain opened a new pull request #1259: Excluding commented to prevent data leak FINERACT-1057
maektwain opened a new pull request #1259:
URL: https://github.com/apache/fineract/pull/1259
## Description
Describe the changes made and why they were made. Ignore if these details are present on the associated Jira ticket
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
- [ ] Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
- [ ] API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm has been updated with details of any API changes.
- [ ] Integration tests have been created/updated for verifying the changes made.
- [ ] All Integrations tests are passing with the new commits.
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)
Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] maektwain commented on pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
maektwain commented on pull request #1259:
URL: https://github.com/apache/fineract/pull/1259#issuecomment-677143461
I am able to build this locally with Duplicate classpath problem since this is a problem with the jaxapi.
In my opinion when we build for oauth we need to ensure it removes or comment out the jaxb-api
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] maektwain commented on pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
maektwain commented on pull request #1259:
URL: https://github.com/apache/fineract/pull/1259#issuecomment-677159707
Correct
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1259:
URL: https://github.com/apache/fineract/pull/1259#issuecomment-676965591
> When you run -Psecurity=oauth, you will see Hikari data leak issue
I've built the jar with -Psecurity=oauth and have ran it - but did not see any evidence of data leaks. Is there something specific I need to do? I'm not familiar with the oauth set-up so would very much appreciate some details on how to reproduce the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1259:
URL: https://github.com/apache/fineract/pull/1259#issuecomment-677292194
So I looked at the dependency tree with and without the excludes - and that clearly shows the problem caused by removing the exclude:
With excludes, both the API and the runtime are pointing to the same version (2.3.3) - which sounds good. We are using the Glassfish JAR for the implementation and the Jakarta JAR for the API - I don't think that's an issue, but of course we could try to switch both to use the Jakarta JAR and see if that makes any difference.
However, if we remove the excludes, we seem to have two different versions of API in the classpath (2.3.1 and 2.3.3) as well as two different versions of the runtime (2.2.3-1 and 2.3.3). Which really does not sound good.
So I don't think this PR - i.e. removing the excludes - is the right solution. Instead would be good to understand what is this data leak and why is it caused by the JAXB version. Is there a JAXB bug that you've been able to trace this to? Or any other insights? I still don't know how to reproduce this so not sure I can help much here...
For reference, here are the dependencies
With exclude:
compile:
+--- jakarta.xml.bind:jakarta.xml.bind-api -> 2.3.3
runtime:
+--- org.ehcache:ehcache -> 3.8.1
| \--- org.glassfish.jaxb:jaxb-runtime:2.3.1 -> 2.3.3
| +--- jakarta.xml.bind:jakarta.xml.bind-api:2.3.3 (*)
+--- jakarta.xml.bind:jakarta.xml.bind-api -> 2.3.3
Without exclude:
compile
+--- com.sun.jersey:jersey-json -> 1.19.4
| +--- com.sun.xml.bind:jaxb-impl:2.2.3-1
| | \--- javax.xml.bind:jaxb-api:2.2.2 -> 2.3.1
+--- jakarta.xml.bind:jakarta.xml.bind-api -> 2.3.3
runtime
+--- com.sun.jersey:jersey-json -> 1.19.4
| +--- com.sun.xml.bind:jaxb-impl:2.2.3-1
| | \--- javax.xml.bind:jaxb-api:2.2.2 -> 2.3.1
+--- org.dom4j:dom4j -> 2.1.3
| \--- javax.xml.bind:jaxb-api:2.2.12 -> 2.3.1 (*)
+--- org.ehcache:ehcache -> 3.8.1
| \--- org.glassfish.jaxb:jaxb-runtime:2.3.1 -> 2.3.3
| +--- jakarta.xml.bind:jakarta.xml.bind-api:2.3.3 (*)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] maektwain closed pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
maektwain closed pull request #1259:
URL: https://github.com/apache/fineract/pull/1259
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1259:
URL: https://github.com/apache/fineract/pull/1259#issuecomment-677151062
So this exclude must have been put in place because there are two versions of JAXB in the classpath without it. If we merge this PR, there will again be two versions in the classpath - which clearly is not a good idea.
So if oauth has some problem with the other JAXB version, perhaps the right solution is to exclude the other one and keep this one instead? That way we would still have only one version in the classpath, but one that oauth works with. Maybe that would be a better solution?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1259:
URL: https://github.com/apache/fineract/pull/1259#issuecomment-680418424
It appears this PR overlaps with #1114 and #1258? Please do not open several parallel competing PRs for the same JIRA, but work on a single 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1259:
URL: https://github.com/apache/fineract/pull/1259#issuecomment-676870140
@maektwain To better understand the problem that this is looking to fix, could you share some more details on the "data leak" this addresses - how did you identify this, and how can I reproduce this data leak? Thanks!
Also it looks like this doesn't pass the first phase of the Travis build i.e. ./gradlew licenseMain licenseTest licenseIntegrationTest check. Does this phase pass for you locally? If not, I'd suggest first looking at any issues that are flagged by that test and resolving those. If it passes for you locally, then we need to understand why Travis gets a different result...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] maektwain commented on pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
maektwain commented on pull request #1259:
URL: https://github.com/apache/fineract/pull/1259#issuecomment-677010754
This is specifically on run time when running it via `bootRun` , I have fixed other issues also meanwhile
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] maektwain commented on pull request #1259: Excluding commented to prevent data leak FINERACT-1057
Posted by GitBox <gi...@apache.org>.
maektwain commented on pull request #1259:
URL: https://github.com/apache/fineract/pull/1259#issuecomment-676904351
When you run -Psecurity=oauth, you will see Hikari data leak issue
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org