Bug 1688774 - Enable "Connection Settings" dialog to Accept file:// and npipe:// proxy#304
Closed
wwtraveler wants to merge 1 commit into
Closed
Bug 1688774 - Enable "Connection Settings" dialog to Accept file:// and npipe:// proxy#304wwtraveler wants to merge 1 commit into
wwtraveler wants to merge 1 commit into
Conversation
…Is in SOCKS Host field of Connection Settings dialog
When a user enters a `file:///path/to/socket` or `npipe:///./pipe/name` URI into the "SOCKS Host" field in Firefox's Connection Settings dialog and clicks OK, the dialog fails to accept the value. The OK button does nothing because the validation in the `beforeaccept` event handler rejects the URI due to the `/` characters in the URI scheme being treated as invalid hostname characters.
The underlying SOCKS implementation already fully supports Unix domain sockets (`file://`) on Unix and named pipes (`npipe://`) on Windows — the problem was only in the **dialog validation**, not in the actual SOCKS connection code.
In `browser/components/preferences/dialogs/connection.js` line 131, the `beforeAccept` handler validates the SOCKS host using `Services.io.isValidHostname()`:
```javascript
} else if (!Services.io.isValidHostname(proxyPref.value)) {
```
`Services.io.isValidHostname()` returns `false` for `file:///path/to/socket` because the `/` characters in the URI scheme aren't valid hostname characters according to the DNS hostname validation rules. This triggers `event.preventDefault()` which blocks the OK button.
The SOCKS implementation already supports these URIs:
- **`nsSOCKSIOLayer.cpp`**: `SetLocalProxyPath()` (line 121) handles `file://` paths on both Unix and Windows, setting `aProxyAddr->raw.family = AF_UNIX` / `AF_LOCAL`
- **`nsSOCKSIOLayer.cpp`**: `IsLocalProxy()` (line 115) checks `IsHostLocalTarget()` which returns `true` when the host starts with `file:`
- **`nsSOCKSSocketProvider.cpp`**: `OpenTCPSocket()` (line 43) checks `StringBeginsWith(proxyHost, "file://")` and sets `family = AF_LOCAL` for Unix domain sockets
The dialog validation just wasn't aware of these URI schemes.
**Before (line 131):**
```javascript
} else if (!Services.io.isValidHostname(proxyPref.value)) {
```
**After (line 131):**
```javascript
} else if (
!Services.io.isValidHostname(proxyPref.value) &&
!(prefName === "socks" && (proxyPref.value.startsWith("file://") || proxyPref.value.startsWith("npipe://")))
) {
```
The change adds a SOCKS-specific check: if the field is the SOCKS field (`prefName === "socks"`) and the value starts with `file://` or `npipe://`, it is considered valid even if `isValidHostname()` returns `false`.
Added a new `test_socks_file_uri()` test that verifies:
1. `file:///var/run/socks5.sock` is accepted for SOCKS
2. `npipe:///./pipe/socks` is accepted for SOCKS (Windows named pipes)
3. Regular SOCKS hostnames like `localhost` still work
| Platform | URI Scheme | Description |
|----------|------------|-------------|
| Unix/Linux | `file:///path/to/socket` | Unix domain socket |
| Windows | `npipe:///./pipe/name` | Named pipe |
| Both | `file:///path/to/socket` | Works on all platforms |
- The underlying SOCKS implementation for `file://` was added in previous patches (see `nsSOCKSIOLayer.cpp` line 156 for `AF_UNIX` and `nsSOCKSSocketProvider.cpp` line 50 for the `file://` check)
- `npipe://` support mirrors the Windows named pipe implementation
- This is not a new feature but rather fixing the dialog to accept what the SOCKS code already understands
- Added `test_socks_file_uri()` in `browser_connection_valid_hostname.js` with three test cases:
1. `file:///var/run/socks5.sock` -- accepted
2. `npipe:///./pipe/socks` -- accepted
3. `localhost` -- still works (regression check)
- Manual testing:
1. Open `about:networking#socks` or `about:preferences#general`
2. Scroll to "Connection Settings"
3. Select "Manual proxy configuration"
4. Enter `file:///var/run/socks5.sock` in the SOCKS Host field
5. Click OK -- dialog accepts the value
- `./mach try fuzzy -q "browser_connection_valid_hostname"`
- `./mach test --headless browser/components/preferences/tests/networking/browser_connection_valid_hostname.js`
No Taskcluster jobs started for this pull requestThe |
Contributor
|
(Automated Close) Please do not file pull requests here, see https://firefox-source-docs.mozilla.org/contributing/how_to_submit_a_patch.html |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Accept file:// and npipe:// URIs in SOCKS Host field of the Connection Settings dialog
When a user enters a
file:///path/to/socketornpipe:///./pipe/nameURI into the "SOCKS Host" field in Firefox's Connection Settings dialog and clicks OK, the dialog fails to accept the value. The OK button does nothing because the validation in thebeforeacceptevent handler rejects the URI because the/characters in the scheme are treated as invalid hostname characters.The underlying SOCKS implementation already fully supports Unix domain sockets (
file://) on Unix and named pipes (npipe://) on Windows — the problem was only in the dialog validation, not in the actual SOCKS connection code.In
browser/components/preferences/dialogs/connection.jsline 131, thebeforeAccepthandler validates the SOCKS host usingServices.io.isValidHostname():Services.io.isValidHostname()returnsfalseforfile:///path/to/socketbecause the/characters in the URI scheme aren't valid hostname characters according to the DNS hostname validation rules. This triggersevent.preventDefault()which blocks the OK button.The SOCKS implementation already supports these URIs:
nsSOCKSIOLayer.cpp:SetLocalProxyPath()(line 121) handlesfile://paths on both Unix and Windows, settingaProxyAddr->raw.family = AF_UNIX/AF_LOCALnsSOCKSIOLayer.cpp:IsLocalProxy()(line 115) checksIsHostLocalTarget()which returnstruewhen the host starts withfile:nsSOCKSSocketProvider.cpp:OpenTCPSocket()(line 43) checksStringBeginsWith(proxyHost, "file://")and setsfamily = AF_LOCALfor Unix domain socketsThe dialog validation just wasn't aware of these URI schemes.
Before (line 131):
After (line 131):
The change adds a SOCKS-specific check: if the field is the SOCKS field (
prefName === "socks") and the value starts withfile://ornpipe://, it is considered valid even ifisValidHostname()returnsfalse.Added a new
test_socks_file_uri()test that verifies:file:///var/run/socks5.sockis accepted for SOCKSnpipe:///./pipe/socksis accepted for SOCKS (Windows named pipes)localhoststill workfile:///path/to/socketfile:///path/to/socketThe underlying SOCKS implementation for
file://was added in previous patches (seensSOCKSIOLayer.cppline 156 forAF_UNIXandnsSOCKSSocketProvider.cppline 50 for thefile://check)npipe://support mirrors the Windows named pipe implementationThis is not a new feature, but rather fixing the dialog to accept what the SOCKS code already understands
Added
test_socks_file_uri()inbrowser_connection_valid_hostname.jswith three test cases:file:///var/run/socks5.sock-- acceptednpipe:///./pipe/socks-- acceptedlocalhost-- still works (regression check)Manual testing:
about:networking#socksorabout:preferences#generalfile:///var/run/socks5.sockin the SOCKS Host field./mach try fuzzy -q "browser_connection_valid_hostname"./mach test --headless browser/components/preferences/tests/networking/browser_connection_valid_hostname.js