[fix][core] do not keep the request's credentials in stored documents (24.05) - #7920
Open
ar2rsawseen wants to merge 1 commit into
Open
[fix][core] do not keep the request's credentials in stored documents (24.05)#7920ar2rsawseen wants to merge 1 commit into
ar2rsawseen wants to merge 1 commit into
Conversation
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>
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.
Why
api_keyandauth_tokenare 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:
remote-configparameters and conditionsalerts/o/alert/list, unprojectedhooks/o/hook/list, unprojectedab-testingexperimentsWhat this does, and what it does not
common.stripRequestCredentials(doc)removesapi_keyandauth_tokenfrom 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 requestapi_keyout 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.common, because ab-testing resolvescommonfrom 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 nestedapi_key, the same object returned so it can be used inline, and non-objects tolerated.