Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reporting/github: incorrect base-url errors are silently ignored #1180

Closed
jimen0 opened this issue Oct 24, 2021 · 2 comments · Fixed by #1183
Closed

reporting/github: incorrect base-url errors are silently ignored #1180

jimen0 opened this issue Oct 24, 2021 · 2 comments · Fixed by #1183
Labels
Status: Completed Nothing further to be done with this issue. Awaiting to be closed. Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors.

Comments

@jimen0
Copy link
Contributor

jimen0 commented Oct 24, 2021

Nuclei version:

Current HEAD efbef30

Current Behavior:

The GitHub client Nuclei uses expects BaseURL.Path path to end with /. Source for the requirement is: https://github.com/google/go-github/blob/14b70d536ebe676e3751459da075b664768b053f/github/github.go#L368-L370

func (c *Client) NewRequest(method, urlStr string, body interface{}) (*http.Request, error) {
	if !strings.HasSuffix(c.BaseURL.Path, "/") {
		return nil, fmt.Errorf("BaseURL must have a trailing slash, but %q does not", c.BaseURL)
	}
        ...

However, the only validation of base-url YAML nuclei does is calling url.Parse against it (see here), which accepts URLs such as http://127.0.0.1:4444 and ends having .Path attribute set to zero value (empty string) rather than /. See here for a raw reproducer https://play.golang.org/p/Y210j3pqMfZ

github:
  base-url: http://something.internal.corp:8444
  username: "autobot"
  owner: "[SNIPPED]"
  token: "[SNIPPED]"
  project-name: "nuclei-findings"
  issue-label: "triage"

Expected Behavior:

Nuclei to fix the URL for me as long as it's a valid one or to report an error after parsing my reporting.yaml file.

Source of the issue:

parsed, err := url.Parse(options.BaseURL)
if err != nil {
return nil, errors.Wrap(err, "could not parse custom baseurl")
}
client.BaseURL = parsed

Fix: append the trailing / to base-url if not present.

diff --git a/v2/pkg/reporting/trackers/github/github.go b/v2/pkg/reporting/trackers/github/github.go
index 672f62ed..720e269f 100644
--- a/v2/pkg/reporting/trackers/github/github.go
+++ b/v2/pkg/reporting/trackers/github/github.go
@@ -58,6 +58,11 @@ func New(options *Options) (*Integration, error) {
 		if err != nil {
 			return nil, errors.Wrap(err, "could not parse custom baseurl")
 		}
+
+		if !strings.HasSuffix(parsed.Path, "/") {
+			parsed.Path += "/"
+		}
+
 		client.BaseURL = parsed
 	}
 	return &Integration{client: client, options: options}, nil

Steps To Reproduce:

  1. Run nuclei with enabled GitHub reporting module and custom base-url with it being a valid URL without a path and not ending with slash. You don't need a custom GitHub instance for testing this. Just compare base-url: https://api.github.com results vs base-url: https://api.github.com/.
  2. See how no issue is created while the error is silently ignored when using the non-slash-ending option.

Anything else:

Happy to submit a CL with the proposed patch.

👋🏻 😃

@jimen0 jimen0 added the Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors. label Oct 24, 2021
@ehsandeep ehsandeep linked a pull request Oct 25, 2021 that will close this issue
4 tasks
@ehsandeep
Copy link
Member

@jimen0 Thank you for opening this GH issue and proposing a solution; it is now is implemented here - #1183, could you kindly confirm this?

@jimen0
Copy link
Contributor Author

jimen0 commented Oct 25, 2021

Closing this since team merged the fix. Thanks @ehsandeep.

@jimen0 jimen0 closed this as completed Oct 25, 2021
@ehsandeep ehsandeep added the Status: Completed Nothing further to be done with this issue. Awaiting to be closed. label Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Completed Nothing further to be done with this issue. Awaiting to be closed. Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants