github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.49k stars 1.49k forks source link

False positive results of Finding spurious @param tags #4870

Open madneal opened 3 years ago

madneal commented 3 years ago

Description of the issue

I have found some false positive results of Finding spurious @param tags. For the below query:

import java

from Callable c, ParamTag pt
where c.getDoc().getJavadoc() = pt.getParent() and
    not c.getAParameter().hasName(pt.getParamName())
select pt, "Spurious @param tag."

The query results of apache/maven are false positive.

image

Obviously, the param is alreay stated in the corresponding param tags. The reson is the comma after param. For example, there is a , after groupId in @param groupId, never {@code null}. Hence, the result of pt.getParamName is groupId, instead of groupId which causes the false positive results.

It's common to add comma after param in the param tag. I have searched for param tag with comma for 7 projects. There are 17 results intotal.

import java

from ParamTag pt
where pt.getParamName().matches("%,")
select pt

image

Hence, it's suggested to replace the special characters in the param tag which should not be valid part of param.

I have created two pr(#4871, #4872) for this problem. One is to modify the result of pt.getParamName(). The other one is to modify the method of getParamName() directly. One of each pr be mereged would be appreciated.

As there may more characters for the param tag which not only limit to ,. It's suggested to utilize regexpReplaceAll to replace more kinds of characters.

aibaars commented 3 years ago

While the comma should technically not be part of the parameter name, it is invalid to have it in the Javadoc. See also https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#@param

The @param tag is followed by the name (not data type) of the parameter, followed by a description of the parameter. .... Dashes or other punctuation should not be inserted before the description, as the Javadoc tool inserts one dash.

The authors should still remove the commas to avoid odd looking punctuation in generated Javadoc:

 artifactId - , never null
aibaars commented 3 years ago

@yo-h This issue is valid; the Java extractor simple takes all letters until the first space as the parameter "name". It would be better if it only accepted a string of identifier letters and left the (trimmed) remainder of the text as the description.

madneal commented 3 years ago

@aibaars As stated in the document, Dashes or other punctuation should not be inserted before the description. But it does not decrible dashes or other punctuation should not be inserted after the name.

aibaars commented 3 years ago

@aibaars As stated in the document, Dashes or other punctuation should not be inserted before the description. But it does not decrible dashes or other punctuation should not be inserted after the name.

The form of the tag is @param parameter-name description

The description starts where the name ends, so "before the description" or "after the name" mean the same thing.

smowton commented 3 years ago

Can we improve the message then, to flag that errant punctuation shouldn't be there rather than report an identifier mismatch?

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 14 days with no activity. Comment or remove the stale label in order to avoid having this issue closed in 7 days.