Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Add MaD support for Attribute.Getter and Attribute.Setter. #17459

Merged
merged 7 commits into from
Sep 18, 2024

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Sep 13, 2024

In this PR we added support for Attribute.Getter and Attribute.Setter as a part of the models as data description in the ext field of a model.
Assume we have the following declarations.

namespace N {
    public class SourceAttribute : System.Attribute { }
  
    public class MyClass {
        [SourceAttribute]
        public object TaggedSrcPropertyGetter { get; }
    }
}

If we add the following source model

- ["N", "SourceAttribute", false, "", "", "Attribute.Getter", "ReturnValue", "local", "manual"]

Then TaggedSrcPropertyGetter is considered a local source of taint.

@michaelnebel michaelnebel marked this pull request as ready for review September 16, 2024 14:00
@michaelnebel michaelnebel requested a review from a team as a code owner September 16, 2024 14:00
@michaelnebel
Copy link
Contributor Author

DCA looks good!

@@ -289,7 +288,7 @@ module ModelValidation {
not signature.regexpMatch("|\\([a-zA-Z0-9_<>\\.\\+\\*,\\[\\]]*\\)") and
result = "Dubious signature \"" + signature + "\" in " + pred + " model."
or
not ext.regexpMatch("|Attribute") and
not ext = ["", "Attribute", "Attribute.Getter", "Attribute.Setter"] and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should mention these new extensions in the comment at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

Comment on lines +416 to +417
not result instanceof Property and
not result instanceof Indexer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we be backwards compatible if we removed these two lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I think we should take a conscious decision and break the backwards compatibility - as it was quite surprising that tagging a property meant that it worked for setters.
Also, the public documentation doesn't contain the explanation on, how to use the ext field, so it is most likely not heavily used (yet).

Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaelnebel michaelnebel merged commit 295861d into github:main Sep 18, 2024
22 checks passed
@michaelnebel michaelnebel deleted the csharp/accessormad branch September 18, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants