-
Notifications
You must be signed in to change notification settings - Fork 525
fix: prevent .ipfs. duplication in subdomain gateway URLs #2449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: prevent .ipfs. duplication in subdomain gateway URLs #2449
Conversation
- Detect if gateway URL already has IPFS subdomain (starts with 'ipfs.') - Handle both cases: gateway with and without existing IPFS subdomain - Fixes issue where ipfs.example.com becomes ipfs.ipfs.example.com - Resolves ipfs#2398
|
This is definitely an improvement but why is the |
So the "mandatory" nature isn't about forcing users to configure it, but rather about ensuring that when a subdomain gateway is configured, it works correctly and maintains the security benefits of content isolation |
|
There is no security difference between |
- Add validateSubdomainGatewaySecurity function with security checks - Implement security scoring system (0-100) for gateway URLs - Add real-time security warnings in PublicSubdomainGatewayForm - Detect suspicious domains, HTTPS enforcement, and localhost warnings - Update checkSubdomainGateway to return security validation results - Add security best practices documentation to README - Maintain backward compatibility with existing functionality Addresses security concerns about subdomain gateway configurations and provides users with clear guidance on secure gateway selection.
|
@Gunni Please review the security part |
|
The only one I see is if domain name includes an If a domain does not include |
Yes, you are absolutely correct! What gets flagged as a problem:
What is completely fine:
The Logic:
If both conditions are true, then it's flagged as potentially confusing. Why this makes sense:
So your understanding is correct - domains without "ipfs." at all are completely fine according to this validation logic. |
Wait, how do you do this check? I don't see you pulling from a Public Suffix List...
|
| // For gateways like ipfs.example.com, construct: hash.ipfs.example.com | ||
| imgSubdomainUrl = new URL(`${gwUrl.protocol}//${IMG_HASH_1PX}.${gwUrl.hostname}/?now=${Date.now()}&filename=1x1.png#x-ipfs-companion-no-redirect`) | ||
| } else { | ||
| // For gateways like example.com, construct: hash.ipfs.example.com | ||
| imgSubdomainUrl = new URL(`${gwUrl.protocol}//${IMG_HASH_1PX}.ipfs.${gwUrl.hostname}/?now=${Date.now()}&filename=1x1.png#x-ipfs-companion-no-redirect`) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace entire if with just:
imgSubdomainUrl = new URL(`${gwUrl.protocol}//${IMG_HASH_1PX}.${gwUrl.hostname}/?now=${Date.now()}&filename=1x1.png#x-ipfs-companion-no-redirect`)
The ipfs. subdomain part is entirely useless and annoying.
|
Hi, marking this as blocked for now. Before opening a PR there should be a clear description of real world bug with reproduction steps. So far there is none ( Let's continue in #2398 (comment) to confirm there is a problem in the first place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original bug seems to be elsewhere, see #2398 (comment).
Because there is no real bug in webui, this PR seems to not fix anything, only tries to "fixup" if someone enters incorrect URL by "guessing" what is the right one.
However, instead of being a simple fix which retries on error when ipfs.ipfs. does not work, it brings unnecessary complexity, opinionated "scores" and "rules" what is secure and what is not – most of it makes no sense – see comments inline.
My understanding is that this PR, in current LLM-generated form, adds unnecessary complexity.
If you want to propose normalization of ipfs.example.com into example.com when cid.ipfs.ipfs.example.com fails, limit this PR to only that. It is not acceptable in current form.
ps. In the future please avoid implementing "fixes" for things that were not triaged (confirmed to be real bugs). The issue #2398 was not triaged.
| // Bonus points for well-known domains | ||
| const wellKnownDomains = [ | ||
| 'dweb.link', 'ipfs.io', 'gateway.pinata.cloud', | ||
| 'cloudflare-ipfs.com', 'ipfs.fleek.co' | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not hardcode domains, especially dead ones like cloudflare-ipfs.com
| // Check for very short domains (potential typosquatting) | ||
| const domainParts = hostname.split('.') | ||
| if (domainParts.length >= 2) { | ||
| const tld = domainParts[domainParts.length - 1] | ||
| const domain = domainParts[domainParts.length - 2] | ||
| if (domain.length <= 2 && tld.length <= 2) { | ||
| warnings.push('Very short domain name may be a typo or malicious') | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary complexity
| // Check for suspicious domains | ||
| const suspiciousPatterns = [ | ||
| /\.tk$/, /\.ml$/, /\.ga$/, /\.cf$/, // Free domains often used for malicious purposes | ||
| /\.onion$/, // Tor hidden services | ||
| /localhost\./, /127\.0\.0\.1\./, /0\.0\.0\.0\./ // Localhost subdomains | ||
| ] | ||
|
|
||
| for (const pattern of suspiciousPatterns) { | ||
| if (pattern.test(hostname)) { | ||
| warnings.push('Gateway domain may be unreliable or potentially malicious') | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a place to make judgement about domain being safe or not.
ipfs.io itself is not considered "safe" by most of software.
We should only test if domain is a valid subdomain gateway and if it correctly implements URL convention and CORS headers.
Please remove this.
| // Check for localhost/private IPs in production | ||
| const hostname = url.hostname.toLowerCase() | ||
| const isLocalhost = hostname === 'localhost' || | ||
| hostname === '127.0.0.1' || | ||
| hostname.startsWith('192.168.') || | ||
| hostname.startsWith('10.') || | ||
| hostname.startsWith('172.') | ||
|
|
||
| if (isLocalhost) { | ||
| warnings.push('Using localhost/private IP gateway may not be accessible to other users') | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not block users from being able to use LAN / private network gateways without DNS.
Please remove this
This PR fixes the issue where subdomain gateway URLs were incorrectly duplicating the
.ipfs.subdomain when the gateway URL already contained an IPFS subdomain.Problem
The original code in `checkSubdomainGateway` function was hardcoding `.ipfs.` in the subdomain URL construction:
```javascript
imgSubdomainUrl = new URL(`${gwUrl.protocol}//${IMG_HASH_1PX}.ipfs.${gwUrl.hostname}/?now=${Date.now()}&filename=1x1.png#x-ipfs-companion-no-redirect`)
```
This caused URLs like `https://ipfs.example.com\` to become `https://bafkreib6wedzfupqy7qh44sie42ub4mvfwnfukmw6s2564flajwnt4cvc4.ipfs.ipfs.example.com\`, which is incorrect and prevents users from using gateways that already have an IPFS subdomain.
Solution
The fix detects if the gateway URL already has an IPFS subdomain by checking if the hostname starts with `ipfs.` and handles both cases appropriately:
Changes
Testing
Verified the fix works correctly for both scenarios:
Related Issues
Fixes #2398