Restrict `RemoteFlowSource` of CAP to only some properties and method calls on it #208

jeongsoolee09 · 2025-07-29T23:30:09Z

What This PR Contributes

Refine the definition of the remote flow source of CAP

This PR refines the remote flow source of CAP by restricting the definition to reads of property on the program elements previously detected as remote flow sources, namely, parameters of handlers belonging to a service that is exposed to a web protocol. This PR achieves it through the following changes:

Stop making HandlerParameterOfExposedService extend RemoteFlowSource, and
Create UserProvidedPropertyReadOfHandlerParameterOfExposedService and make it extend that instead.

Enhance unit tests in `remoteflowsources`

Tidy up the test project

The test project had several rooms of improvement:

The file names that held user-defined application services did not match their name. This made it a bit disorienting to the reader of the test case.
The cds.serve(...).with(...) calls are typically stored in a main entrypoint, so move it there (even if it's technically allowed to be place anywhere).

Make another model test query

Also, this PR adds another complementary unit test, ExposedServices that tests the class HandlerParameterOfExposedService, now demoted from being a remote flow source. It is still an important class so we would want to still test if it is working properly.

Running the test locally

First, compile the .cds files by running the script extractors/cds/tools/workflow/cds-compilation-for-actions.sh. Then, run export LGTM_INDEX_FILTERS=include:**/*.json to enable JSON extraction for your shell session. You should be prepared to create the database with codeql database create --language=javascript.

Future Works

There is another unit test requesthandler, and what it intends to test is already covered in this unit test. So, it can be safely deleted.
I noticed that cds.serve(...) can also accept a path string relative to the entrypoint of the root of the application, but it is not yet modeled. Modeling it can be a future work.

- `service3nocds.js` had a service that was named differently. Clarified the gist of the file using tags. - `service4.js` was identical to `service3nocds.js`, except for the `cds.serve` call. This is better suited for `server.js`. - Since `HandlerParameterOfExposedService` is no longer a `RemoteFlowSource`, we place another query for testing it only. - The case categorization is as follows: - `service1`: Exposed service with reads from properties and method calls, some of which are tainted and not tainted. - `service2`: Service that both receives data from service1 and service4 but does not propagate it further. - `service3`: Service without a CDS declaration, simulating extraction failure. Should be recognized as exposed, from overapproximation. Consists of identical property reads / method calls as those of `service1`. - `service4`: Service that is explicitly declared as internal only. Consists of identical property reads / method calls as those of `service1`, but is not a taint source from being unexposed.

The locations changed a little from altering the definition of `RemoteFlowSource` to the property reads of the parameter, not the parameter itself.

- `server.js` had a reference to a service that doesn't exist. Made it point to Service1 instead. - Create expected results of `ExposedServices`, a complementary test to `RemoteFlowSource`.

…properties

The results from query predicates changed a little from altering the definition of `RemoteFlowSource` to the property reads of the parameter, not the parameter itself.

…properties

knewbury01 · 2025-08-05T19:08:44Z

javascript/frameworks/cap/test/models/cds/remoteflowsources/srv/service4.js

@@ -0,0 +1,45 @@
+const cds = require("@sap/cds");
+
+module.exports = class Service5 extends cds.ApplicationService {


hm I am seeing something I dont quite understand

this testcase fails for me locally, these req locations show up ,

I think its bc this is named service 5 even tho its 4 and 4 is not exposed,
so for a not defined service yes we assume its exposed. ie this should just be service 4, its a typo?

but then why doesnt the CI of the unit tests see that fail?

Ah, good catch, that's a typo. I should have let you know about how to properly run the tests, sorry! I'll put it in the PR body.

Added as a section titled Running the test locally.

data-douser

One minor question about an expected result.

data-douser · 2025-08-05T19:11:00Z

...oginjection/log-injection-without-protocol-none/log-injection-without-protocol-none.expected

 | srv/service1.js:7:19:7:35 | { messageToPass } | srv/service1.js:7:21:7:33 | messageToPass |
 | srv/service1.js:7:19:7:46 | messageToPass | srv/service1.js:9:38:9:50 | messageToPass |
 | srv/service1.js:7:21:7:33 | messageToPass | srv/service1.js:7:19:7:46 | messageToPass |
-| srv/service1.js:7:39:7:41 | req | srv/service1.js:7:39:7:46 | req.data |
+| srv/service1.js:7:39:7:46 | req.data | srv/service1.js:7:19:7:35 | { messageToPass } |


New duplicate result?

Yes, that's expected. In fact, I've noticed this kind of duplicate results even before the changes. These show up when:

N1 --> N3, and

N2 --> N3, but

N1 != N2, with

N1's location == N2's location.

Here, N1 is an internal node (that has both parent(s) and child(ren)) req.data whose predecessor is req, and N2 is req.data is a root node without a predecesor, because it is a RemoteFlowSource.

knewbury01 · 2025-08-05T19:39:54Z

javascript/frameworks/cap/test/models/cds/remoteflowsources/srv/service4withcds.js

-}
-module.exports = TestService
-
-cds.serve('./test-service').with((srv) => {


these had been a bit of an equivalence class test to show that even when the service is not defined (in cds) and there are the non typical registrations out of the init function it is still valid

for example
https://github.com/mpilo-integrove/sap-btp/blob/main/projects/it-inventory/srv/inventory-service.js#L4

but also, that is likely rarer, so if not having even one case of that in a file with a more uniform naming makes sense, then that is fair too!

Good point. But I see in the example that the inventory-service is an existing service declared in inventory-service.cds, and inventory-service.js is implementing it in an unconventional way (I guess the encouraged way is through cds.ApplicationService).

But I guess having a case of cds.serve(...).with(...) for a service lacking a CDS declaration will be valuable, as it is another example of service we consider exposed. Service3 is one such example so I'll add a cds.serve("Service3").with((req) => { ... }).

Added in commit 3a328ad.

Add a `cds.serve(...).with(...)` that expands the functionality of Service3 which lacks a CDS declaration. Since Service3 is missing a CDS declaration, all of the handlers registrated in the `.with` method call should be considered as exposed.

jeongsoolee09 added 12 commits July 29, 2025 14:53

Refine docstring of HandlerParameterOfExposedService

81fa051

Make HandlerParameter extend ParameterNode

7958a80

Add class PayloadPropertyReadOfHandlerParameterOfExposedService

1f027ff

Finish first draft of class

7ae665c

Minor comment formatting

0b18c37

Change indent level to 2

bb57c22

Refine more on req.http.req

de09d59

Edit docstring and remove instanceof PropRead constraint

52d49e7

Update expected results of CQL Injection query

3c0272a

The locations changed a little from altering the definition of `RemoteFlowSource` to the property reads of the parameter, not the parameter itself.

Update expected results of model test remoteflowsource

62f8815

Update server.js and check in expected results of ExposedServices

71ada99

- `server.js` had a reference to a service that doesn't exist. Made it point to Service1 instead. - Create expected results of `ExposedServices`, a complementary test to `RemoteFlowSource`.

jeongsoolee09 marked this pull request as ready for review August 4, 2025 16:50

jeongsoolee09 requested a review from knewbury01 August 4, 2025 16:50

jeongsoolee09 added 3 commits August 4, 2025 13:25

Merge branch 'main' into jeongsoolee09/restrict-cap-remoteflowsource-…

788a1b4

…properties

Update expected results of impacted queries

f1bc052

The results from query predicates changed a little from altering the definition of `RemoteFlowSource` to the property reads of the parameter, not the parameter itself.

Merge branch 'main' into jeongsoolee09/restrict-cap-remoteflowsource-…

985b25e

…properties

knewbury01 reviewed Aug 5, 2025

View reviewed changes

data-douser reviewed Aug 5, 2025

View reviewed changes

knewbury01 reviewed Aug 5, 2025

View reviewed changes

jeongsoolee09 self-assigned this Aug 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Restrict `RemoteFlowSource` of CAP to only some properties and method calls on it #208

Restrict `RemoteFlowSource` of CAP to only some properties and method calls on it #208

jeongsoolee09 commented Jul 29, 2025 •

edited

Loading

Uh oh!

knewbury01 Aug 5, 2025 •

edited

Loading

Uh oh!

jeongsoolee09 Aug 5, 2025

Uh oh!

jeongsoolee09 Aug 5, 2025

Uh oh!

data-douser left a comment

Uh oh!

data-douser Aug 5, 2025

Uh oh!

jeongsoolee09 Aug 5, 2025 •

edited

Loading

Uh oh!

knewbury01 Aug 5, 2025

Uh oh!

jeongsoolee09 Aug 5, 2025 •

edited

Loading

Uh oh!

jeongsoolee09 Aug 5, 2025

Uh oh!

Uh oh!

		@@ -0,0 +1,45 @@
		const cds = require("@sap/cds");

		module.exports = class Service5 extends cds.ApplicationService {

Restrict RemoteFlowSource of CAP to only some properties and method calls on it #208

Are you sure you want to change the base?

Restrict RemoteFlowSource of CAP to only some properties and method calls on it #208

Conversation

jeongsoolee09 commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What This PR Contributes

Refine the definition of the remote flow source of CAP

Enhance unit tests in remoteflowsources

Tidy up the test project

Make another model test query

Running the test locally

Future Works

Uh oh!

knewbury01 Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeongsoolee09 Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

jeongsoolee09 Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

data-douser left a comment

Choose a reason for hiding this comment

Uh oh!

data-douser Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

jeongsoolee09 Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knewbury01 Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

jeongsoolee09 Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeongsoolee09 Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Restrict `RemoteFlowSource` of CAP to only some properties and method calls on it #208

Restrict `RemoteFlowSource` of CAP to only some properties and method calls on it #208

jeongsoolee09 commented Jul 29, 2025 •

edited

Loading

Enhance unit tests in `remoteflowsources`

knewbury01 Aug 5, 2025 •

edited

Loading

jeongsoolee09 Aug 5, 2025 •

edited

Loading

jeongsoolee09 Aug 5, 2025 •

edited

Loading