-
Notifications
You must be signed in to change notification settings - Fork 53
feat: add HTTP caching for PAC fetch using httpcache (RFC 9111 compli… #171
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: master
Are you sure you want to change the base?
Changes from all commits
9f6d13c
63a249b
f2b95bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,5 @@ | |
| __debug_bin* | ||
| .idea | ||
| .DS_Store | ||
| cache/ | ||
| cache/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,28 +1,29 @@ | ||
| module github.com/samuong/alpaca/v2 | ||
|
|
||
| go 1.22.3 | ||
|
|
||
| toolchain go1.22.4 | ||
| go 1.25.3 | ||
|
|
||
| require ( | ||
| github.com/gobwas/glob v0.2.3 | ||
| github.com/keybase/go-keychain v0.0.0-20231219164618-57a3676c3af6 | ||
| github.com/robertkrimen/otto v0.4.0 | ||
| github.com/samuong/go-ntlmssp v0.0.0-20240616070040-65a20607c744 | ||
| github.com/stretchr/testify v1.9.0 | ||
| github.com/sandrolain/httpcache v1.4.0 | ||
| github.com/stretchr/testify v1.11.1 | ||
| github.com/zalando/go-keyring v0.2.5 | ||
| golang.org/x/term v0.21.0 | ||
| golang.org/x/term v0.36.0 | ||
| ) | ||
|
|
||
| require ( | ||
| github.com/alessio/shellescape v1.4.1 // indirect | ||
| github.com/danieljoos/wincred v1.2.0 // indirect | ||
| github.com/davecgh/go-spew v1.1.1 // indirect | ||
| github.com/godbus/dbus/v5 v5.1.0 // indirect | ||
| github.com/google/btree v1.1.3 // indirect | ||
| github.com/peterbourgon/diskv v2.0.1+incompatible // indirect | ||
| github.com/pmezard/go-difflib v1.0.0 // indirect | ||
| golang.org/x/crypto v0.24.0 // indirect | ||
| golang.org/x/sys v0.21.0 // indirect | ||
| golang.org/x/text v0.16.0 // indirect | ||
| golang.org/x/crypto v0.43.0 // indirect | ||
| golang.org/x/sys v0.37.0 // indirect | ||
| golang.org/x/text v0.30.0 // indirect | ||
| gopkg.in/sourcemap.v1 v1.0.5 // indirect | ||
| gopkg.in/yaml.v3 v3.0.1 // indirect | ||
| ) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,7 +11,6 @@ | |||||||||||||||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||
| // See the License for the specific language governing permissions and | ||||||||||||||||
| // limitations under the License. | ||||||||||||||||
|
|
||||||||||||||||
| package main | ||||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
|
|
@@ -25,50 +24,52 @@ import ( | |||||||||||||||
| "runtime" | ||||||||||||||||
| "strings" | ||||||||||||||||
| "time" | ||||||||||||||||
|
|
||||||||||||||||
| "github.com/sandrolain/httpcache" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| // The maximum size (in bytes) allowed for a PAC script. At 1 MB, this matches the limit in Chrome. | ||||||||||||||||
| const maxResponseBytes = 1 * 1024 * 1024 | ||||||||||||||||
|
|
||||||||||||||||
| // The maximum size (in bytes) allowed for a data URL. | ||||||||||||||||
| // Chromium and Firefox limit data URLs to 512MB. | ||||||||||||||||
| // See https://developer.mozilla.org/en-US/docs/Web/URI/Reference/Schemes/data#length_limitations | ||||||||||||||||
| const maxDataURLLength = 512 * 1024 * 1024 | ||||||||||||||||
|
|
||||||||||||||||
| // The time to wait before retrying a failed PAC download. This is similar to Chrome's delay: | ||||||||||||||||
| // https://cs.chromium.org/chromium/src/net/proxy_resolution/proxy_resolution_service.cc?l=96&rcl=3db5f65968c3ecab3932c1ff7367ad28834f9502 | ||||||||||||||||
| var delayAfterFailedDownload = 2 * time.Second | ||||||||||||||||
|
|
||||||||||||||||
| type pacFetcher struct { | ||||||||||||||||
| pacFinder *pacFinder | ||||||||||||||||
| monitor netMonitor | ||||||||||||||||
| client *http.Client | ||||||||||||||||
| connected bool | ||||||||||||||||
| //cache []byte | ||||||||||||||||
| //modified time.Time | ||||||||||||||||
| //fetched time.Time | ||||||||||||||||
| //expiry time.Time | ||||||||||||||||
| //etag string | ||||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is good - this commented out code can stay removed, but we should maintain all the other comments that have been deleted. |
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func newPACFetcher(pacurl string) *pacFetcher { | ||||||||||||||||
| client := &http.Client{Timeout: 30 * time.Second} | ||||||||||||||||
| var client *http.Client | ||||||||||||||||
|
|
||||||||||||||||
| if strings.HasPrefix(pacurl, "file:") { | ||||||||||||||||
| log.Print("Warning: When using a local PAC file, the online/offline status can't ", | ||||||||||||||||
| "be determined by the fact that the PAC file is downloaded. Make sure you ", | ||||||||||||||||
| "check for proxy connectivity in your PAC file!") | ||||||||||||||||
|
|
||||||||||||||||
| client = &http.Client{Timeout: 30 * time.Second} | ||||||||||||||||
|
|
||||||||||||||||
| if runtime.GOOS == "windows" { | ||||||||||||||||
| client.Transport = http.NewFileTransport(http.Dir("C:")) | ||||||||||||||||
| } else { | ||||||||||||||||
| client.Transport = http.NewFileTransport(http.Dir("/")) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| } else { | ||||||||||||||||
| // The DefaultClient in net/http uses the proxy specified in the http(s)_proxy | ||||||||||||||||
| // environment variable, which could be pointing at this instance of alpaca. When | ||||||||||||||||
| // fetching the PAC file, we always use a client that goes directly to the server, | ||||||||||||||||
| // rather than via a proxy. | ||||||||||||||||
| client.Transport = &http.Transport{Proxy: nil} | ||||||||||||||||
| // Base transport without proxy (important: avoid proxy loop) | ||||||||||||||||
| baseTransport := &http.Transport{Proxy: nil} | ||||||||||||||||
|
|
||||||||||||||||
| // ✅ Use in-memory cache (FIXED) | ||||||||||||||||
| cacheTransport := httpcache.NewTransport(httpcache.NewMemoryCache()) | ||||||||||||||||
| cacheTransport.Transport = baseTransport | ||||||||||||||||
|
|
||||||||||||||||
| client = &http.Client{ | ||||||||||||||||
| Timeout: 30 * time.Second, | ||||||||||||||||
| Transport: cacheTransport, | ||||||||||||||||
| } | ||||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better to leave the original And then for the else branch, maybe something like this?
Suggested change
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return &pacFetcher{ | ||||||||||||||||
| pacFinder: newPacFinder(pacurl), | ||||||||||||||||
| monitor: newNetMonitor(), | ||||||||||||||||
|
|
@@ -87,12 +88,8 @@ func requireOK(resp *http.Response, err error) (*http.Response, error) { | |||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // decodeDataURL decodes a data URL (e.g., data:text/plain;base64,SGVsbG8sIFdvcmxkIQ==). | ||||||||||||||||
| // It supports both base64 and URL-encoded data, and enforces the maxResponseBytes size limit. | ||||||||||||||||
| // See https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URLs for details. | ||||||||||||||||
| func decodeDataURL(uri string) ([]byte, error) { | ||||||||||||||||
| parsedURL, err := url.Parse(uri) | ||||||||||||||||
|
|
||||||||||||||||
| if err != nil { | ||||||||||||||||
| return nil, fmt.Errorf("error parsing pac url: %w", err) | ||||||||||||||||
| } | ||||||||||||||||
|
|
@@ -127,17 +124,11 @@ func decodeDataURL(uri string) ([]byte, error) { | |||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func (pf *pacFetcher) download() []byte { | ||||||||||||||||
| // TODO: Combine pacChanged() and findPACURL() as described in | ||||||||||||||||
| // https://github.com/samuong/alpaca/pull/156#issuecomment-3125070335 | ||||||||||||||||
| if !pf.monitor.addrsChanged() && !pf.pacFinder.pacChanged() { | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
| pf.connected = false | ||||||||||||||||
|
|
||||||||||||||||
| // We've just detected a change in network state, so close any "idle" | ||||||||||||||||
| // connections from the previous network. This forces a fresh DNS | ||||||||||||||||
| // lookup and TCP dial during the next PAC download. For context, see | ||||||||||||||||
| // <https://github.com/samuong/alpaca/issues/165>. | ||||||||||||||||
| pf.client.CloseIdleConnections() | ||||||||||||||||
|
|
||||||||||||||||
| pacurl, err := pf.pacFinder.findPACURL() | ||||||||||||||||
|
|
@@ -164,17 +155,18 @@ func (pf *pacFetcher) download() []byte { | |||||||||||||||
|
|
||||||||||||||||
| resp, err := requireOK(pf.client.Get(pacurl)) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| // Sometimes, if we try to download too soon after a network change, the PAC | ||||||||||||||||
| // download can fail. See https://github.com/samuong/alpaca/issues/8 for details. | ||||||||||||||||
| log.Printf("Error downloading PAC file, will retry after %v: %q", | ||||||||||||||||
| delayAfterFailedDownload, err) | ||||||||||||||||
|
|
||||||||||||||||
| time.Sleep(delayAfterFailedDownload) | ||||||||||||||||
|
|
||||||||||||||||
| if resp, err = requireOK(pf.client.Get(pacurl)); err != nil { | ||||||||||||||||
| log.Printf("Error downloading PAC file, giving up: %q", err) | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| defer resp.Body.Close() | ||||||||||||||||
|
|
||||||||||||||||
| var buf bytes.Buffer | ||||||||||||||||
| _, err = io.CopyN(&buf, resp.Body, maxResponseBytes) | ||||||||||||||||
| if err == io.EOF { | ||||||||||||||||
|
|
@@ -191,4 +183,4 @@ func (pf *pacFetcher) download() []byte { | |||||||||||||||
|
|
||||||||||||||||
| func (pf *pacFetcher) isConnected() bool { | ||||||||||||||||
| return pf.connected | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestPACCacheNoFallback(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Write([]byte("function FindProxyForURL() { return 'DIRECT'; }")) | ||
| })) | ||
|
|
||
| pf := newPACFetcher(server.URL) | ||
|
|
||
| data := pf.download() | ||
| if data == nil { | ||
| t.Fatal("expected PAC data on first fetch") | ||
| } | ||
|
|
||
| server.Close() | ||
|
|
||
| data = pf.download() | ||
| if data != nil { | ||
| t.Fatal("expected nil when server is down (no cache fallback)") | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
|
|
||
| package main | ||
|
|
||
| type pacFinder struct{ | ||
| type pacFinder struct { | ||
| pacURL string | ||
| } | ||
|
|
||
|
|
||
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.
i think we just need this once?