JS: support `hana` db client #19118

Napalys · 2025-03-25T17:41:14Z

Modeled @sap/hana-client, @sap/hdbext and hdb packages sql-injection sinks.

…t from `hdbext`.

Copilot

Pull Request Overview

This PR adds support for the SAP HANA database client by modeling SQL injection sinks for the @sap/hana-client, @sap/hdbext, and hdb packages.

Added new test cases in the hana.js file to simulate SQL injection scenarios.
Introduced a YAML extension model file to map SQL injection sinks for the supported packages.
Updated the change notes to reflect the new enhancements.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File	Description
javascript/ql/test/query-tests/Security/CWE-089/untyped/hana.js	New test cases demonstrating various SQL injection sink flows.
javascript/ql/lib/ext/hana-db-client.model.yml	YAML file mapping methods to SQL injection sinks for the packages.
javascript/ql/lib/change-notes/2025-03-26-hana-db-client.md	Added change notes documenting the support for the new clients.

Files not reviewed (1)

javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected: Language not supported

Comments suppressed due to low confidence (1)

javascript/ql/test/query-tests/Security/CWE-089/untyped/hana.js:38

[nitpick] The 'express' module is imported twice with different variable declarations; consider using 'const' consistently for clarity and maintainability.

var express = require('express');

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

erik-krogh · 2025-03-27T11:42:27Z

javascript/ql/test/query-tests/Security/CWE-089/untyped/hana.js

+  let maliciousInput = req.body.data; // $ MISSING: Source
+  client.exec('SELECT * FROM DUMMY' + maliciousInput, function (err, rs) {}); // $ MISSING: Alert
+
+  dbStream.createProcStatement(client, 'CALL PROC_DUMMY (?, ?, ?, ?, ?)' + maliciousInput, function (err, stmt) { // $ MISSING: Alert


I'm not sure this is technically SQL injection. It's loading a stored procedure, and calling that with some arguments.

An attacker can definitely not do the usual SQL-injection attacks (e.g. inserting " OR 1=1 ").
But it still seems wrong to allow a user to control that string, so I suppose it's fine.
Did you have any thoughts on this kind of sink?

The concern I had was that, while typical SQL injection might not be possible, there's still a risk that users could invoke procedures they shouldn’t have access to or perform other unauthorized actions. 🤔

I think it's fine to err on the side of generating more alerts rather than fewer alerts for this case.

erik-krogh · 2025-03-27T11:46:59Z

javascript/ql/test/query-tests/Security/CWE-089/untyped/hana.js

  var client = req.db;
  let maliciousInput = req.body.data; // $ Source
-  client.exec('SELECT * FROM DUMMY' + maliciousInput, function (err, rs) {}); // $ MISSING: Alert
+  client.exec('SELECT * FROM DUMMY' + maliciousInput, function (err, rs) {}); // $ Alert


It would have been nice to restrict this sink to only the cases where we've detected the app1.use(hdbext.middleware(hanaConfig)); call.
But we can't express that with MaD.

So this will support all databases that attach themselves to the req.db property (and have an exec(..) method).
I think that's fine, but I would like a second opinion @asgerf.

Yes, I'm definitely not proud of the way I wrote this SQL sink, but I couldn't think of a way to express that in Model as Data. 🙈 The odds seem to be very low that there would exist path like this unrelated to hdbext.middleware(hanaConfig).

I'd like to go the extra mile here in order to make this work properly, because it's quite common to extend request/response objects from middlewares.

I'll aim to put up a PR that adds support for a new access path component called GuardedRouteHandler which should do what we need here. The idea is that it goes from the middleware object to all the route handlers guarded by it.

We should then be able to write the model like this:

I have a working proof of concept locally just need to polish it up a bit.

You can either

leave this PR open and wait for GuardedRouteHandler support, or

remove the express line from the model and mark the alerts as MISSING and merge as it is. I'll add it back when adding GuardedRouteHandler support.

I'll leave this PR open and wait for GuardedRouteHandler support.

Fixed f3af23e

javascript/ql/lib/change-notes/2025-03-26-hana-db-client.md

Co-authored-by: Erik Krogh Kristensen <[email protected]>

…recision.

erik-krogh · 2025-03-28T16:53:45Z

javascript/ql/lib/ext/hana-db-client.model.yml

+      extensible: typeModel
+    data:
+      - ["hdb.Client", "hdb", "Member[createClient].ReturnValue"]
+      - ["hdb.Client", "@sap/hdbext", "Member[middleware].ReturnValue.GuardedRouteHandler.Parameter[0].Member[db]"]


Could you also add a test that reg.db.exec(..) isn't a sink when there is no call to hdb.createClient().

Added 32d6ac8

…o `Express` are not flagged.

Napalys added 7 commits March 25, 2025 14:29

Added test cases for hana clients.

032cfc1

Add sink model for SQL injection detection in exec clients.

9229962

Added sink models for hana's client prepare function.

d28af95

Modeled execute as potential hana's sink.

e595def

Added @sap/hdbext.loadProccedure as sql sink.

0285cb6

Added createProcStatement as potential sql sink.

7cc0634

Added SQL injection detection for exec method embeded Express clien…

4cdc40d

…t from `hdbext`.

github-actions bot added the JS label Mar 25, 2025

Added change note.

62ab7f5

github-actions bot added the documentation label Mar 26, 2025

Napalys marked this pull request as ready for review March 26, 2025 08:37

Copilot bot review requested due to automatic review settings March 26, 2025 08:37

Napalys requested a review from a team as a code owner March 26, 2025 08:37

Copilot AI reviewed Mar 26, 2025

View reviewed changes

erik-krogh reviewed Mar 27, 2025

View reviewed changes

Napalys and others added 3 commits March 27, 2025 13:01

Update javascript/ql/lib/change-notes/2025-03-26-hana-db-client.md

e69929e

Co-authored-by: Erik Krogh Kristensen <[email protected]>

Merge branch 'main' into js/hana_db_client

f7264d8

Refactored hana's DB client to use GuardedRouteHandler, improving p…

f3af23e

…recision.

Napalys force-pushed the js/hana_db_client branch from b79d906 to f887c4c Compare March 28, 2025 13:07

Napalys added 2 commits March 28, 2025 14:55

Added sources from hana db as MaD.

d0e2aa8

Added test cases for hana db additional sources.

45c8ec9

Napalys force-pushed the js/hana_db_client branch from f887c4c to 45c8ec9 Compare March 28, 2025 14:02

Napalys requested review from asgerf and erik-krogh March 28, 2025 14:03

erik-krogh reviewed Mar 28, 2025

View reviewed changes

Add test case to ensure exec calls without middleware injection int…

32d6ac8

…o `Express` are not flagged.

Napalys force-pushed the js/hana_db_client branch from 01ff06e to 32d6ac8 Compare March 30, 2025 12:09

asgerf approved these changes Mar 31, 2025

View reviewed changes

erik-krogh approved these changes Mar 31, 2025

View reviewed changes

Napalys merged commit de8a328 into github:main Mar 31, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS: support `hana` db client #19118

JS: support `hana` db client #19118

Napalys commented Mar 25, 2025 •

edited

Loading

Copilot AI left a comment

erik-krogh Mar 27, 2025

Napalys Mar 27, 2025

asgerf Mar 27, 2025

erik-krogh Mar 27, 2025

Napalys Mar 27, 2025 •

edited

Loading

asgerf Mar 27, 2025

asgerf Mar 27, 2025

Napalys Mar 27, 2025

Napalys Mar 28, 2025

erik-krogh Mar 28, 2025

Napalys Mar 31, 2025

JS: support hana db client #19118

JS: support hana db client #19118

Conversation

Napalys commented Mar 25, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Napalys Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JS: support `hana` db client #19118

JS: support `hana` db client #19118

Napalys commented Mar 25, 2025 •

edited

Loading

Napalys Mar 27, 2025 •

edited

Loading