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

[Java] Can't match imported annotations #1508

Closed
nightwatchcyber opened this issue Aug 14, 2020 · 8 comments · Fixed by #1521
Closed

[Java] Can't match imported annotations #1508

nightwatchcyber opened this issue Aug 14, 2020 · 8 comments · Fixed by #1521
Assignees
Labels
bug Something isn't working user:external requested by someone outside of r2c

Comments

@nightwatchcyber
Copy link
Contributor

Describe the bug
Matching stops working in Java when "import" is used

To Reproduce
https://semgrep.dev/RrPN

Expected behavior
Matching should work even if import statements exist

Screenshots
If applicable, add screenshots to help explain your problem.

Environment
semgrev.dev and MacOS

@nightwatchcyber
Copy link
Contributor Author

This is based of the example by @minusworld, theirs works:
https://sgrep.live/yjx

Mine doesn't:
https://semgrep.dev/RrPN

Only difference is the import statement

@nightwatchcyber
Copy link
Contributor Author

screenshots attached
with_import
without_import

@nightwatchcyber
Copy link
Contributor Author

Using full package name doesn't help either:
@org.springframework.web.bind.annotation.RequestMapping(method=$M)

@ievans ievans added bug Something isn't working user:external requested by someone outside of r2c labels Aug 14, 2020
@ievans
Copy link
Member

ievans commented Aug 14, 2020

Thanks for the great bug report, @nightwatchcyber. Cc @aryx who I suspect has the most context here

@aryx
Copy link
Collaborator

aryx commented Aug 14, 2020

There's 2 bugs here. First, annotations with qualified names are not correctly represented in the generic AST
so @Foo.bar.whatever is actually the same as @xxx.yyy (not good). Second, if the pattern does not contain a qualified name (e.g., @foo), we should still allow it to match imported name (E.g., import x.y.Foo; @foo ).

aryx added a commit to semgrep/pfff that referenced this issue Aug 17, 2020
This will help semgrep/semgrep#1508

test plan:
See NamedAttr below which now contains ["Foo",();"Bar",()]
instead of a OA_AnnotOther

+ /home/pad/pfff/pfff -dump_ast misc_import_resolve.java
[(DirectiveStmt
    (ImportFrom ((), (DottedName [("foo", ())]), ("Bar", ()), None)));
  (DefStmt
     ({ name = ("Foo", ()); attrs = [];
        info =
        { id_resolved = ref (None); id_type = ref (None);
          id_const_literal = ref (None) };
        tparams = [] },
      (ClassDef
         { ckind = (Class, ()); cextends = []; cimplements = [];
           cmixins = [];
           cbody =
           ((),
            [(FieldStmt
                (DefStmt
                   ({ name = ("foo", ());
                      attrs =
                      [(NamedAttr ((), [("Misc", ())],
                          { id_resolved = ref (None); id_type = ref (None);
                            id_const_literal = ref (None) },
                          ((), [], ())));
                        (NamedAttr ((), [("Foo", ()); ("Bar", ())],
                           { id_resolved = ref (None); id_type = ref (None);
                             id_const_literal = ref (None) },
                           ((),
                            [(ArgKwd (("method", ()),
                                (L (String ("foobar", ())))))
                              ],
                            ())
                           ));
                        (KeywordAttr (Public, ()))];
                      info =
                      { id_resolved = ref (None); id_type = ref (None);
                        id_const_literal = ref (None) };
                      tparams = [] },
                    (FuncDef
                       { fparams =
                         [(ParamClassic
                             { pname = (Some ("x", ()));
                               ptype = (Some (TyBuiltin ("int", ())));
                               pdefault = None; pattrs = [];
                               pinfo =
                               { id_resolved = ref (None);
                                 id_type = ref (None);
                                 id_const_literal = ref (None) }
                               })
                           ];
                         frettype = (Some (TyBuiltin ("int", ())));
                         fbody =
                         (Block
                            ((),
                             [(Return ((),
                                 (Some (Id (("x", ()),
                                          { id_resolved = ref (None);
                                            id_type = ref (None);
                                            id_const_literal = ref (None) }
                                          )))
                                 ))
                               ],
                             ()))
                         }))))
              ],
            ())
           })))
  ]
aryx added a commit to semgrep/pfff that referenced this issue Aug 17, 2020
This will help semgrep/semgrep#1508

test plan:
See NamedAttr below which now contains ["Foo",();"Bar",()]
instead of a OA_AnnotOther

+ /home/pad/pfff/pfff -dump_ast misc_import_resolve.java
[(DirectiveStmt
    (ImportFrom ((), (DottedName [("foo", ())]), ("Bar", ()), None)));
  (DefStmt
     ({ name = ("Foo", ()); attrs = [];
        info =
        { id_resolved = ref (None); id_type = ref (None);
          id_const_literal = ref (None) };
        tparams = [] },
      (ClassDef
         { ckind = (Class, ()); cextends = []; cimplements = [];
           cmixins = [];
           cbody =
           ((),
            [(FieldStmt
                (DefStmt
                   ({ name = ("foo", ());
                      attrs =
                      [(NamedAttr ((), [("Misc", ())],
                          { id_resolved = ref (None); id_type = ref (None);
                            id_const_literal = ref (None) },
                          ((), [], ())));
                        (NamedAttr ((), [("Foo", ()); ("Bar", ())],
                           { id_resolved = ref (None); id_type = ref (None);
                             id_const_literal = ref (None) },
                           ((),
                            [(ArgKwd (("method", ()),
                                (L (String ("foobar", ())))))
                              ],
                            ())
                           ));
                        (KeywordAttr (Public, ()))];
                      info =
                      { id_resolved = ref (None); id_type = ref (None);
                        id_const_literal = ref (None) };
                      tparams = [] },
                    (FuncDef
                       { fparams =
                         [(ParamClassic
                             { pname = (Some ("x", ()));
                               ptype = (Some (TyBuiltin ("int", ())));
                               pdefault = None; pattrs = [];
                               pinfo =
                               { id_resolved = ref (None);
                                 id_type = ref (None);
                                 id_const_literal = ref (None) }
                               })
                           ];
                         frettype = (Some (TyBuiltin ("int", ())));
                         fbody =
                         (Block
                            ((),
                             [(Return ((),
                                 (Some (Id (("x", ()),
                                          { id_resolved = ref (None);
                                            id_type = ref (None);
                                            id_const_literal = ref (None) }
                                          )))
                                 ))
                               ],
                             ()))
                         }))))
              ],
            ())
           })))
  ]
@nbrahms
Copy link
Contributor

nbrahms commented Aug 17, 2020

cf. https://semgrep.dev/jxwN/, which should be a working test case even if we don't change the fqdn resolution behavior (the latter seems more like a design decision we should revisit than a clear-cut bug, @aryx ?).

@nbrahms nbrahms changed the title Matching stops working in Java when "import" is used [Java] Can't match imported annotations Aug 17, 2020
@nbrahms
Copy link
Contributor

nbrahms commented Aug 17, 2020

Here's the AST of the code annotation:

                          "NamedAttr": [
                            "()",
                            [
                              "RequestMapping",
                              "()"
                            ],
                            {
                              "id_const_literal": {
                                "ref@": null
                              },
                              "id_resolved": {
                                "ref@": {
                                  "some": [
                                    {
                                      "ImportedEntity": [
                                        [
                                          "org",
                                          "()"
                                        ],
                                        [
                                          "springframework",
                                          "()"
                                        ],
                                        [
                                          "web",
                                          "()"
                                        ],
                                        [
                                          "bind",
                                          "()"
                                        ],
                                        [
                                          "annotation",
                                          "()"
                                        ],
                                        [
                                          "RequestMapping",
                                          "()"
                                        ]
                                      ]
                                    },
                                    1
                                  ]
                                }
                              },
                              "id_type": {
                                "ref@": null
                              }
                            },
                            [
                              {
                                "ArgKwd": [
                                  [
                                    "method",
                                    "()"
                                  ],
                                  {
                                    "DotAccess": [
                                      {
                                        "Id": [
                                          [
                                            "RequestMethod",
                                            "()"
                                          ],
                                          {
                                            "id_const_literal": {
                                              "ref@": null
                                            },
                                            "id_resolved": {
                                              "ref@": null
                                            },
                                            "id_type": {
                                              "ref@": null
                                            }
                                          }
                                        ]
                                      },
                                      "()",
                                      {
                                        "FId": [
                                          "GET",
                                          "()"
                                        ]
                                      }
                                    ]
                                  }
                                ]
                              }
                            ]
                          ]
                        },

but the pattern annotation is parsed as:

            {
              "OtherAttribute": [
                "OA_AnnotJavaOther",
                [
                  {
                    "Tk": "()"
                  }
                ]
              ]
            },

@aryx aryx self-assigned this Aug 17, 2020
@aryx
Copy link
Collaborator

aryx commented Aug 17, 2020

Yep, I'm almost done fixing it.

aryx added a commit to semgrep/pfff that referenced this issue Aug 17, 2020
This gets closer to the annotation type in AST generic.
This is necessary for semgrep/semgrep#1508
otherwise we get regressions on python semgrep tests.

test plan:
make
aryx added a commit to semgrep/pfff that referenced this issue Aug 17, 2020
This gets closer to the annotation type in AST generic.
This is necessary for semgrep/semgrep#1508
otherwise we get regressions on python semgrep tests.

test plan:
make
aryx added a commit that referenced this issue Aug 17, 2020
This should fix #1508

Test plan:
test file included
aryx added a commit that referenced this issue Aug 18, 2020
This should fix #1508

Test plan:
test file included
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working user:external requested by someone outside of r2c
Development

Successfully merging a pull request may close this issue.

4 participants