Conversation
Tests and CLI docs used two real domains as examples/fixtures: - tissolution.it -> my.example.com (own domain) - rsgarage.eu -> example.net (info@ mailbox in mailbox_progress_test) Swap both for RFC 2606 reserved names so no real domain ships in the repo. Same-length substitutions keep struct/doc column alignment intact; behavior is unchanged (validation/scope/label tests assert on structure, not on the specific domain).
… zeroing Some cPanel builds report a fractional value for disk fields (e.g. Ftp::list_ftp_with_disk diskused = "57632.08" or a bare 13558.40). The old flexInt64 decoder only tried ParseInt, so any fractional value silently collapsed to 0, zeroing out every disk figure. Add a ParseFloat fallback that truncates to the integer part; null/empty/non-numeric still yield 0 without failing the surrounding decode (the field is informational only). Adds TestFlexInt64AcceptsFloats.
…yout Rebuilding a destination account under the SAME main domain legitimately puts the docroot at ~/public_html, which the classifier and the webfiles guard previously refused unconditionally (fail-closed). Add a narrow, defense-in-depth carve-out: - migrate: sameNameMainToMain relaxes the block ONLY when src and dest are both the MAIN domain, the canonical FQDN matches, and there is a single main_domain docroot. - webfiles: WebPlanItem.AllowDestPublicHTMLRoot (threaded through CanonicalDestDocroot / destDocrootEnv / Transfer and the guard script's ALLOW_PUBLIC_HTML_ROOT branch) relaxes ONLY the exact public_html-root equality refusal. Every other containment/escape/absolute/.. check stays fail-closed, and backupDest still refuses to back up the root even under the flag. Adds mainroot_test.go regression coverage.
…e cpsrvd decode
cpsrvd form-url-decodes uapi CLI argument values ('+' -> space, '%XX' ->
byte). The tool sends the MySQL password raw to Mysql::create_user /
set_password, so any generated or reused DB password containing '+' or '%'
was stored DECODED while wp-config and MYSQL_PWD kept the raw value,
silently breaking the migrated site's DB auth (the API still returns
success). The generated-password alphabet includes both '+' and '%', so
this hit a large fraction of runs.
encodeUAPIArgValue pre-encodes '%'->%25 and '+'->%2B in uapiArgsScript, so
after cpsrvd decodes, the stored value equals the original. Values with
neither byte are unchanged (no regression for existing calls).
Verified live on a sacrificial cPanel account (Mysql::create_user): a raw
'+'/'%2B' password authenticates only via its decoded form, confirming the
decode; the pre-encoded form round-trips exactly.
Add internal/events (stdlib-only): an optional Emitter on migrate.Options
drives a JSONL event stream (per-phase started/completed/failed across the
full run) plus an end-of-run report.json, behind four new flags: --run-id,
--output-dir, --json-events, --report-json. Zero-value Emitter = off; on the
CLI path the collector is always wired. Event Data payloads pass through a
key-based secret-redaction net before serialization; HostRef is {IP,User}.
Wired into runner.go (Options fields + emitEvent helper + run/connect/analyze
/compare emissions) and apply.go/apply_mailboxes.go (apply-phase emissions).
main.go grafts the flags onto dev's flat-flag flow; a broader namespace
rewrite and its --account-inventory are NOT included.
Built via a 3-constructor + judge panel; validated live against real cPanel
servers (dry-run: 18 events + report.json; apply --db: apply-phase events,
migrate_db/verify_db, redaction-clean). No new go.mod deps.
Replace every em-dash (U+2014) inside Go string literals with a context-appropriate colon (status/label prefixes) or comma (sentence continuations), so no em-dash reaches an operator via stdout, logs, --help, or the events.jsonl/report.json output. 332 replacements across 55 files; test goldens and *.golden fixtures updated in lockstep with their producers. No en-dashes existed. Pure text change with no behavior change (the character was never a runtime delimiter); build and the full test suite are green. Deferred as an optional follow-up: about 875 em-dashes in code comments across 169 files, non-operator-visible, left untouched for now. Two string-literal em-dashes inside embedded bash comments in remote-script raw strings are excluded (not operator-visible).
Add read-only cPanel inventory collectors (library, no CLI caller yet): FTP, SSL, PHP-per-vhost, MIME redirects, email accounts/forwarders/ autoresponders/filters/routing/default-address, DNS zones, and cron (read via `crontab -l` with command redaction). Each calls only List*/ Get*/Fetch* over UAPI or the new API2 (cpapi2) transport; no write path is reachable. api.go: refactor RunUAPI onto a shared runUAPIExec body and add RunUAPIRaw plus the API2 transport (api2ArgsScript/RunAPI2/parseAPI2). The already committed encodeUAPIArgValue stays applied to every UAPI value. types.go gains flexStringList (SSL SANs) and the generic api2Envelope. The DNS/email/cron WRITERS are deliberately left behind (separate unit). Customer PII is excluded: the realserver fixtures and their tests were dropped, only synthetic fixtures are used. No new go.mod deps.
…redact Key-based secret redaction was copied three times with drifting key lists: internal/events/redact.go, internal/cpanel/debug.go, and the cron command redactor in internal/cpanel/cron.go. The drift caused a real leak: a live PrestaShop cron job authenticated with secure=<token>, which slipped through because the cron list covered "secret" but not "secure". Add internal/redact (stdlib-only leaf: IsSensitiveKey, RedactMap, RedactJSON, Placeholder, Fragments) with one SUPERSET fragment list (token, secret, pass, pwd, key, auth, cred, cookie, session, bearer, secure) = the union of all three former lists, so every caller now redacts at least as much as any single copy ever did. The canonical list is unexported; Fragments() returns a copy for cron to build its command-line regex, so no caller can mutate and weaken it. events, debug.go and cron.go delegate to the shared package; cron sources its fragments from Fragments() so it cannot drift again. A completeness test fails if any historical fragment is dropped. No behavior weakened; no new go.mod deps.
Add internal/accountinventory (net-new): the offline brain over the read-only cpanel collectors. Collect drives every collector into a NormalizedInventory; the engines (diff, policy, coverage, dns/email/cron plan, checklist, acceptance merge) are stdlib-only and deterministic; the *_write.go files are LOCAL artifact serializers (JSON/Markdown), not server writes. This makes the previously-dead collectors live. CLI: a minimal "inventory" subcommand dispatch (diff|policy|dns-plan|email-plan |cron-plan|checklist, all offline) plus --account-inventory (collect both sides, write inventory_source.json / inventory_destination.json / inventory_report.md), hand-merged into dev's flat-flag main.go without any write namespaces. Excluded (Batch D): every server writer/verifier (dns/cron/email apply + verify). Consolidations: accountinventory.DNSRecordEntry collapsed onto the existing cpanel.DNSRecord; normalized types clashing with cpanel names renamed to Norm* (NormForwarderEntry/NormAutoresponderEntry/NormEmailFilterEntry). Cron redaction is inherited from internal/redact via the cpanel collector. Secret hygiene: the raw cron command/env fields (command_clear/raw_line/ value_clear) are json:"-" and are never populated or serialized in this read-only path, so the inventory/diff/cron-plan artifacts carry only redacted commands plus SHA256 hashes (a cron authenticating with secure=<token> cannot leak the token). Job and env identity ride SHA256, keeping create/skip/manual classification unchanged. The raw fields remain defined for the future Batch-D apply writer, which re-collects the source crontab at apply time; a regression test plants a secret cron and asserts the artifact carries only the redacted form. Collect is implemented faithfully; the N+1 SSH batching optimization is a deferred follow-up. All real customer PII is scrubbed to synthetic / RFC5737. No new go.mod deps.
…ver-side execs The read-only account-inventory Collect issued ~42 sequential SSH round-trips per side (an N+1 over per-domain forwarders/autoresponders/DNS and per-mailbox filters). Fold it into 3 constant "bash -s" batched execs per side, reusing the proven sshx.StreamNul framed-output primitive that internal/migrate/collect.go already uses: pass 1 runs the no-arg calls + crontab, pass 2 runs the per-domain loops server-side, pass 2b runs the detail lookups (autoresponder bodies, get_filter). Round-trips go from ~13+2N+A+Z+M+F to a constant 3 per side. New internal/accountinventory/batch package: a prefetch/replay shim that captures every uapi/cpapi2/crontab response server-side (tag + NUL-framed, JSON verbatim) and replays them through the UNCHANGED cpanel.parseUAPI/parseAPI2 decoders and the UNCHANGED Collect assembly, so the NormalizedInventory stays byte-identical. No Go-side concurrency (one session at a time; no MaxSessions pressure, no race). cpanel gains only an exported EncodeUAPIArgValue passthrough; sshx.StreamNul is reused. The Batch-B cron redaction holds: raw crontab transits the framed stream but is redacted in Go before the artifact (command_clear/raw_line stay json:"-"). Verified: an in-process sequential-vs-batched equivalence test asserts byte-identical output, and a live A/B against a real cPanel account confirms the two inventories are byte-identical (apart from the collection timestamp and a pre-existing, server non-deterministic SSL-parse warning that varies run-to-run on the old binary too). Measured ~2.2x wall-clock on that account (28.5s to 13.1s). No new go.mod deps.
Real cPanel builds return the SSL cert numeric fields (not_before, not_after, is_self_signed, modulus_length) as JSON STRINGS, not numbers. SSLCertEntry declared them int64/int, so parseUAPI rejected the whole list_certs response on a real host and the account-inventory SSL section came back empty with a "cannot unmarshal string into ... of type int64" warning. The test fixture used integers, hiding it; a live run surfaced it. Retype the four fields to flexInt64 (which already backs Domains via flexStringList) so string and number both parse; cast at the two int64 consumers in the inventory SSL projection. Adds a string-numerics parse test. Live-verified: the SSL section now populates (25 certs) with no warning. No new go.mod deps.
Add internal/workbench (net-new leaf): a filesystem-backed migration-session model (14-state Status + forward-only Step transition matrix, non-secret SetupMeta/Endpoint, content-addressed artifact registry, provenance timeline, atomic 0600 writes + cross-process flock). Only non-stdlib dependency is internal/version. The safety_test firewall (no sshx/cpanel/config dependencies, no credential json tag, no write-verb) is preserved. CLI: a "migration" flat-flag dispatch (init|list|show|set-status|attach-artifact |archive) over a local store under CPANEL_MIGRATION_HOME, mirroring the inventory dispatch. Read/local-only, never dials a host, no server write. internal/webui and ui_cmd.go are NOT included (Batch E). Security: AttachArtifact now derives the artifact dir from the validated store root + session id, never from the session.json artifact_dir field, so a crafted session.json cannot escape the store root. Live-verified: a session.json with artifact_dir="/tmp/ESCAPE" still writes inside root. Adds a containment regression test. flock.go gains a flock_windows.go stub so cross-platform builds compile. Timeline strings are in English; non-ASCII stripped. No new go.mod deps.
Bumps the minor-updates group with 1 update: [golang.org/x/crypto](https://github.com/golang/crypto). Updates `golang.org/x/crypto` from 0.53.0 to 0.54.0 - [Commits](golang/crypto@v0.53.0...v0.54.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.54.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: minor-updates ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps the actions-updates group with 4 updates: [github/codeql-action/init](https://github.com/github/codeql-action), [github/codeql-action/analyze](https://github.com/github/codeql-action), [golangci/golangci-lint-action](https://github.com/golangci/golangci-lint-action) and [github/codeql-action/upload-sarif](https://github.com/github/codeql-action). Updates `github/codeql-action/init` from 4.36.3 to 4.37.0 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@54f647b...99df26d) Updates `github/codeql-action/analyze` from 4.36.3 to 4.37.0 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@54f647b...99df26d) Updates `golangci/golangci-lint-action` from db9de0fc1a667e1a49d2291a1a042dff081d78f6 to ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a - [Release notes](https://github.com/golangci/golangci-lint-action/releases) - [Commits](golangci/golangci-lint-action@db9de0f...ba0d7d2) Updates `github/codeql-action/upload-sarif` from 4.36.3 to 4.37.0 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@54f647b...99df26d) --- updated-dependencies: - dependency-name: github/codeql-action/init dependency-version: 4.37.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions-updates - dependency-name: github/codeql-action/analyze dependency-version: 4.37.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions-updates - dependency-name: golangci/golangci-lint-action dependency-version: ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a dependency-type: direct:production dependency-group: actions-updates - dependency-name: github/codeql-action/upload-sarif dependency-version: 4.37.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions-updates ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Add the DNS apply/verify vertical to dev: - internal/cpanel/dns_apply.go: mass_edit_zone write primitives (MassEditZoneAdd/Remove/Batch), raw zone fetch, SOA serial extract and the stale serial detector. Runner-typed, transport agnostic. - internal/cpanel/dns_safety_test.go: AST allowlist guard confining the DNS write verbs to the two writer files, hardened with a wrapper call check the string scan alone lacks (the CamelCase MassEditZone* identifiers evade it). - internal/accountinventory: dnsapply/dnsverify report, backup and verify engines. Backup and verify retyped to cpanel.DNSRecord (dev keeps its cpanel.DNSRecord model; a separate DNSRecordEntry refactor is not adopted). - cmd/cpanel-self-migration: dns apply (+ --rollback) and dns verify orchestrators, plus a dns dispatch branch in main.go. Preview by default: dns apply writes nothing without --yes-apply-writes and dns verify is read only. - internal/sshx: a dev-native password-based DialDest (dest only), instead of an auth-based dialer. Safety preserved: preview-default gate, backup-or-nothing, SOA serial optimistic lock, replace preconditions, verify-after, apex as FQDN, 0600 artifacts, rollback refuses empty-old, verify stale-plan gate. Fixed a pre-existing verify-after bug that could resurrect an empty-records op to applied, with a regression test. Non-ASCII and long dashes scrubbed, test PII replaced with reserved example domains and RFC 5737 IPs. No new go.mod deps. build + vet + gofmt + full suite green.
Adds `cron apply` and `cron verify` on the destination account, mirroring the DNS write vertical (Batch D1). Both are preview by default: `cron apply` writes nothing without --yes-apply-writes and `cron verify` is read-only. Design: dev never persists cron clear-text (the inventory keeps only redacted commands for secret hygiene), so `cron apply` re-collects the source crontab fresh at apply time to obtain the clear installable command in memory. The offline plan supplies the reviewed decisions (which entries to create); the clear text is sourced live from the source host and never written to disk. The apply merges the create lines into the destination crontab and installs via the whole-crontab `crontab -` primitive. `cron verify` re-reads the destination and matches each op by its canonical redacted identity, so an entry installed in the clear still matches its redacted plan line and substring collisions cannot produce a false match. Safety contracts: - backup of the destination crontab written 0600 and hashed before the first write (backup or nothing); failure means nothing is written - whole-crontab merge, never a blind replace; `crontab -r` forbidden everywhere - a create whose source entry or schedule drifted since the plan is refused, never installed as a guess or with an unreviewed schedule - source re-collection is read-only; InstallCrontab runs only against the destination, funneled through a string+AST write guard and two allowlisted files - source clear-text is never persisted: the post-install crontab is not written into the report, and the install-failure output is redacted - exit codes: 0 ok or drift, 1 input/dial/write failure, 2 flags, 3 gated refusal (stale, refused precondition, fail-on-drift) Verify cannot value-verify a redacted secret; the report discloses that a clean verdict means the destination is consistent with the plan, not that every secret value matches.
…h D2 Adds `email apply` and `email verify` on the destination account, mirroring the DNS (D1) and cron (D3) write verticals. Both preview by default: `email apply` writes nothing without --yes-apply-writes and `email verify` is read-only. Scope is credential-free by design: the vertical writes only email CONFIG (forwarders, default/catch-all address, autoresponders, filters, mail routing) via UAPI/API2. Mailbox creation and passwords stay in the existing migrate flow; no password verb (add_pop / passwd_pop / password_hash) appears anywhere in the vertical. Adaptation: the three raw inventory types were retyped to dev's pre-adapted Norm* names (NormForwarderEntry / NormAutoresponderEntry / NormEmailFilterEntry), qualifier-aware so cpanel's own read types are untouched; the duplicate deriveMDPath was dropped; the `email` dispatch branch was added to main.go. Safety contracts (verified): preview-default (fully offline), destination-only (the source is never dialed or written), backup-or-nothing 0600 before the first write, per-op freshness guard with a second pre-write re-check for the upsert verbs, never-overwrite of a human's autoresponder/filter (manual at plan time, refused at apply time), unconditional per-op verify-after, and rollback that inverts only the tool's own applied ops with the backup/report pairing enforced (sha256 + account + non-empty-sha-with-applied-ops + report format-version). Reports and backups are 0600; email content (bodies, filter rules) is persisted in clear as the rollback content-match basis (content, not credentials). Guard: a string+AST test funnels the eight email write verbs through the two allowlisted files, including a CamelCase-wrapper AST check (AddForwarder etc.) matching the DNS/cron guards. Autoresponder test bodies were translated from Italian to English (code is English-only); no real customer PII is present (the real-domain realserver fixtures were intentionally left out of scope).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
gosec found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
Too many files changed for review. ( Bypass the limit by tagging |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Important Review skippedToo many files! This PR contains 238 files, which is 188 over the limit of 50. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (238)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Dependency ReviewThe following issues were found:
License Issuesgo.mod
OpenSSF Scorecard
Scanned Files
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Make `golangci-lint run ./...` clean by encoding project conventions in .golangci.yml (v2 schema) and fixing the remaining real findings. Config: - errcheck: exclude fmt.Fprint/Fprintf/Fprintln (writes to log/stderr and strings.Builder sinks are deliberate) and _test.go (deferred Close and cleanup discards are idiomatic; other linters stay active on tests). - staticcheck: restate the six default-disabled ST* checks (the checks list replaces the default, it does not append) and additionally disable the QF1001/QF1002 style quickfixes. - drop issues.new: the revgrep diff failed on the shallow CI checkout (bad revision HEAD~) and made the run nondeterministic; uncap max-issues-per-linter / max-same-issues so the gate sees the whole tree. Code: - errcheck: discard best-effort cleanup/close/unlock returns with _ = in workbench artifacts.go, store.go, flock.go and flock_windows.go, and wrap deferred Close in main.go, runner.go and sshtest.go. - staticcheck: use strings.EqualFold for the case-insensitive domain compare (emailapply) and a boolean return simplification (cronplan). - ineffassign: drop the dead redactedValue seed (cpanel/cron). - unused: remove the dead wrapAPI2 test helper. golangci-lint reports 0 issues on linux, windows and darwin; go build, go vet and the touched-package tests are green.
The account-inventory, plan/verify and event output writers created their output directories with 0o755 (world readable and traversable) while the files themselves are written 0o600. gosec G301 flagged the directories as overly permissive. Create them 0o700 instead, matching the 0600 files and the existing 0700 convention (migrate runner output, workbench store, sshx hostkey). 33 MkdirAll sites across 15 files.
… G304/G302) Resolve the 4 gosec G304 (file inclusion via variable) and 2 G302 findings structurally, with zero #nosec. G304 -> os.Root confinement, the repo's own nosec-free idiom (see sshx.openContained / migrate.createLogFile): - workbench/store.go readSession, flock.go + flock_windows.go lockFile, events/writer.go: open the fixed/validated child through an os.Root scoped to the store root / output dir. - workbench/artifacts.go AttachArtifact: the source is an operator-supplied arbitrary path that may be a symlink, so resolve it with filepath.EvalSymlinks first (preserving os.Open symlink following), then open the resolved file through an os.Root on its parent. Adds TestAttachArtifactFollowsSymlinkSource. G302 -> the os.Chmod(dir, 0700) that gosec misreads as a loose FILE perm is removed; the dirs are already created 0700 via os.MkdirAll and the files inside are 0600. workbench/store.go NewStore and migrate/runner.go createLogFile no longer force-tighten a pre-existing directory (a minor defense-in-depth change; content stays owner-only either way). gosec 0 on linux and the CI-pinned v2.27.1; build linux + windows, go vet, golangci-lint and the touched-package tests all green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated release PR for
v2.3.0.Summary
Add three cPanel write verticals (DNS, cron, email), a read-only account inventory, and a structured-output foundation on top of
v2.2.3.New Features:
apply(+--rollback) andverifycommands, preview by default (no writes without--yes-apply-writes), backup-or-nothing at 0600, verify-after, and optimistic locking (SOA serial / crontab hash / per-op freshness).cpapi2) transport, plus an offline engine (diff, policy, coverage, dns/email/cron plan, checklist, acceptance) writing JSON/Markdown artifacts.--run-id,--output-dir,--json-events,--report-json.migrationCLI.Bug Fixes:
+or%survive the cpsrvd decode.flexInt64accepts fractional and string-valued numeric fields (disk figures, SSL cert numbers) instead of collapsing them to 0.Performance:
Security:
json:"-"); artifacts carry only redacted commands plus SHA256 identity, and the write path re-collects the source fresh at apply time.internal/redactpackage.CI:
golang.org/x/cryptoto 0.54.0 and the GitHub Actions groups.Chores: