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/1uLk3YPcjnXk7RqF8etsTzIuN59CDU > 6sgBxpZul__1V4?usp=gmail > [2] https://drive.google.com/open?id=1TdwwHM2K1gMb6qILEX7gmzU8dVXcH > GBdh569aFJfB2U&usp=gmail > [3] https://github.com/apache/fineract/blob/develop/ > fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/ > 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=0B6WV3fK5Tak7Uy1uOWk0SW81W >>>> m8>, >>>> > >>> >>> 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=0B6WV3fK5Tak7ZVJkVGV3WVZ3O >>>> E0>. >>>> > >>> >>> >>>> > >>> >>> 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) > > > >
