Conversation
marcelocantos
left a comment
There was a problem hiding this comment.
Blocking
-
JS injection in FindProxyForURL (pacrunner.go:98)
vm.RunString("FindProxyForURL(" + fmt.Sprintf("%q", u.String()) + ", " + ...)
%q produces Go string literals, not JS. They diverge on U+2028/U+2029 (JS line terminators), and more importantly this throws
away the argument-passing safety the original vm.Call(...) had. Use goja's Callable:
fn, ok := goja.AssertFunction(vm.Get("FindProxyForURL"))
val, err := fn(goja.Undefined(), vm.ToValue(u.String()), vm.ToValue(u.Hostname())) -
Panic on non-string PAC return (pacrunner.go:102)
return val.Export().(string), nil
Original code returned an error if the PAC returned non-string; this now panics the proxy. Restore the check: s, ok :=
val.Export().(string); if !ok { return "", errors.New(...) }. -
Global var pacVM *goja.Runtime
Package-level mutable runtime referenced from toValue. *goja.Runtime is not goroutine-safe, and during Update()'s
RunString(pacjs) the PAC script's calls into these helpers use whichever VM pacVM happened to point at — which may be the old
one while populating the new. Pass the runtime through closure instead:
set := func(name string, handler func(goja.FunctionCall) goja.Value) { ... }
// convert via vm.ToValue(...) using the closure-captured vm
Should fix
-
otto still in go.mod require block — bumped to 0.5.1 but there are no remaining otto. imports. Run go mod tidy.
-
Lost comments. Several explanatory "why" comments were deleted (CONNECT-request scheme fallback, HTTPS path stripping,
date-range case documentation, myIpAddress Chromium reference). Restore them — they're load-bearing. -
Unused call params on myIpAddress/myIpAddressEx — were _ before, now named but unreferenced. Put the _ back.
-
Redundant arms in dateRange type switch — float64/int64/int cases are identical. Collapse to a single numeric branch.
Also, previously ToInteger() on a fractional JS number would surface an error; now int64(v) silently truncates.
Nit
- Test helpers build JS source strings (toJSArg + RunString("...") with fmt.Sprintf("%q", ...)). Same injection concern as
#1, just in tests. Use goja.AssertFunction + fn(...) to call directly with typed args.
The otto→goja swap itself is fine, but the calling convention needs to stay value-based, not string-concatenated.
|
Thanks Marcelo! I agree the JS injection stuff is particularly sloppy, and the other points you mentioned aren't great either. Maybe hold off on sinking too much time into reviewing this though, it was really just a quick and dirty way for me to see how much work is involved in migrating, and I'll be looking into other engines too. I'll ping you later if you're interested in chatting about JS engines or anything else. |
Don't worry, I didn't. 😉 |
Vibe coded with AI, I still need to review this...
Fixes #98