security(configs): mark credential configs secret and withhold them on read (24.05) - #7871
Open
ar2rsawseen wants to merge 4 commits into
Open
security(configs): mark credential configs secret and withhold them on read (24.05)#7871ar2rsawseen 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>
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.
Backport of #7870 to
release.24.05. Clean cherry-pick, no adaptation needed.Adds
plugins.setSecretConfigs(namespace, {key: true})so configuration that holds a credential is declared where it is defined. Declared values are masked in/o/configsfor anyone who is not a global admin, and left out of the namespaces serialized into the dashboard page.Marked:
security.proxy_username,security.proxy_password,push.proxyuser,push.proxypass,reports.secretKey,recaptcha.secret_key.Not marked, because the browser needs them:
recaptcha.site_keyandtracking.self_tracking_app_key.getConfig()is untouched, so server code keeps reading real values. Full reasoning in #7870.Verified on this branch
The call sites differ in line numbers but not in shape, so I checked each landed where intended rather than trusting a clean apply: the mask in
/o/configs, the three narrowed namespaces infrontend/express/app.js, and all four declarations.19 tests pass here: 15 unit plus 4 API.
🤖 Generated with Claude Code