feat(qa-lab): add Convex credential broker and admin CLI#65596
feat(qa-lab): add Convex credential broker and admin CLI#65596
Conversation
🔒 Aisle Security AnalysisWe found 8 potential security issue(s) in this PR:
1. 🟠 SSRF and bearer token exfiltration via unvalidated OPENCLAW_QA_CONVEX_SITE_URL in credential leasing client
DescriptionThe Convex credential leasing client builds request URLs from
Vulnerable code: const response = await params.fetchImpl(params.url, {
method: "POST",
headers: {
authorization: `Bearer ${params.authToken}`,
"content-type": "application/json",
},
body: JSON.stringify(params.body),
});RecommendationPrevent attacker-controlled outbound destinations for authenticated broker calls. Options:
Example (hostname allowlist): function assertAllowedConvexHost(url: URL) {
const host = url.hostname.toLowerCase();
const allowed = host.endsWith(".convex.site") || host.endsWith(".convex.cloud");
if (!allowed) throw new Error("OPENCLAW_QA_CONVEX_SITE_URL must point to a Convex domain");
}
function normalizeConvexSiteUrl(raw: string, env: NodeJS.ProcessEnv): string {
const url = new URL(raw);
if (url.protocol !== "https:") throw new Error("...");
assertAllowedConvexHost(url);
return url.toString().replace(/\/$/, "");
}Additionally, consider not sending the bearer token until after URL validation is complete (fail closed). 2. 🟠 SSRF and maintainer secret leakage via --site-url / OPENCLAW_QA_CONVEX_SITE_URL in qa-credentials admin client
DescriptionThe QA credentials admin client allows overriding the Convex broker base URL via
Vulnerable code: response = await params.fetchImpl(params.url, {
method: "POST",
headers: {
authorization: `Bearer ${params.authToken}`,
"content-type": "application/json",
},
body: JSON.stringify(params.body),
});RecommendationTreat the Convex broker base URL as a sensitive, non-user-controllable configuration.
Example allowlist enforcement: function assertAllowedBrokerHost(url: URL) {
const host = url.hostname.toLowerCase();
if (!(host.endsWith(".convex.site") || host.endsWith(".convex.cloud"))) {
throw new QaCredentialAdminError({
code: "INVALID_SITE_URL",
message: "--site-url must point to a Convex domain",
});
}
}
function normalizeConvexSiteUrl(raw: string, env: NodeJS.ProcessEnv): string {
const url = new URL(raw);
if (url.protocol !== "https:") throw ...;
assertAllowedBrokerHost(url);
return url.toString().replace(/\/$/, "");
}Also consider warning users that 3. 🟡 Bypassable loopback check allows insecure HTTP to non-loopback hosts
Description
Because Vulnerable code: function isLoopbackHostname(hostname: string) {
return hostname === "localhost" || hostname === "::1" || hostname.startsWith("127.");
}Impact:
RecommendationMake the loopback decision based on IP literals only, not arbitrary hostnames, and avoid prefix matching. Example fix: import { isIP } from "node:net";
function isLoopbackHost(url: URL) {
const host = url.hostname;
if (host === "localhost") return true;
const ipVersion = isIP(host);
if (ipVersion === 4) return host.startsWith("127.");
if (ipVersion === 6) return host === "::1";
return false; // any other hostname must use https
}Optionally, if you truly want to allow custom local dev hostnames, resolve DNS and verify the resolved address is loopback (with care for DNS rebinding). 4. 🟡 Information disclosure via reflected internal error messages in credential broker HTTP API
DescriptionThe Convex credential broker HTTP layer returns raw exception messages to the client for unexpected errors.
Vulnerable code: if (error instanceof Error) {
return {
httpStatus: 500,
payload: {
status: "error",
code: "INTERNAL_ERROR",
message: error.message || "Internal credential broker error.",
},
};
}RecommendationDo not reflect raw internal exception messages to clients. Return a generic error message for unhandled exceptions, and log the detailed error server-side (with care to avoid logging credential payloads). Example: function normalizeError(error: unknown) {
if (error instanceof BrokerHttpError) {
return {
httpStatus: error.httpStatus,
payload: { status: "error", code: error.code, message: error.message },
};
}
// Server-side logging only
console.error("Broker unhandled error", error);
return {
httpStatus: 500,
payload: {
status: "error",
code: "INTERNAL_ERROR",
message: "Internal credential broker error.",
},
};
}Optionally add a request correlation id and return that to the client for debugging without leaking details. 5. 🟡 QA credential CLI can output raw secret payloads to stdout/JSON
DescriptionThe QA credentials administration CLI includes an option to return/print credential This is risky because:
Vulnerable code: if (opts.json) {
process.stdout.write(
`${JSON.stringify({ status: "ok", action: "list", count: result.credentials.length, credentials: result.credentials }, null, 2)}\n`,
);
return;
}
...
if (opts.showSecrets && result.credentials.length > 0) {
process.stdout.write("\nPayloads:\n");
for (const credential of result.credentials) {
process.stdout.write(`${credential.credentialId}: ${JSON.stringify(credential.payload ?? null)}\n`);
}
}Although RecommendationReduce the chance of accidental secret exposure:
Example (redact payloads in JSON unless explicitly acknowledged): const allowSecrets = opts.showSecrets && opts.iKnowThisPrintsSecrets;
const credentials = allowSecrets
? result.credentials
: result.credentials.map(({ payload, ...rest }) => ({ ...rest, payload: undefined }));
if (opts.json) {
process.stdout.write(JSON.stringify({ status: "ok", action: "list", count: credentials.length, credentials }, null, 2) + "\n");
return;
}
if (allowSecrets) {
process.stderr.write("WARNING: printing credential payloads (secrets) to output\n");
// ...print payloads...
}6. 🟡 Lease role not bound in heartbeat/release allows cross-role lease manipulation with leaked lease token
DescriptionThe credential broker stores an Impact:
Vulnerable code: if (row.lease.ownerId !== args.ownerId || row.lease.leaseToken !== args.leaseToken) {
return brokerError("LEASE_NOT_OWNER", "Credential lease owner/token mismatch.");
}
// Missing: row.lease.actorRole === args.actorRoleAlthough the HTTP layer enforces RecommendationBind the lease to the role by verifying Example fix: if (row.lease.ownerId !== args.ownerId || row.lease.leaseToken !== args.leaseToken) {
return brokerError("LEASE_NOT_OWNER", "Credential lease owner/token mismatch.");
}
if (row.lease.actorRole !== args.actorRole) {
return brokerError("LEASE_ROLE_MISMATCH", "Credential lease role mismatch.");
}Additionally, consider:
7. 🟡 Bypassable loopback check allows insecure HTTP to non-loopback hosts (admin client)
Description
Hostnames that merely begin with Vulnerable code: function isLoopbackHostname(hostname: string) {
return hostname === "localhost" || hostname === "::1" || hostname.startsWith("127.");
}RecommendationRestrict loopback allowance to IP literals (and exact localhost), not arbitrary hostnames. Example: import { isIP } from "node:net";
function isLoopbackHost(url: URL) {
const host = url.hostname;
if (host === "localhost") return true;
const v = isIP(host);
if (v === 4) return host.startsWith("127.");
if (v === 6) return host === "::1";
return false;
}Then use 8. 🔵 Authentication error responses disclose broker deployment configuration and token role matches
DescriptionThe credential broker HTTP auth helpers return distinguishable HTTP status codes/messages for different authentication failure modes. This allows an unauthenticated remote caller to infer details about deployment configuration and (if a secret is already compromised) whether a presented token matches the CI secret vs. being completely invalid. Impacts:
Vulnerable code (examples): if (!maintainerSecret) {
throw new BrokerHttpError(
500,
"SERVER_MISCONFIGURED",
"Admin endpoints require OPENCLAW_QA_CONVEX_SECRET_MAINTAINER on this deployment.",
);
}
...
if (ciSecret && token === ciSecret) {
throw new BrokerHttpError(403, "AUTH_ROLE_MISMATCH", "Admin endpoints require maintainer credentials.");
}RecommendationReturn a uniform error shape/status for authentication/authorization failures to reduce information disclosure. Suggested approach:
Example hardening: function assertMaintainerAdminAuth(token: string | null) {
const maintainerSecret = process.env.OPENCLAW_QA_CONVEX_SECRET_MAINTAINER?.trim();
const ciSecret = process.env.OPENCLAW_QA_CONVEX_SECRET_CI?.trim();
// Log config problems internally, but do not reveal to caller
if (!maintainerSecret) {
console.error("Missing maintainer secret");
throw new BrokerHttpError(401, "AUTH_INVALID", "Invalid credentials.");
}
if (!token || token !== maintainerSecret) {
// do not distinguish CI secret vs invalid token
throw new BrokerHttpError(401, "AUTH_INVALID", "Invalid credentials.");
}
}Analyzed PR: #65596 at commit Last updated on: 2026-04-13T03:06:02Z |
openclaw-barnacle
bot
added
docs
Greptile SummaryAdds a Convex-backed credential lease broker for the QA Telegram lane, a standalone Convex scaffold ( Confidence Score: 5/5Safe to merge; all findings are P2 style suggestions with no blocking correctness or security issues. Lease lifecycle mutations are correctly serialized by Convex's OCC, preventing TOCTOU races. Role-scoped auth, idempotent release semantics, and leaseToken redaction from list responses are all correct. The three findings are dead code cleanup, a theoretical timing-attack surface on an internal tool, and a missing .gitignore entry — none affect correctness or security in practice. No files require special attention for correctness. qa/convex-credential-broker/convex/http.ts has the timing-safe comparison note worth a quick look before production hardening.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const brokerError = toBrokerError({ | ||
| payload, | ||
| fallback: `Convex credential acquire failed for kind "${opts.kind}".`, | ||
| }); | ||
| if (brokerError) { | ||
| throw brokerError; |
There was a problem hiding this comment.
postConvexBroker
postConvexBroker already parses the response body for { status: "error" } and throws a QaCredentialBrokerError before returning. If the call returns without throwing, the payload is guaranteed to not be a broker error response, so the second toBrokerError check here can never match and is dead code.
| const brokerError = toBrokerError({ | |
| payload, | |
| fallback: `Convex credential acquire failed for kind "${opts.kind}".`, | |
| }); | |
| if (brokerError) { | |
| throw brokerError; | |
| const acquired = convexAcquireSuccessSchema.parse(payload); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/qa-lab/src/live-transports/shared/credential-lease.runtime.ts
Line: 353-358
Comment:
**Redundant broker-error check after `postConvexBroker`**
`postConvexBroker` already parses the response body for `{ status: "error" }` and throws a `QaCredentialBrokerError` before returning. If the call returns without throwing, the payload is guaranteed to not be a broker error response, so the second `toBrokerError` check here can never match and is dead code.
```suggestion
const acquired = convexAcquireSuccessSchema.parse(payload);
```
How can I resolve this? If you propose a fix, please make it concise.| if (maintainerSecret && token === maintainerSecret) { | ||
| return "maintainer"; | ||
| } | ||
| if (ciSecret && token === ciSecret) { | ||
| return "ci"; | ||
| } | ||
| if (sharedSecret && token === sharedSecret) { | ||
| return "shared"; | ||
| } | ||
| throw new BrokerHttpError(401, "AUTH_INVALID", "Credential broker shared secret is invalid."); |
There was a problem hiding this comment.
All three token === secret comparisons use JavaScript's native string equality, which can leak secret length via timing. crypto.timingSafeEqual (available in the Convex V8 environment via the Web Crypto API) is the conventional defense. For an internal QA tool the risk is low, but the pattern is worth correcting before broader deployment.
where timingSafeEqual wraps crypto.subtle.timingSafeEqual on equal-length buffers (or returns false on length mismatch without leaking which side is shorter).
Prompt To Fix With AI
This is a comment left during a code review.
Path: qa/convex-credential-broker/convex/http.ts
Line: 62-71
Comment:
**Timing-unsafe bearer-token comparison**
All three `token === secret` comparisons use JavaScript's native string equality, which can leak secret length via timing. `crypto.timingSafeEqual` (available in the Convex V8 environment via the Web Crypto API) is the conventional defense. For an internal QA tool the risk is low, but the pattern is worth correcting before broader deployment.
where `timingSafeEqual` wraps `crypto.subtle.timingSafeEqual` on equal-length buffers (or returns `false` on length mismatch without leaking which side is shorter).
How can I resolve this? If you propose a fix, please make it concise.| .convex | ||
| convex/_generated | ||
|
|
||
| .env.local |
There was a problem hiding this comment.
node_modules not excluded in broker-local .gitignore
This package is a standalone Convex project (not a pnpm workspace member), so running npm install or pnpm install here will produce a local node_modules directory. The root .gitignore likely covers this globally, but being explicit in the broker's own .gitignore makes the standalone nature clearer and prevents accidental commits if the root glob ever changes.
| .convex | |
| convex/_generated | |
| .env.local | |
| .convex | |
| convex/_generated | |
| node_modules | |
| .env.local |
Prompt To Fix With AI
This is a comment left during a code review.
Path: qa/convex-credential-broker/.gitignore
Line: 1-4
Comment:
**`node_modules` not excluded in broker-local `.gitignore`**
This package is a standalone Convex project (not a pnpm workspace member), so running `npm install` or `pnpm install` here will produce a local `node_modules` directory. The root `.gitignore` likely covers this globally, but being explicit in the broker's own `.gitignore` makes the standalone nature clearer and prevents accidental commits if the root glob ever changes.
```suggestion
.convex
convex/_generated
node_modules
.env.local
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4df3e25c90
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const acquired = convexAcquireSuccessSchema.parse(payload); | ||
| const parsedPayload = opts.parsePayload(acquired.payload); | ||
| const leaseTtlMs = acquired.leaseTtlMs ?? config.leaseTtlMs; |
There was a problem hiding this comment.
After a successful acquire response, the code parses acquired.payload before returning the lease object; if parsePayload throws (for example, a malformed Telegram credential row), execution jumps to the catch path and never calls the broker release endpoint. That leaves the credential locked until TTL expiry, and repeated attempts can exhaust the pool even though no run actually started. Add a best-effort release when validation fails after credentialId/leaseToken are available.
Useful? React with 👍 / 👎.
joshavant
force-pushed
the
feat/qa-lab-convex-credentials
branch
from
4df3e25 to
0edf663
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0edf663cad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!trimmed) { | ||
| return DEFAULT_ENDPOINT_PREFIX; | ||
| } | ||
| const prefixed = trimmed.startsWith("/") ? trimmed : `/${trimmed}`; |
There was a problem hiding this comment.
// endpoint prefixes before URL join
The endpoint-prefix normalization currently accepts values like //attacker.example, and joinConvexEndpoint later passes that into new URL(...); scheme-relative paths override the Convex host. In that case, acquire/heartbeat/release calls are redirected off-deployment while still sending the Authorization: Bearer ... secret, so a typo or poisoned env var can exfiltrate broker credentials. Reject endpoint prefixes that start with // (and ideally any absolute URL form) before building request URLs.
Useful? React with 👍 / 👎.
|
|
||
| function normalizeCredentialId(raw: string) { | ||
| // Convex Ids are opaque strings. We only enforce non-empty shape at HTTP boundary. | ||
| return raw; |
There was a problem hiding this comment.
This helper returns any non-empty string as a credential id, then handlers cast it to Id<"credential_sets"> for runMutation. Malformed IDs (for example a typo from CLI) can fail Convex arg validation and fall into the generic catch path, which is returned as 500 INTERNAL_ERROR instead of a client 400 INVALID_BODY. That makes user input mistakes look like server failures on heartbeat/release/admin remove; add explicit shape validation here and throw BrokerHttpError(400, ...) for invalid ids.
Useful? React with 👍 / 👎.
|
Found 1 test failure on Blacksmith runners: Failure
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95ecbcdf80
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| function isLoopbackHostname(hostname: string) { | ||
| return hostname === "localhost" || hostname === "::1" || hostname.startsWith("127."); |
There was a problem hiding this comment.
With OPENCLAW_QA_ALLOW_INSECURE_HTTP=1, this check accepts any hostname that starts with 127., including public DNS names like 127.attacker.example. That breaks the intended “loopback-only” guard and can send Convex broker Bearer secrets to a non-local host over plaintext HTTP if OPENCLAW_QA_CONVEX_SITE_URL is mistyped or poisoned. The check should validate true loopback literals (localhost, 127.0.0.0/8 IPs, and IPv6 loopback) instead of a string prefix.
Useful? React with 👍 / 👎.
joshavant
force-pushed
the
feat/qa-lab-convex-credentials
branch
from
95ecbcd to
55f8295
Compare
|
Landed via temp rebase onto main. Thanks @joshavant! |
|
Follow-up gate note: the requested full gate command was executed, but two lanes currently fail on latest main outside this PR scope:
/a2ui.bundle.js chunk │ size: 503.60 kB ✔ rolldown v1.0.0-rc.12 Finished in 39.04 ms
OK: All 4 required plugin-sdk exports verified. |
|
Correction (safe quoting): The requested full gate command was executed, but two lanes currently fail on latest
|
There was a problem hiding this comment.
💡 Codex Review
Ensure lease release runs for preflight failures
The lease is acquired before preflight validation (normalizeQaProviderMode, findScenario) but the try/finally that stops heartbeat and releases the lease starts later, so any error in this setup window exits without releasing the Convex lease. A simple bad scenario id (unknown Telegram QA scenario) can therefore leave credentials locked until TTL expiry and gradually exhaust the shared pool.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| function isLoopbackHostname(hostname: string) { | ||
| return hostname === "localhost" || hostname === "::1" || hostname.startsWith("127."); |
There was a problem hiding this comment.
The loopback check treats any hostname beginning with 127. as local, so values like http://127.attacker.example pass when OPENCLAW_QA_ALLOW_INSECURE_HTTP=1. In that configuration, admin commands can POST the maintainer bearer secret to an external host over plaintext HTTP if the site URL is mistyped or poisoned. Restrict this to true loopback literals (localhost, IPv4 127/8 addresses, and IPv6 loopback literal).
Useful? React with 👍 / 👎.
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write
N/A. If the cause is unclear, writeUnknown.Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should catch this. Otherwise write
N/A.extensions/qa-lab/src/cli.test.ts,extensions/qa-lab/src/qa-credentials-admin.runtime.test.ts,extensions/qa-lab/src/live-transports/shared/credential-lease.runtime.test.ts,extensions/qa-lab/src/live-transports/telegram/telegram-live.runtime.test.ts,extensions/qa-lab/src/live-transports/matrix/cli.runtime.test.tsUser-visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write
None.openclaw qa credentialscommand group:add --kind <kind> --payload-file <path> [--note] [--json]list [--kind] [--status] [--limit] [--show-secrets] [--json]remove --credential-id <id> [--json]openclaw qa telegramnow supports pooled credentials via--credential-source convex.--credential-role maintainer|ci.Diagram (if applicable)
For UI changes or non-trivial logic flows, include a small ASCII diagram reviewers can scan quickly. Otherwise write
N/A.Security Impact (required)
Yes/No) YesYes/No) YesYes/No) YesYes/No) YesYes/No) YesYes, explain risk + mitigation:maintainervsci), maintainer-only admin endpoints, soft-delete guard for active leases, lease TTL + heartbeat expiry, minimal event logging with retention cleanup, and default redaction on list responses unless explicitly requested.Repro + Verification
Environment
Steps
pnpm test extensions/qa-lab/src/cli.test.ts extensions/qa-lab/src/qa-credentials-admin.runtime.test.ts extensions/qa-lab/src/live-transports/shared/credential-lease.runtime.test.ts extensions/qa-lab/src/live-transports/telegram/telegram-live.runtime.test.ts extensions/qa-lab/src/live-transports/matrix/cli.runtime.test.tsExpected
Actual
acquire/heartbeat/releaseandadmin add/remove/listsurfaces.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm check, fullpnpm test, and broad multi-channel/manual E2E rerun in this pass.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) YesYes/No) NoRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.