Skip to content

Javascript: js/path-injection misses Sails Action2 request-derived inputs #21773

@invoke1442

Description

@invoke1442

Hi CodeQL team,

I think js/path-injection is missing a fairly common Node/Sails pattern: request-derived values that have already been normalized by the framework into an inputs object before they reach the handler body.

The concrete example is a Sails Action2 controller from Planka's pre-fix code for CVE-2022-2653. The vulnerable flow is:

module.exports = {
  inputs: {
    id: { type: 'string', regex: /^[0-9]+$/, required: true },
    filename: { type: 'string', required: true },
  },

  async fn(inputs, exits) {
    // ...
    const filePath = path.join(
      sails.config.custom.attachmentsPath,
      attachment.dirname,
      'thumbnails',
      inputs.filename,
    );

    return exits.success(fs.createReadStream(filePath));
  },
};

The route was:

'GET /attachments/:id/download/thumbnails/:filename': {
  action: 'attachments/download-thumbnail',
  skipAssets: false,
},

and the fix was simply to remove the free-form :filename route parameter and hardcode the thumbnail name (cover-256.jpg).

What stands out here is that the sink side already seems to be in scope for js/path-injection:

  • the path is assembled with path.join(...)
  • it then flows into a standard file-read sink (fs.createReadStream)
  • there is no meaningful sanitizer or path-boundary check in between

So this does not look like a missing path sink or a tricky propagation step. It looks much more like a missing source model for Action2-style handlers where the remote input shows up as inputs.<name> instead of req.params.<name> / req.query.<name> / req.body.<name>.

I think this is worth covering because it is not a one-off project convention. In Sails apps, async fn(inputs, exits) is a standard handler shape, and once the framework has mapped route/query/body data into inputs, a lot of otherwise straightforward remote-input queries lose visibility if they only recognize the more direct Express/Koa-style request objects.

I have not tried to turn this into a broad claim about every JavaScript query, but the source-model gap itself seems generic.

I also think the fix surface is reasonably small and low-risk. This probably does not need any new sink modeling for js/path-injection. A targeted improvement along these lines may be enough:

  • recognize Sails Action2 handlers of the form module.exports = { inputs: ..., fn(inputs, exits) { ... } }
  • treat property reads from the first handler parameter (inputs.foo) as request-derived sources when that parameter corresponds to Action2 inputs
  • let the existing sink/propagation logic do the rest

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions