Add nosprintfhostport linter

[stbenjam]

Repo:
https://github.com/stbenjam/no-sprintf-host-port

Description:
no-sprintf-host-port checks that Sprintf is not used to construct a host:port
combination in a URL that possibly contains an IPv6 address.

Details:
A frequent pattern is for a developer to construct a URL like this:

fmt.Sprintf("http://%s:%d/foo", host, port)

However, if "host" is an IPv6 address like 2001:4860:4860::8888, the URL
constructed will be invalid. IPv6 addresses must be bracketed, like
this:

http://[2001:4860:4860::8888]:9443

The linter really only looks for the most obvious cases, but where it's
possible to infer that a URL is being constructed with Sprintf containing
a :, this informs the user to use net.JoinHostPort instead.

Running it against some real world code bases like OpenShift and
Kubernetes has found a number of cases that would break in IPv6
environments.

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description about the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must have a valid license and the file must contain the required information by the license, ex: author, year, etc.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic(), log.fatal(), os.exit(), or similar.
• It must not have false positives/negatives. (the team will help to verify that)
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.example.yml

• The linter must be added to the list of available linters (alphabetical case-insensitive order).
  • enable and disable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.example.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
• The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.

Recommendations

• The linter should not use SSA. (currently, SSA does not support generics)
• The linter repository should have a CI, tests, a readme and linting.
• The linter should be published as a binary. (useful to diagnose bug origins)

---

The golangci-lint team will edit this comment to check the boxes before and during the review.

This checklist does not imply that we will accept the linter.

[Antonboom]

@stbenjam, is it necessary to have the prefix go-?
(yes, I saw goprintffuncname).

I think nosprintfhostport is more suitable.

And it looks like a more general problem nosprintf, for example

• don't use fmt.Sprintf to build the URL and use url.URL instead.
• don't use errors.New(fmt.Sprintf("...")) and use fmt.Errorf("...") instead.
• ...

What do you think?

[stbenjam]

@stbenjam, is it necessary to have the prefix go-? (yes, I saw goprintffuncname).

No, I can fix that. nosprintfhostport then? I can also do nosprintf if we can come up with some more general cases. For the two you gave:

don't use fmt.Sprintf to build the URL and use url.URL instead.

Do you have an example of what this would look like? I think this turns any quick URL construction into many lines. Not sure I would want that forced on me -- the problem (for me anyway) is pretty limited to using Sprintf to construct the host:port portion of a URL only, but otherwise I think Sprintf is fine.

don't use errors.New(fmt.Sprintf("...")) and use fmt.Errorf("...") instead.

Looks like gosimple already checks that:

foo.go:9:14: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...)) (gosimple)

[stbenjam]

Hi, I think I've addressed what I can from the checklist and comments. Could you have another look? Thank you 🙏

[ldez]

I tested the linter with several large code-base, and I haven't seen false positives or false negatives.
I feel that this linter can have this problem, but as I don't have any proof of that, I will approve.

[stbenjam]

@ldez Could you look again? I had to rebase as lintersdb got a new entry that caused a conflict.