Re: [Codenarc-developer] Mass Assignment rule
Brought to you by:
chrismair
From: Chris M. <chr...@ea...> - 2014-02-04 23:02:49
|
Thanks Brian. >> I moved it to the Grails package but included it in both Grails and security rulesets. Is that a valid configuration? Unfortunately, no. Some of the CodeNarc infrastructure assumes only one rule per rule name, so it could cause confusion and unexpected results if you have the same rule in multiple rulesets. I can add a reference to the rule from the "security" ruleset page. I am fine with the new name. I see the pull request. Thanks for the contribution. Chris From: Brian Soby [mailto:bri...@ta...] Sent: Monday, February 03, 2014 2:41 PM To: Chris Mair Cc: Artur Gajowy; cod...@li... Subject: Re: [Codenarc-developer] Mass Assignment rule Hi Chris, I updated it based on your suggestions. Role-based rule groupings seem to make more sense to me than technology-based ones. For example, I only ever run the security ruleset since that's my role. I'm less concerned with the technology in which the issue is found. I can imagine the dev, QA, performance, etc folk would have similar views. Of course, I do see the need to exclude irrelevant rulesets that may lead to false positives. I moved it to the Grails package but included it in both Grails and security rulesets. Is that a valid configuration? Also, for naming, it's probably correctly called property binding but the class of vulnerability is almost exclusively referred to as mass assignment, mostly due to the GitHub incident. Due to it being the more recognizable name, would you mind me leaving it as mass assignment? http://en.wikipedia.org/wiki/Mass_assignment_vulnerability http://www.infoq.com/news/2012/03/GitHub-Compromised Thanks -Brian Soby On Sun, Feb 2, 2014 at 7:25 AM, Chris Mair <chr...@ea... <mailto:chr...@ea...> > wrote: Brian, Nice job on the rule. Is that GrailsDomainClass interface still necessary? If you decide to submit this as a contribution to CodeNarc: * Include the rule test if you submit a pull request * I may consider renaming the rule. It misled me at first because I expected it to be about multiple assignments. I looked at the Grails documentation <http://grails.org/doc/2.3.x/guide/single.html#dataBinding> , and they refer to this as data binding, and specifically as the "mass property binding mechanism". That is a mouthful, so I'm not sure what the best rule name would be - GrailsMassPropertyBindingRule? * I might move this into the Grails package and/or ruleset. I know it is also appropriate under "security". That is one of my regrets about the original design of this project - organizing around rulesets instead of something more flexible like tags. Thanks. Chris From: Brian Soby [mailto:bri...@ta... <mailto:bri...@ta...> ] Sent: Thursday, January 30, 2014 4:08 PM To: Chris Mair Cc: Artur Gajowy; cod...@li... <mailto:cod...@li...> Subject: Re: [Codenarc-developer] (no subject) As it turns out, CodeNarc can't detect the Grails interfaces so I ended up reverting to the basic property and variable name checks. It's not ideal, but I don't see another way at this point. I ran it through our production code base and it seems solid (no false positives and a good number of hits). It's possible that's just due to our devs sticking to convention and our codebase. I'm not sure what your bar for fragility is but here's the test (caveat: I started learning groovy 2 weeks ago): https://github.com/soby/CodeNarc/blob/master/src/main/groovy/org/codenarc/ru le/security/MassAssignmentRule.groovy On Wed, Jan 29, 2014 at 4:15 PM, Chris Mair <chr...@ea... <mailto:chr...@ea...> > wrote: Re: running custom rules from the Grails CodeNarc plugin, unfortunately: http://grails.org/plugin/codenarc Note that your custom ruleset cannot refer to a custom rule defined within the Grails application. This is due to a Grails classpath issue, discussed here <http://grails.1312388.n4.nabble.com/Can-a-plugin-use-an-application-level-c lass-td3330014.html> . It sounds like you could put the rule into another plugin or maybe a jar? I know, not cool. :( From: Brian Soby [mailto:bri...@ta... <mailto:bri...@ta...> ] Sent: Wednesday, January 29, 2014 6:27 PM To: Artur Gajowy Cc: cod...@li... <mailto:cod...@li...> Subject: Re: [Codenarc-developer] (no subject) Hi Artur, Changing the phase worked with my test case to detect the interface once I started using AstUtil.classNodeImplementsType for the check. The outstanding question I had was whether I could skip doing the grails import of org.codehaus.groovy.grails.commons.GrailsDomainClass by declaring a simple GrailsDomainClass interface in the rule and checking for that. It looks like some of the checks done by AstUtil.classNodeImplementsType may match on simple class name while omitting the package name, which presumably would work. The other problem I ran into was how to run my custom CodeNarc rules in the grails codenarc plugin. Any ideas here? I see comments saying it's not trivially done. Thanks -Brian Soby On Wed, Jan 29, 2014 at 2:06 PM, Artur Gajowy <art...@gm... <mailto:art...@gm...> > wrote: Did you change your rule's compilerPhase to SEMANTIC_ANALYSIS or greater? It should detect the GrailsDomainClass interface in your test, however I'm not sure if it would detect the implicit interface implementation done by Grails. Be aware of the limitations: http://codenarc.sourceforge.net/codenarc-enhanced-classpath-rules.html https://github.com/CodeNarc/CodeNarc/pull/27 https://github.com/CodeNarc/CodeNarc/issues/29 If, however, you're ok with running CodeNarc as a test in your project, the classpath problems won't affect you. On the other hand - CodeNarc users running it without Grails classes on classpath won't be able to benefit from the rule. If you decide to implement the rule - make sure you get the GrailsDomainClass interface's package right and test it on a real project :) Best regards, Artur Gajowy On 29 January 2014 19:21, Brian Soby <bri...@ta... <mailto:bri...@ta...> > wrote: Hi all, I'm working on a mass assignment rule (http://sourceforge.net/p/codenarc/feature-requests/392/) and I can't seem to get type or interface based detection to work. Here's my test code: public interface GrailsDomainClass {} class Person implements GrailsDomainClass { String name Boolean isAdmin } params = [name: 'John', isAdmin: true] def person = new Person(params) Ideally, I'd 1) visit the Person constructor 2) Check that it implements GrailsDomainClass 3) and check that the parameter implements java.util.Map. Unfortunately, getInterfaces()/getAllInterfaces() seems to always return an empty set. I've resorted to checking for a variable named "params" but that seems too fragile. Is there anything I can do to have more robust type and interface information available? Thanks -Brian Soby ---------------------------------------------------------------------------- -- WatchGuard Dimension instantly turns raw network data into actionable security intelligence. It gives you real-time visual feedback on key security issues and trends. Skip the complicated setup - simply import a virtual appliance and go from zero to informed in seconds. http://pubads.g.doubleclick.net/gampad/clk?id=123612991 <http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktr k> &iu=/4140/ostg.clktrk _______________________________________________ Codenarc-developer mailing list Cod...@li... <mailto:Cod...@li...> https://lists.sourceforge.net/lists/listinfo/codenarc-developer No virus found in this message. Checked by AVG - www.avg.com <http://www.avg.com> Version: 2014.0.4259 / Virus Database: 3684/7045 - Release Date: 01/30/14 No virus found in this message. Checked by AVG - www.avg.com <http://www.avg.com> Version: 2014.0.4259 / Virus Database: 3684/7054 - Release Date: 02/02/14 |