Skip to content

refactor(mesi): split fetchUrl.go into fetch.go + ssrf.go#156

Merged
s2x merged 2 commits into
mainfrom
refactor/split-fetch-url
May 6, 2026
Merged

refactor(mesi): split fetchUrl.go into fetch.go + ssrf.go#156
s2x merged 2 commits into
mainfrom
refactor/split-fetch-url

Conversation

@s2x
Copy link
Copy Markdown
Contributor

@s2x s2x commented May 5, 2026

Summary

  • Split mesi/fetchUrl.go into mesi/ssrf.go (SSRF validation) and mesi/fetch.go (HTTP fetching)
  • Split mesi/fetchUrl_test.go into mesi/ssrf_test.go and mesi/fetch_test.go
  • No public API changes
  • All 285 tests pass, go vet clean

Closes #120

Copy link
Copy Markdown
Contributor Author

@s2x s2x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor findings:

1. Removed inline comments in singleFetchUrlWithContext

A few explanatory comments were stripped from the function body that documented non-obvious decisions:

  • Line ~74: // Allowed host with private-IP bypass opt-in - use standard client without SSRF protection. — explains why AllowPrivateIPsForAllowedHosts skips dial-time IP blocking
  • Line ~82: // Note: When HTTPClient is provided, callers are responsible for setting appropriate timeouts and SSRF protection on the client. Use NewSSRFSafeTransport(config) to create a transport with dial-time private IP blocking. — important gotcha for external users
  • Line ~126: // Use LimitReader to cap response size — explains the +1 buffer on MaxResponseSize
  • Line ~134: // No limit - backward compatibility — explains the zero-limit default behavior

Consider restoring these — they help future contributors understand the branching logic at a glance.

2. Redundant error wrapping at fetch.go:68

if err := isURLSafe(requestedURL, config); err != nil {
    return "", false, fmt.Errorf("%w: %s", ErrSSRFBlocked, err.Error())
}

isURLSafe already wraps its errors with ErrSSRFBlocked (see ssrf.go:49), so this creates ErrSSRFBlocked: ErrSSRFBlocked: ... chains. Harmless for errors.Is, but makes error messages unnecessarily verbose. Worth a follow-up cleanup — not blocking.


Otherwise the split looks clean, all tests pass, no API changes. LGTM.

@s2x s2x force-pushed the refactor/split-fetch-url branch from 071b73f to add90c0 Compare May 6, 2026 17:34
@s2x s2x merged commit 7f176d1 into main May 6, 2026
4 checks passed
@s2x s2x deleted the refactor/split-fetch-url branch May 6, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[mesi] Split fetchUrl.go into fetch.go + ssrf.go (rename to snake_case)

1 participant