Remove FPs of js/ui5-log-injection-to-http
#190
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Minor comment formatting 2. Unify the TypeTrackers into one (hasDependency) 3. Some more modeling
This allows reuse of UI5LogInjection in UI5LogsToHttp.
and correctly skip the FP case
...but why has it been working even before this fix?
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...eries/UI5LogInjection/log-entry-flows-to-http-getLogEntries/webapp/utils/LogEntriesToHttp.js
Dismissed
Show dismissed
Hide dismissed
...eries/UI5LogInjection/log-entry-flows-to-http-getLogEntries/webapp/utils/LogEntriesToHttp.js
Dismissed
Show dismissed
Hide dismissed
...Injection/log-entry-flows-to-http-log-listener-js-object/webapp/controller/app.controller.js
Dismissed
Show dismissed
Hide dismissed
...Injection/log-entry-flows-to-http-log-listener-js-object/webapp/controller/app.controller.js
Dismissed
Show dismissed
Hide dismissed
...gInjection/log-entry-flows-to-http-log-listener-sap-module/webapp/utils/CustomLogListener.js
Dismissed
Show dismissed
Hide dismissed
...gInjection/log-entry-flows-to-http-log-listener-sap-module/webapp/utils/CustomLogListener.js
Dismissed
Show dismissed
Hide dismissed
This step involves `DataFlow::FlowLabel`s that is subjective to the query that is using this step. Therefore, remove it from the shared flow step, and put it only in the query or query library which define the labels to be propagated through this step.
lcartey
approved these changes
May 15, 2025
Contributor
lcartey
left a comment
lcartey
left a comment
There was a problem hiding this comment.
Approved - only an unused state that I think should be removed.
| UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource | ||
| class UI5LogEntryFlowState extends DataFlow::FlowLabel { | ||
| UI5LogEntryFlowState() { | ||
| this = ["not-logged-not-accessed", "logged-not-accessed", "logged-and-accessed"] |
Contributor
There was a problem hiding this comment.
I think logged-not-accessed is unused.
…ced-security/codeql-sap-js into jeongsoolee09/fix-ui5-loginjection
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR contributes
Eliminate FPs
The query
js/ui5-log-injection-to-httpyielded false positives on code that does not depend on UI5, because the query did not check if the data being sent via HTTP is indeed derived from a UI5 log entry (by means ofonLogEntryorgetLogEntries). Therefore, enforce it by using flow labels.Tidy up code
Factor out various type trackers into one (
hasDependency/1)There were
sapControl/0,sapController/0,sapRenderer/0, andsapComponent/0that gives rise to classes such asCustomControl,CustomController, and so on. However, all of them differed in only the dependency paths they referred to, and keeping them more or less pollluted the code. Therefore, add ahasDependency/1parameterized with the dependency path.Split up
log-entry-flows-to-sinksThe unit test case
log-entry-flows-to-sinkscombined several kinds of sinks all in one test case (hence the name). Split it up into several smaller ones:log-entry-flows-to-http-getLogEntries: getLogEntries is used to fetch a log entry to be sent in an XmlHttpRequest.log-entry-flows-to-http-log-listener-js-object: a log listener as a plain JS object (that implements it implicitly) has a methodonLogEntrieswhich pulls a log entry on a log event and sends it in an XmlHttpRequest.log-entry-flows-to-http-log-listener-sap-module: Same as the example right above, instead it's a UI5 module that explicitly implements it.log-entry-flows-to-http-log-listener-sap-module-imported: See Future Work below.Miscellaneous
Fix access path of
SapLogEntriesin the library modelOne of the rows contributing to the type
SapLogEntrieshad this row:["SapLogEntries", "SapLogger", "Member[addLogListener].Parameter[0].Member[onLogEntry].Argument[0]"]It confused
ParameterwithArgumentand vice versa, so put them in the right places.Also, as per the UI5 documentation on sap/base/Log.Logger, all of the various logging functions do not return another Logger (they are of void type), so delete this row:
["SapLogger", "SapLogger", "Member[debug,error,fatal,info,setLevel,trace,warning,getLogger].ReturnValue"]Separate out
UI5LogInjectionQueryinto a library moduleThis is to reuse the configuration in
UI5LogsToHttp.Future work
log-entry-flows-to-http-log-listener-sap-module-imported). This is currently partially blocked as of now due to limitations inAmdModuleDefinition::Range(see github/codeql, PR #19359); this prevents us from modelingsap.ui.require, an another way of requiring a UI5 module.