Keep vendored and generated code out of CodeQL - #1152
Conversation
maximthomas
left a comment
There was a problem hiding this comment.
praise: The exclusions sit exactly where the noise is.
com_sun_web_ui/js/**is ignored whilecom_sun_web_ui/jspstays analysed, so the JSPs with OpenAM changes keep full coverage.- The Java rejection is real (
rejecting SARIF, as there are more results per run than allowed (58863 > 25000), run 36687081889). The three JAXB trees hold 17,272 of the 17,279java/reference-equality-on-stringsresults, and that count reproduces at the head. js/syntax-errorcarries nosecuritytag. A parse failure in a hand-written script still shows up throughjs/diagnostics/extraction-errors: master job 109976439988 listsauthentication-server-side.js#L28.
question (non-blocking): Is it acceptable to drop the Java results beyond the top 5,000, or should the upload stay under 5,000?
.github/workflows/codeql.yml:106-111, :115-121
Code scanning keeps only the top 5,000 results of an accepted run, ranked by severity (the SARIF limits table on docs.github.com). The PR's own table leaves 8,742 java/missing-override-annotation results outside the excluded trees, and master's 244/244-query total puts the remainder at about 10,800 at most. After the merge the upload is accepted, but about half of the Java results, the recommendation-level ones, are still dropped on every run. Security results rank higher and are kept. If every Java result is meant to be recorded, one more filter fixes it, and this stays a Minor. If only the rejection matters, a line in the comment is enough.
query-filters:
- exclude:
id: js/syntax-error
# Code scanning keeps only the top 5,000 results of a run, by severity;
# this rule alone leaves about 8,700 outside the JAXB trees.
- exclude:
id: java/missing-override-annotationsuggestion (non-blocking): The JavaScript test-plan item would pass on this PR's run whether or not the exclusions work.
.github/workflows/codeql.yml:103-105, :115-121
The PR's Analyze jobs are diff-informed (Computing PR diff ranges... Persisted 2 diff range(s) across 1 file(s) in run 36740189432). Every analysis on refs/pull/1152/merge therefore has results_count 0, and the check was already green at the base. The description says this for the Java item. The JavaScript item ("reports none of the excluded paths or js/syntax-error") would be ticked from a run that cannot report any result. The Summary already expects the alerts to close with the first scan on master, so the test plan can say that too.
# After the first push run of codeql.yml on master:
gh api 'repos/OpenIdentityPlatform/OpenAM/code-scanning/analyses?ref=refs/heads/master&per_page=20' \
--jq '.[] | select(.category | test("java|javascript")) | "\(.category) \(.results_count) \(.commit_sha[0:10])"'
# expect java-kotlin < 25000 on the merge commit
gh api --paginate 'repos/OpenIdentityPlatform/OpenAM/code-scanning/alerts?state=open&tool_name=CodeQL&per_page=100' \
--jq '.[] | select(.rule.id == "js/syntax-error"
or (.most_recent_instance.location.path | test("assets/lib/yui/|com_sun_web_ui/js/|Bluff-0\\.3\\.6\\.2/")))
| .number' | wc -l
# expect 0Pin: use these two checks instead of the second test-plan item. Reverting lines 103-121 makes them fail, while the PR run stays green either way.
The JavaScript analysis reports 780 alerts that no change to OpenAM can address: - 482 in unmodified third-party libraries served by openam-server-only: YUI 2.3.0 (assets/lib/yui), the Sun Web UI scripts (com_sun_web_ui/js) and Bluff 0.3.6.2. They are now listed in paths-ignore next to the other vendored JavaScript. The com_sun_web_ui JSPs carry OpenAM changes and stay analysed. - 298 js/syntax-error notes. The extractor parses script blocks of JSPs (<%= %>), Velocity templates (#if, $context) and the default authentication script, which is injected into scripting.xml and therefore XML-escaped, as plain JavaScript. It skips a block it cannot parse either way, so the note carries no finding; the query is excluded through query-filters.
Since the switch to security-and-quality (OpenIdentityPlatform#1140) the Java SARIF holds 58863 results, above the 25000 Code scanning accepts, so every Java upload on master is rejected and the alerts fixed by OpenIdentityPlatform#1133-OpenIdentityPlatform#1139 still show as open. About 50,000 of those results come from openam-schema's liberty, saml2 and wsfederation modules: 2439 of their 2443 sources were generated by JAXB 1.0.6 in 2012, the other 4 are a copy of its com.sun.xml.bind runtime. Most are missing-override-annotation (23280), reference-equality-on-strings (17272), unused-label (4137) and local-variable-is-never-read (3335). Listing their src/main/java in paths-ignore leaves the rest of the Java code on the full suite.
12f287e to
bd97099
Compare
|
Both points checked against a full run of this branch: vharseko/OpenAM run 36855689676, the whole question (5,000 kept results): Dropping them is acceptable; bd97099 records it in the comment next to the JAXB exclusion, no extra filter. The real count is higher than the estimate: 14370 Java results, under 25000, so the upload is accepted. Of those, 1486 are suggestion (test plan): Taken. The JavaScript item is now backed by the full run, not the diff-informed PR run: 181 results (973 on |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The JAXB comment now carries the measured figures and the cap the upload runs into.
.github/workflows/codeql.yml:107-108matches the measured run: 58,863 Java results, about 44,500 of them in the three JAXB trees..github/workflows/codeql.yml:109-111holds: the java.sarif of vharseko/OpenAM run 36855689676 recounts to 371 error + 1,115 warning + 12,884 note = 14,370, and all 365 results with asecurity-severityare error or warning, so the 1,486 error/warning results all fit under 5,000.
Summary
Two changes to
.github/workflows/codeql.yml:masterafter the merge.Vendored libraries →
paths-ignore(482 alerts)openam-server-only/src/main/webapp/assets/lib/yui/**openam-server-only/src/main/webapp/com_sun_web_ui/js/**openam-server-only/src/main/webapp/js/Bluff-0.3.6.2/**None of these directories has been changed since the initial import. The JSPs under
com_sun_web_ui/jspcarry ForgeRock/3A changes, so they stay analysed.js/syntax-error→query-filters(298 alerts)The JavaScript extractor parses as plain JavaScript:
<%= %>, 293 alerts)config/options.htmandconfig/wizard/step5.htm(#if,$context)openam-scripting/src/main/js/authentication-server-side.js, whichopenam-scripting/pom.xmlinjects verbatim intoscripting.xml; its<is the intended XML escaping.The extractor skips any block it cannot parse, so the note carries no finding. It is not a bug in the code.
Generated JAXB sources →
paths-ignore(Java upload limit)Since #1140 switched to
security-and-quality, the Java SARIF has 58863 results, more than the 25000 that Code scanning accepts. Every Java upload onmasterhas been rejected since then (run 36687081889). As a result, the Java alerts fixed by #1133–#1139 are still shown as open, and new ones are not recorded.A local run of the same bundle (CodeQL 2.27.1,
build-mode none,security-and-quality) completed 151 of 244 queries before the machine ran out of memory. In those queries, about 50,600 of about 66,900 result rows fall inopenam-schema/openam-{liberty,saml2,wsfederation}-schema/src/main/java. Of the 2443 files there, 2439 were generated by JAXB 1.0.6 in 2012 ("Any modifications to this file will be lost upon recompilation"); the other 4 are a copy of itscom.sun.xml.bindruntime.java/missing-override-annotationjava/reference-equality-on-stringsjava/unused-labeljava/local-variable-is-never-readA full run of this branch (the whole
security-and-qualitysuite, CodeQL 2.27.1, vharseko/OpenAM run 36855689676, SARIF kept as an artifact instead of uploaded) gives 14370 Java results, so the upload is accepted again; the excluded trees accounted for about 44,500 of the 58863.Code scanning keeps only the top 5,000 results of an accepted run, ranked by severity (SARIF limits). Of the 14370, 1486 are
errororwarning(365 of them carry asecurity-severity), and all of them are kept. The other 12884 arerecommendation; about 9,400 of those are dropped on every run (java/missing-override-annotation8365,java/unused-parameter1499,java/deprecated-call938, ...). That is accepted here: recording every maintainability note is not the goal of this PR, and the comment incodeql.ymlsays so.Trade-off: with
build-mode none, CodeQL does not extract the excluded files, so the JAXB types are unresolved whereopenam-federation-libraryuses them. For generated beans this is acceptable.Test plan
configparse. The paths-ignore/query-filters keys are checked against the current open-alert list: 780 alerts match.master), none in the excluded paths and nojs/syntax-error. The PR run cannot show this: CodeQL analyses a pull request diff-informed (only the changed lines, herecodeql.yml), so every analysis onrefs/pull/1152/mergehas 0 results.master: