FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Improve hapi coverage by jacknojo · Pull Request #22522 · github/codeql · GitHub

/ codeql Public

Improve hapi coverage - #22522

Open
jacknojo wants to merge 7 commits into
mainfrom
jacknojo/enhance_hapi_coverage
Open

jacknojo wants to merge 7 commits into
mainfrom
jacknojo/enhance_hapi_coverage

Conversation

jacknojo commented Sep 8, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

The false negative reproduced on this PR as a regression test had four separate breaks in the flow:

  1. Any handler given as part of server.route(config) was ignored when config came from a helper, like server.route(makeConfig(handler)).
  2. Forwarding the whole request.query object was not treated as a source.
  3. The custom endpoint registry's definition.handler.call(...) had no resolved target.
  4. Arguments passed through createCached did not reach the wrapped function:
    function createCached(handler) {
     return (...args) => handler(...args);
    }
    
    const cachedHandler = createCached((request) => {
      use(request.query.id);
    });
    
    cachedHandler(hapiRequest);
    Here the flow of hapiRequest stops at ...args.

In javascript/ql/lib/semmle/javascript/frameworks/Hapi.qll we:

  • Now recognize Hapi route and ext calls independently of whether their handlers can immediately be resolved. We backtrack to find the handler.
  • Now track request.query, request.params, and request.payload across supported data-flow steps and mark fields read from those values as sources, including reads in functions called by the handler. This follows the existing Express modeling pattern.
  • Introduce a Hapi-specific flow step for custom route registries. It connects a function stored in a route definition’s handler property with later calls to that property on the same definition object. Other property names than "handler" are not currently supported.

For javascript/ql/lib/semmle/javascript/dataflow/internal/FunctionWrapperSteps.qll

  • CodeQL already recognized createCached(fn) as a forwarding wrapper, including the rest/spread pattern (...args) => fn(...args).
  • It did not use that knowledge when the returned wrapper was later invoked.
  • The new step backtracks an invoked wrapper to its concrete wrapped function and maps each call argument to the corresponding function parameter.

This is a change to the overall JavaScript CodeQL code and while both DCAs have run fine, I am not confident in what the implications of that change is.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Copilot review overview

🟡 Changes recommended

Moderate unresolved issues remain in wrapper argument mapping and Hapi request-source tracking.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite (auto)
Findings: 1

Note

Copilot is running an experiment and ran this review at Lite.

Open findings (1) What changed in this PR

Improves JavaScript CodeQL modeling for indirect Hapi routes, request inputs, custom handler registries, and forwarding wrappers.

Changes:

  • Expands Hapi route and request-flow tracking.
  • Adds invocation flow through forwarding wrappers.
  • Adds regression coverage and documentation.
File Summary
javascript/​ql/​test/​library-tests/​frameworks/​hapi/​WrappedRouteFlow.qll Defines regression flow coverage.
javascript/​ql/​test/​library-tests/​frameworks/​hapi/​tests.ql Includes the regression test.
javascript/​ql/​test/​library-tests/​frameworks/​hapi/​tests.expected Records expected test results.
javascript/​ql/​test/​library-tests/​frameworks/​hapi/​src/​wrapped-route.js Adds the regression fixture.
javascript/​ql/​lib/​semmle/​javascript/​frameworks/​Hapi.qll Expands Hapi route and request modeling.
javascript/​ql/​lib/​semmle/​javascript/​dataflow/​internal/​FunctionWrapperSteps.qll Adds flow through invoked wrappers.
javascript/​ql/​lib/​change-notes/​2026-09-08-hapi-wrapped-routes.md Documents the change.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8390a2d1-70e3-4a01-84d3-3e5747d5da0b

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Copilot review overview

🔵 Needs a closer look

Moderate data-flow findings remain in Hapi request-source and reflective-call handling.

Review tier: Lite (auto)
Findings: None

Note

Copilot is running an experiment and ran this review at Lite.

Resolved findings (1) Previously missed findings (1)

In code that hasn't changed since last review

Track request properties after aliasing the request object

javascript/​ql/​lib/​semmle/​javascript/​frameworks/​Hapi.qll:109

This starts from a property read on the original parameter before applying type tracking, so it cannot follow the request object through a helper and then load query, params, or payload (for example, use(getRequest(request).query.id)). The HTTP abstraction explicitly exposes getARequestSource() for RequestSource.ref() tracking, and the analogous Express model uses req.ref() before reading these properties. Start from that request reference before loading property so aliases and calls are covered.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8390a2d1-70e3-4a01-84d3-3e5747d5da0b

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Copilot review overview

🟡 Changes recommended

The handler-property flow does not yet preserve receivers passed through .call, and additional payload/params coverage is requested.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite (auto)
Findings: 1

Note

Copilot is running an experiment and ran this review at Lite.

Open findings (1)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8390a2d1-70e3-4a01-84d3-3e5747d5da0b

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Copilot review overview

🔵 Needs a closer look

Three unresolved moderate findings remain in Hapi.qll.

Review tier: Lite (auto)
Findings: None

Note

Copilot is running an experiment and ran this review at Lite.

Resolved findings (1)

jacknojo marked this pull request as ready for review September 14, 2026 09:32
jacknojo requested a review from a team as a code owner September 14, 2026 09:32
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL