🐛 fix special character in search query to fix fuzzing check #1241

asraa · 2021-11-11T16:49:42Z

Signed-off-by: Asra Ali [email protected]

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features)
PR title follows the guidelines defined in https://github.com/ossf/scorecard/blob/main/CONTRIBUTING.md#pr-process

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fixes a bug where special character / was causeing a false negative on the fuzzing query that queries for repositories
By github search API docs:

You can't use the following wildcard characters as part of your search query: . , : ; / \ ` ' " = * ! ? # $ & + ^ | ~ < > ( ) { } [ ] @. The search will simply ignore these symbols.

https://docs.github.com/en/search-github/searching-on-github/searching-code#considerations-for-code-search

What is the current behavior? (You can also link to an open issue here)
The "Fuzzing" check doesn't seem to always work correctly #1231
What is the new behavior (if this is a feature change)?
Successfully detects fuzzing for the repos
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:

Signed-off-by: Asra Ali <[email protected]>

asraa · 2021-11-11T16:51:17Z

I don't think any of the special characters are relevant for repository searches, but if you think it makes sense to replace all of those, let me know!

laurentsimon · 2021-11-11T16:54:52Z

clients/githubrepo/search.go

@@ -59,7 +59,7 @@ func (handler *searchHandler) buildQuery(request clients.SearchRequest) (string,
 	}
 	var queryBuilder strings.Builder
 	if _, err := queryBuilder.WriteString(
-		fmt.Sprintf("%s repo:%s/%s", request.Query, handler.owner, handler.repo)); err != nil {
+		fmt.Sprintf("%s repo:%s/%s", strings.ReplaceAll(request.Query, "/", " "), handler.owner, handler.repo)); err != nil {


why was there a / in the query that caused the issue? I thought org and repo did not have if they had been extracted properly from owner/repo?

Ah, for fuzzing we check if the URI is in a project.yaml.

e.g. we search for github.com/util-linux/util-linux in the main_repo field of the YAML https://github.com/google/oss-fuzz/blob/56b840aad7907d69ad0de08a9339ba7e18a03514/projects/util-linux/project.yaml#L17

laurentsimon · 2021-11-11T16:56:04Z

go.mod

@@ -40,71 +38,6 @@ require (
 	gotest.tools v2.2.0+incompatible
 )

-require (
-	cloud.google.com/go v0.94.1 // indirect


is it normal all of the below dependencies are removed?

hmmm somehow that showed up in my go mod tidy.

azeemshaikh38

Thanks for the quick turnaround @asraa !

clients/githubrepo/search.go

azeemshaikh38 · 2021-11-11T16:56:15Z

go.mod

@@ -4,9 +4,7 @@ go 1.17

 require (
 	cloud.google.com/go/bigquery v1.24.0
-	cloud.google.com/go/monitoring v0.1.0 // indirect


Are these go.mod changes expected?

azeemshaikh38 · 2021-11-11T16:58:55Z

I don't think any of the special characters are relevant for repository searches, but if you think it makes sense to replace all of those, let me know!

+1. I think its ok for this PR. But maybe just leave a comment so we know this could be a potential issue.

evverx · 2021-11-11T17:11:36Z

Thanks! I can confirm that now both util-linux and selinux are fuzzed according to scorecard.

(on a somewhat unrelated note It seems the "Fuzzing" check doesn't take disabled projects (where disabled is set to true) like https://github.com/google/oss-fuzz/blob/master/projects/influxdb/project.yaml into account. It has nothing to with this PR though)

Signed-off-by: Asra Ali <[email protected]>

asraa · 2021-11-11T17:23:21Z

(on a somewhat unrelated note It seems the "Fuzzing" check doesn't take disabled projects (where disabled is set to true) like https://github.com/google/oss-fuzz/blob/master/projects/influxdb/project.yaml into account. It has nothing to with this PR though)

Ooh right! Should we open an issue about this? I suppose we could search for the right project.yaml and then check for a disabled line.

evverx · 2021-11-11T18:20:58Z

@asraa I'll try to open a new issue once I figure out why scorecard says that influxdb is fuzzed while libra isn't even though they're both disabled on OSS-Fuzz.

evverx · 2021-11-11T18:28:27Z

Looks like libra triggered another corner case. It was renamed to "diem" but the "project.yaml" file wasn't changed accordingly so scorecard somehow looks for diem/diem instead of libra/libra. Basically it seems scorecard can't handle redirects so if for example karelzak/util-linux is looked for it redirects to util-linux/util-linux and because google/oss-fuzz#6800 was merged it automagically works but if the project.yaml file hadn't been changed scorecard would say that the project isn't fuzzed. I'll try to open another issue then

asraa · 2021-11-15T16:29:39Z

ping? anything else to do here? other issues with the fuzzing check have open issues now

azeemshaikh38 · 2021-11-15T16:36:03Z

ping? anything else to do here? other issues with the fuzzing check have open issues now

Nothing from my side. Looks good to submit. Auto-merge is on.

laurentsimon · 2021-11-15T16:43:19Z

Looks like libra triggered another corner case. It was renamed to "diem" but the "project.yaml" file wasn't changed accordingly so scorecard somehow looks for diem/diem instead of libra/libra. Basically it seems scorecard can't handle redirects so if for example karelzak/util-linux is looked for it redirects to util-linux/util-linux and because google/oss-fuzz#6800 was merged it automagically works but if the project.yaml file hadn't been changed scorecard would say that the project isn't fuzzed. I'll try to open another issue then

yes, please!

laurentsimon · 2021-11-15T16:45:33Z

ping? anything else to do here? other issues with the fuzzing check have open issues now

Nothing from my side. Looks good to submit.

LGTM too. Thanks @asraa !

fix fuzzing path separator

e237c48

Signed-off-by: Asra Ali <[email protected]>

asraa mentioned this pull request Nov 11, 2021

The "Fuzzing" check doesn't seem to always work correctly #1231

Closed

laurentsimon self-requested a review November 11, 2021 16:54

laurentsimon approved these changes Nov 11, 2021

View reviewed changes

azeemshaikh38 approved these changes Nov 11, 2021

View reviewed changes

add comment

e825313

Signed-off-by: Asra Ali <[email protected]>

Merge branch 'main' into fix-fuzzing-bug

2176b76

azeemshaikh38 enabled auto-merge (squash) November 15, 2021 16:36

azeemshaikh38 merged commit 5950fde into ossf:main Nov 15, 2021

evverx mentioned this pull request Feb 12, 2022

projects: Add OpenSSF scorecard google/oss-fuzz#7268

Merged

evverx mentioned this pull request Feb 22, 2023

Add Git repo URL to build logs google/oss-fuzz#9782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 fix special character in search query to fix fuzzing check #1241

🐛 fix special character in search query to fix fuzzing check #1241

asraa commented Nov 11, 2021 •

edited

Loading

asraa commented Nov 11, 2021

laurentsimon Nov 11, 2021

asraa Nov 11, 2021

laurentsimon Nov 11, 2021

asraa Nov 11, 2021

azeemshaikh38 left a comment

azeemshaikh38 Nov 11, 2021

azeemshaikh38 commented Nov 11, 2021

evverx commented Nov 11, 2021

asraa commented Nov 11, 2021

evverx commented Nov 11, 2021

evverx commented Nov 11, 2021

asraa commented Nov 15, 2021

azeemshaikh38 commented Nov 15, 2021 •

edited by laurentsimon

Loading

laurentsimon commented Nov 15, 2021

laurentsimon commented Nov 15, 2021

🐛 fix special character in search query to fix fuzzing check #1241

🐛 fix special character in search query to fix fuzzing check #1241

Conversation

asraa commented Nov 11, 2021 • edited Loading

asraa commented Nov 11, 2021

laurentsimon Nov 11, 2021

Choose a reason for hiding this comment

asraa Nov 11, 2021

Choose a reason for hiding this comment

laurentsimon Nov 11, 2021

Choose a reason for hiding this comment

asraa Nov 11, 2021

Choose a reason for hiding this comment

azeemshaikh38 left a comment

Choose a reason for hiding this comment

azeemshaikh38 Nov 11, 2021

Choose a reason for hiding this comment

azeemshaikh38 commented Nov 11, 2021

evverx commented Nov 11, 2021

asraa commented Nov 11, 2021

evverx commented Nov 11, 2021

evverx commented Nov 11, 2021

asraa commented Nov 15, 2021

azeemshaikh38 commented Nov 15, 2021 • edited by laurentsimon Loading

laurentsimon commented Nov 15, 2021

laurentsimon commented Nov 15, 2021

asraa commented Nov 11, 2021 •

edited

Loading

azeemshaikh38 commented Nov 15, 2021 •

edited by laurentsimon

Loading