From 761c638523f74b8a5149846a60bca5966c639325 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 27 Nov 2024 10:04:31 +0000 Subject: [PATCH 1/5] Remove binding path cartesian product `source` was not bound within MkConstBindingPathComponentList, and was semantically unused outside the class itself. --- .../frameworks/ui5/BindingStringParser.qll | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll index 9d9d103a..629d7636 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll @@ -839,10 +839,10 @@ module BindingStringParser { then exists(BindingPathComponentList tail | mkBindingPathComponentList(getNextSkippingWhitespace(nextToken), tail, last) and - list = MkConstBindingPathComponentList(name, tail, first) + list = MkConstBindingPathComponentList(name, tail) ) else ( - list = MkConstBindingPathComponentList(name, MkEmptyBindingPathComponentList(), first) and + list = MkConstBindingPathComponentList(name, MkEmptyBindingPathComponentList()) and last = name ) ) @@ -850,7 +850,7 @@ module BindingStringParser { private newtype TBindingPathComponentList = MkEmptyBindingPathComponentList() or - MkConstBindingPathComponentList(NameToken headToken, BindingPathComponentList tail, Token source) { + MkConstBindingPathComponentList(NameToken headToken, BindingPathComponentList tail) { exists(Token nextToken | nextToken = getNextSkippingWhitespace(headToken) | if nextToken instanceof ForwardSlashToken or nextToken instanceof DotToken then mkBindingPathComponentList(getNextSkippingWhitespace(nextToken), tail, _) @@ -863,18 +863,16 @@ module BindingStringParser { this = MkEmptyBindingPathComponentList() and result = "" or exists(NameToken head, BindingPathComponentList tail | - this = MkConstBindingPathComponentList(head, tail, _) and + this = MkConstBindingPathComponentList(head, tail) and if tail instanceof MkEmptyBindingPathComponentList then result = head.toString() else result = head.toString() + "/" + tail.toString() ) } - NameToken getHead() { this = MkConstBindingPathComponentList(result, _, _) } + NameToken getHead() { this = MkConstBindingPathComponentList(result, _) } - BindingPathComponentList getTail() { this = MkConstBindingPathComponentList(_, result, _) } - - Token getSource() { this = MkConstBindingPathComponentList(_, _, result) } + BindingPathComponentList getTail() { this = MkConstBindingPathComponentList(_, result) } } predicate mkAbsoluteBindingPath(Token first, BindingPath path, Token last) { From af45f84765cdc81b33617b157de1a1b057f77f1a Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 27 Nov 2024 10:29:24 +0000 Subject: [PATCH 2/5] Restrict UI5 binding search to relevant files This ensures we don't look for UI5 bindings in, for example, cds.json files. --- .../javascript/frameworks/ui5/Bindings.qll | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll index 032d41a8..4bf729ba 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll @@ -4,6 +4,7 @@ import javascript import advanced_security.javascript.frameworks.ui5.BindingStringParser as MakeBindingStringParser +import advanced_security.javascript.frameworks.ui5.UI5View private class ContextBindingAttribute extends XmlAttribute { ContextBindingAttribute() { this.getName() = "binding" } @@ -15,8 +16,12 @@ private class ContextBindingAttribute extends XmlAttribute { // TODO: add support for binding strings in strings such as `description: "Some {/description}"` private newtype TBindingString = TBindingStringFromLiteral(StringLiteral stringLiteral) { stringLiteral.getValue().matches("{%}") } or - TBindingStringFromXmlAttribute(XmlAttribute attribute) { attribute.getValue().matches("{%}") } or + TBindingStringFromXmlAttribute(XmlAttribute attribute) { + attribute.getLocation().getFile() instanceof XmlView and + attribute.getValue().matches("{%}") + } or TBindingStringFromJsonProperty(JsonObject object, string propertyName) { + object.getFile() instanceof UI5View and object.getPropStringValue(propertyName).matches("{%}") } or TBindingStringFromBindElementMethodCall(BindElementMethodCallNode bindElement) { From 2dd375bea0628fd45b823881c99738bcff23d1a9 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 28 Nov 2024 10:57:34 +0000 Subject: [PATCH 3/5] Bindings: support HTML views - Ensure HTML views are covered within TBindingStrings - Update test case to use a valid html view (.view.html) --- .../advanced_security/javascript/frameworks/ui5/Bindings.qll | 2 +- javascript/frameworks/ui5/test/lib/Bindings/Bindings.expected | 4 ++-- .../ui5/test/lib/Bindings/{test.html => test.view.html} | 0 3 files changed, 3 insertions(+), 3 deletions(-) rename javascript/frameworks/ui5/test/lib/Bindings/{test.html => test.view.html} (100%) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll index 4bf729ba..b03048dd 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll @@ -17,7 +17,7 @@ private class ContextBindingAttribute extends XmlAttribute { private newtype TBindingString = TBindingStringFromLiteral(StringLiteral stringLiteral) { stringLiteral.getValue().matches("{%}") } or TBindingStringFromXmlAttribute(XmlAttribute attribute) { - attribute.getLocation().getFile() instanceof XmlView and + attribute.getLocation().getFile() instanceof UI5View and attribute.getValue().matches("{%}") } or TBindingStringFromJsonProperty(JsonObject object, string propertyName) { diff --git a/javascript/frameworks/ui5/test/lib/Bindings/Bindings.expected b/javascript/frameworks/ui5/test/lib/Bindings/Bindings.expected index 6620e64c..982a8b19 100644 --- a/javascript/frameworks/ui5/test/lib/Bindings/Bindings.expected +++ b/javascript/frameworks/ui5/test/lib/Bindings/Bindings.expected @@ -1,5 +1,3 @@ -| test.html:5:11:5:31 | XML property binding: data-value to {/input} | -| test.html:8:11:8:33 | XML property binding: data-content to {/input} | | test.js:10:20:10:33 | Early JavaScript property binding: value to "{/root/name}" | | test.js:21:28:21:34 | JavaScript context binding: oInput to "/root" | | test.js:23:38:23:43 | Late JavaScript property binding: value to "name" | @@ -9,6 +7,8 @@ | test.json:5:9:22:9 | JSON property binding: items to {/Base} | | test.json:11:17:16:17 | JSON property binding: value to {input} | | test.json:17:17:20:17 | JSON property binding: content to {path : /input, formatter : ".valueFormatter"} | +| test.view.html:5:11:5:31 | XML property binding: data-value to {/input} | +| test.view.html:8:11:8:33 | XML property binding: data-content to {/input} | | test.xml:2:5:2:28 | XML property binding: value to {foo} | | test.xml:3:5:3:29 | XML property binding: value to {/foo} | | test.xml:4:5:4:34 | XML property binding: value to {model>foo} | diff --git a/javascript/frameworks/ui5/test/lib/Bindings/test.html b/javascript/frameworks/ui5/test/lib/Bindings/test.view.html similarity index 100% rename from javascript/frameworks/ui5/test/lib/Bindings/test.html rename to javascript/frameworks/ui5/test/lib/Bindings/test.view.html From 58d673424815a7761eb2ac5f04f4a0eae03f9552 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 28 Nov 2024 11:00:49 +0000 Subject: [PATCH 4/5] Bindings: update test case to use correct extensions All XML, HTML and JSON views should have a .view. extension format. --- .../ui5/test/lib/Bindings/Bindings.expected | 28 +++++++++---------- .../Bindings/{test.json => test.view.json} | 0 .../lib/Bindings/{test.xml => test.view.xml} | 0 3 files changed, 14 insertions(+), 14 deletions(-) rename javascript/frameworks/ui5/test/lib/Bindings/{test.json => test.view.json} (100%) rename javascript/frameworks/ui5/test/lib/Bindings/{test.xml => test.view.xml} (100%) diff --git a/javascript/frameworks/ui5/test/lib/Bindings/Bindings.expected b/javascript/frameworks/ui5/test/lib/Bindings/Bindings.expected index 982a8b19..94372d34 100644 --- a/javascript/frameworks/ui5/test/lib/Bindings/Bindings.expected +++ b/javascript/frameworks/ui5/test/lib/Bindings/Bindings.expected @@ -4,19 +4,19 @@ | test.js:27:19:33:12 | Early JavaScript property binding: value to {\\n ... } | | test.js:38:48:44:9 | Late JavaScript property binding: value to {\\n ... } | | test.js:48:19:48:42 | Early JavaScript property binding: text to "{/#foo ... label}" | -| test.json:5:9:22:9 | JSON property binding: items to {/Base} | -| test.json:11:17:16:17 | JSON property binding: value to {input} | -| test.json:17:17:20:17 | JSON property binding: content to {path : /input, formatter : ".valueFormatter"} | | test.view.html:5:11:5:31 | XML property binding: data-value to {/input} | | test.view.html:8:11:8:33 | XML property binding: data-content to {/input} | -| test.xml:2:5:2:28 | XML property binding: value to {foo} | -| test.xml:3:5:3:29 | XML property binding: value to {/foo} | -| test.xml:4:5:4:34 | XML property binding: value to {model>foo} | -| test.xml:5:5:5:35 | XML property binding: value to {model>/foo} | -| test.xml:6:5:8:29 | XML context binding: binding to {/root} | -| test.xml:6:5:8:29 | XML property binding: value to {foo} | -| test.xml:9:5:9:70 | XML property binding: value to {path : foo, type : "sap.ui.model.type.String"} | -| test.xml:10:5:10:71 | XML property binding: value to {path : /foo, type : "sap.ui.model.type.String"} | -| test.xml:11:5:11:77 | XML property binding: value to {path : model>/foo, type : "sap.ui.model.type.String"} | -| test.xml:12:5:12:76 | XML property binding: value to {path : model>foo, type : "sap.ui.model.type.String"} | -| test.xml:14:5:22:45 | XML property binding: value to {parts : [{path : foo}, {path : bar/baz}, {path : quux}], formatter : "some.formatter"} | +| test.view.json:5:9:22:9 | JSON property binding: items to {/Base} | +| test.view.json:11:17:16:17 | JSON property binding: value to {input} | +| test.view.json:17:17:20:17 | JSON property binding: content to {path : /input, formatter : ".valueFormatter"} | +| test.view.xml:2:5:2:28 | XML property binding: value to {foo} | +| test.view.xml:3:5:3:29 | XML property binding: value to {/foo} | +| test.view.xml:4:5:4:34 | XML property binding: value to {model>foo} | +| test.view.xml:5:5:5:35 | XML property binding: value to {model>/foo} | +| test.view.xml:6:5:8:29 | XML context binding: binding to {/root} | +| test.view.xml:6:5:8:29 | XML property binding: value to {foo} | +| test.view.xml:9:5:9:70 | XML property binding: value to {path : foo, type : "sap.ui.model.type.String"} | +| test.view.xml:10:5:10:71 | XML property binding: value to {path : /foo, type : "sap.ui.model.type.String"} | +| test.view.xml:11:5:11:77 | XML property binding: value to {path : model>/foo, type : "sap.ui.model.type.String"} | +| test.view.xml:12:5:12:76 | XML property binding: value to {path : model>foo, type : "sap.ui.model.type.String"} | +| test.view.xml:14:5:22:45 | XML property binding: value to {parts : [{path : foo}, {path : bar/baz}, {path : quux}], formatter : "some.formatter"} | diff --git a/javascript/frameworks/ui5/test/lib/Bindings/test.json b/javascript/frameworks/ui5/test/lib/Bindings/test.view.json similarity index 100% rename from javascript/frameworks/ui5/test/lib/Bindings/test.json rename to javascript/frameworks/ui5/test/lib/Bindings/test.view.json diff --git a/javascript/frameworks/ui5/test/lib/Bindings/test.xml b/javascript/frameworks/ui5/test/lib/Bindings/test.view.xml similarity index 100% rename from javascript/frameworks/ui5/test/lib/Bindings/test.xml rename to javascript/frameworks/ui5/test/lib/Bindings/test.view.xml From 3b73b61bd87ad72466408d762eab93bb9f6cd8d7 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 28 Nov 2024 23:01:50 +0000 Subject: [PATCH 5/5] Capture and report CDS compilation errors This makes a compilation failure in CDS non-fatal, instead capturing and reporting the error via the tool status page in GitHub. --- extractors/cds/tools/index-files.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/extractors/cds/tools/index-files.sh b/extractors/cds/tools/index-files.sh index 8fb60461..7d9c2124 100755 --- a/extractors/cds/tools/index-files.sh +++ b/extractors/cds/tools/index-files.sh @@ -52,10 +52,13 @@ echo "Processing CDS files to JSON" # the same name while IFS= read -r cds_file; do echo "Processing CDS file $cds_file to:" - $cds_command compile "$cds_file" \ - -2 json \ - -o "$cds_file.json" \ - --locations + if ! $cds_command compile "$cds_file" -2 json -o "$cds_file.json" --locations 2> "$cds_file.err"; then + stderr_truncated=`grep "^\[ERROR\]" "$cds_file.err" | tail -n 4` + error_message=$'Could not compile the file '"$cds_file"$'.\nReported error(s):\n```\n'"$stderr_truncated"$'\n```' + echo "$error_message" + # Log an error diagnostic which appears on the status page + "$CODEQL_DIST/codeql" database add-diagnostic --extractor-name cds --ready-for-status-page --source-id cds/compilation-failure --source-name "Failure to compile one or more SAP CAP CDS files" --severity error --markdown-message "$error_message" --file-path "$cds_file" "$CODEQL_EXTRACTOR_CDS_WIP_DATABASE" + fi done < "$response_file" # Check if the JS extractor variables are set, and set them if not