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.

Sign up for GitHub

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

Jump to bottom

Fix semgrep dgryski.semgrep-go issues #3511

Merged
merged 4 commits into from
Mar 25, 2024
Merged

Fix semgrep dgryski.semgrep-go issues #3511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
de9a3b8
fix semgrep dgryski.semgrep-go issues
dmitris Feb 15, 2024
12ed1a0
add comment about legacy text/plain content type
dmitris Mar 13, 2024
92602d0
fix semgrep dgryski issue with w.Write, add nosemgrep
dmitris Mar 13, 2024
9cc55f4
add nosemgrep ignore for marshal-json-pointer-receiver
dmitris Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions endpoints/events/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (e *eventEndpoint) Handle(w http.ResponseWriter, r *http.Request, _ httprou
w.WriteHeader(http.StatusBadRequest)

for _, err := range errs {
w.Write([]byte(fmt.Sprintf("invalid request: %s\n", err.Error())))
fmt.Fprintf(w, "invalid request: %s\n", err.Error())
}

return
Expand All @@ -81,7 +81,7 @@ func (e *eventEndpoint) Handle(w http.ResponseWriter, r *http.Request, _ httprou

if err != nil {
w.WriteHeader(http.StatusUnauthorized)
w.Write([]byte(fmt.Sprintf("Account '%s' is required query parameter and can't be empty", AccountIdParameter)))
fmt.Fprintf(w, "Account '%s' is required query parameter and can't be empty", AccountIdParameter)
return
}
eventRequest.AccountID = accountId
Expand All @@ -105,15 +105,15 @@ func (e *eventEndpoint) Handle(w http.ResponseWriter, r *http.Request, _ httprou
w.WriteHeader(status)

for _, message := range messages {
w.Write([]byte(fmt.Sprintf("Invalid request: %s\n", message)))
fmt.Fprintf(w, "Invalid request: %s\n", message)
}
return
}

// Check if events are enabled for the account
if !account.Events.Enabled {
w.WriteHeader(http.StatusUnauthorized)
w.Write([]byte(fmt.Sprintf("Account '%s' doesn't support events", eventRequest.AccountID)))
fmt.Fprintf(w, "Account '%s' doesn't support events", eventRequest.AccountID)
return
}

Expand Down
12 changes: 6 additions & 6 deletions endpoints/events/vtrack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,15 @@ func (v *vtrackEndpoint) Handle(w http.ResponseWriter, r *http.Request, _ httpro
// account id is required
if accountId == "" {
w.WriteHeader(http.StatusBadRequest)
w.Write([]byte(fmt.Sprintf("Account '%s' is required query parameter and can't be empty", AccountParameter)))
fmt.Fprintf(w, "Account '%s' is required query parameter and can't be empty", AccountParameter)
return
}

// get integration value from request parameter
integrationType, err := getIntegrationType(r)
if err != nil {
w.WriteHeader(http.StatusBadRequest)
w.Write([]byte(fmt.Sprintf("Invalid integration type: %s\n", err.Error())))
fmt.Fprintf(w, "Invalid integration type: %s\n", err.Error())
return
}

Expand All @@ -92,7 +92,7 @@ func (v *vtrackEndpoint) Handle(w http.ResponseWriter, r *http.Request, _ httpro
// check if there was any error while parsing puts request
if err != nil {
w.WriteHeader(http.StatusBadRequest)
w.Write([]byte(fmt.Sprintf("Invalid request: %s\n", err.Error())))
fmt.Fprintf(w, "Invalid request: %s\n", err.Error())
return
}

Expand All @@ -106,7 +106,7 @@ func (v *vtrackEndpoint) Handle(w http.ResponseWriter, r *http.Request, _ httpro
w.WriteHeader(status)

for _, message := range messages {
w.Write([]byte(fmt.Sprintf("Invalid request: %s\n", message)))
fmt.Fprintf(w, "Invalid request: %s\n", message)
}
return
}
Expand All @@ -118,7 +118,7 @@ func (v *vtrackEndpoint) Handle(w http.ResponseWriter, r *http.Request, _ httpro
if len(errs) > 0 {
w.WriteHeader(http.StatusInternalServerError)
for _, err := range errs {
w.Write([]byte(fmt.Sprintf("Error(s) updating vast: %s\n", err.Error())))
fmt.Fprintf(w, "Error(s) updating vast: %s\n", err.Error())

return
}
Expand All @@ -128,7 +128,7 @@ func (v *vtrackEndpoint) Handle(w http.ResponseWriter, r *http.Request, _ httpro

if err != nil {
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(fmt.Sprintf("Error serializing pbs cache response: %s\n", err.Error())))
fmt.Fprintf(w, "Error serializing pbs cache response: %s\n", err.Error())

return
}
Expand Down
13 changes: 9 additions & 4 deletions endpoints/openrtb2/amp_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
if errortypes.ContainsFatalError(errL) {
w.WriteHeader(http.StatusBadRequest)
for _, err := range errortypes.FatalOnly(errL) {
w.Write([]byte(fmt.Sprintf("Invalid request: %s\n", err.Error())))
fmt.Fprintf(w, "Invalid request: %s\n", err.Error())
}
labels.RequestStatus = metrics.RequestStatusBadInput
return
Expand Down Expand Up @@ -224,7 +224,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
w.WriteHeader(httpStatus)
labels.RequestStatus = metricsStatus
for _, err := range errortypes.FatalOnly(errL) {
w.Write([]byte(fmt.Sprintf("Invalid request: %s\n", err.Error())))
fmt.Fprintf(w, "Invalid request: %s\n", err.Error())
}
ao.Errors = append(ao.Errors, acctIDErrs...)
return
Expand All @@ -237,7 +237,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
if errortypes.ContainsFatalError(errs) {
w.WriteHeader(http.StatusBadRequest)
for _, err := range errortypes.FatalOnly(errs) {
w.Write([]byte(fmt.Sprintf("Invalid request: %s\n", err.Error())))
fmt.Fprintf(w, "Invalid request: %s\n", err.Error())
}
labels.RequestStatus = metrics.RequestStatusBadInput
return
Expand Down Expand Up @@ -398,8 +398,13 @@ func sendAmpResponse(
ao.AmpTargetingValues = targets

// Fixes #231
enc := json.NewEncoder(w)
enc := json.NewEncoder(w) // nosemgrep: json-encoder-needs-type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we ignoring this entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

enc.SetEscapeHTML(false)
// 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.
w.Header().Set("Content-Type", "text/plain; charset=utf-8")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to add this header set?

Copy link
Contributor Author

@dmitris dmitris Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@bsardo bsardo Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"


// If an error happens when encoding the response, there isn't much we can do.
// If we've sent _any_ bytes, then Go would have sent the 200 status code first.
Expand Down
2 changes: 1 addition & 1 deletion endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2409,7 +2409,7 @@ func writeError(errs []error, w http.ResponseWriter, labels *metrics.Labels) boo
w.WriteHeader(httpStatus)
labels.RequestStatus = metricsStatus
for _, err := range errs {
w.Write([]byte(fmt.Sprintf("Invalid request: %s\n", err.Error())))
fmt.Fprintf(w, "Invalid request: %s\n", err.Error())
}
rc = true
}
Expand Down
2 changes: 1 addition & 1 deletion usersync/cookie.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ type cookieJson struct {
OptOut bool `json:"optout,omitempty"`
}

func (cookie *Cookie) MarshalJSON() ([]byte, error) {
func (cookie *Cookie) MarshalJSON() ([]byte, error) { // nosemgrep: marshal-json-pointer-receiver
return jsonutil.Marshal(cookieJson{
UIDs: cookie.uids,
OptOut: cookie.optOut,
Expand Down
Loading