feat(v3:obfuscation): add Garble support with stable binding IDs#5337
feat(v3:obfuscation): add Garble support with stable binding IDs#53370x3st3 wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Garble obfuscation: CLI/generator flags, Taskfile/Docker plumbing to run Garble conditionally, a generator that emits stable binding-ID metadata, a runtime registry to use those IDs, explicit JSON tags on runtime payloads, tests, and docs. ChangesGarble Obfuscation Integration
Sequence DiagramsequenceDiagram
participant User as CLI
participant Task as Taskfiles
participant Gen as Generator
participant Docker as Docker/Cross-build
participant Garble as Garble
participant Runtime as Runtime
User->>Task: pass --obfuscated / --garbleargs
Task->>Task: set OBFUSCATED=true, GARBLE_ARGS=...
Task->>Gen: run "generate bindings" with -obfuscated
Gen->>Gen: collect binding registrations
Gen->>Gen: write wails_obfuscated.gen.go (init() registers IDs)
Task->>Docker: run container with OBFUSCATED env (if cross-build)
Task->>Garble: invoke garble ${GARBLE_ARGS} build (when OBFUSCATED)
Garble-->>Task: produce obfuscated binary
Runtime->>Runtime: init() from generated file calls RegisterBindingMethodID
Client->>Runtime: invoke binding by stable ID
Runtime-->>Client: lookup registered ID and dispatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
v3/internal/commands/build_assets/windows/Taskfile.yml (1)
47-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame
go install mvdan.cc/garble@latesttoolchain compatibility issue as indarwin/Taskfile.ymlThis precondition message has the same garble version/Go toolchain mismatch risk described in the macOS Taskfile comment above. Apply the same fix: pin the install instruction to a tested version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/commands/build_assets/windows/Taskfile.yml` around lines 47 - 49, The precondition message for the garble check uses an unpinned installer "go install mvdan.cc/garble@latest" which can cause Go toolchain compatibility issues; update the message in the preconditions entry (the sh: '{{if eq .OBFUSCATED "true"}}...{{end}}' string) to pin the garble install to a tested release (for example replace "@latest" with a specific version such as "@v0.11.0") so the instruction consistently recommends a known-working toolchain version.
🧹 Nitpick comments (2)
v3/pkg/application/bindings.go (1)
241-244: 💤 Low valueNo observability when a stable ID overrides the FNV hash
globalApplication.debug(...)at line 142 logs the finalmethod.ID(already overridden), but there's no indication that the ID was sourced from the registry vs the FNV hash. A debug-level log line here would make Garble-related binding issues much easier to diagnose.💡 Suggested observability addition
methodID := hash.Fnv(fqn) if registeredID, ok := getRegisteredBindingMethodID(methodDef); ok { methodID = registeredID + // visible in debug output alongside the fqn/id log in Add() + // globalApplication.debug("Using stable binding ID", "fqn", fqn, "id", methodID) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/bindings.go` around lines 241 - 244, The code computes methodID := hash.Fnv(fqn) then possibly overrides it with a stable ID from getRegisteredBindingMethodID(methodDef) but doesn't log which source was used; add a debug log immediately after the override check (using methodID, fqn, and methodDef or method.ID) that records whether the ID came from the registry or from the FNV hash so you can distinguish registry-sourced IDs from hashed IDs in logs (refer to methodID, hash.Fnv, getRegisteredBindingMethodID and the existing globalApplication.debug usage for consistent logging format).v3/internal/commands/task_wrapper_test.go (1)
299-347: 💤 Low value
TestBuildCommandWithObfuscationcovers the new path correctly.The boilerplate for mocking
runTaskFuncand saving/restoringos.Args/GOOS/GOARCHis replicated across everyTestBuildCommand*function. Extracting it into a test helper would significantly reduce copy-paste. Since this is a pre-existing pattern the new test simply follows, deferring is reasonable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/commands/task_wrapper_test.go` around lines 299 - 347, Extract the repeated test setup/teardown in TestBuildCommandWithObfuscation into a reusable helper to reduce duplication: create a test helper function (e.g., setupRunTaskMock) that saves/restores runTaskFunc, os.Args, and GOOS/GOARCH env vars, installs the runTaskFunc mock that captures *RunTaskOptions and otherArgs, and returns a cleanup function; then replace the inline mock and defer blocks in TestBuildCommandWithObfuscation (and other TestBuildCommand* tests) to call setupRunTaskMock and invoke its cleanup with defer; keep the assertions against capturedOptions, capturedOtherArgs and the call to Build unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/internal/commands/build_assets/darwin/Taskfile.yml`:
- Around line 45-49: The precondition message for OBFUSCATED builds currently
tells users to run "go install mvdan.cc/garble@latest", which can install a
garble release incompatible with some Go toolchains; update the Taskfile
precondition (the preconditions entry using the OBFUSCATED conditional and the
msg text) to pin the install to the tested garble release (e.g. use
mvdan.cc/garble@v0.16.0 instead of `@latest`) and update the msg text to state the
tested garble version and the supported Go versions (e.g., "Install garble
v0.16.0: go install mvdan.cc/garble@v0.16.0 — tested with Go 1.25–1.26"), and
make the same change in the equivalent Taskfile entries that reference
garble/OBFUSCATED so users get an explicit, pinned install instruction and
documented Go/garble pairing.
In `@v3/internal/generator/binding_ids.go`:
- Around line 236-265: In buildPackageAliases, reserve the import name
"application" so it never gets assigned to a package alias (to avoid colliding
with the explicit import in writeMetadataImports); e.g. mark
used["application"]=true before allocating aliases and then continue using the
existing logic (sanitizeAlias(seen[path]) -> base fallback -> increment suffix
loop) so any package named "application" will get a different alias like
application2 instead of "application".
- Around line 73-102: In resolveObfuscatedOutput, normalize the chosen dir
(whether generator.options.ObfuscatedOutput or generator.mainPackageDir) to an
absolute/clean path before any logging and before calling
resolveTargetPackageName so relative values like "./cmd/app" match package
lookup keys; use filepath.Abs (and filepath.Clean), handle the Abs error by
logging a warning and falling back to the original dir, and then proceed to call
resolveTargetPackageName(dir) and emit logs referencing the normalized path;
update references to generator.options.ObfuscatedOutput and
generator.mainPackageDir accordingly.
In `@v3/pkg/application/bindings_test.go`:
- Around line 193-218: TestRegisteredBindingMethodID permanently mutates the
package-level sync.Map used by RegisterBindingMethodID
(registeredBindingMethodIDs); add a test-only cleanup by exposing an
UnregisterBindingMethodID function (or move the test into package application)
and call it via defer in TestRegisteredBindingMethodID to remove the mapping for
(*TestService).String after registering stableID; specifically implement
UnregisterBindingMethodID(symbol interface{}) that deletes the key from
registeredBindingMethodIDs and call defer
application.UnregisterBindingMethodID((*TestService).String) right after
RegisterBindingMethodID in the test.
---
Duplicate comments:
In `@v3/internal/commands/build_assets/windows/Taskfile.yml`:
- Around line 47-49: The precondition message for the garble check uses an
unpinned installer "go install mvdan.cc/garble@latest" which can cause Go
toolchain compatibility issues; update the message in the preconditions entry
(the sh: '{{if eq .OBFUSCATED "true"}}...{{end}}' string) to pin the garble
install to a tested release (for example replace "@latest" with a specific
version such as "@v0.11.0") so the instruction consistently recommends a
known-working toolchain version.
---
Nitpick comments:
In `@v3/internal/commands/task_wrapper_test.go`:
- Around line 299-347: Extract the repeated test setup/teardown in
TestBuildCommandWithObfuscation into a reusable helper to reduce duplication:
create a test helper function (e.g., setupRunTaskMock) that saves/restores
runTaskFunc, os.Args, and GOOS/GOARCH env vars, installs the runTaskFunc mock
that captures *RunTaskOptions and otherArgs, and returns a cleanup function;
then replace the inline mock and defer blocks in TestBuildCommandWithObfuscation
(and other TestBuildCommand* tests) to call setupRunTaskMock and invoke its
cleanup with defer; keep the assertions against capturedOptions,
capturedOtherArgs and the call to Build unchanged.
In `@v3/pkg/application/bindings.go`:
- Around line 241-244: The code computes methodID := hash.Fnv(fqn) then possibly
overrides it with a stable ID from getRegisteredBindingMethodID(methodDef) but
doesn't log which source was used; add a debug log immediately after the
override check (using methodID, fqn, and methodDef or method.ID) that records
whether the ID came from the registry or from the FNV hash so you can
distinguish registry-sourced IDs from hashed IDs in logs (refer to methodID,
hash.Fnv, getRegisteredBindingMethodID and the existing globalApplication.debug
usage for consistent logging format).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c5a264f-c3ba-4ab9-bd09-cc09a6069011
📒 Files selected for processing (20)
v3/UNRELEASED_CHANGELOG.mdv3/internal/capabilities/capabilities.gov3/internal/commands/build_assets/Taskfile.tmpl.ymlv3/internal/commands/build_assets/darwin/Taskfile.ymlv3/internal/commands/build_assets/docker/Dockerfile.crossv3/internal/commands/build_assets/linux/Taskfile.ymlv3/internal/commands/build_assets/windows/Taskfile.ymlv3/internal/commands/task_wrapper.gov3/internal/commands/task_wrapper_test.gov3/internal/commands/taskfile_obfuscation_test.gov3/internal/flags/bindings.gov3/internal/flags/task_wrapper.gov3/internal/generator/binding_ids.gov3/internal/generator/binding_ids_test.gov3/internal/generator/generate.gov3/internal/operatingsystem/os.gov3/pkg/application/bindings.gov3/pkg/application/bindings_test.gov3/pkg/application/environment.gov3/pkg/application/screenmanager.go
|
This PR shows most CI checks passing but is currently BLOCKED. This may be due to missing required branch protection checks. Please verify that all required checks have run. If some checks are not appearing, you may need to re-run them or update your branch to trigger the full CI suite. Once CI is fully passing, this PR will need testing on all three platforms to verify the Garble obfuscation support works correctly. |
I don't understand, I think you need to approve the workflow for the two CI checks to run @leaanthony since it is my first time here |
|
Bot comment. I will Thanks for opening this! Will review it today. CC @fbbdev |
leaanthony
added
Enhancement
|
This PR has merge conflicts. Please rebase your branch on the latest master and resolve the conflicts before requesting review again. Once the conflicts are resolved and the PR is mergeable, it will be reconsidered for testing. |
- Introduce application.RegisterBindingMethodID, a sync.Map keyed by the method's function pointer that getMethods consults before falling back to the FQN hash. Function-pointer identity survives Garble's exported-name renaming, so the lookup stays valid in obfuscated builds. - Add `wails3 generate bindings -obfuscated`, which emits a single aggregated wails_obfuscated.gen.go (build-tagged wails_obfuscated) next to the loaded `package main`, registering every service method's stable ID via init(). Pass `-obfuscated-output <dir>` to override the destination (useful for monorepos with no main package among the loaded patterns). - Add `wails3 build --obfuscated --garbleargs ...` and the matching Taskfile plumbing: a `wails_obfuscated` build tag, a garble invocation gated on `OBFUSCATED=true`, env propagation through nested tasks, and a `command -v garble` precondition so missing garble fails fast instead of silently producing an unobfuscated binary. The cross-compile Dockerfile installs garble. Refs wailsapp#4563
Garble obfuscates exported field names of types in obfuscated packages. The runtime ships several structs to the frontend via encoding/json, where reflection picks up the renamed fields and breaks the wire contract. Adding explicit json:"…" tags pins the wire-level field names so the wire format is identical with or without obfuscation. Tagged: EnvironmentInfo, operatingsystem.OS, Screen, Rect, Point, Size, Capabilities. CustomEvent and deviceInfo were already tagged. Refs wailsapp#4563
0x3st3
force-pushed
the
feat/garble-obfuscation
branch
from
267fc8e to
5e41b69
Compare
Done ! |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
v3/internal/generator/binding_ids.go (2)
236-265:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReserve the
applicationalias before assigning service-package imports.Line 200 always imports
github.com/wailsapp/wails/v3/pkg/applicationasapplication, butbuildPackageAliases()can still assign the same alias to a service package namedapplication. That makes the generated file fail to compile with duplicate import names.Suggested fix
func buildPackageAliases(registrations []bindingIDRegistration, selfPkgPath string) map[string]string { aliases := make(map[string]string) - used := make(map[string]bool) + used := map[string]bool{ + "application": true, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/generator/binding_ids.go` around lines 236 - 265, buildPackageAliases can assign the alias "application" to a service package which collides with the explicit import of github.com/wailsapp/wails/v3/pkg/application; to fix, reserve that alias before choosing aliases by setting used["application"]=true (or otherwise marking "application" as taken) early in buildPackageAliases (before iterating over paths), so the loop that computes alias := base and checks used[alias] will never reuse "application" for another import; update references in buildPackageAliases and ensure the aliases map still maps package paths to their unique alias names.
73-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
-obfuscated-outputbefore resolving package identity.Line 37 compares
outputDiragainstdirToPkgPath, but this helper returnsgenerator.options.ObfuscatedOutputunchanged. A relative value like./cmd/appwill not match the absolute directory keys built frompkg.GoFiles, so same-package services get emitted as imports and the generated file self-imports instead of using unqualified(*Type).Method.Suggested fix
func (generator *Generator) resolveObfuscatedOutput() (dir, packageName string, ok bool) { dir = generator.options.ObfuscatedOutput if dir == "" { if generator.mainPackageDirErr != nil { generator.logger.Errorf( "obfuscated bindings: %v; pass -obfuscated-output to set the destination directory explicitly", generator.mainPackageDirErr, ) return "", "", false } dir = generator.mainPackageDir - } else { + } + + absDir, err := filepath.Abs(dir) + if err != nil { + generator.logger.Errorf("obfuscated bindings: resolve output dir %s: %v", dir, err) + return "", "", false + } + dir = filepath.Clean(absDir) + + if generator.options.ObfuscatedOutput != "" { // User-routed destination: registration runs only if the chosen // package is reachable from main's import graph. generator.logger.Infof( "obfuscated bindings: writing metadata file to %s; ensure this package is imported (directly or transitively) by your main package so its init() runs", dir, ) } packageName, err := resolveTargetPackageName(dir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/generator/binding_ids.go` around lines 73 - 102, Normalize the obfuscated output path before resolving package identity: in Generator.resolveObfuscatedOutput, when you set dir from generator.options.ObfuscatedOutput (and likewise when falling back to generator.mainPackageDir), call filepath.Clean and filepath.Abs (and optionally filepath.EvalSymlinks) so dir is an absolute, normalized path prior to any comparisons against package-directory keys (e.g. the dirToPkgPath map) and before calling resolveTargetPackageName; this ensures relative values like "./cmd/app" match the absolute keys built from pkg.GoFiles and prevents self-imports.
🧹 Nitpick comments (1)
v3/internal/commands/build_assets/docker/Dockerfile.cross (1)
199-205: 💤 Low valueMinor:
GARBLE_ARGScannot carry arguments containing spaces.
COMPILER="garble ${GARBLE_ARGS} build"followed by unquoted${COMPILER}relies on shell word-splitting, which is fine for simple flags (-literals -tiny) but silently mangles values like-seed="my seed"or-debugdir="some path". Worth either documenting the limitation in the user-facing flag description or switching to an array (set --/eval) so quoted args survive.Sketch using `set --` to preserve word boundaries
-COMPILER="go build" -if [ "$OBFUSCATED" = "true" ]; then - COMPILER="garble ${GARBLE_ARGS} build" - TAGS="${TAGS},wails_obfuscated" -fi - -${COMPILER} -tags "$TAGS" -trimpath -buildvcs=false -ldflags="$LDFLAGS" -o bin/${APP}-${GOOS}-${GOARCH}${EXT} . +if [ "$OBFUSCATED" = "true" ]; then + TAGS="${TAGS},wails_obfuscated" + # shellcheck disable=SC2086 + set -- garble ${GARBLE_ARGS} build +else + set -- go build +fi + +"$@" -tags "$TAGS" -trimpath -buildvcs=false -ldflags="$LDFLAGS" -o "bin/${APP}-${GOOS}-${GOARCH}${EXT}" .This still relies on word splitting for
GARBLE_ARGS, but at least makes the construction explicit; users requiring spaces in values would need a separate documented mechanism.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/commands/build_assets/docker/Dockerfile.cross` around lines 199 - 205, The COMPILER variable is built as COMPILER="garble ${GARBLE_ARGS} build" and then expanded unquoted, which breaks arguments containing spaces; change the invocation so GARBLE_ARGS is preserved as separate words instead of relying on shell splitting — e.g., use a positional-args approach (set -- garble $GARBLE_ARGS build; then run "$@" -tags "$TAGS" ...) or otherwise avoid embedding multiple arguments into a single COMPILER string and invoke an array/quoted expansion; update the build invocation that currently expands ${COMPILER} to use the preserved positional/array form so quoted garble args like -seed="my seed" or -debugdir="some path" remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@v3/internal/generator/binding_ids.go`:
- Around line 236-265: buildPackageAliases can assign the alias "application" to
a service package which collides with the explicit import of
github.com/wailsapp/wails/v3/pkg/application; to fix, reserve that alias before
choosing aliases by setting used["application"]=true (or otherwise marking
"application" as taken) early in buildPackageAliases (before iterating over
paths), so the loop that computes alias := base and checks used[alias] will
never reuse "application" for another import; update references in
buildPackageAliases and ensure the aliases map still maps package paths to their
unique alias names.
- Around line 73-102: Normalize the obfuscated output path before resolving
package identity: in Generator.resolveObfuscatedOutput, when you set dir from
generator.options.ObfuscatedOutput (and likewise when falling back to
generator.mainPackageDir), call filepath.Clean and filepath.Abs (and optionally
filepath.EvalSymlinks) so dir is an absolute, normalized path prior to any
comparisons against package-directory keys (e.g. the dirToPkgPath map) and
before calling resolveTargetPackageName; this ensures relative values like
"./cmd/app" match the absolute keys built from pkg.GoFiles and prevents
self-imports.
---
Nitpick comments:
In `@v3/internal/commands/build_assets/docker/Dockerfile.cross`:
- Around line 199-205: The COMPILER variable is built as COMPILER="garble
${GARBLE_ARGS} build" and then expanded unquoted, which breaks arguments
containing spaces; change the invocation so GARBLE_ARGS is preserved as separate
words instead of relying on shell splitting — e.g., use a positional-args
approach (set -- garble $GARBLE_ARGS build; then run "$@" -tags "$TAGS" ...) or
otherwise avoid embedding multiple arguments into a single COMPILER string and
invoke an array/quoted expansion; update the build invocation that currently
expands ${COMPILER} to use the preserved positional/array form so quoted garble
args like -seed="my seed" or -debugdir="some path" remain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1983a4d0-2c05-4a37-9656-de169788fd17
📒 Files selected for processing (20)
v3/UNRELEASED_CHANGELOG.mdv3/internal/capabilities/capabilities.gov3/internal/commands/build_assets/Taskfile.tmpl.ymlv3/internal/commands/build_assets/darwin/Taskfile.ymlv3/internal/commands/build_assets/docker/Dockerfile.crossv3/internal/commands/build_assets/linux/Taskfile.ymlv3/internal/commands/build_assets/windows/Taskfile.ymlv3/internal/commands/task_wrapper.gov3/internal/commands/task_wrapper_test.gov3/internal/commands/taskfile_obfuscation_test.gov3/internal/flags/bindings.gov3/internal/flags/task_wrapper.gov3/internal/generator/binding_ids.gov3/internal/generator/binding_ids_test.gov3/internal/generator/generate.gov3/internal/operatingsystem/os.gov3/pkg/application/bindings.gov3/pkg/application/bindings_test.gov3/pkg/application/environment.gov3/pkg/application/screenmanager.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/internal/operatingsystem/os.go
|
Thanks for this comprehensive feature addition! The implementation looks thorough and well-documented. Before I can accept this for testing, I have a few questions:
Decision: COMMENT-AND-WAIT - Awaiting clarification and additional testing CC @leaanthony |
|
Investigating on Windows 11 Pro (24H2, Build 26100). Test environment: Go 1.26.2 · Garble v0.16.0 Unit tests ( Note: Binding generation:
//go:build wails_obfuscated
// +build wails_obfuscated
package main
import (
"github.com/wailsapp/wails/v3/pkg/application"
)
func init() {
application.RegisterBindingMethodID((*GreetService).Greet, 1411160069)
}Build results:
Windows Defender note: Garble-built binaries are flagged by Windows Defender as potentially unwanted (expected behaviour for obfuscated executables — not a bug in this PR). Production releases should be Authenticode-signed to establish SmartScreen reputation. CC @leaanthony |
|
Hello @0x3st3 @leaanthony I started looking at this PR but I have been stuck with just my phone for a few days so I'm struggling to read the most involved parts. Technical details will have to wait until I have some free time to spend on my PC. From what I could glance, the general idea is to hardcode a method-to-ID mapping into a go file that gets compiled alongside the package. Because the in-package go file can reference methods directly (rather then through reflection), it is able to sidestep obfuscation entirely and recover the method pointer from the ID. Please correct me if I'm wrong @0x3st3 . As I said elsewhere, I've been thinking about this problem for a long time and I believe this is the only viable workaround short of modifying garble. Generally I like and support your approach. I already told @leaanthony many times in private that fixing the issue on wails' side is IMO preferable to shipping a modified garble, due to the implied security guarantees. I have started writing down some points I'd like to discuss with you but I'm really starved for time this week.. in the meantime just wanted to let you know that I'm not ghosting your PR 😅 |
Hello ! Nothing to correct, this is exactly the idea ! Do not worry, I don't want to rush this, take your time. I also need to find the time to investigate on what you said earlier ! Would be happy to discuss the points you wrote once you have the time ! Thanks a lot for supporting this :) |
|
This is a major feature adding Garble obfuscation support to v3. The implementation is thorough with good tests and documentation, but given the scope (CLI changes, Taskfile updates, build system integration, and the nature of obfuscation), I'd like a maintainer to review this before merging. CC @leaanthony |
|
@0x3st3 - I'll endeavour to get this reviewed this weekend. We won't rush it but it's a very important change. Again, thanks for taking the time to raise it 🙏 |
Linux verification + response to open review threadsTested on Ubuntu 24.04.4 LTS, Go 1.23.4, garble v0.16.0. All PR unit tests pass; obfuscated binary builds and runs correctly. Addressing each open review thread: Thread:
|
|
@0x3st3 - Looks like there's some things to address. I'm liking this direction though! |
- Pin garble install to @v0.16.0 in all three platform Taskfiles to avoid incompatibility with Go versions outside the garble-supported range - Normalize obfuscated-output path via filepath.Abs before self-package lookup in resolveObfuscatedOutput, fixing the relative-path bug that caused empty selfPkgPath and invalid self-imports - Reserve "application" alias in buildPackageAliases to prevent alias collision when a service package is also named "application" - Add UnregisterBindingMethodID for test-cleanup and use it in TestRegisteredBindingMethodID to keep the global registry hermetic Fixes coderabbitai review threads 1-4 on PR wailsapp#5337. Co-authored-by: multica-agent <github@multica.ai>
Addressing CodeRabbit review threadsAll four CodeRabbit threads have been resolved in commit 4d8a374 pushed directly to the PR branch. Thread 1 —
|
Merge conflict resolutionThe PR branch has fallen behind master and has 5 files with conflicts. All conflicts are mechanical — they arise because master refactored the cross-Docker build tasks to use Resolution strategy: keep master's Files to resolve
# Before (PR version with old mounts)
- docker run --rm -v "{{.ROOT_DIR}}:/app" {{.GO_CACHE_MOUNT}} {{.REPLACE_MOUNTS}} ... {{if eq .OBFUSCATED "true"}}-e OBFUSCATED=true{{end}} {{if .GARBLE_ARGS}}-e GARBLE_ARGS="{{.GARBLE_ARGS}}"{{end}} "{{.CROSS_IMAGE}}" linux {{.DOCKER_ARCH}}
- docker run --rm -v "{{.ROOT_DIR}}:/app" alpine chown -R $(id -u):$(id -g) /app/bin
# After (use master's DOCKER_MOUNTS + keep garble env vars)
- docker run --rm -v "{{.ROOT_DIR}}:/app" {{.DOCKER_MOUNTS}} -e APP_NAME="{{.APP_NAME}}" {{if .EXTRA_TAGS}}-e EXTRA_TAGS="{{.EXTRA_TAGS}}"{{end}} {{if eq .OBFUSCATED "true"}}-e OBFUSCATED=true{{end}} {{if .GARBLE_ARGS}}-e GARBLE_ARGS="{{.GARBLE_ARGS}}"{{end}} "{{.CROSS_IMAGE}}" linux {{.DOCKER_ARCH}}
- cmd: docker run --rm -v "{{.ROOT_DIR}}:/app" alpine chown -R $(id -u):$(id -g) /app/bin
platforms: [linux, darwin]Replace
# Master added new Opn/Cls template syntax; PR added -obfuscated flag
# Resolved:
- wails3 generate bindings -f '{{.Opn}}.BUILD_FLAGS{{.Cls}}' -clean=true{{.Opn}}if eq .OBFUSCATED "true"{{.Cls}} -obfuscated{{.Opn}}end{{.Cls}}{{if .Typescript}} -ts{{end}}
To rebase: git fetch upstream master
git rebase upstream/master
# resolve conflicts as above
git push origin feat/garble-obfuscation --force-with-leaseCC @leaanthony Taliesin is an AI agent. CC @leaanthony |
Conflicts arose because master refactored Docker cross-compilation tasks
to use the new {{.DOCKER_MOUNTS}} variable (from wails3 tool docker-mounts),
while this PR independently added OBFUSCATED/GARBLE_ARGS env var forwarding
to the same docker run commands.
Resolution strategy for each conflict:
- darwin/linux/windows Taskfiles: adopt master's {{.DOCKER_MOUNTS}} variable
(replaces the old GO_CACHE_MOUNT + REPLACE_MOUNTS pair) and preserve the
garble env var forwarding (-e OBFUSCATED / -e GARBLE_ARGS conditionals);
also adopt master's `cmd:` + `platforms: [linux, darwin]` form for the
chown step.
- Taskfile.tmpl.yml: adopt master's {{.Opn}}/{{.Cls}} template syntax for
the BUILD_FLAGS substitution and add the -obfuscated flag using the same
syntax ({{.Opn}}if eq .OBFUSCATED "true"{{.Cls}} ... {{.Opn}}end{{.Cls}}).
- UNRELEASED_CHANGELOG.md: keep master's empty Added section (the French/
German entries were already processed into the main changelog) and add
back only the Garble obfuscation entry from this PR.
Merge conflict resolutionResolved the 5 remaining conflicts between this branch and master. All conflicts arose from master's #5470 refactor that replaced the old Resolution applied to each file:
The branch is now clean and conflict-free.
|
- New guide: docs/src/content/docs/guides/build/obfuscation.mdx
Covers prerequisites (garble v0.16.0, Go 1.24+), the JSON tag
requirement and why it is necessary (interface{} boundary), the
two-step workflow (generate -obfuscated then build --obfuscated),
cross-platform usage, the -obfuscated-output advanced option, and
a troubleshooting section.
- Update docs/src/content/docs/reference/cli.mdx
- wails3 build row: remove stale "the only build-time flag is --tags"
claim; document --obfuscated and --garbleargs.
- generate bindings row: add -obfuscated and -obfuscated-output flags.
- Update docs/astro.config.mjs
Add "Obfuscated Builds" entry to the Building & Packaging sidebar
section.
Closes the documentation gap identified in PR wailsapp#5337 review.
Documentation added (commit 291541e)Three files updated to address the documentation gap identified in the review: New:
Updated:
Updated:
Note: a follow-up issue #5495 tracks automating the two-step workflow via a |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/content/docs/guides/build/obfuscation.mdx`:
- Around line 129-130: Replace the sentence "stable IDs are never registered and
binding calls will fail silently." in the paragraph that mentions package
import/init with wording that does not claim silence; instead state that if the
destination package isn't reachable its init() won't run, stable IDs won't be
registered, and binding attempts will produce a visible "binding not found" (or
similar) error as documented later. Update the sentence so it references the
registration of stable IDs and the explicit "binding not found" error to keep
this guide consistent with the later error documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c37eb7e7-0cff-4616-985f-ed2634978b02
📒 Files selected for processing (3)
docs/astro.config.mjsdocs/src/content/docs/guides/build/obfuscation.mdxdocs/src/content/docs/reference/cli.mdx
✅ Files skipped from review due to trivial changes (2)
- docs/astro.config.mjs
- docs/src/content/docs/reference/cli.mdx
leaanthony
left a comment
leaanthony
left a comment
There was a problem hiding this comment.
Review from VM testing (lin-node1 / mac-node1 / win-node1)
I ran the full test matrix for this PR across three machines before approving. Summary of findings:
Test matrix — all passed
| Test | lin-node1 (Ubuntu 26.04) | mac-node1 (macOS arm64) | win-node1 (Windows 11) |
|---|---|---|---|
Unit tests (go test ./...) |
✅ | ✅ | ✅ |
| Native obfuscated build | ✅ | ✅ | ✅ |
| Cross-compile darwin/arm64 from Linux (Docker) | ✅ | — | — |
| Cross-compile windows/amd64 from Linux (native Go) | ✅ | — | — |
Stable binding ID was identical on all platforms: GreetService.Greet → 1411160069.
🔴 Blocking bug: wrong Go version in Dockerfile.cross
v3/internal/commands/build_assets/docker/Dockerfile.cross line 16:
# current (broken)
FROM golang:1.25-bookworm
# required
FROM golang:1.26-bookwormGarble v0.16.0 (which this Dockerfile installs) requires Go ≥ 1.26.2. Building the image with golang:1.25-bookworm (Go 1.25.10) fails immediately with:
mvdan.cc/garble@v0.16.0 requires go >= 1.26.2 (running go 1.25.10; GOTOOLCHAIN=local)
🔴 Blocking bug: wrong Go version in documentation
docs/src/content/docs/guides/build/obfuscation.mdx Prerequisites section:
# current (wrong)
- **Go 1.24 or later** — required by Garble v0.16.0
# correct
- **Go 1.26.2 or later** — required by Garble v0.16.0
Same issue — Garble v0.16.0 does not work with Go < 1.26.2.
🟡 Windows Defender blocks obfuscated builds (needs docs)
On Windows 11 with default Defender settings, wails3 build --obfuscated fails with:
open C:\Users\...\AppData\Local\Temp\go-build...\a.out.exe: The file contains a virus or potentially unwanted software.
Garble's obfuscated binaries (no debug symbols, packed appearance) trigger Defender's heuristic scanner on the Go build temp directory. The fix:
Add-MpPreference -ExclusionPath "$env:TEMP"
Add-MpPreference -ExclusionPath "C:\path\to\your\project"This should be added to the troubleshooting section of the obfuscation docs.
🟡 Taskfile caching gap
In Taskfile.tmpl.yml, the generate:bindings task's generates: list only includes frontend/bindings/**/*. When OBFUSCATED=true, the command also writes wails_obfuscated.gen.go, but since that file isn't in generates:, go-task's fingerprinter is blind to it.
Consequence: if wails_obfuscated.gen.go is absent (e.g. user cloned fresh, or cleaned) but the other sources haven't changed, go-task may skip generate:bindings and the build fails with "binding not found" errors at runtime.
A minimal fix would add wails_obfuscated.gen.go to the sources: list (so a missing file forces re-run) or move to a status: check. Fine to track as a follow-up if the template change is complex.
Pre-existing compile error in PR (unrelated)
v3/internal/commands/task_wrapper.go and v3/pkg/doctor-ng/packagemanager/xbps.go have a type error (InstallCommand is a string, not a map) that prevents go build ./... from compiling without a patch. This blocks building the wails3 CLI from this branch on machines that don't have it pre-installed. Not introduced by this PR but worth noting.
The two blocking bugs (Dockerfile Go version, docs Go version) must be fixed before merge. The rest can be follow-ups.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@0x3st3 I think the necessary changes are all in. Some fixes have been added as well as some documentation. If you could let me know if you're ok with these changes. @fbbdev - if you have any time at all, it would be great to get your input on this PR as is. If we are in agreement then I'll get this merged in. |
…v0.16.0 garble v0.16.0 requires Go 1.26.2 (not 1.24 as previously documented). Update the prerequisite in the obfuscation guide and bump the cross- compilation Dockerfile base from golang:1.25-bookworm to golang:1.26-bookworm.
…rsion in troubleshooting section
4 participants
Description
Fixes #4563 ("[v3] How to enable obfuscation", opened by @ToQuery). Builds on #3192.
PR #3192 made v3 compile under garble; the maintainer noted at merge time that "method invocation through
Call.ByIDandCall.ByNamemay be affected by obfuscation" and that they were "in discussions with the Garble team about how best to support this moving forward". This PR closes that gap so frontend method calls and built-in runtime payloads stay valid in an obfuscated binary.Two gaps closed:
Call.ByID/Call.ByName— collapse under garble because the FQN hash relies on reflection-visible names, which garble rewritesChanges
New:
application.RegisterBindingMethodID(v3/pkg/application/bindings.go)sync.Mapkeyed by the method's function pointer, consulted bygetMethodsbefore the FQN-hash fallback. Function-pointer identity survives garble's renaming, so the lookup stays valid in obfuscated builds.New:
wails3 generate bindings -obfuscated(v3/internal/generator/binding_ids.go)Emits a single aggregated
wails_obfuscated.gen.gonext to the loadedpackage main, build-taggedwails_obfuscated, registering each service method's stable ID viainit(). Pass-obfuscated-output <dir>to override the destination.New:
wails3 build --obfuscated --garbleargs ...(v3/internal/flags/task_wrapper.go,v3/internal/commands/task_wrapper.go)Forwards two new task variables (
OBFUSCATED,GARBLE_ARGS) into the per-platform Taskfile.Taskfile / Dockerfile changes (
v3/internal/commands/build_assets/{darwin,linux,windows}/Taskfile.yml,…/docker/Dockerfile.cross):wails_obfuscatedbuild tag and switch fromgo buildtogarble {{.GARBLE_ARGS}} buildwhenOBFUSCATED=truecommand -v garbleprecondition so missing garble fails fastgo install mvdan.cc/garble@…JSON tags on runtime payloads (
v3/pkg/application/environment.go,screenmanager.go,v3/internal/operatingsystem/os.go,v3/internal/capabilities/capabilities.go):EnvironmentInfo,OSInfo,Screen,Rect,Point,Size,Capabilitiesgain explicitjson:"…"tags so the wire format is identical with or without obfuscation. User services must carry their own tags as a documented garble prerequisite.Type of change
How Has This Been Tested?
End-to-end with a real 28-service v3 app:
garble build -tags wails_obfuscated …produces a binary whose binding-ID lookups, system runtime calls, JSON payloads, and event delivery all behave identically to the non-obfuscated build.Unit tests added:
pkg/application/bindings_test.go::TestRegisteredBindingMethodIDpkg/application/bindings_test.go::TestRegisterBindingMethodIDPanicsForNonFunctioninternal/generator/binding_ids_test.go::TestBindingIDMetadataSource*(single-package, alias collision, self-package, error paths)internal/commands/task_wrapper_test.go::TestBuildCommandWithObfuscationinternal/commands/taskfile_obfuscation_test.go::TestBuildAssetsSupportObfuscation— tripwire that the embedded Taskfile/Dockerfile assets keep the obfuscation contractAdditional tooling used for E2E:
garble v0.16.0(go install mvdan.cc/garble@v0.16.0)wails3CLI built from this branch (revision shown below)Windows
macOS
Linux
Help welcome verifying Linux/Windows hosts and the Docker cross-compile path.
Test Configuration
Checklist:
(v2 only) I have updatedwebsite/src/pages/changelog.mdxwith details of this PR (v3 changelog entries are added automatically)Summary by CodeRabbit
New Features
Bug Fixes / Runtime
Tests
Documentation