diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index 4ca7d12cc21..0cf6ca3380c 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -1321,6 +1321,19 @@ function uploadFile(myfile, id, callback) { * @apiDescription: Get feedback widgets with or without filters * @apiParam: 'app_key', app_key of related application provided by sdk request */ + //These two lookups serve the sdk, so they answer without a session and without an + //app_id to scope by, and they have to keep doing that or widgets stop rendering. + //What they must not do is hand out the fields the app-scoped /feedback/widgets + //deliberately withholds: targeting, which is the audience segmentation query, and + //cohortID, which that handler fetches only to test membership and then deletes with + //the comment "no need to return more data than needed". + // + //Excluded rather than allow-listed on purpose. These endpoints render every widget + //type, so an allow-list drawn from the rating-only projection above would drop the + //fields surveys and nps need, and the caller is an sdk in the field that cannot be + //redeployed. Naming the internal fields cannot break rendering. + const WIDGET_INTERNAL_FIELDS = {targeting: 0, cohortID: 0}; + plugins.register('/o/feedback/multiple-widgets-by-id', function(ob) { var params = ob.params; var collectionName = 'feedback_widgets'; @@ -1339,7 +1352,7 @@ function uploadFile(myfile, id, callback) { _id: { $in: widgetIdsArray } - }).toArray(function(err, docs) { + }, {projection: WIDGET_INTERNAL_FIELDS}).toArray(function(err, docs) { if (!err) { if (docs.length) { common.returnOutput(params, docs); @@ -1501,7 +1514,7 @@ function uploadFile(myfile, id, callback) { common.db.collection(collectionName).findOne({ "_id": widgetId - }, function(err, doc) { + }, {projection: WIDGET_INTERNAL_FIELDS}, function(err, doc) { if (err) { common.returnMessage(params, 500, err.message); } diff --git a/test/unit-tests/plugins.star-rating.widget-by-id-fields.js b/test/unit-tests/plugins.star-rating.widget-by-id-fields.js new file mode 100644 index 00000000000..fc9be367c0e --- /dev/null +++ b/test/unit-tests/plugins.star-rating.widget-by-id-fields.js @@ -0,0 +1,39 @@ +require("should"); +var fs = require("fs"); + +// /o/feedback/multiple-widgets-by-id and /o/feedback/widget answer the sdk without a +// session and without an app_id to scope by, so they cannot be gated without breaking +// widget rendering. What they can do is stop returning the fields the app-scoped +// /feedback/widgets deliberately withholds: targeting, the audience segmentation +// query, and cohortID, which that handler deletes with "no need to return more data +// than needed". +// +// Asserted against the source because both handlers are registered on a plugin bus at +// load time and their bodies are not reachable as functions from a unit test. + +describe("feedback widget by-id lookups", function() { + var src = fs.readFileSync(__dirname + "/../../plugins/star-rating/api/api.js", "utf8"); + + it("declares the internal fields as excluded", function() { + src.should.match(/const WIDGET_INTERNAL_FIELDS = \{targeting: 0, cohortID: 0\}/); + }); + + it("applies the exclusion to the batch lookup", function() { + var batch = src.slice(src.indexOf("'/o/feedback/multiple-widgets-by-id'")); + batch = batch.slice(0, batch.indexOf("plugins.register", 10)); + batch.should.match(/\$in: widgetIdsArray[\s\S]{0,80}projection: WIDGET_INTERNAL_FIELDS/); + }); + + it("applies the exclusion to the single lookup", function() { + var single = src.slice(src.indexOf("'/o/feedback/widget'")); + single = single.slice(0, single.indexOf("plugins.register", 10)); + single.should.match(/"_id": widgetId[\s\S]{0,60}projection: WIDGET_INTERNAL_FIELDS/); + }); + + it("excludes rather than allow-lists, so widget types beyond rating still render", function() { + // an inclusion projection here would drop the fields surveys and nps need, and + // the caller is an sdk in the field that cannot be redeployed + var decl = src.match(/const WIDGET_INTERNAL_FIELDS = \{[^}]*\}/)[0]; + decl.should.not.match(/: 1/); + }); +});