Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mbaluda
force-pushed
the
fix/cds-cap-ui5-modeling
branch
from
November 7, 2026 13:55
7795f72 to
3c3e63a
Compare
mbaluda
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Sensitive managed foreign keys can evade exposure alerts through aliases or overlooked referenced-key annotations, and CSN-only projects remain excluded from discovery.
2 open issues
What changed in this PR
This PR improves SAP model extraction and analysis by refining CDS/CSN compilation, CAP sensitive-data tracking, and UI5 model handling.
Changes:
- Centralizes CDS compilation, preserves project ownership and source filters, and prefers locked compiler versions.
- Extends CAP sensitive-exposure modeling for managed foreign keys and collection callbacks.
- Adds filtered UI5 XML indexing and isolates JSON-model predicates to improve query performance.
| File | Description |
|---|---|
javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5DataModels.qll |
Isolates JSON-file resolution in non-inlined predicates. |
javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.js |
Adds foreign-key and callback exposure cases. |
javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected |
Records expected sensitive foreign-key alerts. |
javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.cds |
Adds association and target-entity fixtures. |
javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql |
Models managed foreign keys and callback result properties. |
extractors/javascript/tools/pre-finalize.sh |
Delegates XML indexing to the shared helper. |
extractors/javascript/tools/pre-finalize.cmd |
Uses the shared indexing helper on Windows. |
extractors/javascript/tools/index-ui5.js |
Selects XML files with ordered filters and argument files. |
extractors/javascript/README.md |
Documents helper requirements and path escaping. |
extractors/cds/tools/workflow/compile-test-cds.mjs |
Reuses production target selection and compiler invocation. |
extractors/cds/tools/test/src/paths-ignore.test.ts |
Tests ordered filters and literal path characters. |
extractors/cds/tools/test/src/packageManager/cacheInstaller.test.ts |
Tests workspace lockfile resolution and cache separation. |
extractors/cds/tools/test/src/index-ui5.test.ts |
Tests XML filter normalization and filename escaping. |
extractors/cds/tools/test/src/extractor-scenarios.test.ts |
Adds compilation, filtering, and workspace scenarios. |
extractors/cds/tools/test/src/environment.test.ts |
Tests extraction batches and CSN input selection. |
extractors/cds/tools/test/src/codeql.test.ts |
Tests batch execution and configuration diagnostics. |
extractors/cds/tools/test/src/cds/parser/files-to-compile.test.ts |
Covers model selection and entry-point precedence. |
extractors/cds/tools/test/src/cds/parser/build-graph.test.ts |
Covers selected CSN projects and graph metadata. |
extractors/cds/tools/test/src/cds/compiler/retry.test.ts |
Checks ownership preservation during retries. |
extractors/cds/tools/test/src/cds/compiler/graph.test.ts |
Checks ownership preservation during compilation. |
extractors/cds/tools/test/src/cds/compiler/compile.test.ts |
Tests bounded invocation, literal arguments, and cleanup. |
extractors/cds/tools/test/src/cds/compiler/compile-spawn.test.ts |
Tests Windows command escaping and invocation size. |
extractors/cds/tools/test/src/cds/compiler/command.test.ts |
Adds a model-discovery mock. |
extractors/cds/tools/test/cds-extractor.test.ts |
Checks removal of premature installation diagnostics. |
extractors/cds/tools/src/paths-ignore.ts |
Implements ordered source filters and path normalization. |
extractors/cds/tools/src/packageManager/cacheInstaller.ts |
Resolves compiler versions from workspace lockfiles. |
extractors/cds/tools/src/environment.ts |
Selects extraction inputs and bounds filter batches. |
extractors/cds/tools/src/codeql.ts |
Executes extraction batches with failure diagnostics. |
extractors/cds/tools/src/cds/parser/types.ts |
Adds child-project boundaries and CSN metadata. |
extractors/cds/tools/src/cds/parser/graph.ts |
Preserves ownership and retains selected CSN models. |
extractors/cds/tools/src/cds/parser/functions.ts |
Centralizes filtered CDS/CSN target selection. |
extractors/cds/tools/src/cds/compiler/retry.ts |
Passes complete project metadata to retries. |
extractors/cds/tools/src/cds/compiler/graph.ts |
Passes complete project metadata to compilation. |
extractors/cds/tools/src/cds/compiler/compile.ts |
Adds shared, bounded compiler invocation using cross-spawn. |
extractors/cds/tools/README.md |
Documents model selection and dependency caching. |
extractors/cds/tools/package.json |
Adds cross-spawn and its type definitions. |
extractors/cds/tools/package-lock.json |
Locks the added dependencies and runtime requirements. |
extractors/cds/tools/esbuild.config.mjs |
Removes explicit Node built-in external declarations. |
extractors/cds/tools/dist/compile-test-cds-lib.cjs |
Updates the generated workflow compilation library. |
extractors/cds/tools/compile-test-cds-lib.ts |
Exports the shared compiler runner. |
extractors/cds/tools/cds-extractor.ts |
Allows compiler fallback when dependency caches are unavailable. |
.gitattributes |
Marks extractor distribution files as generated. |
Review effort: Balanced
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The source-pruning logic can suppress sensitive-data alerts by overlooking sensitive or unresolved entries in mixed projection arrays and incorrectly applying nested-query projections to outer results.
2 open issues
3 resolved since last review
Review effort: Balanced
mbaluda
changed the title
mbaluda
changed the title
mbaluda
changed the title
mbaluda
force-pushed
the
fix/cds-cap-ui5-modeling
branch
from
November 9, 2026 21:15
b331e81 to
8302b63
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed to evaluate projection and annotation-resolution gaps that can suppress sensitive-data alerts or incorrectly flag public constants.
No open issues 🎉
5 resolved since last review
Review effort: Balanced
Comment on lines
+208
to
+210
| const from = object(query?.from); | ||
| const source = reference(from?.ref)?.[0]; | ||
| if (!result && source) { |
Comment on lines
+196
to
+199
| predicate isBarrierIn(DataFlow::Node node, FlowState state) { | ||
| state = "record" and | ||
| node instanceof DataFlow::PropRead and | ||
| not isElementRead(node.(DataFlow::PropRead)) |
| exists(string field | | ||
| result = getDefaultPrivacyAttribute(owner, field) and | ||
| propertyPath = outputPath.substring(0, outputPath.length() - 1) + field and | ||
| not getAProjectedProperty(getAProjectionArgument(query), propertyPath, _) |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical projection-handling and joined-view annotation-provenance defects suppress valid sensitive-data exposure alerts, while wildcard handling incorrectly flags redacted constant fields.
4 open issues
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Projection handling misses sensitive exposures in nested, qualified, and unresolved selections and falsely flags wildcard fields replaced by constants.
6 open issues
Review effort: Balanced
Comment on lines
+550
to
+553
| exists(string item | | ||
| isProjectionColumn(argument) and item = getAProjectionItem(argument.getStringValue()) | ||
| | | ||
| path = getAProjectedStringColumn(item) and |
Comment on lines
+623
to
+626
| exists(string rest | | ||
| rest = path.regexpCapture("^[^.]+\\.(.+)$", 1) and | ||
| result = | ||
| getProjectedPathEntity(entity.getAttribute(path.splitAt(".", 0)).getTargetEntity(), rest) |
Comment on lines
+55
to
+57
| argument = getAProjectionArgument(query) and | ||
| getAProjectedProperty(argument, outputPath, cdsPath) and | ||
| resolveProjectedPath(entity, cdsPath, owner, name) and |
mbaluda
force-pushed
the
fix/cds-cap-ui5-modeling
branch
from
November 10, 2026 22:10
0511e3a to
6de61e0
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved projections, string-form expansions, and helper property reads can miss sensitive-data alerts, and the code-scanning baseline contains stale results.
7 open issues
Review effort: Balanced
Track sensitive CAP query results with field and record flow states so
alerts link the specific annotated field that is logged:
* Respect explicit projections: `columns(...)`, `SELECT(...)` shorthand,
`SELECT.from(entity, key, columns)` overloads, column lists held in
variables, aliases, `expand`/`inline`, nested blocks, computed columns,
and string literals inside projection strings.
* Resolve managed foreign keys such as `manager_userId`.
* Follow results through array elements, `for...of` loops, and inline
callbacks of array methods, but not through shared helper parameters.
* Only treat `@PersonalData.IsPotentially{Personal,Sensitive}` annotations
with value `true` as sensitive.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mbaluda
force-pushed
the
fix/cds-cap-ui5-modeling
branch
from
November 10, 2026 22:39
6de61e0 to
355abae
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved projections, nested expansions, and helper-based field reads can cause sensitive data exposure alerts to be missed.
9 open issues
Review effort: Balanced
Comment on lines
+557
to
+559
| predicate getAProjectedProperty(Expr argument, string propertyName, string path) { | ||
| exists(string item | | ||
| isProjectionColumn(argument) and item = getAProjectionItem(argument.getStringValue()) |
Comment on lines
+57
to
+61
| resolveProjectedPath(entity, cdsPath, owner, name) and | ||
| ( | ||
| name != "*" and | ||
| propertyPath = outputPath and | ||
| result = getPrivacyPropertyAttribute(owner, name) |
Contributor
Author
|
Missing coverage:
|
This branch has not been deployed
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
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.
No description provided.