Four structural things came up repeatedly while working on #30, #31 and #33.
None is a bug and none needs fixing to ship anything, so I did not touch them.
Raising them as one issue rather than arriving with a large diff, because they
are the kind of change you should get to decide on before someone writes it.
Happy to implement any subset, none of it, or something different if you have a
shape in mind.
1. GenerateOptions takes 38 positional parameters
func GenerateOptions(stage, sleeptime, jitter, useragent, uri, customuri,
customuriGET, customuriPOST, beacon_PE, processinject_min_alloc,
Post_EX_Process_Name, metadata, injector, Host, Profile, ProfilePath,
outFile, custom_cert, cert_password, CDN, CDN_Value, datajitter,
Keylogger string, Forwarder bool, ...)
Twenty-three consecutive string parameters means transposing any two of them
compiles cleanly and silently produces a wrong profile. Adding a flag means
touching the signature, the call site, and every intermediate.
The type to pass already exists: Loader.FlagOptions is declared at the top of
Loader.go and is never referenced anywhere. Passing that struct instead would
make argument order unrepresentable as a bug class, and adding a flag would
become a one-line change.
2. num_Profile and Post are mutable package globals
GenerateProfile reads num_Profile expecting GenerateHTTPVaribles to have
already set it. That ordering is not expressed anywhere, so reordering the calls
in GenerateOptions breaks profile selection silently rather than failing to
compile. Returning the resolved profile number and passing it explicitly would
make the dependency visible.
3. Build() is nine copies of the same nine lines
Each of the nine template sections repeats parse, check error, reset buffer,
execute, check error, capture string. That is roughly 110 lines doing one thing
nine times. A slice of {name, template, data} and a loop would be about
fifteen. Relatedly, the nine types Beacon_Com, Beacon_Stage_p1 through
p3, Process_Inject, Beacon_PostEX, Beacon_GETPOST,
Beacon_GETPOST_Profile and Beacon_SSL are all identical:
type X struct {
Variables map[string]string
}
One type used nine times would do, and the templates would not notice.
4. log.Fatal throughout makes error paths untestable
Every validation failure calls log.Fatal, which exits the process. That means
none of them can be covered by a test, because the test binary dies with them.
The tests I added in #30 and #33 cover the success paths and the pure helpers;
the error paths are covered only by running the binary by hand.
Returning errors up to main and calling log.Fatal in exactly one place would
make every validation rule testable. This is the largest of the four and the
one I would understand you declining, since it touches every function.
Why I am not sending these as PRs
Each one is mechanical but wide, and a wide diff is hard to review next to the
behavioural changes already in flight. #1 and #3 are behaviour-neutral and
mostly mechanical. #2 is small. #4 is the invasive one. If any of them appeal,
say which and I will send them one at a time, separately from the fixes, so the
diffs stay readable.
Four structural things came up repeatedly while working on #30, #31 and #33.
None is a bug and none needs fixing to ship anything, so I did not touch them.
Raising them as one issue rather than arriving with a large diff, because they
are the kind of change you should get to decide on before someone writes it.
Happy to implement any subset, none of it, or something different if you have a
shape in mind.
1.
GenerateOptionstakes 38 positional parametersTwenty-three consecutive
stringparameters means transposing any two of themcompiles cleanly and silently produces a wrong profile. Adding a flag means
touching the signature, the call site, and every intermediate.
The type to pass already exists:
Loader.FlagOptionsis declared at the top ofLoader.goand is never referenced anywhere. Passing that struct instead wouldmake argument order unrepresentable as a bug class, and adding a flag would
become a one-line change.
2.
num_ProfileandPostare mutable package globalsGenerateProfilereadsnum_ProfileexpectingGenerateHTTPVariblesto havealready set it. That ordering is not expressed anywhere, so reordering the calls
in
GenerateOptionsbreaks profile selection silently rather than failing tocompile. Returning the resolved profile number and passing it explicitly would
make the dependency visible.
3.
Build()is nine copies of the same nine linesEach of the nine template sections repeats parse, check error, reset buffer,
execute, check error, capture string. That is roughly 110 lines doing one thing
nine times. A slice of
{name, template, data}and a loop would be aboutfifteen. Relatedly, the nine types
Beacon_Com,Beacon_Stage_p1throughp3,Process_Inject,Beacon_PostEX,Beacon_GETPOST,Beacon_GETPOST_ProfileandBeacon_SSLare all identical:One type used nine times would do, and the templates would not notice.
4.
log.Fatalthroughout makes error paths untestableEvery validation failure calls
log.Fatal, which exits the process. That meansnone of them can be covered by a test, because the test binary dies with them.
The tests I added in #30 and #33 cover the success paths and the pure helpers;
the error paths are covered only by running the binary by hand.
Returning errors up to
mainand callinglog.Fatalin exactly one place wouldmake every validation rule testable. This is the largest of the four and the
one I would understand you declining, since it touches every function.
Why I am not sending these as PRs
Each one is mechanical but wide, and a wide diff is hard to review next to the
behavioural changes already in flight. #1 and #3 are behaviour-neutral and
mostly mechanical. #2 is small. #4 is the invasive one. If any of them appeal,
say which and I will send them one at a time, separately from the fixes, so the
diffs stay readable.