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.

Sign up for GitHub

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

Jump to bottom

Detect XSS involving server-side models and controller handler parameters #67

Merged

jeongsoolee09 merged 152 commits into main from jeongsoolee09/unknown-remote-model

Jan 26, 2024

Contributor

jeongsoolee09 commented Oct 21, 2023 •

edited

Loading

What this PR contributes

Add two types of sources:
- Server-side models (e.g. ODatav2)
- Parameters of an event handler belonging to a controller
Add more UI5 classes:
- Model/Control/View references
- Internal/External models
- Component
- Manifests in manifest.json
- Routes, Router
Improve over UI5BindingPath and logic around it
- Complete UI5BindingPath.getModel
- Completely overhauled binding path parser by @rvermeulen (PR Extend bindings modeling #69, Expand javascript bindings #73)
- Cover all variants: JS, JSON, XML, and HTML
Add more / refine:
- path graph edges, additional flow steps

Closes issue #44.

Future work

Cover handler call syntax with a binding path passed as an argument (e.g. .doSomething2(${/input}) in test xss-event-handlers).

jeongsoolee09 added 9 commits

October 20, 2023 17:07


          Remove unneeded private qualifiers for imports

4c10735


          Make TypeTracker callers private

e05674a


          Expose CustomController's name

537b9dc


          Add Component, ManifestJson, and DataSource classes

84a6fc4


          Bifurcate UI5Model into Internal and External models, add `ODat…

af3785b

…aServiceModel`


          Refine definition of Metadata and Extension, simplify some predicates


          Explicitly state that accessesModel captures accesses to `UI5Intern…

a46bc00

…alModel`


          Make bindingPathCapture public

3d6f9fc


          Remove unnecessary comment

33ad5a5

jeongsoolee09 requested a review from rvermeulen

October 21, 2023 00:21

jeongsoolee09 self-assigned this

jeongsoolee09 added 3 commits

October 20, 2023 17:21


          Merge branch 'main' into jeongsoolee09/unknown-remote-model

c97eda1


          Add class ExternalModelDefinition and debug predicates

847d5be


          Debug all new classes and predicates

4b8deb6

rvermeulen reviewed

View reviewed changes

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll Outdated Show resolved Hide resolved

rvermeulen reviewed

View reviewed changes

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll Outdated Show resolved Hide resolved

rvermeulen reviewed

View reviewed changes

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll Outdated Show resolved Hide resolved

rvermeulen reviewed

View reviewed changes

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll Outdated Show resolved Hide resolved

rvermeulen reviewed

View reviewed changes

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll Outdated Show resolved Hide resolved

rvermeulen reviewed

View reviewed changes

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll Outdated Show resolved Hide resolved

rvermeulen reviewed

View reviewed changes

rvermeulen left a comment

Initial review. Overall looks good, left some suggestions.

jeongsoolee09 added 9 commits

October 30, 2023 14:46


          Wrap manifest.json related defs into own module

56c549e


          Refine QLDoc of bindingPathCapture

74048ec


          Minor formatting

5321b4b


          Capture bidirectional flow between model to which commentText is bo…

b759652

…und and its control property


          Merge branch 'main' into jeongsoolee09/unknown-remote-model

7d38447


          First round of addressing reviews

cd89122


          Remove unnecessary comments

3646c67


          Abstract out PropertyMetadata

7bfc989


          Improve docstring and naming of symbols

f124cf5

jeongsoolee09 added 10 commits

January 16, 2024 17:56


          Implement more abstract predicates for each UI5Views

d935f6d


          Debug JsViewBindingPath

1bfb972


          Semi-fix xss-js-control

926cf08

It's still got (1) no location info, and (2) duplicate alarms


          Add constraints to variant TJsonControl and TJsControl

74da4e1


          Merge branch 'main' into jeongsoolee09/unknown-remote-model

fbfc592


          Formatting and update callee predicate names

8d1c85a


          Fix doSomething1, doSomething3, and doSomething4 of `xss-event-…

77a6b8f

…handlers`


          Change docstring of abstract getPropertyName

da8ef86


          Update .expected for UI5LogInjection

64e2b3d


          Fix the lack of location information in xss-js-view and `xss-json-v…

237f04c

…iew`

github-advanced-security bot found potential problems

View reviewed changes

javascript/frameworks/ui5/test/models/multiple_models/webapp/view/app.view.xml Fixed Show fixed Hide fixed

javascript/frameworks/ui5/test/models/multiple_models/webapp/view/app.view.xml Fixed Show fixed Hide fixed

jeongsoolee09 added 10 commits

January 18, 2024 14:04


          Minor formatting

4c3933d


          Merge branch 'main' into jeongsoolee09/unknown-remote-model

c85135d


          Merge branch 'jeongsoolee09/unknown-remote-model' of github.com:advan…

f4b8aa2

…ced-security/codeql-sap-js into jeongsoolee09/unknown-remote-model


          Fix xss-js-view, xss-json-view, and bindingTest

d4648d3


          Ignore currently broken test on xss-event-handlers

0630a9b

In order to fix `.doSomething2`, the binding path parser must be extended.
Since it would require significant amount of time and effort, delegate
that to the next possible iteration.


          Fix sourceTest

27597ac


          Merge branch 'main' into jeongsoolee09/unknown-remote-model

cd3bfd4


          Fix pathSinkTest in a very hacky manner

939195d


          Fix xssSinkTest in a very hacky manner

0dc1718


          Merge branch 'main' into jeongsoolee09/unknown-remote-model

fb5c27a

jeongsoolee09 marked this pull request as ready for review

January 23, 2024 22:41

jeongsoolee09 requested review from rvermeulen and mbaluda

January 23, 2024 22:42

mbaluda approved these changes

View reviewed changes

Contributor

mbaluda left a comment

It looks good to me!
Would you create an issue to get back the doSomething2 test in the future?

javascript/frameworks/ui5/test/models/sink/xssSinkTest.ql Outdated Show resolved Hide resolved

javascript/frameworks/ui5/test/models/sink/pathSinkTest.ql Outdated Show resolved Hide resolved

jeongsoolee09 and others added 2 commits

January 26, 2024 12:10


          Update javascript/frameworks/ui5/test/models/sink/xssSinkTest.ql

149d4a5

Co-authored-by: Mauro Baluda <[email protected]>


          Update javascript/frameworks/ui5/test/models/sink/pathSinkTest.ql

43d4b77

Co-authored-by: Mauro Baluda <[email protected]>

jeongsoolee09 merged commit 51508a1 into main

jeongsoolee09 deleted the jeongsoolee09/unknown-remote-model branch

January 26, 2024 20:18

jeongsoolee09 mentioned this pull request

Add UI5Control implementations for Json, HTML and Javascript #44

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet