SONARSEC-6429: Create rule S5144 Server-side requests should not be vulnerable to forging attacks for Go #5053
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
daniel-teuchert-sonarsource
changed the title
…ction attacks for Go (#5039) * Add go to rule S2091 * Add initial docs * Use regexp for compliant examples * Tidy up --------- Co-authored-by: teemu-rytilahti-sonarsource <teemu-rytilahti-sonarsource@users.noreply.github.com> Co-authored-by: Teemu Rytilahti <teemu.rytilahti@sonarsource.com>
* Add go to rule S5145 * Add description and example for Go standard library * Add example for Fiber --------- Co-authored-by: daniel-teuchert-sonarsource <daniel-teuchert-sonarsource@users.noreply.github.com> Co-authored-by: Daniel Teuchert <daniel.teuchert@sonarsource.com> Co-authored-by: daniel-teuchert-sonarsource <141642369+daniel-teuchert-sonarsource@users.noreply.github.com>
daniel-teuchert-sonarsource
force-pushed
the
rule/S5144-add-go
branch
from
5530906 to
22d273e
Compare
| URL `\https://example.commit.malicious.io`. | ||
| URL `https://example.commit.malicious.io`. |
There was a problem hiding this comment.
I would avoid using nothing else but the IANA-reserved example domains for examples, especially if this domain is not under our control.
| "https://example.com": true, | ||
| "https://example.com/": true, |
There was a problem hiding this comment.
Is it intentional to use the same domain with and without a trailing slash?
I guess that this pattern gets used for reverse proxying in production, so maybe worth considering to show an example with prefix-matching even when it's a bit more complex?
| func Example(c *fiber.Ctx) error { | ||
| url := c.Query("url") | ||
| agent := fiber.Get(url) | ||
| statusCode, body, errs := agent.Bytes() |
There was a problem hiding this comment.
| statusCode, body, errs := agent.Bytes() | |
| statusCode, body, errs := agent.String() |
This would avoid the need to do a string cast below.
|
|
||
| func Example(w http.ResponseWriter, req *http.Request) { | ||
| url := req.FormValue("url") | ||
| resp, err := http.Get(url) // Noncompliant |
There was a problem hiding this comment.
| resp, err := http.Get(url) // Noncompliant | |
| resp, err := http.Get(url) |
|
|
||
| statusCode, body, err := fasthttp.Get(nil, url) | ||
| if err != nil { | ||
| fmt.Fprintf(ctx, "Error when making request: %s\n", err.Error()) |
There was a problem hiding this comment.
The diff view is showing that this line has been changed between the compliant and non compliant versions, which looks off. Any ideas if we can influence that anyhow?
| "https://example.com/": true, | ||
| } | ||
| if _, ok := allowedURLs[url]; !ok { | ||
| fmt.Fprint(ctx, "Specified host is not allowed") |
There was a problem hiding this comment.
| fmt.Fprint(ctx, "Specified host is not allowed") | |
| fmt.Fprint(ctx, "Specified url is not allowed") |
Would be consistent with the allowedUrls naming. Applies also to other examples, so I'm not repeating myself below.
|
|
You can preview this rule here (updated a few minutes after each push).
Review
A dedicated reviewer checked the rule description successfully for: