From 04768b481c1d3a1e0f2b62621bc17dde2f0cf437 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 18:39:39 +0300 Subject: [PATCH 1/4] security(configs): mark credential configs secret and withhold them on read Some configuration holds credentials rather than settings: an outbound proxy password, a reCAPTCHA secret, the key that signs report subscribe tokens. Those values were treated like any other setting, so they were returned to callers who can read configuration but cannot set it, and they were included in the values serialized into the dashboard page. Adds plugins.setSecretConfigs(namespace, {key: true}), mirroring the existing setUserConfigs, so a credential is declared where it is defined. Declared values are masked in /o/configs for anyone who is not a global admin, and are left out of what is serialized into the page. getConfig() is untouched, so server code keeps reading real values rather than authenticating with the placeholder. Masking builds a copy, so a read cannot alter the live configuration. An unset value stays empty instead of showing a mask, so "not configured" stays distinguishable from "configured but hidden". Marked: security.proxy_username, security.proxy_password, push.proxyuser, push.proxypass, reports.secretKey, recaptcha.secret_key. Deliberately not marked, because the browser needs them and hiding a value from an API response while it remains in page source achieves nothing: recaptcha.site_key, which is public by design, and tracking.self_tracking_app_key, which the dashboard uses to send its own telemetry. Every countlyGlobal.security and countlyGlobal.tracking reference was checked first; the dashboard uses only the password policy keys from security. Writes need no extra guard: /i/configs is validateGlobalAdmin, so the only callers who can write are the only callers who see real values, and a mask cannot be written back over a value. Co-Authored-By: Claude --- api/api.js | 9 ++ frontend/express/app.js | 12 +- plugins/pluginManager.js | 98 ++++++++++++++ plugins/plugins/api/api.js | 8 ++ plugins/plugins/tests.js | 119 ++++++++++++++++- plugins/push/api/api.js | 6 + plugins/recaptcha/frontend/app.js | 6 + plugins/reports/api/reports.js | 7 + .../plugins.pluginManager.secret-configs.js | 120 ++++++++++++++++++ 9 files changed, 381 insertions(+), 4 deletions(-) create mode 100644 test/unit-tests/plugins.pluginManager.secret-configs.js diff --git a/api/api.js b/api/api.js index c2c2d75f621..b435c59979c 100644 --- a/api/api.js +++ b/api/api.js @@ -124,6 +124,15 @@ plugins.connectToAllDatabases().then(function() { proxy_type: "https" }); + //the outbound proxy credentials. The whole security namespace used to be + //serialized into the dashboard page and returned to any app admin, so these were + //readable well below the operator who set them. The dashboard only uses the + //password policy keys from this namespace, so nothing needs them. + plugins.setSecretConfigs("security", { + proxy_username: true, + proxy_password: true + }); + /** * Set tracking config */ diff --git a/frontend/express/app.js b/frontend/express/app.js index b64ed4503c8..293b775cc5f 100644 --- a/frontend/express/app.js +++ b/frontend/express/app.js @@ -992,9 +992,15 @@ Promise.all([plugins.dbConnection(countlyConfig), plugins.dbConnection("countly_ csrf_token: req.csrfToken(), auth_token: req.session.auth_token, member: member, - config: req.config, - security: plugins.getConfig("security"), - tracking: plugins.getConfig("tracking"), + //these namespaces are serialized into the page, so every logged in + //user can read them in the source regardless of their permissions. + //Secret values are dropped rather than masked: the page has no use + //for a placeholder. Anything the browser genuinely needs must not + //be marked secret, since masking what is already in page source + //would achieve nothing. + config: plugins.omitSecretConfigs("frontend", req.config), + security: plugins.omitSecretConfigs("security", plugins.getConfig("security")), + tracking: plugins.omitSecretConfigs("tracking", plugins.getConfig("tracking")), plugins: plugins.getPlugins(), pluginsFull: plugins.getPlugins(true), path: countlyConfig.path || "", diff --git a/plugins/pluginManager.js b/plugins/pluginManager.js index 5b44727de75..ff7eebd7371 100644 --- a/plugins/pluginManager.js +++ b/plugins/pluginManager.js @@ -26,6 +26,11 @@ var pluginDependencies = require('./pluginDependencies.js'), configextender = require('../api/configextender'); var pluginConfig = {}; +//stands in for a secret configuration value when configuration is read by someone +//who may know the value is set but not what it is. Deliberately not a plausible +//value, so it is obvious in a UI and in a bug report. +const SECRET_CONFIG_MASK = "********"; + /** * This module handles communicaton with plugins * @module "plugins/pluginManager" @@ -41,6 +46,9 @@ var pluginManager = function pluginManager() { var defaultConfigs = {}; var configsOnchanges = {}; var excludeFromUI = {plugins: true}; + //configuration values that hold credentials rather than settings, by namespace. + //see setSecretConfigs() + var secretConfigs = {}; var finishedSyncing = true; var expireList = []; var masking = {}; @@ -388,6 +396,96 @@ var pluginManager = function pluginManager() { } }; + /** + * Mark configuration values as secrets, so they are not handed out on read. + * + * Some configuration holds credentials rather than settings: a proxy password, an + * API key, the key report subscribe tokens are signed with. Those are readable by + * anyone who can read configuration at all, which is a lower bar than the + * operator who set them. Marking them here masks the value everywhere + * configuration is returned to a caller who is not a global admin, and keeps it + * out of the values exposed to the dashboard page. + * + * A value the browser needs is not a secret and must not be marked: once it is in + * page source, masking is theatre. tracking.self_tracking_app_key and + * recaptcha.site_key are deliberately not marked for that reason. + * + * getConfig() is unaffected, so server code keeps reading the real value. + * + * @param {string} namespace - namespace of configuration, usually plugin name + * @param {object} conf - object whose keys are secret when the value is true + **/ + this.setSecretConfigs = function(namespace, conf) { + if (!secretConfigs[namespace]) { + secretConfigs[namespace] = {}; + } + for (let i in conf) { + secretConfigs[namespace][i] = conf[i]; + } + }; + + /** + * Whether a configuration value is marked secret. + * @param {string} namespace - namespace of configuration + * @param {string} key - configuration key + * @returns {boolean} true when the value must not be handed out + **/ + this.isSecretConfig = function(namespace, key) { + return !!(secretConfigs[namespace] && secretConfigs[namespace][key] === true); + }; + + /** + * Copy of a full configuration object with every secret value replaced by a + * placeholder. Used for callers who may see that configuration exists but not what + * it is set to. + * + * The input is not modified, so the caller cannot accidentally hand a masked + * object to code that needs the real values. + * + * @param {object} confs - object of namespace -> configuration object + * @returns {object} copy with secret values masked + **/ + this.maskSecretConfigs = function(confs) { + var masked = {}; + for (let namespace in confs) { + var conf = confs[namespace]; + if (!conf || typeof conf !== "object" || Array.isArray(conf)) { + masked[namespace] = conf; + continue; + } + masked[namespace] = {}; + for (let key in conf) { + masked[namespace][key] = this.isSecretConfig(namespace, key) && conf[key] !== "" && conf[key] !== null && typeof conf[key] !== "undefined" + ? SECRET_CONFIG_MASK + : conf[key]; + } + } + return masked; + }; + + /** + * Copy of one namespace's configuration with secret keys removed entirely, for + * values that are serialized into the dashboard page. Removed rather than masked: + * the page has no use for a placeholder, and an absent key cannot be mistaken for + * a real one. + * + * @param {string} namespace - namespace of configuration + * @param {object} conf - configuration object for that namespace + * @returns {object} copy without the secret keys + **/ + this.omitSecretConfigs = function(namespace, conf) { + if (!conf || typeof conf !== "object" || Array.isArray(conf)) { + return conf; + } + var out = {}; + for (let key in conf) { + if (!this.isSecretConfig(namespace, key)) { + out[key] = conf[key]; + } + } + return out; + }; + /** * Get configuration from specific namespace and populate empty values with provided defaults * @param {string} namespace - namespace of configuration, usually plugin name diff --git a/plugins/plugins/api/api.js b/plugins/plugins/api/api.js index 02165726e8a..bd967513317 100644 --- a/plugins/plugins/api/api.js +++ b/plugins/plugins/api/api.js @@ -282,6 +282,14 @@ var plugin = {}, plugins.loadConfigs(common.db, function() { var confs = plugins.getAllConfigs(); delete confs.services; + //an app admin may see which configuration exists, and how their app + //differs from the server default, but not the values that are + //credentials: a proxy password, an API key, the key report subscribe + //tokens are signed with. Only a global admin can set those, so only a + //global admin is shown them. + if (!params.member.global_admin) { + confs = plugins.maskSecretConfigs(confs); + } common.returnOutput(params, confs); }); }); diff --git a/plugins/plugins/tests.js b/plugins/plugins/tests.js index 1ce26c98f7d..cae464fbaed 100644 --- a/plugins/plugins/tests.js +++ b/plugins/plugins/tests.js @@ -38,4 +38,121 @@ describe('Testing Plugins', function() { done(); }); }); -}); \ No newline at end of file +}); + +// Configuration values that hold credentials are marked with setSecretConfigs() and +// masked for anyone who is not a global admin. /o/configs is validateAppAdmin, so an +// app admin can see which configuration exists and how their app differs from the +// server default, but not the credential values. Only a global admin can set those +// (/i/configs is validateGlobalAdmin), so only a global admin is shown them. +// +// reports.secretKey is the case that matters most: it self-generates, so it is set on +// every install, and it signs the tokens that /subscribe_report accepts without +// authentication. +describe('Testing configs secret masking', function() { + var MASK = "********"; + var API_KEY_ADMIN = ""; + var APP_ID = ""; + var memberApiKey = ""; + var memberUserId = ""; + var uniq = Date.now(); + + it('should read the real secret as a global admin', function(done) { + API_KEY_ADMIN = testUtils.get("API_KEY_ADMIN"); + APP_ID = testUtils.get("APP_ID"); + request + .get('/o/configs?api_key=' + API_KEY_ADMIN + '&app_id=' + APP_ID) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + var ob = JSON.parse(res.text); + ob.should.have.property("reports"); + ob.reports.should.have.property("secretKey"); + ob.reports.secretKey.should.not.equal(MASK); + ob.reports.secretKey.length.should.be.above(0); + done(); + }); + }); + + it('should create an app admin who is not a global admin', function(done) { + var permission = { + _: {a: [APP_ID], u: [[APP_ID]]}, + c: {}, + r: {}, + u: {}, + d: {} + }; + var userParams = { + full_name: "cfgadmin" + uniq, + username: "cfgadmin" + uniq, + password: "p4ssw0rD!", + email: "cfgadmin" + uniq + "@mail.test", + permission: permission + }; + request + .get('/i/users/create?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify(userParams))) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + memberApiKey = res.body.api_key; + memberUserId = res.body._id; + memberApiKey.should.not.equal(API_KEY_ADMIN); + done(); + }); + }); + + it('should mask secrets for an app admin', function(done) { + request + .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + var ob = JSON.parse(res.text); + ob.reports.secretKey.should.equal(MASK); + ob.security.should.have.property("proxy_password"); + ob.security.proxy_password.should.not.match(/[a-z0-9]{6,}/i); + done(); + }); + }); + + it('should still return non-secret configuration to the app admin', function(done) { + request + .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + var ob = JSON.parse(res.text); + // the app management screen needs these, so masking must not touch them + ob.should.have.property("api"); + ob.api.should.have.property("session_duration_limit"); + ob.api.should.have.property("event_limit"); + ob.security.should.have.property("password_min"); + ob.security.password_min.should.not.equal(MASK); + // public by design, must not be masked + if (ob.recaptcha) { + ob.recaptcha.should.have.property("site_key"); + ob.recaptcha.site_key.should.not.equal(MASK); + } + done(); + }); + }); + + after(function(done) { + if (!memberUserId) { + return done(); + } + request + .get('/i/users/delete?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify({user_ids: [memberUserId]}))) + .end(function() { + done(); + }); + }); +}); diff --git a/plugins/push/api/api.js b/plugins/push/api/api.js index 7ebc47aaa6e..7674343cd89 100644 --- a/plugins/push/api/api.js +++ b/plugins/push/api/api.js @@ -70,6 +70,12 @@ plugins.setConfigs(FEATURE_NAME, { default_content_available: false, // sets content-available: 1 by default for ios }); +//outbound proxy credentials for sending pushes +plugins.setSecretConfigs(FEATURE_NAME, { + proxyuser: true, + proxypass: true +}); + plugins.internalEvents.push('[CLY]_push_sent'); plugins.internalEvents.push('[CLY]_push_action'); plugins.internalDrillEvents.push('[CLY]_push_action'); diff --git a/plugins/recaptcha/frontend/app.js b/plugins/recaptcha/frontend/app.js index ec32327a47c..0637ea95cb8 100644 --- a/plugins/recaptcha/frontend/app.js +++ b/plugins/recaptcha/frontend/app.js @@ -9,6 +9,12 @@ plugins.setConfigs("recaptcha", { secret_key: "" }); +//only secret_key. site_key is public by design: it goes to the browser to render the +//challenge, so marking it secret would achieve nothing. +plugins.setSecretConfigs("recaptcha", { + secret_key: true +}); + (function(plugin) { plugin.init = function(app, countlyDb) { plugins.loadConfigs(countlyDb, function() { diff --git a/plugins/reports/api/reports.js b/plugins/reports/api/reports.js index 1cc57ae2f0c..a439e5eec44 100644 --- a/plugins/reports/api/reports.js +++ b/plugins/reports/api/reports.js @@ -24,6 +24,13 @@ plugins.setConfigs("reports", { secretKey: countlyApiConfig?.encryption?.reports_key || "Ydqa7Omkd3yhV33M3iWV1oFcOEk898h9", }); +//this key signs the subscribe/unsubscribe tokens, and /subscribe_report and +///unsubscribe_report accept those without authentication. It self-generates, so +//unlike the other secrets it is always set on every install. +plugins.setSecretConfigs("reports", { + secretKey: true +}); + versionInfo.page = (!versionInfo.title) ? "https://count.ly" : null; versionInfo.title = versionInfo.title || "Countly"; var metrics = { diff --git a/test/unit-tests/plugins.pluginManager.secret-configs.js b/test/unit-tests/plugins.pluginManager.secret-configs.js new file mode 100644 index 00000000000..b4c90b0d69e --- /dev/null +++ b/test/unit-tests/plugins.pluginManager.secret-configs.js @@ -0,0 +1,120 @@ +require("should"); +var plugins = require("../../plugins/pluginManager.js"); + +// Configuration values that hold credentials rather than settings are marked with +// setSecretConfigs(). Marking one masks it wherever configuration is returned to a +// caller who is not a global admin, and drops it from the values serialized into the +// dashboard page. +// +// The two invariants that matter: +// - getConfig() is unaffected, or server code would start authenticating with the +// placeholder instead of the password; +// - the input object is never mutated, or masking one response would corrupt the +// live configuration for everything else. +describe("pluginManager secret configs", function() { + before(function() { + plugins.setConfigs("unittest_secret", { + limit: 10, + enabled: true, + proxy_password: "REAL_PASSWORD", + api_key: "REAL_KEY", + site_key: "public-site-key", + unset_secret: "" + }); + plugins.setSecretConfigs("unittest_secret", { + proxy_password: true, + api_key: true, + unset_secret: true + }); + }); + + describe("isSecretConfig", function() { + it("is true only for marked keys", function() { + plugins.isSecretConfig("unittest_secret", "proxy_password").should.equal(true); + plugins.isSecretConfig("unittest_secret", "api_key").should.equal(true); + }); + it("is false for unmarked keys in the same namespace", function() { + plugins.isSecretConfig("unittest_secret", "limit").should.equal(false); + plugins.isSecretConfig("unittest_secret", "site_key").should.equal(false); + }); + it("is false for an unknown namespace", function() { + plugins.isSecretConfig("unittest_no_such_namespace", "proxy_password").should.equal(false); + }); + it("is false for inherited Object.prototype keys", function() { + plugins.isSecretConfig("unittest_secret", "constructor").should.equal(false); + plugins.isSecretConfig("unittest_secret", "toString").should.equal(false); + plugins.isSecretConfig("constructor", "constructor").should.equal(false); + }); + }); + + describe("getConfig", function() { + it("still returns the real value, so server code keeps working", function() { + var conf = plugins.getConfig("unittest_secret"); + conf.proxy_password.should.equal("REAL_PASSWORD"); + conf.api_key.should.equal("REAL_KEY"); + }); + }); + + describe("maskSecretConfigs", function() { + it("masks marked values and leaves the rest alone", function() { + var masked = plugins.maskSecretConfigs({unittest_secret: plugins.getConfig("unittest_secret")}); + masked.unittest_secret.proxy_password.should.not.equal("REAL_PASSWORD"); + masked.unittest_secret.api_key.should.not.equal("REAL_KEY"); + masked.unittest_secret.limit.should.equal(10); + masked.unittest_secret.enabled.should.equal(true); + masked.unittest_secret.site_key.should.equal("public-site-key"); + }); + it("does not modify the object it was given", function() { + var input = {unittest_secret: plugins.getConfig("unittest_secret")}; + plugins.maskSecretConfigs(input); + input.unittest_secret.proxy_password.should.equal("REAL_PASSWORD"); + }); + it("leaves an unset secret empty, so 'not configured' stays visible", function() { + var masked = plugins.maskSecretConfigs({unittest_secret: plugins.getConfig("unittest_secret")}); + masked.unittest_secret.unset_secret.should.equal(""); + }); + it("leaves namespaces it does not know untouched", function() { + var masked = plugins.maskSecretConfigs({other: {password: "not marked"}}); + masked.other.password.should.equal("not marked"); + }); + it("tolerates a non-object namespace value", function() { + var masked = plugins.maskSecretConfigs({a: null, b: "str", c: [1, 2]}); + (masked.a === null).should.equal(true); + masked.b.should.equal("str"); + masked.c.should.eql([1, 2]); + }); + it("masks every marked key, not just the first", function() { + var masked = plugins.maskSecretConfigs({unittest_secret: plugins.getConfig("unittest_secret")}); + masked.unittest_secret.proxy_password.should.equal(masked.unittest_secret.api_key); + }); + }); + + describe("omitSecretConfigs", function() { + it("removes marked keys entirely rather than masking them", function() { + var exposed = plugins.omitSecretConfigs("unittest_secret", plugins.getConfig("unittest_secret")); + exposed.should.not.have.property("proxy_password"); + exposed.should.not.have.property("api_key"); + exposed.should.have.property("limit", 10); + exposed.should.have.property("site_key", "public-site-key"); + }); + it("does not modify the object it was given", function() { + var conf = plugins.getConfig("unittest_secret"); + plugins.omitSecretConfigs("unittest_secret", conf); + conf.proxy_password.should.equal("REAL_PASSWORD"); + }); + it("returns a namespace with no secrets unchanged in content", function() { + var exposed = plugins.omitSecretConfigs("unittest_no_such_namespace", {a: 1, b: 2}); + exposed.should.eql({a: 1, b: 2}); + }); + it("tolerates a missing config object", function() { + (plugins.omitSecretConfigs("unittest_secret", null) === null).should.equal(true); + (typeof plugins.omitSecretConfigs("unittest_secret", undefined)).should.equal("undefined"); + }); + }); + + // Which keys this codebase actually marks is deliberately not asserted here: a + // unit test requiring pluginManager directly never loads api/api.js or the plugin + // modules that declare them, so every such assertion would pass or fail for the + // wrong reason. That belongs in plugins/plugins/tests.js, which runs against a + // started server with the declarations in place. +}); From 8c08b7e9bd513ccea59e91344fa94d8445ba79c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 12:54:28 +0300 Subject: [PATCH 2/4] security(configs): return only declared configuration to non global admins Marking credentials as secret, added in the previous commit, is the wrong way round on its own. Everything is returned by default and safety depends on whoever adds the next setting remembering to annotate it, so one forgotten annotation puts a credential back in the response with no signal. Adds plugins.setReadableConfigs(namespace, {key: true}) and inverts the default: a caller who is not a global admin receives only values some part of the dashboard needs, and a namespace with nothing declared is dropped entirely. A setting added tomorrow is private until somebody declares it, so the cost of forgetting is a missing input rather than a leak. Both mechanisms now apply, in that order. The allow-list is the control. Masking is the backstop, so a credential declared readable by mistake still has its value withheld. Declared, from surveying every reader across countly-server and countly-enterprise-plugins: api the 13 keys App Management renders, plus domain, which crash symbolication uses to build its return URL security the password policy the dashboard validates against feedback the three colours the ratings widget previews with concurrent_users alert_interval, declared by the alerts plugin that reads it, since the namespace belongs to an enterprise plugin in another repository A namespace being withheld is logged at debug, because the dashboard degrades silently when configuration is missing: the call site uses .always(), the model initialises to {}, and the loop is guarded, so an undeclared value shows up as an input quietly absent from a screen rather than an error. The App Management list is read out of the frontend by the test rather than duplicated in it, so adding a key there without declaring it readable fails CI instead of removing a setting from the screen. Enterprise plugins need matching declarations for the namespaces they own and read: crashes.custom_domain, journey_engine.approver_enabled and whatever data-manager needs. Until then those degrade to their existing fallbacks. Co-Authored-By: Claude --- api/api.js | 37 ++++++++ plugins/alerts/api/api.js | 8 ++ plugins/pluginManager.js | 70 +++++++++++++++ plugins/plugins/api/api.js | 28 ++++-- plugins/plugins/tests.js | 88 +++++++++++++------ plugins/star-rating/api/api.js | 8 ++ .../plugins.pluginManager.secret-configs.js | 66 +++++++++++++- 7 files changed, 273 insertions(+), 32 deletions(-) diff --git a/api/api.js b/api/api.js index b435c59979c..b09ae9ea22c 100644 --- a/api/api.js +++ b/api/api.js @@ -90,6 +90,43 @@ plugins.connectToAllDatabases().then(function() { trim_trailing_ending_spaces: false }); + //What a non global admin may read from the api namespace. The first group is what + //App Management renders: the dashboard infers each input's widget from the type of + //the value here, so a key missing from this list means that setting disappears from + //the screen. Keep it in step with showInAppManagment in the plugins frontend. + // + //domain is separate: crash symbolication builds its return URL from it, and falls + //back to the browser's origin when it is absent, which is wrong behind a custom + //domain rather than merely cosmetic. + plugins.setReadableConfigs("api", { + safe: true, + session_duration_limit: true, + country_data: true, + city_data: true, + event_limit: true, + event_segmentation_limit: true, + event_segmentation_value_limit: true, + metric_limit: true, + session_cooldown: true, + total_users: true, + prevent_duplicate_requests: true, + metric_changes: true, + trim_trailing_ending_spaces: true, + domain: true + }); + + //the dashboard reads the password policy to validate a new password before sending + //it. Nothing else from this namespace is readable, which is what keeps the proxy + //credentials out of the response. + plugins.setReadableConfigs("security", { + password_min: true, + password_char: true, + password_number: true, + password_symbol: true, + password_expiration: true, + password_autocomplete: true + }); + /** * Set Plugins APPs Config */ diff --git a/plugins/alerts/api/api.js b/plugins/alerts/api/api.js index 6bf378e155e..7f71891a4ff 100644 --- a/plugins/alerts/api/api.js +++ b/plugins/alerts/api/api.js @@ -10,6 +10,14 @@ const FEATURE_NAME = 'alerts'; const commonLib = require("./parts/common-lib.js"); const moment = require('moment-timezone'); +//the alert drawer reads concurrent_users.alert_interval to bound the interval it +//offers, falling back to a hardcoded 3 minutes when it cannot. The namespace belongs +//to an enterprise plugin, so it is declared here by the consumer that needs it rather +//than by its owner. +plugins.setReadableConfigs("concurrent_users", { + alert_interval: true +}); + /** * Alerts that can be triggered when an event is received. * see module file for details. diff --git a/plugins/pluginManager.js b/plugins/pluginManager.js index ff7eebd7371..4113128adcd 100644 --- a/plugins/pluginManager.js +++ b/plugins/pluginManager.js @@ -49,6 +49,9 @@ var pluginManager = function pluginManager() { //configuration values that hold credentials rather than settings, by namespace. //see setSecretConfigs() var secretConfigs = {}; + //configuration values a caller who is not a global admin may read, by namespace. + //Nothing is readable unless it appears here. See setReadableConfigs() + var readableConfigs = {}; var finishedSyncing = true; var expireList = []; var masking = {}; @@ -396,6 +399,73 @@ var pluginManager = function pluginManager() { } }; + /** + * Declare which configuration values a caller who is not a global admin may read. + * + * Nothing is readable unless declared here. That direction is deliberate: marking + * secrets instead would mean every new setting is exposed until somebody remembers + * to mark it, so one forgotten annotation is a leaked credential. Declaring what is + * needed fails the other way, and a missing entry costs a missing input in the UI, + * which someone notices and fixes. + * + * Declare the keys your own code reads. A plugin may declare a key from another + * namespace when it is the consumer: the reader knows what it needs, and the + * namespace owner may live in a different repository. + * + * This governs reads by other people. It has nothing to do with what the server + * itself can read, which is always the real configuration through getConfig(). + * + * @param {string} namespace - namespace of configuration, usually plugin name + * @param {object} conf - object whose keys are readable when the value is true + **/ + this.setReadableConfigs = function(namespace, conf) { + if (!readableConfigs[namespace]) { + readableConfigs[namespace] = {}; + } + for (let i in conf) { + readableConfigs[namespace][i] = conf[i]; + } + }; + + /** + * Whether a caller who is not a global admin may read a configuration value. + * @param {string} namespace - namespace of configuration + * @param {string} key - configuration key + * @returns {boolean} true when the value may be handed out + **/ + this.isReadableConfig = function(namespace, key) { + return !!(Object.prototype.hasOwnProperty.call(readableConfigs, namespace) + && readableConfigs[namespace][key] === true); + }; + + /** + * Copy of a full configuration object reduced to the values a caller who is not a + * global admin may read. A namespace with nothing declared is dropped entirely. + * + * The input is not modified. + * + * @param {object} confs - object of namespace -> configuration object + * @returns {object} copy containing only declared values + **/ + this.filterReadableConfigs = function(confs) { + var out = {}; + for (let namespace in confs) { + var conf = confs[namespace]; + if (!conf || typeof conf !== "object" || Array.isArray(conf)) { + continue; + } + for (let key in conf) { + if (this.isReadableConfig(namespace, key)) { + if (!out[namespace]) { + out[namespace] = {}; + } + out[namespace][key] = conf[key]; + } + } + } + return out; + }; + /** * Mark configuration values as secrets, so they are not handed out on read. * diff --git a/plugins/plugins/api/api.js b/plugins/plugins/api/api.js index bd967513317..2be8344b2eb 100644 --- a/plugins/plugins/api/api.js +++ b/plugins/plugins/api/api.js @@ -282,13 +282,29 @@ var plugin = {}, plugins.loadConfigs(common.db, function() { var confs = plugins.getAllConfigs(); delete confs.services; - //an app admin may see which configuration exists, and how their app - //differs from the server default, but not the values that are - //credentials: a proxy password, an API key, the key report subscribe - //tokens are signed with. Only a global admin can set those, so only a - //global admin is shown them. + //A caller who is not a global admin gets only the values some part of + //the dashboard needs in order to work: the app management settings and + //a handful of display values. Nothing else, whether or not anybody + //remembered it was sensitive. + // + //Two mechanisms on purpose, in this order. The allow-list is the + //control: a value nobody declared is not returned, so a new setting is + //private by default and forgetting costs a missing input rather than a + //leak. Masking is the backstop: if a credential is ever declared + //readable by mistake, its value is still withheld. if (!params.member.global_admin) { - confs = plugins.maskSecretConfigs(confs); + var before = Object.keys(confs); + confs = plugins.maskSecretConfigs(plugins.filterReadableConfigs(confs)); + //a namespace disappearing here is how an undeclared value looks + //from the outside, and the UI degrades silently when it happens, so + //leave a trail rather than nothing + var dropped = before.filter(function(ns) { + return !confs[ns]; + }); + if (dropped.length) { + log.d("Withheld configuration namespaces from a non global admin" + + common.reqInfo(params) + ": " + dropped.join(", ")); + } } common.returnOutput(params, confs); }); diff --git a/plugins/plugins/tests.js b/plugins/plugins/tests.js index cae464fbaed..674ccdab10a 100644 --- a/plugins/plugins/tests.js +++ b/plugins/plugins/tests.js @@ -1,5 +1,6 @@ -/*global describe,it */ +/*global describe,it,before,after */ var request = require('supertest'); +var should = require('should'); var testUtils = require("../../test/testUtils"); request = request(testUtils.url); @@ -40,16 +41,19 @@ describe('Testing Plugins', function() { }); }); -// Configuration values that hold credentials are marked with setSecretConfigs() and -// masked for anyone who is not a global admin. /o/configs is validateAppAdmin, so an -// app admin can see which configuration exists and how their app differs from the -// server default, but not the credential values. Only a global admin can set those -// (/i/configs is validateGlobalAdmin), so only a global admin is shown them. +// Configuration read by someone who is not a global admin is reduced twice over. // -// reports.secretKey is the case that matters most: it self-generates, so it is set on -// every install, and it signs the tokens that /subscribe_report accepts without -// authentication. -describe('Testing configs secret masking', function() { +// The allow-list is the control: setReadableConfigs declares what some part of the +// dashboard needs, and nothing else is returned. A setting added tomorrow is private +// until somebody declares it, so forgetting costs a missing input rather than a leak. +// +// Masking is the backstop: a value marked with setSecretConfigs is withheld even if it +// is declared readable by mistake. +// +// /o/configs is validateAppAdmin, so an app admin reaches it; /i/configs is +// validateGlobalAdmin, so only a global admin can write. That is why masking needs no +// write guard: the only callers who can write are the only callers who see real values. +describe('Testing configs read reduction', function() { var MASK = "********"; var API_KEY_ADMIN = ""; var APP_ID = ""; @@ -57,7 +61,7 @@ describe('Testing configs secret masking', function() { var memberUserId = ""; var uniq = Date.now(); - it('should read the real secret as a global admin', function(done) { + it('should give a global admin the full configuration', function(done) { API_KEY_ADMIN = testUtils.get("API_KEY_ADMIN"); APP_ID = testUtils.get("APP_ID"); request @@ -68,10 +72,13 @@ describe('Testing configs secret masking', function() { return done(err); } var ob = JSON.parse(res.text); + // the signing key self-generates, so it is always set ob.should.have.property("reports"); - ob.reports.should.have.property("secretKey"); ob.reports.secretKey.should.not.equal(MASK); ob.reports.secretKey.length.should.be.above(0); + // and the namespaces a non-global admin will not get + ob.should.have.property("security"); + ob.security.should.have.property("proxy_password"); done(); }); }); @@ -105,7 +112,7 @@ describe('Testing configs secret masking', function() { }); }); - it('should mask secrets for an app admin', function(done) { + it('should withhold undeclared namespaces from an app admin entirely', function(done) { request .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID) .expect(200) @@ -114,14 +121,18 @@ describe('Testing configs secret masking', function() { return done(err); } var ob = JSON.parse(res.text); - ob.reports.secretKey.should.equal(MASK); - ob.security.should.have.property("proxy_password"); - ob.security.proxy_password.should.not.match(/[a-z0-9]{6,}/i); + // nothing in these is needed by the dashboard, so the whole namespace + // goes, credential or not + ob.should.not.have.property("reports"); + ob.should.not.have.property("push"); + ob.should.not.have.property("recaptcha"); + // no value anywhere in the response looks like the signing key + JSON.stringify(ob).should.not.match(/secretKey/); done(); }); }); - it('should still return non-secret configuration to the app admin', function(done) { + it('should withhold the proxy credentials while keeping the password policy', function(done) { request .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID) .expect(200) @@ -130,17 +141,44 @@ describe('Testing configs secret masking', function() { return done(err); } var ob = JSON.parse(res.text); - // the app management screen needs these, so masking must not touch them - ob.should.have.property("api"); - ob.api.should.have.property("session_duration_limit"); - ob.api.should.have.property("event_limit"); + ob.should.have.property("security"); + ob.security.should.not.have.property("proxy_password"); + ob.security.should.not.have.property("proxy_username"); + // the dashboard validates a new password against these before sending it ob.security.should.have.property("password_min"); ob.security.password_min.should.not.equal(MASK); - // public by design, must not be masked - if (ob.recaptcha) { - ob.recaptcha.should.have.property("site_key"); - ob.recaptcha.site_key.should.not.equal(MASK); + done(); + }); + }); + + it('should still return every setting App Management renders', function(done) { + // App Management infers each input's widget from the type of the value here, so + // a key missing from the allow-list makes that setting disappear from the screen + // with no error. The list is read from the frontend rather than duplicated, so + // adding a key there without declaring it readable fails this test. + var fs = require('fs'); + var path = require('path'); + var viewsFile = path.resolve(__dirname, 'frontend/public/javascripts/countly.views.js'); + var src = fs.readFileSync(viewsFile, 'utf8'); + var block = /var showInAppManagment\s*=\s*\{\s*"api"\s*:\s*\{([\s\S]*?)\}/.exec(src); + should.exist(block); + var wanted = (block[1].match(/"([a-z_]+)"\s*:\s*true/g) || []).map(function(m) { + return /"([a-z_]+)"/.exec(m)[1]; + }); + wanted.length.should.be.above(0); + + request + .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); } + var ob = JSON.parse(res.text); + ob.should.have.property("api"); + wanted.forEach(function(key) { + ob.api.should.have.property(key); + }); done(); }); }); diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index 4ca7d12cc21..6a9f18b6410 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -28,6 +28,14 @@ if (!surveysEnabled) { feedback_logo: "" }); + + //the ratings and surveys widget previews style themselves from these when the app + //has no per-app override, so a non global admin needs to read them + plugins.setReadableConfigs("feedback", { + main_color: true, + font_color: true, + feedback_logo: true + }); } const FEATURE_NAME = 'star_rating'; diff --git a/test/unit-tests/plugins.pluginManager.secret-configs.js b/test/unit-tests/plugins.pluginManager.secret-configs.js index b4c90b0d69e..6192973ea92 100644 --- a/test/unit-tests/plugins.pluginManager.secret-configs.js +++ b/test/unit-tests/plugins.pluginManager.secret-configs.js @@ -112,7 +112,71 @@ describe("pluginManager secret configs", function() { }); }); - // Which keys this codebase actually marks is deliberately not asserted here: a + describe("setReadableConfigs / filterReadableConfigs", function() { + before(function() { + plugins.setConfigs("unittest_readable", { + needed: 1, + also_needed: "yes", + not_needed: "internal", + brand_new_api_key: "NOBODY_MARKED_THIS" + }); + plugins.setReadableConfigs("unittest_readable", { + needed: true, + also_needed: true + }); + }); + + it("returns declared values", function() { + var out = plugins.filterReadableConfigs({unittest_readable: plugins.getConfig("unittest_readable")}); + out.unittest_readable.should.have.property("needed", 1); + out.unittest_readable.should.have.property("also_needed", "yes"); + }); + it("withholds anything not declared", function() { + var out = plugins.filterReadableConfigs({unittest_readable: plugins.getConfig("unittest_readable")}); + out.unittest_readable.should.not.have.property("not_needed"); + }); + it("withholds a newly added credential nobody marked secret", function() { + // the point of the allow-list. Marking secrets is the other direction and + // leaks until someone remembers; this leaks only if someone opts in. + var out = plugins.filterReadableConfigs({unittest_readable: plugins.getConfig("unittest_readable")}); + out.unittest_readable.should.not.have.property("brand_new_api_key"); + }); + it("drops a namespace with nothing declared, rather than returning it empty", function() { + var out = plugins.filterReadableConfigs({unittest_undeclared_ns: {a: 1, b: 2}}); + out.should.not.have.property("unittest_undeclared_ns"); + }); + it("does not modify the object it was given", function() { + var input = {unittest_readable: plugins.getConfig("unittest_readable")}; + plugins.filterReadableConfigs(input); + input.unittest_readable.should.have.property("not_needed", "internal"); + }); + it("is false for inherited Object.prototype keys", function() { + plugins.isReadableConfig("unittest_readable", "constructor").should.equal(false); + plugins.isReadableConfig("constructor", "needed").should.equal(false); + }); + it("tolerates non-object namespace values", function() { + var out = plugins.filterReadableConfigs({a: null, b: "str", c: [1]}); + Object.keys(out).length.should.equal(0); + }); + }); + + describe("the two mechanisms together", function() { + before(function() { + plugins.setConfigs("unittest_both", {shown: 1, credential: "REAL_VALUE"}); + plugins.setSecretConfigs("unittest_both", {credential: true}); + }); + + it("masks a credential even when it is wrongly declared readable", function() { + // the allow-list is the control and the mask is the backstop, so declaring a + // credential readable by mistake still does not hand out its value + plugins.setReadableConfigs("unittest_both", {shown: true, credential: true}); + var out = plugins.maskSecretConfigs(plugins.filterReadableConfigs({unittest_both: plugins.getConfig("unittest_both")})); + out.unittest_both.should.have.property("shown", 1); + out.unittest_both.credential.should.not.equal("REAL_VALUE"); + }); + }); + + // Which keys this codebase actually declares is deliberately not asserted here: a // unit test requiring pluginManager directly never loads api/api.js or the plugin // modules that declare them, so every such assertion would pass or fail for the // wrong reason. That belongs in plugins/plugins/tests.js, which runs against a From 957946b3c99aaf5339db37e546c786fa1d88ff72 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 13:01:45 +0300 Subject: [PATCH 3/4] test(configs): cover the happy paths, not just what is withheld The reduction is only correct if everything that legitimately reads configuration still works, so assert the values arrive with their real contents rather than only that credentials are gone: the app admin receives real values, of the right types, with no placeholder anywhere in the response, since App Management infers each input's widget from the type it sees every value the app admin can read is identical to what a global admin reads, because the app settings screen shows how an app differs from the server default and that is only meaningful against the real defaults the ratings widget colours arrive, which is the styling path a global admin can still write configuration, written back unchanged so the test leaves nothing altered an app admin still cannot write, which is the reason a mask can never be saved back over a real value Co-Authored-By: Claude --- plugins/plugins/tests.js | 112 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/plugins/plugins/tests.js b/plugins/plugins/tests.js index 674ccdab10a..464096bdbe0 100644 --- a/plugins/plugins/tests.js +++ b/plugins/plugins/tests.js @@ -183,6 +183,118 @@ describe('Testing configs read reduction', function() { }); }); + // Happy paths. The reduction is only correct if everything that legitimately reads + // configuration still works, so these assert the values actually arrive, with their + // real contents, rather than merely that secrets are gone. + + it('should give the app admin real values, not placeholders, for what it may read', function(done) { + request + .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + var ob = JSON.parse(res.text); + // types matter: App Management infers each input's widget from them, so a + // masked or missing value would render the wrong control + ob.api.event_limit.should.be.a.Number(); + ob.api.session_duration_limit.should.be.a.Number(); + ob.api.country_data.should.be.a.Boolean(); + ob.api.safe.should.be.a.Boolean(); + ob.security.password_min.should.be.a.Number(); + JSON.stringify(ob).should.not.match(/\*{8}/); + done(); + }); + }); + + it('should agree with the global admin on every value the app admin can read', function(done) { + // the app settings screen shows how an app differs from the server default, which + // is only meaningful if the defaults it sees are the real ones + request + .get('/o/configs?api_key=' + API_KEY_ADMIN + '&app_id=' + APP_ID) + .expect(200) + .end(function(err, adminRes) { + if (err) { + return done(err); + } + var full = JSON.parse(adminRes.text); + request + .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID) + .expect(200) + .end(function(err2, memberRes) { + if (err2) { + return done(err2); + } + var reduced = JSON.parse(memberRes.text); + Object.keys(reduced).forEach(function(ns) { + Object.keys(reduced[ns]).forEach(function(key) { + JSON.stringify(reduced[ns][key]) + .should.equal(JSON.stringify(full[ns][key])); + }); + }); + done(); + }); + }); + }); + + it('should give the app admin the ratings widget colours', function(done) { + request + .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + var ob = JSON.parse(res.text); + ob.should.have.property("feedback"); + ob.feedback.should.have.property("main_color"); + ob.feedback.should.have.property("font_color"); + ob.feedback.main_color.length.should.be.above(0); + done(); + }); + }); + + it('should let a global admin still write configuration', function(done) { + // masking needs no write guard only because writing stays global-admin only, so + // that has to keep working. Writes the value back unchanged, to avoid leaving the + // test environment altered. + request + .get('/o/configs?api_key=' + API_KEY_ADMIN + '&app_id=' + APP_ID) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + var current = JSON.parse(res.text).api.event_limit; + var payload = {api: {event_limit: current}}; + request + .get('/i/configs?api_key=' + API_KEY_ADMIN + '&app_id=' + APP_ID + '&configs=' + encodeURIComponent(JSON.stringify(payload))) + .expect(200) + .end(function(err2, res2) { + if (err2) { + return done(err2); + } + var after = JSON.parse(res2.text); + after.api.event_limit.should.equal(current); + done(); + }); + }); + }); + + it('should not let the app admin write configuration', function(done) { + // the reason a mask can never be saved back over a real value + request + .get('/i/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID + '&configs=' + encodeURIComponent(JSON.stringify({api: {event_limit: 1}}))) + .expect(401) + .end(function(err) { + if (err) { + return done(err); + } + done(); + }); + }); + after(function(done) { if (!memberUserId) { return done(); From aee9e0933838d1f25a5088dfa9818eabb2b61b84 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 13:57:54 +0300 Subject: [PATCH 4/4] test(configs): do not assume star-rating is enabled The ratings colours assertion required the feedback namespace to be present. getAllConfigs omits a namespace whose plugin is not enabled, so in a run without star-rating the namespace is absent for a global admin too and there is nothing to reduce. The test was asserting which plugins the run enabled rather than the behaviour under test, and failed on the countly-platform shard that excludes it. Now asserts the property that holds either way: if the namespace arrives, its declared keys are present with real values. Co-Authored-By: Claude --- plugins/plugins/tests.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/plugins/plugins/tests.js b/plugins/plugins/tests.js index 464096bdbe0..58f7abfc883 100644 --- a/plugins/plugins/tests.js +++ b/plugins/plugins/tests.js @@ -238,7 +238,11 @@ describe('Testing configs read reduction', function() { }); }); - it('should give the app admin the ratings widget colours', function(done) { + it('should give the app admin a plugin namespace it declared, when that plugin is on', function(done) { + // feedback belongs to star-rating. getAllConfigs omits a namespace whose plugin + // is not enabled, so in a test run without star-rating there is nothing to + // reduce and nothing to assert. Asserting the namespace exists would be + // asserting which plugins the run enabled, which is not what this covers. request .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID) .expect(200) @@ -247,10 +251,14 @@ describe('Testing configs read reduction', function() { return done(err); } var ob = JSON.parse(res.text); - ob.should.have.property("feedback"); + if (!ob.feedback) { + return done(); + } + // present, so the declared keys must have arrived with real values ob.feedback.should.have.property("main_color"); ob.feedback.should.have.property("font_color"); ob.feedback.main_color.length.should.be.above(0); + ob.feedback.main_color.should.not.equal(MASK); done(); }); });