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/06/19 22:32:25 UTC
[GitHub] [fineract] nnatarajan opened a new pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
nnatarajan opened a new pull request #1090:
URL: https://github.com/apache/fineract/pull/1090
…n issues (and other security related problems)
## 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] thesmallstar commented on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-646886423
@nnatarajan you have spotless violations in your code in the files you didn't change. I am not sure what's going on, but maybe you can run ./gradlew spotlessApply and rebase this?
----------------------------------------------------------------
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] nnatarajan commented on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan commented on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-647234793
@thesmallstar , I just found out that this was actually your issue and I mistakenly took this on. I am going to close this PR as you are much more familiar with this.
----------------------------------------------------------------
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 a change in pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#discussion_r443158341
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/filter/TenantAwareTenantIdentifierFilter.java
##########
@@ -97,7 +97,7 @@ public void doFilter(final ServletRequest req, final ServletResponse res, final
// allows for Cross-Origin
// Requests (CORs) to be performed against the platform API.
- response.setHeader("Access-Control-Allow-Origin", "*");
+ response.setHeader("Access-Control-Allow-Origin", "Fineract-Platform-TenantId");
Review comment:
> entered what seemed most logical in the class.
I don't think this hard-coded String (`Fineract-Platform-TenantId`) makes any sense at all, and I suspect that this would break UI clients.
----------------------------------------------------------------
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] nnatarajan removed a comment on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan removed a comment on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-646889240
@thesmallstar all the checks ran ok before I sent PR, I will look into it
----------------------------------------------------------------
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 a change in pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#discussion_r443108066
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/filter/TenantAwareTenantIdentifierFilter.java
##########
@@ -97,7 +97,7 @@ public void doFilter(final ServletRequest req, final ServletResponse res, final
// allows for Cross-Origin
// Requests (CORs) to be performed against the platform API.
- response.setHeader("Access-Control-Allow-Origin", "*");
+ response.setHeader("Access-Control-Allow-Origin", "Fineract-Platform-TenantId");
Review comment:
I don't think this is correct? But it's been YEARS since I've last dealt with CORS...
----------------------------------------------------------------
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] thesmallstar commented on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-647180351
@nnatarajan do you know if some reports are generated for possible SQL injection codes? Do we need to add any config? If no, does this really help to find SQL injections? (since the build passes without any significant changes in the code).
----------------------------------------------------------------
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] nnatarajan commented on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan commented on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-647017536
@vorburger , it seems that both checks passed with rebase. Please let me know if I should investigate CORS further. I don't mind if this is a concern.
----------------------------------------------------------------
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] thesmallstar commented on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-646888801
This should not fail due to "Spotless" once #1091 is merged (and this branch is rebased).
----------------------------------------------------------------
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] nnatarajan closed pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan closed pull request #1090:
URL: https://github.com/apache/fineract/pull/1090
----------------------------------------------------------------
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] thesmallstar edited a comment on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-647235452
no no! you can pick this up, I did some research so I had those doubts. I thought maybe you could answer them. I barely know anything about this :P
----------------------------------------------------------------
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] nnatarajan commented on a change in pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan commented on a change in pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#discussion_r443153594
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/filter/TenantAwareTenantIdentifierFilter.java
##########
@@ -97,7 +97,7 @@ public void doFilter(final ServletRequest req, final ServletResponse res, final
// allows for Cross-Origin
// Requests (CORs) to be performed against the platform API.
- response.setHeader("Access-Control-Allow-Origin", "*");
+ response.setHeader("Access-Control-Allow-Origin", "Fineract-Platform-TenantId");
Review comment:
The star must be replaced by a string and I entered what seemed most logical in the class.
----------------------------------------------------------------
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] nnatarajan commented on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan commented on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-646889669
@thesmallstar , thank you for letting me know.
----------------------------------------------------------------
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] nnatarajan commented on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan commented on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-646895987
@thesmallstar, I appreciate your help. I ran ./gradlew spotlessApply, which did update five files. I will wait for 1091 merge.
----------------------------------------------------------------
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] nnatarajan commented on a change in pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan commented on a change in pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#discussion_r443132623
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/filter/TenantAwareTenantIdentifierFilter.java
##########
@@ -97,7 +97,7 @@ public void doFilter(final ServletRequest req, final ServletResponse res, final
// allows for Cross-Origin
// Requests (CORs) to be performed against the platform API.
- response.setHeader("Access-Control-Allow-Origin", "*");
+ response.setHeader("Access-Control-Allow-Origin", "Fineract-Platform-TenantId");
Review comment:
@vorburger When I ran this check locally, the build passed all tests. Can you let me know what you are looking for specifically?
----------------------------------------------------------------
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] thesmallstar commented on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-647235452
no no! you can pick this up, I did some research so I had those doubts. I thought maybe you could answer them.
----------------------------------------------------------------
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] nnatarajan commented on a change in pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan commented on a change in pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#discussion_r443132941
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/filter/TenantAwareTenantIdentifierFilter.java
##########
@@ -97,7 +97,7 @@ public void doFilter(final ServletRequest req, final ServletResponse res, final
// allows for Cross-Origin
// Requests (CORs) to be performed against the platform API.
- response.setHeader("Access-Control-Allow-Origin", "*");
+ response.setHeader("Access-Control-Allow-Origin", "Fineract-Platform-TenantId");
Review comment:
The files updated were related PR 1091. After the fact, the same updates were requested on the develop branch. The failing checks were independent 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] nnatarajan commented on pull request #1090: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injectio…
Posted by GitBox <gi...@apache.org>.
nnatarajan commented on pull request #1090:
URL: https://github.com/apache/fineract/pull/1090#issuecomment-646889240
@thesmallstar all the checks ran ok before I sent PR, I will look into it
----------------------------------------------------------------
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