Skip to content
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.

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

JS: Add GuardedRouteHandler access path component #19132

Merged
merged 5 commits into from
Mar 28, 2025

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Mar 27, 2025

Adds a new MaD token called GuardedRouteHandler which goes from a middleware function installed in an HTTP router framework to all route handlers guarded by it.

For example

  const express = require('express')
  const app = express()

  app.use(require('@example/middleware').injectData())

  app.get('/foo', (req, res) => {
    req.data; // found by '@example/middleware'; Member[injectData].ReturnValue.GuardedRouteHandler.Parameter[0].Member[data]
  });

The immediate motivation for this is to support this use-case

For testing purposes, I temporarily included this test commit, and ran this evaluation. It shows good performance and some new taint sources were found (based on the model from the test commit).

@asgerf asgerf added the no-change-note-required This PR does not need a change note label Mar 28, 2025
@asgerf asgerf marked this pull request as ready for review March 28, 2025 08:25
@Copilot Copilot bot review requested due to automatic review settings March 28, 2025 08:25
@asgerf asgerf requested a review from a team as a code owner March 28, 2025 08:25
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new MaD token named GuardedRouteHandler to enable middleware to inject data into guarded route handlers. It adds new test definitions in the YAML file to map the token and provides a new test file to validate access to the injected data both before and after middleware installation.

Reviewed Changes

Copilot reviewed 2 out of 5 changed files in this pull request and generated no comments.

File Description
javascript/ql/test/library-tests/frameworks/data/test.ext.yml Added two new token mappings for GuardedRouteHandler parameters.
javascript/ql/test/library-tests/frameworks/data/guardedRouteHandler.js Created a new test file to verify data injection behavior before and after middleware installation.
Files not reviewed (3)
  • docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst: Language not supported
  • javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll: Language not supported
  • javascript/ql/test/library-tests/frameworks/data/test.expected: Language not supported
Comments suppressed due to low confidence (2)

javascript/ql/test/library-tests/frameworks/data/guardedRouteHandler.js:16

  • [nitpick] The property name 'injectedReqData' in this test differs from the documented example that uses 'data'. If this is intentional, consider adding a comment to explain the distinction; if not, update the property name for consistency.
    sink(req.injectedReqData); // NOT OK

javascript/ql/test/library-tests/frameworks/data/guardedRouteHandler.js:20

  • [nitpick] The property name 'injectedResData' differs from what is shown in the documented example. For consistency and clarity, consider aligning the naming or adding a comment to clarify why different names are used.
    sink(res.injectedResData); // NOT OK

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

Copy link
Contributor

@Napalys Napalys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, probably would be good if @erik-krog would review as well.

Comment on lines +385 to +392
- Since we're adding a new taint source, we add a tuple to the **sourceModel** extensible predicate.
- The first column, **"@example/middleware"**, begins the search at imports of the hypothetical NPM package **@example/middleware**.
- **Member[injectData]** selects accesses to the **injectData** member.
- **ReturnValue** selects the return value of the call to **injectData**.
- **GuardedRouteHandler** interprets the current value as a middleware function and selects all route handlers guarded by that middleware. Since the current value is passd to **app.use()**, the callback subsequently passed to **app.get()** is seen as a guarded route handler.
- **Parameter[0]** selects the first parameter of the callback (the parameter named **req**).
- **Member[data]** selects accesses to the **data** property of the **req** object.
- Finally, the kind **remote** indicates that this is considered a source of remote flow.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be helpful to provide an example of what happens to this after adding such MaD?

app.post('/email', (req, res) => {
    req.data;
} 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example above covers that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a quick chat about this; @Napalys wanted to highlight that it works for both .get and .post handlers. But since the model doesn't mention anything about get I don't think it's necessary.

@asgerf asgerf merged commit 7904db0 into github:main Mar 28, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants