Fix WhatsApp accountId path traversal in login-status/login - #119
Open
jay79-boop wants to merge 1 commit into
Open
Fix WhatsApp accountId path traversal in login-status/login#119jay79-boop wants to merge 1 commit into
jay79-boop wants to merge 1 commit into
Conversation
resolveWhatsAppCredentialCandidatePaths() joins accountId straight into
a credentials file path (OPENCLAW_DIR/credentials/whatsapp/<accountId>/
creds.json). normalizeChannelAccountId() -- the only normalization
applied to it -- just trims and defaults to "default"; it never rejects
`../`. Three call sites feed it accountId values with no validation:
- GET /api/channels/accounts/login-status (routes/agents.js): accountId
comes straight from req.query.accountId
- POST /api/channels/accounts/login: same, from req.body.accountId
- readPairedCountsByAccount (gateway.js, channel status reporting):
accountId comes from the *keys* of the live openclaw.json config's
channels.whatsapp.accounts object -- which, for an imported
workspace, is promoted directly from whatever the imported repo's
config contained
hasSavedWhatsAppCredentials() reads whatever file resolves and returns
whether it exists and is non-empty -- so accountId=../../../../etc/shadow
turns the (authenticated) login-status endpoint into a boolean
exists-and-non-empty oracle for arbitrary host files, reaching well
outside the OPENCLAW_DIR sandbox the Browse routes otherwise enforce
carefully for this same admin.
This codebase already has the right validator for exactly this field --
isValidChannelAccountId() (kChannelAccountIdPattern, lowercase/digits/
hyphens only) -- and createChannelAccount() already uses it. The two
sibling functions, runChannelAccountLogin() and
getChannelAccountLoginStatus(), just never got the same check.
Fixed at both layers:
- agents/channels.js: runChannelAccountLogin() and
getChannelAccountLoginStatus() now validate accountId with
isValidChannelAccountId() before doing anything else, matching
createChannelAccount()'s existing behavior.
- agents/shared.js: resolveWhatsAppCredentialCandidatePaths() also
filters its own candidate paths to stay within the credentials
directory, closing the config-derived path (readPairedCountsByAccount)
that doesn't go through the two HTTP-facing validators above.
Added tests: agents-service.test.js covers both HTTP-facing rejections
(with an assertion that readFileSync is never reached), and a new
agents-shared.test.js exercises the containment check directly with a
real external file, verified locally to fail without the fix (the
external file was reported as "linked") and pass with it.
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.
Summary
resolveWhatsAppCredentialCandidatePaths()(lib/server/agents/shared.js) joinsaccountIdstraight into a credentials file path:OPENCLAW_DIR/credentials/whatsapp/<accountId>/creds.json.normalizeChannelAccountId()— the only normalization applied to it — just trims and defaults to"default"; it never rejects../.Three call sites feed it
accountIdvalues with no validation:GET /api/channels/accounts/login-status(routes/agents.js):accountIdcomes straight fromreq.query.accountIdPOST /api/channels/accounts/login: same, fromreq.body.accountIdreadPairedCountsByAccount(gateway.js, channel status reporting):accountIdcomes from the keys of the liveopenclaw.jsonconfig'schannels.whatsapp.accountsobject — which, for an imported workspace, is promoted directly from whatever the imported repo's config containedhasSavedWhatsAppCredentials()reads whatever file resolves and returns whether it exists and is non-empty — soaccountId=../../../../etc/shadowturns the (authenticated) login-status endpoint into a boolean exists-and-non-empty oracle for arbitrary host files, reaching well outside theOPENCLAW_DIRsandbox the Browse routes otherwise enforce carefully for this same admin.This codebase already has the right validator for exactly this field —
isValidChannelAccountId()(kChannelAccountIdPattern, lowercase/digits/hyphens only) — andcreateChannelAccount()already uses it. The two sibling functions,runChannelAccountLogin()andgetChannelAccountLoginStatus(), just never got the same check.Fix
Fixed at both layers:
agents/channels.js:runChannelAccountLogin()andgetChannelAccountLoginStatus()now validateaccountIdwithisValidChannelAccountId()before doing anything else, matchingcreateChannelAccount()'s existing behavior.agents/shared.js:resolveWhatsAppCredentialCandidatePaths()also filters its own candidate paths to stay within the credentials directory, closing the config-derived path (readPairedCountsByAccount) that doesn't go through the two HTTP-facing validators above.Test plan
agents-service.test.js: covers both HTTP-facing rejections, with an assertion thatreadFileSyncis never reached for the rejectedaccountId.agents-shared.test.js: exercises the containment check directly with a real external file. Verified locally to fail without the fix (the external file was reported aslinked: true) and pass with it.agents-service/agents-shared/routes-agents/agents-service-coveragesuites pass (pre-existing unrelated failures on this environment are Windows-path-separator artifacts in the mock-fs test harness, not caused by this change).