samuong / alpaca

A local HTTP proxy for command-line tools. Supports PAC scripts and NTLM authentication.
Apache License 2.0
184 stars 31 forks source link

Serve /alpaca.pac to send non-DIRECT requests to alpaca #16

Closed camh- closed 4 years ago

camh- commented 4 years ago

Serve a PAC file on /alpaca.pac that only returns DIRECT or PROXY localhost:<listen-port> (usually 3128). If the PAC file that alpaca is using would return DIRECT for a URL, the wrapped PAC also returns DIRECT. For everything else, it returns a proxy directive to send traffic to alpaca. If alpaca does not have a PAC file (wrong network, etc), then the PAC function only returns DIRECT.

This allows the system (Mac OSX, Gnome, etc) to use alpaca as the proxy, and to configure alpaca with the network proxy URL. When the system evaluates a URL with the wrapped PAC file and gets a DIRECT response, it will not proxy through alpaca and go direct instead. This helps with the odd application that doesn't seem to handle going through a proxy at all. It also means that when you're off the network that serves the PAC file, no requests go through alpaca. This makes alpaca work well as a whole-system proxy instead of just for CLI apps as originally intended.

A bit of refactoring and lint fixes made it into this PR. The most significant change would be the use of http handler wrappers (also known in some circles as http middleware). Once the code went from 1 to >1 handlers, it made sense to factor out some functionality into a handler wrapper. Some additional functionality was added that way too.

Please review this commit-by-commit and read the commit messages. I hope that makes it clearer to understand the changes and their reason. I'm happy to entertain the most nit-picky requests/reviews, so don't hold back, no matter how trivial it seems.

Fixes: #14

juliaogris commented 4 years ago

@samuong - IMO you should protect and "require pull request reviews before merging" for your master branch. Maybe ;)

camh- commented 4 years ago

@juliaogris @anzdaddy Changes pushed as fixup commits. PTAL, and resolve all comments that you feel are now resolved. Or continue to argue as you see fit.

samuong commented 4 years ago

@samuong - IMO you should protect and "require pull request reviews before merging" for your master branch. Maybe ;)

@juliaogris The only reason I haven't done that in the past is that I didn't have anyone else to review my own pull requests. You all are the only ones (other than me) who have commented on PRs, so if you're happy to be collaborators and review my code, then I'll add you to the repo.

camh- commented 4 years ago

@marcelocantos @juliaogris @samuong All comments resolved. I have forced-pushed my squashed series, so back to just the commits that matter without fixups. I have removed the //nolint directives and added a couple of TODOs.

PTAL

camh- commented 4 years ago

@samuong PTAL

I've pushed fixup commits and linked them to the comments so you can see how I have addressed your comments. When you've re-reviewed, I'll run git rebase -i --autosquash master to squash the fixups into the relevant commits and force push so there is still a clean series.

camh- commented 4 years ago

PTAL @samuong. Again, fixup commits pushed which I'll squash into the main series when you're happy with it.

samuong commented 4 years ago

Thanks Cam! LGTM.

I'll let you squash everything before I actually approve it, since I think your squash will invalidate my approval anyway. But once you're done I'm happy to give the actual approval.

camh- commented 4 years ago

Ok. Squashed and force-pushed. If you click on the "force-pushed" link where it says "camh- force-pushed the camh-:pacwrap branch" you can verify the push did not alter anything.

camh- commented 4 years ago

@samuong I wondered if this was going to happen. It looks like you're not a CODEOWNER so you cannot approve this merge. Only @juliaogris or @marcelocantos can :-)

If you want to raise a PR to add yourself to CODEOWNERS, I can approve it. (or you can just use your admin superpowers and merge anyway)

samuong commented 4 years ago

https://github.com/samuong/alpaca/pull/25