Re: [Codenarc-developer] Mass Assignment rule

[Brian S. <bri...@ta...>]

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...> 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...]
> *Sent:* Thursday, January 30, 2014 4:08 PM
> *To:* Chris Mair
> *Cc:* Artur Gajowy; 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/rule/security/MassAssignmentRule.groovy
>
>
>
> On Wed, Jan 29, 2014 at 4:15 PM, Chris Mair <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-class-td3330014.html>.*
>
>
>
> It sounds like you could put the rule into another plugin or maybe a jar?
> I know, not cool... L
>
>
>
> *From:* Brian Soby [mailto:bri...@ta...]
> *Sent:* Wednesday, January 29, 2014 6:27 PM
> *To:* Artur Gajowy
> *Cc:* 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...>
> 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...> 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&iu=/4140/ostg.clktrk
> _______________________________________________
> Codenarc-developer mailing list
> Cod...@li...
> https://lists.sourceforge.net/lists/listinfo/codenarc-developer
>
>
>
>
>
>
>
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2014.0.4259 / Virus Database: 3684/7045 - Release Date: 01/30/14
>