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
Hi CodeQL team,
I think
js/path-injectionis missing a fairly common Node/Sails pattern: request-derived values that have already been normalized by the framework into aninputsobject 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:
The route was:
and the fix was simply to remove the free-form
:filenameroute 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:path.join(...)fs.createReadStream)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 ofreq.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 intoinputs, 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:module.exports = { inputs: ..., fn(inputs, exits) { ... } }inputs.foo) as request-derived sources when that parameter corresponds to Action2 inputs