security(configs): return only declared configuration to non global admins, and mask credentials - #7870
Open
ar2rsawseen wants to merge 4 commits into
Open
security(configs): return only declared configuration to non global admins, and mask credentials#7870ar2rsawseen wants to merge 4 commits into
ar2rsawseen wants to merge 4 commits into
Conversation
…n 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 <noreply@anthropic.com>
…dmins
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 <noreply@anthropic.com>
ar2rsawseen
changed the title
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Configuration was returned wholesale to anyone who could read it at all, and some of it holds credentials rather than settings: an outbound proxy password, a reCAPTCHA secret, the key that signs report subscribe/unsubscribe tokens.
Two mechanisms, applied in this order.
1. An allow-list, which is the control
plugins.setReadableConfigs(namespace, {key: true})declares what a caller who is not a global admin may read. Everything else is withheld, and a namespace with nothing declared is dropped entirely.The direction matters. Marking credentials as secret and returning the rest is a deny-list: every new setting is exposed until somebody remembers to annotate it, so one forgotten annotation is a leaked credential with no signal. Declaring what is needed fails the other way, and the cost of forgetting is a missing input in the UI, which someone notices and files.
Declared, from surveying every reader across countly-server and countly-enterprise-plugins:
apishowInAppManagment, plusdomainsecurityfeedbackmain_color,font_color,feedback_logoconcurrent_usersalert_interval2. Marking credentials, which is the backstop
plugins.setSecretConfigs(namespace, {key: true})masks a value as********. It still earns its place with the allow-list in front of it:/o/configsMarked:
security.proxy_username,security.proxy_password,push.proxyuser,push.proxypass,reports.secretKey,recaptcha.secret_key.Values the browser needs are deliberately not marked
recaptcha.site_keyis public by design, andtracking.self_tracking_app_keyis what the dashboard uses to send its own telemetry. A value the browser needs cannot be secret: hiding it from an API response while it stays in page source buys nothing. If a real credential is ever needed browser-side, the fix is to proxy the call server-side.Before dropping anything from the page I enumerated every
countlyGlobal.security.*andcountlyGlobal.tracking.*reference in all three repositories. The dashboard uses exactly five keys fromsecurity, all password policy.Three things that could have gone wrong
getConfig()is untouched. Server code keeps reading real values; masking there would have push and reports authenticating with the literal string********.getConfig()returns a fresh object per namespace and neither filter writes into it, so a read cannot alter live configuration. Asserted by test.Writes need no guard
/i/configsisvalidateGlobalAdmin, so the only callers who can write configuration are the only callers who see real values, and a mask cannot be written back over a value. Worth remembering if that gate is ever loosened.The failure mode this is built around
The dashboard degrades silently when configuration is missing: the call site uses
.always(), the model initialises to{}, and the input loop is guarded byif (pluginsData[key]). So an undeclared value shows up as a setting quietly absent from a screen, with no error anywhere.Two things address that:
showInAppManagmentwithout declaring it readable fails CI instead of removing a setting from the screenTests
test/unit-tests/plugins.pluginManager.secret-configs.js, 23 cases: both filters, non-mutation, unknown namespaces, prototype keys,getConfig()still returning real values, a newly added credential nobody marked being withheld, and a credential wrongly declared readable still being masked.plugins/plugins/tests.js, 5 cases through the real endpoints with a real non-global app admin.Full unit suite: 208 passing. The 2 failures are
Countly Request, which makes real HTTP calls and fails without network on a clean checkout too.Enterprise follow-up
Enterprise plugins need declarations for namespaces they own and read:
crashes.custom_domain,journey_engine.approver_enabled, and whateverdata-manageruses. Until then those fall back to their existing defaults, which forjourney_engineis a value already injected into the page, and for symbolication is the browser origin.Separate issue, not fixed here
/i/configsrecords the full configuration as thebeforepayload of its systemlogs entry, persisting these values indefinitely. Global-admin only to read, so not a boundary crossing, but worth masking there too.🤖 Generated with Claude Code