Hi Mark, That's great. If I get the consent to create new constant classes I will do that since that is the most appropriate solution. And will modify the PR and will update the document.
Thanks & Regards On Thu, Jun 1, 2017 at 3:08 AM, Mark Reynolds <[email protected]> wrote: > Thisura, > > Your second PR is also fine. I am moving forward to write a brief document > about the security > analysis, which you will get Thursday. > > On Wed, May 31, 2017 at 4:08 PM, Mark Reynolds <[email protected]> wrote: > >> Thisura, >> >> My impression is that you add to the API, but you cannot modify the >> argument list of any existing method. >> Therefore, I think adding constant classes, and also removing these >> constants from the interfaces, is fine. >> I have reviewed your first PR and it looks great. I am working on your >> second PR right now. I will also >> create a brief document that addresses your security analysis. >> >> - Mark >> >> On Mon, May 29, 2017 at 10:59 PM, Thisura Philips <[email protected]> >> wrote: >> >>> Hi all, >>> >>> Please note the that the documents[1][2] are updated with the latest >>> findings. There are suggestions to improve the fixes as per my >>> understanding. I am under the the impression of not doing API changes. >>> There fore didn't create any new classes for constants. I have raised this >>> problem above in this thread. AFAIK It would be better if we can create >>> constant classes to keep these constant sets rather than trying to keep >>> them in interfaces (as it was) or in constant classes in a seperate package >>> (where we can't use protected key word) >>> >>> The problem with keeping those in the interfaces is the following >>> vulnerabilities. >>> >>> - MITRE, CWE-582 <http://cwe.mitre.org/data/definitions/582.html> - >>> Array Declared Public, Final, and Static >>> - MITRE, CWE-607 <http://cwe.mitre.org/data/definitions/607.html> - >>> Public Static Final Field References Mutable Object >>> >>> As per the fix, I have moved most of those sets which were in interfaces >>> to the respective classes and made them private. As all of them were only >>> used in that class it is not a problem. >>> >>> But if we take a class like DepositApiConstants.java [3] >>> <https://github.com/apache/fineract/blob/develop/fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/DepositsApiConstants.java>, >>> it is better if we can create a new consant class in the package >>> apache.fineract.portfolio.savings.data package and make them protected. >>> Or else we can extend the constant class from the consuming class, if the >>> consuming classes don't extend other classes (which is the case at the >>> moment). Then we can make those Sets protected easily. >>> >>> But doing so is a API change. However I kept those to discuss, since >>> there were major changes. Lets dicuss and decide what to do with theose >>> yellow colured issues in the document >>> <https://docs.google.com/spreadsheets/d/1uLk3YPcjnXk7RqF8etsTzIuN59CDU6sgBxpZul__1V4?usp=gmail> >>> . >>> >>> [1] https://docs.google.com/spreadsheets/d/1uLk3YPcjnXk7RqF8 >>> etsTzIuN59CDU6sgBxpZul__1V4?usp=gmail >>> [2] https://drive.google.com/open?id=1TdwwHM2K1gMb6qILEX7gmz >>> U8dVXcHGBdh569aFJfB2U&usp=gmail >>> [3] https://github.com/apache/fineract/blob/develop/fineract >>> -provider/src/main/java/org/apache/fineract/portfolio/saving >>> s/DepositsApiConstants.java >>> >>> Thanks and Regards. >>> >>> On Tue, May 30, 2017 at 5:13 AM, Ed Cable <[email protected]> wrote: >>> >>>> Adding Nazeer too. >>>> >>>> Ed >>>> >>>> On May 27, 2017 9:43 PM, "Thisura Philips" <[email protected]> >>>> wrote: >>>> >>>>> Hi all, >>>>> >>>>> I have sent a PR <https://github.com/apache/fineract/pull/357> [1] >>>>> resolving the rest of the issues of FINERACT-436 >>>>> <https://issues.apache.org/jira/browse/FINERACT-436> [2]. For >>>>> tracking purpose I have created a new ticket at FINERACT-470 >>>>> <https://issues.apache.org/jira/browse/FINERACT-470>. This is the >>>>> second PR connected with the previously merged PR >>>>> <https://github.com/apache/fineract/pull/343> [3]. >>>>> >>>>> Please review and let me know if any thing is needed to be changed. >>>>> >>>>> I am moving on to fixing the FINERACT-437 >>>>> <https://issues.apache.org/jira/browse/FINERACT-437>. >>>>> >>>>> [1] https://github.com/apache/fineract/pull/357 >>>>> [2] https://issues.apache.org/jira/browse/FINERACT-436 >>>>> [3] https://github.com/apache/fineract/pull/343 >>>>> >>>>> Thanks & Regard >>>>> >>>>> On Thu, May 11, 2017 at 1:58 AM, Ed Cable <[email protected]> wrote: >>>>> >>>>>> Thisura, I have cc'd Mark on this email >>>>>> >>>>>> On May 10, 2017 14:07, "Thisura Philips" <[email protected]> >>>>>> wrote: >>>>>> >>>>>> > Hi Nikhil, >>>>>> > >>>>>> > I am really glad to get selected to GOSC 2017. I have being looking >>>>>> on TOIF >>>>>> > a little bit in the past few days. First I will complete the SONAR >>>>>> issues. >>>>>> > >>>>>> > Since Mark Reynolds is the mentor of the project, I would like to >>>>>> get him >>>>>> > in this thread too. Couldn't find his email though. Please some one >>>>>> provide >>>>>> > me the email of Mark. >>>>>> > >>>>>> > Thanks & Regards >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>> > On Sun, Apr 30, 2017 at 7:41 AM, Thisura Philips < >>>>>> [email protected]> >>>>>> > wrote: >>>>>> > >>>>>> > > Hi all, >>>>>> > > >>>>>> > > I have sent the PR [1] >>>>>> > > <https://github.com/apache/incubator-fineract/pull/343> >>>>>> including the >>>>>> > > changes needed to resolve the FINERACT-436 [2] >>>>>> > > <https://issues.apache.org/jira/browse/FINERACT-436> in >>>>>> accounting and >>>>>> > > infrastructure packages. Please see the updated scanning report >>>>>> [3] >>>>>> > > <https://docs.google.com/spreadsheets/d/1uLk3YPcjnXk7RqF8ets >>>>>> TzIuN59CDU >>>>>> > 6sgBxpZul__1V4/> >>>>>> > > . >>>>>> > > >>>>>> > > It would be great if you can review and give feedback to continue >>>>>> fixing >>>>>> > > other vulnerabilities. >>>>>> > > >>>>>> > > I have done the LAPSE scanning for a part of accounting package >>>>>> as well. >>>>>> > > Will send the results soon. >>>>>> > > >>>>>> > > [1] https://github.com/apache/incubator-fineract/pull/343 >>>>>> > > [2] https://issues.apache.org/jira/browse/FINERACT-436 >>>>>> > > [3] https://docs.google.com/spreadsheets/d/ >>>>>> > 1uLk3YPcjnXk7RqF8etsTzIuN59CDU >>>>>> > > 6sgBxpZul__1V4/ >>>>>> > > >>>>>> > > Thanks & Regards >>>>>> > > >>>>>> > > On Sat, Apr 29, 2017 at 8:12 AM, Thisura Philips < >>>>>> [email protected]> >>>>>> > > wrote: >>>>>> > > >>>>>> > >> Hi Myrle, >>>>>> > >> >>>>>> > >> Agreed with the concern in doing an API change. >>>>>> > >> >>>>>> > >> The problem is any one can change the values in these arrays >>>>>> since they >>>>>> > >> are accessible public. It is nice if we can protect these >>>>>> arrays, (set) >>>>>> > as >>>>>> > >> they are mutable. >>>>>> > >> But as we know we can't use protected or private access >>>>>> modifiers in an >>>>>> > >> interface. It would be perfect if we can move the Sets in to the >>>>>> > respective >>>>>> > >> classes where they are used. >>>>>> > >> >>>>>> > >> supportedParameters = > org.apache.fineract.accounting. >>>>>> > >> provisioning.serialization.ProvisioningEntriesDefinitionJ >>>>>> > sonDeserializer >>>>>> > >> (Used only in this class) >>>>>> > >> >>>>>> > >> PROVISIONING_ENTRY_PARAMETERS => org.apache.fineract.accounting >>>>>> > >> .provisioning.api.ProvisioningEntriesApiResource (Used only in >>>>>> this >>>>>> > >> class) >>>>>> > >> >>>>>> > >> ALL_PROVISIONING_ENTRIES => org.apache.fineract.accounting >>>>>> > >> .provisioning.api.ProvisioningEntriesApiResource (Used only in >>>>>> this >>>>>> > >> class) >>>>>> > >> >>>>>> > >> Since all of these sets are used in the mentioned classes only, >>>>>> we can >>>>>> > >> make them private. >>>>>> > >> >>>>>> > >> >>>>>> > >> Thanks & Regards >>>>>> > >> >>>>>> > >> >>>>>> > >> On Fri, Apr 28, 2017 at 5:49 PM, Myrle Krantz <[email protected]> >>>>>> wrote: >>>>>> > >> >>>>>> > >>> By constant class Thisura, do you mean a final class with a >>>>>> private >>>>>> > >>> constructor? >>>>>> > >>> >>>>>> > >>> It would be possible to change >>>>>> > >>> org.apache.fineract.accounting.provisioning.constant.Provisi >>>>>> > >>> oningEntriesApiConstants. >>>>>> > >>> This would be a breaking change though. Given that the >>>>>> interface has >>>>>> > >>> no methods, and no one is likely to have derived from it, >>>>>> there's >>>>>> > >>> probably no code to break by changing it, but you still have to >>>>>> answer >>>>>> > >>> the question "Why would I change that?" >>>>>> > >>> >>>>>> > >>> Yes, I know that Bloch ("Effective Java", 2nd Ed., Chapter 19) >>>>>> said >>>>>> > >>> it's bad. But his arguments only make sense when referring to >>>>>> an >>>>>> > >>> interface which contains methods. This interface doesn't. So >>>>>> given >>>>>> > >>> that Apache Fineract is communicated with over a REST >>>>>> interface, what >>>>>> > >>> harm does this interface cause that would justify making an >>>>>> > >>> API-breaking change (though a minor one) to remediate the >>>>>> situation? >>>>>> > >>> >>>>>> > >>> Best Regards, >>>>>> > >>> Myrle >>>>>> > >>> >>>>>> > >>> >>>>>> > >>> On Sun, Apr 23, 2017 at 8:00 PM, Thisura Philips < >>>>>> [email protected] >>>>>> > > >>>>>> > >>> wrote: >>>>>> > >>> > Hi all, >>>>>> > >>> > >>>>>> > >>> > I have done some of the fixes for FINERACT-436 >>>>>> > >>> > <https://issues.apache.org/jira/browse/FINERACT-436>. Please >>>>>> see the >>>>>> > >>> > updated document >>>>>> > >>> > <https://drive.google.com/open?id=1uLk3YPcjnXk7RqF8etsTzIuN5 >>>>>> > >>> 9CDU6sgBxpZul__1V4> >>>>>> > >>> > . >>>>>> > >>> > >>>>>> > >>> > Is there any particular reason to >>>>>> > >>> > have org.apache.fineract.accounting >>>>>> .provisioning.constant.Provisi >>>>>> > >>> oningEntriesApiConstants >>>>>> > >>> > as an interface. It is true that variables in interfaces are >>>>>> static, >>>>>> > >>> final >>>>>> > >>> > by default. But yet this can cause the following >>>>>> vulnerabilities. >>>>>> > >>> > >>>>>> > >>> > - MITRE, CWE-582 <http://cwe.mitre.org/data/def >>>>>> initions/582.html> >>>>>> > - >>>>>> > >>> > Array Declared Public, Final, and Static >>>>>> > >>> > - MITRE, CWE-607 <http://cwe.mitre.org/data/def >>>>>> initions/607.html> >>>>>> > - >>>>>> > >>> > Public Static Final Field References Mutable Object >>>>>> > >>> > >>>>>> > >>> > >>>>>> > >>> > Can't we change this to a constant class? ASFAIK this is the >>>>>> correct >>>>>> > >>> way to >>>>>> > >>> > maintain a set of constants. >>>>>> > >>> > >>>>>> > >>> > Thanks & Regards >>>>>> > >>> > >>>>>> > >>> > >>>>>> > >>> > On Sun, Apr 23, 2017 at 5:53 PM, Thisura Philips < >>>>>> > [email protected] >>>>>> > >>> > >>>>>> > >>> > wrote: >>>>>> > >>> > >>>>>> > >>> >> Hi all, >>>>>> > >>> >> >>>>>> > >>> >> I have created two tickets [1][2] to track the fixes for >>>>>> security >>>>>> > >>> >> vulnerabilities reported by sonar. >>>>>> > >>> >> Thanks & Regards. >>>>>> > >>> >> [1] https://issues.apache.org/jira/browse/FINERACT-436 >>>>>> > >>> >> [2] https://issues.apache.org/jira/browse/FINERACT-437 >>>>>> > >>> >> >>>>>> > >>> >> On Fri, Apr 21, 2017 at 10:31 AM, Thisura Philips < >>>>>> > >>> [email protected]> >>>>>> > >>> >> wrote: >>>>>> > >>> >> >>>>>> > >>> >>> Hi all, >>>>>> > >>> >>> >>>>>> > >>> >>> As per the long discussion in the thread "[Mifos-developer] >>>>>> > >>> Application >>>>>> > >>> >>> for GSOC 2017( Static Analysis of Apache Fineract )", I have >>>>>> > >>> >>> >>>>>> > >>> >>> * done the static analysis with SonarQube >>>>>> > >>> >>> * generated the vulnerability report, - sonarlint report [1] >>>>>> > >>> >>> <https://drive.google.com/open >>>>>> ?id=0B6WV3fK5Tak7Uy1uOWk0SW81Wm8>, >>>>>> > >>> >>> sonarqube <https://drive.google.com/open >>>>>> > >>> ?id=0B6WV3fK5Tak7OHJENF9oZFE2X2c> report >>>>>> > >>> >>> [2] <https://drive.google.com/open >>>>>> ?id=0B6WV3fK5Tak7OHJENF9oZFE2X2c >>>>>> > > >>>>>> > >>> >>> * summarized >>>>>> > >>> >>> <https://drive.google.com/open >>>>>> ?id=1TdwwHM2K1gMb6qILEX7gmzU8d >>>>>> > >>> VXcHGBdh569aFJfB2U> >>>>>> > >>> >>> [3] the types of vulnerabilities, >>>>>> > >>> >>> * attended each of those vulnerabilities to check whether >>>>>> they are >>>>>> > >>> not >>>>>> > >>> >>> false positives and >>>>>> > >>> >>> * prepared the checklist [4] >>>>>> > >>> >>> <https://drive.google.com/open >>>>>> ?id=1uLk3YPcjnXk7RqF8etsTzIuN5 >>>>>> > >>> 9CDU6sgBxpZul__1V4> >>>>>> > >>> >>> of vulnerabilities with fixes >>>>>> > >>> >>> >>>>>> > >>> >>> All the reports which are generated using different >>>>>> plugins, tools >>>>>> > >>> can be >>>>>> > >>> >>> found here [5] >>>>>> > >>> >>> <https://drive.google.com/open >>>>>> ?id=0B6WV3fK5Tak7ZVJkVGV3WVZ3OE0>. >>>>>> > >>> >>> >>>>>> > >>> >>> Now we can go ahead and do the necessary changes to fix the >>>>>> > reported >>>>>> > >>> >>> vulnerabilities in the codebase. I am looking forward to >>>>>> creating >>>>>> > >>> tickets >>>>>> > >>> >>> for each type of issues reported in summary. >>>>>> > >>> >>> >>>>>> > >>> >>> We need to verify the problems (vulnerabilities[4] >>>>>> > >>> >>> <https://drive.google.com/open >>>>>> ?id=1uLk3YPcjnXk7RqF8etsTzIuN5 >>>>>> > >>> 9CDU6sgBxpZul__1V4>) >>>>>> > >>> >>> and solutions that I have suggested in the summary [3] >>>>>> > >>> >>> <https://drive.google.com/open >>>>>> ?id=1TdwwHM2K1gMb6qILEX7gmzU8d >>>>>> > >>> VXcHGBdh569aFJfB2U> >>>>>> > >>> >>> . >>>>>> > >>> >>> >>>>>> > >>> >>> According to the findings [4] >>>>>> > >>> >>> <https://drive.google.com/open >>>>>> ?id=1uLk3YPcjnXk7RqF8etsTzIuN5 >>>>>> > >>> 9CDU6sgBxpZul__1V4>, >>>>>> > >>> >>> I will create two tickets for co-related fixes for each >>>>>> topic >>>>>> > (type >>>>>> > >>> of >>>>>> > >>> >>> vulnerability) such as >>>>>> > >>> >>> >>>>>> > >>> >>> * Mutable fields should not be "public static" && "enum" >>>>>> fields >>>>>> > >>> should >>>>>> > >>> >>> not be publicly mutable && "public static" fields should be >>>>>> > constant >>>>>> > >>> >>> * Generic exceptions should never be thrown && Throwable >>>>>> and Error >>>>>> > >>> should >>>>>> > >>> >>> not be caught >>>>>> > >>> >>> >>>>>> > >>> >>> Expect the community ideas regarding this to validate the >>>>>> suggested >>>>>> > >>> >>> solutions. >>>>>> > >>> >>> >>>>>> > >>> >>> [1] https://drive.google.com/open? >>>>>> id=0B6WV3fK5Tak7Uy1uOWk0SW81Wm8 >>>>>> > >>> >>> [2] https://drive.google.com/open? >>>>>> id=0B6WV3fK5Tak7OHJENF9oZFE2X2c >>>>>> > >>> >>> [3] https://drive.google.com/open? >>>>>> id=1TdwwHM2K1gMb6qILEX7gmz >>>>>> > >>> >>> U8dVXcHGBdh569aFJfB2U >>>>>> > >>> >>> [4] https://drive.google.com/open? >>>>>> id=1uLk3YPcjnXk7RqF8etsTzI >>>>>> > >>> >>> uN59CDU6sgBxpZul__1V4 >>>>>> > >>> >>> [5] https://drive.google.com/open? >>>>>> id=0B6WV3fK5Tak7ZVJkVGV3WVZ3OE0 >>>>>> > >>> >>> >>>>>> > >>> >>> Thanks & regards >>>>>> > >>> >>> -- >>>>>> > >>> >>> T.T.C Philips (BSc.Eng (Undergrad)) >>>>>> > >>> >>> Computer Science and Engineering, >>>>>> > >>> >>> Sri Lanka Institute of Information Technology(SLIIT) >>>>>> > >>> >>> >>>>>> > >>> >>> >>>>>> > >>> >>> >>>>>> > >>> >>> >>>>>> > >>> >> >>>>>> > >>> >> >>>>>> > >>> >> -- >>>>>> > >>> >> T.T.C Philips (BSc.Eng (Undergrad)) >>>>>> > >>> >> Computer Science and Engineering, >>>>>> > >>> >> Sri Lanka Institute of Information Technology(SLIIT) >>>>>> > >>> >> >>>>>> > >>> >> >>>>>> > >>> >> >>>>>> > >>> >> >>>>>> > >>> > >>>>>> > >>> > >>>>>> > >>> > -- >>>>>> > >>> > T.T.C Philips (BSc.Eng (Undergrad)) >>>>>> > >>> > Computer Science and Engineering, >>>>>> > >>> > Sri Lanka Institute of Information Technology(SLIIT) >>>>>> > >>> >>>>>> > >> >>>>>> > >> >>>>>> > >> >>>>>> > >> -- >>>>>> > >> T.T.C Philips (BSc.Eng (Undergrad)) >>>>>> > >> Computer Science and Engineering, >>>>>> > >> Sri Lanka Institute of Information Technology(SLIIT) >>>>>> > >> >>>>>> > >> >>>>>> > >> >>>>>> > >> >>>>>> > > >>>>>> > > >>>>>> > > -- >>>>>> > > T.T.C Philips (BSc.Eng (Undergrad)) >>>>>> > > Computer Science and Engineering, >>>>>> > > Sri Lanka Institute of Information Technology(SLIIT) >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> > >>>>>> > >>>>>> > -- >>>>>> > T.T.C Philips (BSc.Eng (Undergrad)) >>>>>> > Computer Science and Engineering, >>>>>> > Sri Lanka Institute of Information Technology(SLIIT) >>>>>> > >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> T.T.C Philips (BSc.Eng (Undergrad)) >>>>> Computer Science and Engineering, >>>>> Sri Lanka Institute of Information Technology(SLIIT) >>>>> >>>>> >>>>> >>>>> >>> >>> >>> -- >>> T.T.C Philips (BSc.Eng (Undergrad)) >>> Computer Science and Engineering, >>> Sri Lanka Institute of Information Technology(SLIIT) >>> >>> >>> >>> >> > -- T.T.C Philips (BSc.Eng (Undergrad)) Computer Science and Engineering, Sri Lanka Institute of Information Technology(SLIIT)
