Skip to content

[fix][core] do not keep the request's credentials in stored documents - #7919

Open
ar2rsawseen wants to merge 1 commit into
masterfrom
security/no-request-credentials-in-documents
Open

[fix][core] do not keep the request's credentials in stored documents#7919
ar2rsawseen wants to merge 1 commit into
masterfrom
security/no-request-credentials-in-documents

Conversation

@ar2rsawseen

Copy link
Copy Markdown
Member

Why

api_key and auth_token are accepted as request parameters (api/utils/rights.js). Any handler that keeps input it does not recognise therefore persists the caller's own credential into the document, and a read that returns the document hands that credential to everyone allowed to read it. That is exactly how a date preset came to disclose its owner's api key.

Four subsystems store a parsed client payload without declaring their own field set:

Handler Write paths Read
remote-config parameters and conditions 4 dashboard list, unprojected
alerts 1 /o/alert/list, unprojected
hooks 1 /o/hook/list, unprojected
ab-testing experiments 2 dashboard, unprojected

What this does, and what it does not

common.stripRequestCredentials(doc) removes api_key and auth_token from an object about to be written. It is called on the parsed payload at each of those write paths.

This is the floor, not the fix. Only storing declared fields is the fix, and it needs a per-plugin survey of every writer plus the fields the UI round-trips through storage, which is a larger change with real regression risk for each plugin. This change is deliberately the part that needs no knowledge of the payload's shape: a handler that cannot enumerate itself can still refuse to keep a credential.

Top level only, deliberately. That is where a copy of the request puts them. A value the caller nested inside their own payload is their own to disclose, and descending to arbitrary depth would mean guessing at shapes. The test pins that boundary.

It sits next to common.reqInfo, which already keeps request api_key out of the logs for the same reason.

Notes on the branches

  • release.24.05's hooks has two parse sites where master has one, the second being /i/hook/test, which runs a mock rather than storing. Stripped there as well, for consistency and at no cost.
  • In countly-enterprise-plugins the two lines are written out rather than calling common, because ab-testing resolves common from the server tree at runtime and this way the change does not depend on which repo merges first.

Tests

test/unit-tests/api.utils.common.request-credentials.js, 4 cases: both parameters removed, everything else untouched including a nested api_key, the same object returned so it can be used inline, and non-objects tolerated.

api_key and auth_token are accepted as request parameters, so any handler that
keeps input it does not recognise persists the caller's credential, and a document
read back later hands that credential to whoever may read it. That is what made a
date preset able to disclose its owner's api key.

common.stripRequestCredentials removes those two from an object about to be
written, and is called on the parsed payload in remote-config (four write paths),
alerts, hooks and ab-testing. None of those declares its own field set, and this
does not try to make them: it is the floor rather than the fix. A handler that
cannot enumerate its shape can still refuse to keep a credential.

Top level only, deliberately. That is where a copy of the request puts them, while
a value the caller nested inside their own payload is their own to disclose, and
descending to arbitrary depth would mean guessing at shapes.

Sits next to common.reqInfo, which already keeps request api_key out of the logs
for the same reason.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant