Security & reliability hardening: SSRF, atomic writes, bounded URL replace, config caps#13
Merged
Conversation
Add command-level integration and fuzz coverage across parser flows. Fix config-driven structured output when no links are found or all links are ignored. Fix redirect auto-fixes for duplicate URLs across files and make filter tracking race-safe.
Refactor check and fix into injected runners, expand command and UI tests, add end-to-end benchmarks and fixtures, and adopt pond/conc in checker and parser hot paths.
Route interactive through an injected runner, pass configured checker options into the UI path, and clear the repository lint backlog while keeping tests and integration coverage green.
…fixer Scanner now skips symlinks during WalkDir (via d.Type()&os.ModeSymlink) to prevent following links that point outside the user-specified scan root. Fixer now refuses to rewrite a path whose Lstat reports a symlink, preventing an attacker-controlled symlink from redirecting writes onto arbitrary files (e.g. ~/.ssh/authorized_keys) when the user runs 'gone fix' over a directory they don't fully control. Tests cover both behaviours, with Unix-only skips.
…ta ranges
Default-deny outbound TCP to dangerous CIDRs (loopback 127.0.0.0/8 &
::1, RFC1918, link-local incl. 169.254.169.254 IMDS, IPv6 unique-local
fc00::/7, multicast, unspecified). Defense in depth:
1. validateURL rejects http/https URLs whose literal host resolves
into a blocked range and rejects non-http(s) schemes.
2. safeDialContext wraps the underlying net.Dialer.DialContext so
hostnames whose DNS resolution lands in a blocked range are
refused at TCP-dial time. This catches DNS rebinding because the
check happens after resolution, on each connection.
3. followRedirectChain re-validates every hop, so a public URL that
302s to http://169.254.169.254/latest/meta-data is blocked.
Users with legitimate intranet docs can opt in via --allow-private-hosts
or output.allow_private_hosts: true in .gonerc.yaml. Test suites that
hit httptest.NewServer (127.0.0.1) explicitly opt in via
WithAllowPrivateHosts(true) so the production default stays default-deny.
Adds internal/checker/safety_test.go with table-driven coverage:
blocked-IP set, scheme rejection, URL validator, safe dial wrapper
covering blocked literal IPs and DNS that resolves into private space,
and end-to-end redirect-chain refusal.
ApplyToFile previously called strings.ReplaceAll(content, OldURL, NewURL), which would corrupt any URL in the file that contained OldURL as a prefix. Concrete example: a file with both [a](https://x.com/a) and [b](https://x.com/abc) being fixed for https://x.com/a -> https://y.com/a would silently rewrite the second link to https://y.com/abc. Replace with replaceBoundedURL: each candidate match is accepted only when the bytes immediately before and after are not RFC 3986 path/query/fragment continuation chars. The continuation set is deliberately narrow ('(', ')', '[', ']', ',', ';', '\'' are excluded because they act as terminators in markdown / JSON / YAML / HTML even though RFC 3986 reserves them). Adds table-driven coverage of 28 boundary scenarios (alnum, path, query, fragment, percent encoding, quotes, brackets, newlines, tabs, unicode, preceding-char prefix attacks) plus three end-to-end ApplyToFile tests exercising the longer-URL non-corruption guarantee.
A crash, SIGKILL, OOM, or disk-full during 'gone fix' previously left
user files truncated or half-rewritten — the worst possible outcome
for a tool whose explicit job is to edit other people's source files.
The same was true of --output report files.
Introduce internal/atomicfile.WriteFile:
1. CreateTemp in the same directory (rename is only atomic within
one filesystem)
2. Write + fsync + close the temp file
3. chmod to the requested perm
4. Rename over the destination
If any step fails the temp file is removed; the destination is never
touched until the rename succeeds.
Apply at the two production write sites: internal/fixer/fixer.go and
internal/output/output.go.
Test coverage: 12 cases covering new file, overwrite, perm bits, empty
payload, leftover-temp leak detection, empty-path rejection, missing
directory cleanup, concurrent distinct paths, concurrent same path
(all-or-nothing), 1 MiB payload, and a reader-during-overwrite race
ensuring readers always observe full content (skipped on Windows where
rename-over-open differs).
…etries
Validate() previously rejected only negative values, so a typo or
malicious .gonerc.yaml of:
check:
concurrency: 100000
timeout: 99999
retries: 999
would spawn 100k HTTP workers, hang a CI job for ~28 hours per URL,
and retry each failure a thousand times. Add hard caps:
- MaxConcurrency = 1024 (well above any legitimate parallelism)
- MaxTimeout = 300s (5 min upper bound per request)
- MaxRetries = 10
Validation now rejects anything above the caps with a clear error that
names both the field and the limit. Adds nine new sub-tests:
ConcurrencyAboveMax / AtMaxOK, TimeoutAboveMax / AtMaxOK,
RetriesAboveMax / AtMaxOK, HugeConcurrencyRejected,
HugeTimeoutRejected, and an all-at-max combo.
FindAndLoad previously walked from startDir all the way to the filesystem root looking for a .gonerc.yaml. Running 'gone' inside an unrelated subdirectory of a shared or world-writable workspace (/tmp/..., a co-located project, etc.) could silently pick up config that altered behavior — different ignore rules, looser allow_private_hosts, redirected output formats — without any signal to the user. Require an explicit stopAt boundary. The walk includes stopAt and stops there; an empty stopAt collapses to startDir (no walk at all). If stopAt isn't a genuine ancestor of startDir (e.g. a prefix collision like /tmp/foo vs /tmp/foobar, or an unrelated sibling directory) the search is restricted to startDir — never silently redirected elsewhere. Six new sub-tests cover: StopAtBoundsUpwardWalk, EmptyStopAtRestrictsToStartDir, StopAtUnrelatedDirIsRestrictiveNotEscape, StopAtPrefixCollisionRejected, StopAtEqualsStartDirNoWalk, and a direct table-driven test of isAncestorOrEqual including the /a vs /ab prefix-collision case.
leonardomso
force-pushed
the
leonardomso/lyon-v1
branch
from
23db942 to
c7cce3e
Compare
Bump module Go directive to 1.26.3 and the CI workflow toolchains to match. Refresh all direct and indirect dependencies to their latest patch/minor releases via `go get -u ./...` + `go mod tidy`.
Rename replaceBoundedURL return values so gocritic stops flagging unnamed multi-return results, and run gofmt on safety.go and fixer.go. No behavior change.
Add Checker.Close which calls http.Client.CloseIdleConnections so the underlying transport's readLoop and writeLoop goroutines exit once a test finishes. Register t.Cleanup(checker.Close) on every test site so goleak.VerifyTestMain no longer fails on leaked net/http persistConn goroutines. Also break a few long fluent-builder lines so revive's line-length-limit (120) stops tripping.
# Conflicts: # cmd/check_runner.go # cmd/e2e_bench_test.go # cmd/fix_runner.go # cmd/helpers_test.go # cmd/integration_test.go # cmd/interactive_runner.go # go.mod # go.sum # internal/fixer/fixer.go # internal/scanner/scanner_test.go
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
1 participant
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.
Summary
Hardens the link-checker against a set of security and reliability issues
found during a Senior Go Engineer / Security Reviewer style pass. Each
finding lands as its own commit with comprehensive table-driven tests.
Security
RFC1918, link-local (incl. 169.254.169.254 IMDS), IPv6 unique-local,
multicast, unspecified. Two layers: a URL-literal validator and a
TCP-dial-time check that catches DNS rebinding. Redirects are
re-validated per hop. Opt-in via
--allow-private-hosts/output.allow_private_hostsfor legitimate intranet docs.WalkDir; fixer refuses to rewrite any path whoseLstatreports asymlink. Prevents attacker-controlled links from redirecting writes
(e.g. into
~/.ssh/authorized_keys).FindAndLoad(23db942). RequiredstopAtargumentbounds the upward
.gonerc.yamlwalk. Guards against silentpickup of configs from
/tmpor shared parent dirs. Ancestor checkdefeats prefix collisions like
/tmp/foovs/tmp/foobar.Reliability
internal/atomicfiledoes temp +fsync + rename. Used by
internal/fixerandinternal/output. Acrash mid-
gone fixcan no longer leave a user's source filetruncated or half-rewritten.
ApplyToFilepreviouslyused
strings.ReplaceAll, silently corrupting any URL that hadthe fixed URL as a prefix. New
replaceBoundedURLonly matches whenboth sides are non-URL-continuation characters. 28 boundary
scenarios covered.
check.concurrency ≤ 1024,check.timeout ≤ 300s,check.retries ≤ 10. Prevents a typo fromspawning 100k workers or hanging CI for a day.
Earlier branch work (pre-hardening)
7eed113ci: fix bounded fuzz workflow2d369edfix(cmd): align interactive mode and clean lint6f05278refactor(cmd): improve reliability and testability7c93ad9fix(cmd): harden cli test coverage pathsTest plan
go build ./...cleango test ./...— 1102 passed across 18 packagesinternal/atomicfile— 12 tests includingconcurrent same-path "all-or-nothing" and 1 MiB payload
internal/checkerSSRF coverage (safety_test.go): table-drivenblocked-IP set, scheme rejection, URL validator, safe-dial wrapper
for blocked literal IPs and DNS that resolves into private space,
end-to-end redirect-chain refusal
internal/fixerURL-boundary table — 28 cases incl. unicode,tab, percent-encoding, preceding-char prefix attacks
internal/config— new bounds tests plus 6 stopAt boundary testsgone checkagainst a repo with a publicURL that 302s to
http://169.254.169.254/...and confirm it'sreported as blocked
🤖 Generated with Claude Code