Fix semgrep dgryski.semgrep-go issues

[dmitris]

Fix most of the semgrep issues with the
http://semgrep.dev/r/dgryski.semgrep-go ruleset
(semgrep --config http://semgrep.dev/r/dgryski.semgrep-go). Left the issue with Content-Type text/plain on json.Encode in endpoints/openrtb2/amp_auction.go since changing to application/json breaks the AMP unit tests, and issues with the pointer receiver for MarshalJSON in usersync/cookie.go.

Fix #3509.

[SyntaxNode]

Why is it necessary to add this header set?

[dmitris]

it is done anyway automatically - added it explicitly to make clear that the text encoding is intended (I assume due to the AMP requirements) despite the JSON Encoder used . Would you prefer to remove it? @SyntaxNode

[bsardo]

@SyntaxNode and I discussed offline. You're correct in that text/plain is what PBS-Go returns. We're not sure why that is as it predates us and I can't find any documentation on the subject. I did notice that application/json is returned by PBS-Java in the 200 success case, which is probably what the content type should be. However, I suggest we keep the behavior as text/plain since it has been this way for years without complaint and keep this line of code but add a comment above it that is something like:

"Explicitly set content type to text/plain, which had previously been the implied behavior from the time the project was launched. It's unclear why text/plain was chosen or if it was an oversight, nevertheless we will keep it as such for compatibility reasons"

[SyntaxNode]

@dmitris We discussed the content-type change. Please review the comment from @bsardo. We're recommending a comment to provide additional context and are good with the change.

[dmitris]

@dmitris We discussed the content-type change. Please review the comment from @bsardo. We're recommending a comment to provide additional context and are good with the change.

done in a3c9e36, thanks

[guscarreon]

Changes in usersync/encoder.go and config/usersync.go do not seem to be listed in the prebid-semgrep-dgryski.txt. What's the reason behind the EncoderSigner interface? Why do we need HMACSignature config field?

[dmitris]

Changes in usersync/encoder.go and config/usersync.go do not seem to be listed in the prebid-semgrep-dgryski.txt. What's the reason behind the EncoderSigner interface? Why do we need HMACSignature config field?

@guscarreon I'm sorry - confused my branches and inadvertently added unrelated changes 🤦, thanks for catching! Removed it now and rebased, the usersync files are unchanged.

The only remaining dgryski issue is on this line - https://github.com/prebid/prebid-server/blob/master/usersync/cookie.go#L228:

    usersync/cookie.go
   ❯❯❱ Users.dmitris.gh.dgryski.semgrep-go.marshal-json-pointer-receiver
          MarshalJSON with a pointer receiver has surprising results:
          https://github.com/golang/go/issues/22967                  
                                                                     
          228┆ func (cookie *Cookie) MarshalJSON() ([]byte, error) {

[guscarreon]

I'm sorry

Don't be sorry @dmitris. Thank you for fixing this. Left a minor nitpick below

[guscarreon]

Why are we ignoring this entry?

[dmitris]

without the nosemgrep ignore, semgrep run produces the following finding:

    endpoints/openrtb2/amp_auction.go
   ❯❯❱ Users.dmitris.gh.dgryski.semgrep-go.json-encoder-needs-type
          calling json.Encode() on an http.ResponseWriter will set Content-
          Type text/plain

          401┆ enc := json.NewEncoder(w)
          402┆ enc.SetEscapeHTML(false)
          403┆ // Explicitly set content type to text/plain, which had
               previously been
          404┆ // the implied behavior from the time the project was
               launched.
          405┆ // It's unclear why text/plain was chosen or if it was an
               oversight,
          406┆ // nevertheless we will keep it as such for compatibility
               reasons.
          407┆ w.Header().Set("Content-Type", "text/plain;
               charset=utf-8")
          408┆

actually there is another similar one in usersync/cookie.go:

    usersync/cookie.go
   ❯❯❱ Users.dmitris.gh.dgryski.semgrep-go.marshal-json-pointer-receiver
          MarshalJSON with a pointer receiver has surprising results:
          https://github.com/golang/go/issues/22967

          228┆ func (cookie *Cookie) MarshalJSON() ([]byte, error) {

Do we want or can we change the pointer receiver to by-value Cookie? This is part of the public (exported) interface of the package. I think for this PR we should either add the // nosemgrep skip to both cases, or remove the nosemgrep and leave it with the flagged findings. I've done the former for now - 9cc55f4 adding another nosemgrep to get a clean semgrep run (and for the new issues to "stick out" if they appear), but if you prefer, I can remove both // nosemgrep comments.

[guscarreon]

Interesting. I'm ok with leaving the nosemgrep comments for now.

[guscarreon]

LGTM. Thanks for addressing my feedback