fix(appraisal): reject RPC URL credentials (#257) - #346
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The RPC_URL invalid-URL error path can still echo credential-bearing input via JSON.stringify(value) when URL parsing fails, which risks leaking credentials in diagnostics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens Appraisal API configuration parsing to reject HTTP(S) RPC URLs that contain embedded credentials (username/password), returning a safe AppraisalConfigError that avoids leaking secrets.
Changes:
- Added credential detection in
parseRpcUrl()to reject URLs withurl.usernameorurl.password. - Ensured
AppraisalConfigErrorthrown during RPC URL parsing is rethrown (not wrapped) to avoid attaching a credential-bearing cause. - Added tests covering multiple credential forms (username-only, password-only, combined, encoded) plus valid HTTP(S) control URLs.
File summaries
| File | Description |
|---|---|
| services/appraisal-api/src/config.ts | Adds RPC URL credential rejection and preserves safe error behavior by rethrowing AppraisalConfigError. |
| services/appraisal-api/src/config.test.ts | Adds regression tests asserting embedded-credential URLs are rejected and that credential content is not echoed. |
Review details
Suppressed comments (1)
services/appraisal-api/src/config.ts:134
- The RPC_URL parse error path still echoes the raw env value via JSON.stringify(value). If someone supplies a malformed URL that includes credentials (e.g. "https://user:pass@"), the resulting AppraisalConfigError message will include those credentials, which can leak secrets in diagnostics. Consider redacting userinfo-like substrings before interpolating the value for non-credential-related parse failures.
throw new AppraisalConfigError(
"RPC_URL",
`must be a valid http(s) URL, got ${JSON.stringify(value)}`,
{ cause },
);
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+163
to
+181
| describe("RPC URL embedded credentials", () => { | ||
| for (const credentials of ["private-user@", ":private-password@", "private-user:private-password@", "private%2Duser:private%2Dpassword@"]) { | ||
| test(`rejects credential form ${credentials.indexOf(":") >= 0 ? "password" : "username"}`, () => { | ||
| assert.throws(() => configFromEnv({ ...MINIMAL_ENV, RPC_URL: `https://${credentials}rpc.example/` }), (error: unknown) => { | ||
| assert.ok(error instanceof AppraisalConfigError); | ||
| assert.equal(error.variable, "RPC_URL"); | ||
| assert.match(error.message, /embedded credentials/); | ||
| assert.doesNotMatch(error.stack ?? error.message, /private-user|private-password|private%2D/); | ||
| assert.equal(error.cause, undefined); | ||
| return true; | ||
| }); | ||
| }); | ||
| } | ||
| for (const RPC_URL of ["http://localhost:8000", "https://rpc.example/path"]) { | ||
| test(`accepts credential-free ${RPC_URL}`, () => { | ||
| assert.equal(configFromEnv({ ...MINIMAL_ENV, RPC_URL }).rpcUrl, RPC_URL); | ||
| }); | ||
| } | ||
| }); |
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.
Closes #257.
Rejects parsed username/password credentials using a safe AppraisalConfigError without attaching the URL or a credential-bearing cause. Covers username-only, password-only, combined and encoded credentials plus valid HTTP(S) controls.
Validation: Appraisal tests and typecheck pass on the combined verification branch.