Thisura, Yes, I will review your PR 357 today.
On Thu, Jun 8, 2017 at 4:22 AM, Thisura Philips <[email protected]> wrote: > Hi Mark, > > I have updated the PR[1] <https://github.com/apache/fineract/pull/357> > adding new constant classes and making those sets protected, to make those > sets secure. > Please review and let me know if we need to update it. > > [1] https://github.com/apache/fineract/pull/357 > > On Thu, Jun 1, 2017 at 6:11 AM, Thisura Philips <[email protected]> > wrote: > >> 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) >> >> >> >> > > > -- > T.T.C Philips (BSc.Eng (Undergrad)) > Computer Science and Engineering, > Sri Lanka Institute of Information Technology(SLIIT) > > > >
