-
-
Notifications
You must be signed in to change notification settings - Fork 751
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
[Suggestion] Apply operator whitelist option only for "external" provider by default #2445
Comments
Can you post an example? If I understand your request, you could whitelist the entire set of query operators and then do a nested key search on the query object against a list of blocklist operators. // given this
let params= {
query: {
$superSearch: "raw_string",
createdAt: {
$alt_lt: new Date().getTime() - 10000
},
$or: [
{ archived: {
$ne: true,
$loop: [
{yes:"no"}
]
}
},
{ roomId: 2 }
]
}
}
// a quick function to get all the keys as an array
const deepKeys = (obj) => {
// get the keys
return Object.keys(obj).reduce((res, el) => {
if( Array.isArray(obj[el]) ) {
return [...res, el, ...obj[el].map(deepKeys).flat()]
} else if( typeof obj[el] === 'object' && obj[el] !== null ) {
return [...res, el, ...deepKeys(obj[el])];
}
return [...res, el];
}, []);
}
// then the hook is a simple check
let hook = (options) => async (ctx) => {
let keys = deepKeys(ctx.params.query)
const filteredArray = options.blacklist.filter(value => keys.includes(value));
if(filteredArray.length > 0){
new BadRequest('Invalid Query Parameters', {
errors: 'Cannot use query parameters: '+ filteredArray.join(" ")
});
}
}
hook({blacklist: ["$loop"]})({params}) |
@jamesvillarrubia sure, thanks for your example, but it's not what I'm referencing, but maybe could help to create a PR. The current logic allows querying with whitelisted operators inclusive internally and externally, which is in fact a potential vulnerability. const { MyService } = require('./my-service.class');
const createModel = require('../../models/my-service.model');
const hooks = require('./my-service.hooks');
module.exports = function (app) {
const options = {
Model: createModel(app),
paginate: app.get('paginate'),
whitelist: ['$populate', '$regex', '$options'] // <--- Need to add operators to whitelist to use it inclusive for internal query
};
// Initialize our service with any options it requires
app.use('/my-service', new MyService(options, app));
// Get our initialized service so that we can register hooks
const service = app.service('my-service');
service.hooks(hooks);
}; ProposalSo, I'm proposing to apply the service whitelist only to const options = {
Model: createModel(app),
paginate: app.get('paginate'),
whitelist: ['$populate', '$regex', '$options'] // <--- Only apply to user request not internal query, ex: context.provider === 'external'
};
app.use('/my-service', new MyService(options, app)); That's because don't have the sense to apply restrictions to the server-side querying. Whitelist as a function (extra proposal)Also, would be cool if can set a function as a value for the const options = {
Model: createModel(app),
paginate: app.get('paginate'),
whitelist: context => {
const { ip } = context.params;
if (fromVPC(ip)) return ['$where'];
else return ['$populate'];
}
};
app.use('/my-service', new MyService(options, app)); That will allow to customize different |
Could be possible in the next version
5.x
or current, to block all non-standard query operators for "external" provider and allow internally?Migration could be frustrating if on API you use non-standard query operators internally (not client-side). 🥵
I understand that
params
onfind
,get
,create
,patch
andremove
, receive theprovider
key can easily identify if is secure allow non-whitelisted query operators.The text was updated successfully, but these errors were encountered: