fix: restrict interactive and metadata URLs to HTTP(S) and require in…#336
Conversation
…teractiveDomain for SEP-24 flows
There was a problem hiding this comment.
Code Review
This pull request ensures that server.interactiveDomain is configured for interactive deposit flows, returning a server_misconfigured error if it is absent. It also refactors URL construction to use this domain and updates ValidationUtils.isValidUrl to only accept http: and https: protocols. The review feedback suggests normalizing interactiveDomain by stripping trailing slashes to prevent double slashes in the generated interactive_url and more_info_url endpoints.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const serverConfig = this.config.get('server'); | ||
| if (!serverConfig.interactiveDomain) { | ||
| sendJson(res, 500, { | ||
| error: 'server_misconfigured', | ||
| message: 'server.interactiveDomain must be configured for interactive flows', | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If server.interactiveDomain is configured with a trailing slash (e.g., https://example.com/), appending /deposit/... directly will result in double slashes (e.g., https://example.com//deposit/...).
It is safer to strip any trailing slashes from interactiveDomain before constructing the URLs. We can define a normalized interactiveDomain variable here and reuse it.
| const serverConfig = this.config.get('server'); | |
| if (!serverConfig.interactiveDomain) { | |
| sendJson(res, 500, { | |
| error: 'server_misconfigured', | |
| message: 'server.interactiveDomain must be configured for interactive flows', | |
| }); | |
| return; | |
| } | |
| const serverConfig = this.config.get('server'); | |
| if (!serverConfig.interactiveDomain) { | |
| sendJson(res, 500, { | |
| error: 'server_misconfigured', | |
| message: 'server.interactiveDomain must be configured for interactive flows', | |
| }); | |
| return; | |
| } | |
| const interactiveDomain = serverConfig.interactiveDomain.replace(/\/+$/, ''); |
| asset_issuer: selectedAsset.issuer, | ||
| account: created.account, | ||
| interactive_url: `${this.config.get('server').interactiveDomain ?? 'http://localhost:3000'}/deposit/${created.id}`, | ||
| interactive_url: `${serverConfig.interactiveDomain}/deposit/${created.id}`, |
| asset_issuer: selectedAsset.issuer, | ||
| account: created.account, | ||
| interactive_url: `${this.config.get('server').interactiveDomain ?? 'http://localhost:3000'}/deposit/${created.id}`, | ||
| interactive_url: `${serverConfig.interactiveDomain}/deposit/${created.id}`, |
| if (serverConfig.interactiveDomain) { | ||
| responseData.interactive_url = `${serverConfig.interactiveDomain}/deposit/${transaction.id}`; | ||
| responseData.more_info_url = `${serverConfig.interactiveDomain}/deposit/${transaction.id}`; | ||
| } |
There was a problem hiding this comment.
Normalize interactiveDomain by stripping trailing slashes here as well to prevent double slashes in the generated interactive_url and more_info_url.
| if (serverConfig.interactiveDomain) { | |
| responseData.interactive_url = `${serverConfig.interactiveDomain}/deposit/${transaction.id}`; | |
| responseData.more_info_url = `${serverConfig.interactiveDomain}/deposit/${transaction.id}`; | |
| } | |
| if (serverConfig.interactiveDomain) { | |
| const interactiveDomain = serverConfig.interactiveDomain.replace(/\/+$/, ''); | |
| responseData.interactive_url = `${interactiveDomain}/deposit/${transaction.id}`; | |
| responseData.more_info_url = `${interactiveDomain}/deposit/${transaction.id}`; | |
| } |
Summary
This PR fixes SEP-24 interactive and metadata URL handling by restricting server.interactiveDomain and metadata.tomlUrl to only http: and https: schemes, and by requiring server.interactiveDomain before emitting SEP-24 interactive URLs.
Please provide a brief description of the changes in this PR.
Checklist
bun run testandbun run lintlocally.Issue Reference
Closes #204