fix: restrict insecure Stellar RPC URLs#262
Open
newmattock wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR hardens HTTP security across Stellar RPC and Horizon client initialization by restricting plain-HTTP to loopback addresses only and disabling it in production. A new security utility validates URLs, enforces loopback-only allowance, and throws on non-compliant URLs. The SDK and web app are updated to apply this policy during RPC/Horizon server construction, configuration types are extended with optional ChangesHTTP RPC Security Policy: Loopback-Only with Opt-In
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/sdk/src/__tests__/ContractDeployer.test.ts (1)
166-184: ⚡ Quick winAdd assertions for error code and message to strengthen test validation.
The tests correctly verify that
DeployerErroris thrown, but don't check the error code or message. According to the PR context, the error code should beUNSAFE_HTTP_RPC_URL. Adding specific assertions would better validate the contract and improve regression protection.🧪 Proposed enhancement for error assertions
it('rejects non-loopback HTTP RPC URLs by default', () => { - expect( - () => - new ContractDeployer({ - rpcUrl: 'http://example.com', - networkPassphrase: 'Test SDF Network ; September 2015', - }) - ).toThrow(DeployerError); + try { + new ContractDeployer({ + rpcUrl: 'http://example.com', + networkPassphrase: 'Test SDF Network ; September 2015', + }); + expect.fail('Expected DeployerError to be thrown'); + } catch (err) { + expect(err).toBeInstanceOf(DeployerError); + expect((err as DeployerError).code).toBe('UNSAFE_HTTP_RPC_URL'); + expect((err as DeployerError).message).toContain('http://example.com'); + } }); it('requires explicit opt-in for local HTTP RPC URLs', () => { - expect( - () => - new ContractDeployer({ - rpcUrl: 'http://localhost:8000', - networkPassphrase: 'Test SDF Network ; September 2015', - }) - ).toThrow(DeployerError); + try { + new ContractDeployer({ + rpcUrl: 'http://localhost:8000', + networkPassphrase: 'Test SDF Network ; September 2015', + }); + expect.fail('Expected DeployerError to be thrown'); + } catch (err) { + expect(err).toBeInstanceOf(DeployerError); + expect((err as DeployerError).code).toBe('UNSAFE_HTTP_RPC_URL'); + expect((err as DeployerError).message).toContain('allowHttp: true'); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/__tests__/ContractDeployer.test.ts` around lines 166 - 184, The tests currently only assert that a DeployerError is thrown; update both test cases in ContractDeployer.test.ts to capture the thrown error and assert its code equals 'UNSAFE_HTTP_RPC_URL' and that the error message contains a clear substring (e.g., 'HTTP RPC URL is unsafe' or the specific message used by ContractDeployer) to ensure precise validation; locate the throws around new ContractDeployer({ rpcUrl: 'http://example.com', ... }) and new ContractDeployer({ rpcUrl: 'http://localhost:8000', ... }), catch the error, assert instance of DeployerError, then assert error.code === 'UNSAFE_HTTP_RPC_URL' and assert the message includes the expected text.apps/web/src/services/stellar.service.ts (1)
65-78: 💤 Low valueConsider extracting
allowLocalHttpto a shared utility.This helper is duplicated in
contract.deployer.ts. For maintainability, consider extracting it to a shared location (e.g.,@/utils/http-security.ts) so both services use the same implementation.That said, the duplication is minimal and self-contained, so this is not urgent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/services/stellar.service.ts` around lines 65 - 78, Extract the duplicated allowLocalHttp logic (LOOPBACK_HOSTS and function allowLocalHttp) into a single shared utility module (e.g., create a new exported function in a utils file such as http-security) and replace the local definitions in both stellar.service.ts and contract.deployer.ts with imports from that module; keep the implementation identical (same host set, NODE_ENV production check, URL parsing and protocol/hostname checks, and try/catch behavior) so callers of allowLocalHttp behave unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/web/src/services/stellar.service.ts`:
- Around line 65-78: Extract the duplicated allowLocalHttp logic (LOOPBACK_HOSTS
and function allowLocalHttp) into a single shared utility module (e.g., create a
new exported function in a utils file such as http-security) and replace the
local definitions in both stellar.service.ts and contract.deployer.ts with
imports from that module; keep the implementation identical (same host set,
NODE_ENV production check, URL parsing and protocol/hostname checks, and
try/catch behavior) so callers of allowLocalHttp behave unchanged.
In `@packages/sdk/src/__tests__/ContractDeployer.test.ts`:
- Around line 166-184: The tests currently only assert that a DeployerError is
thrown; update both test cases in ContractDeployer.test.ts to capture the thrown
error and assert its code equals 'UNSAFE_HTTP_RPC_URL' and that the error
message contains a clear substring (e.g., 'HTTP RPC URL is unsafe' or the
specific message used by ContractDeployer) to ensure precise validation; locate
the throws around new ContractDeployer({ rpcUrl: 'http://example.com', ... })
and new ContractDeployer({ rpcUrl: 'http://localhost:8000', ... }), catch the
error, assert instance of DeployerError, then assert error.code ===
'UNSAFE_HTTP_RPC_URL' and assert the message includes the expected text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 140446d6-4d2f-4bb6-9188-d4a9c3722599
📒 Files selected for processing (9)
apps/web/src/hooks/use-distribution-transaction.tsapps/web/src/services/contract.deployer.tsapps/web/src/services/stellar.service.tspackages/sdk/DEVELOPMENT_GUIDE.mdpackages/sdk/src/__tests__/ContractDeployer.test.tspackages/sdk/src/deployer/ContractDeployer.tspackages/sdk/src/deployer/types.tspackages/sdk/src/utils/GasEstimator.tspackages/sdk/src/utils/httpSecurity.ts
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
1 participant
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:
allowHttp: truefrom SDK and web production code paths.Related bounty or issue:
allowHttp: truepermits unencrypted RPC/Horizon connections in production #255What changed:
allowHttp?: booleantoDeployerConfigandGasEstimatorOptions.allowHttpoptions behind non-production loopback URLs.allowHttpoptions where the URL is always HTTPS.Validation:
pnpm --filter @fundable/sdk exec vitest --run src/__tests__/ContractDeployer.test.ts -t "RPC URL safety"passed.pnpm --filter @fundable/sdk exec vitest --run src/__tests__/GasEstimator.test.tspassed.Notes:
pnpm --filter @fundable/sdk exec vitest --run src/__tests__/ContractDeployer.test.tsstill has 3 pre-existing deploy-contract test failures:(intermediate value).toScAddress is not a function.pnpm --filter @fundable/sdk buildstill fails on existing TypeScript/project issues such as extensionless imports under NodeNext, old Stellar SDK names, and missing test helpers.pnpm --filter @fundable/web exec tsc --noEmit --pretty falsestill fails on existing syntax errors insrc/app/(overview)/distribution/page.tsxandsrc/components/modules/payment-stream/CreatePaymentStream.tsx.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests