Semgrep rules for adapters #2833

onkarvhanumante · 2023-06-09T07:40:55Z

PR introduces semgrep rules for adapters code. These rules check whether,
- response status codes are checked using utils
- bid value in typeBid is assigned correctly
- adapter.bidder interface is named correctly
PR also adds unit tests for each semgrep rule. Note that semgrep looks for tests based on the rule filename and the languages specified in the rule. For example, we have rule for no-content-status.yml check. For this rule we have added no-content-status.go. This file includes patterns to be flagged by semgrep and patterns not to be flagged by semgrep.
Note that this is initial step of integrating semgrep with adapter code reviews. As next step plan is to,
- Run rules for existing adapters and fix them.
- Introduce GitHub action script that scans PR changes, run semgrep tests and add semgrep result as PR comment. Refer https://github.com/onkarvhanumante/prebid-server/pull/31. Therefore message field in each semgrep rule is written using markup language used for Github comment.
Run semgrep unit tests

~/prebid-repos/prebid-server (add-semgrep-rules*) » semgrep --test  /Users/oh0387/prebid-repos/prebid-server/.semgrep/adapter/                                                                                                                                                  
4/4: ✓ All tests passed
No tests for fixes found.

Use following command to run semgrep tests against adapter code base

~/prebid-repos/prebid-server (add-semgrep-rules) » semgrep --gitlab-sast --config=.semgrep/adapter/ ./adapters/ | jq '[.vulnerabilities[] | {"file": .location.file, "start": .location.start_line, "end": .location.end_line, "message": (.message | gsub("\\n"; "\n"))}]' | jq -c


┌─────────────┐
│ Scan Status │
└─────────────┘
  Scanning 2492 files tracked by git with 4 Code rules:
  Scanning 191 files with 4 go rules.
  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00


┌──────────────┐
│ Scan Summary │
└──────────────┘
Some files were skipped or only partially analyzed.
  Scan was limited to files tracked by git.
  Scan skipped: 320 files matching .semgrepignore patterns
  For a full list of skipped files, run semgrep with the --verbose flag.

Ran 4 rules on 191 files: 477 findings.

A new version of Semgrep is available. See https://semgrep.dev/docs/upgrading
[{"file":"adapters/33across/33across.go","start":282,"end":282,"message":"Scope of `TtxAdapter` is limited to this adapter package. Therefore `TtxAdapter` can be renamed to `adapter`. Refer following example.\n```\n  type adapter struct {\n    endpoint string\n  }\n  func  Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) {\n    return &adapter{endpoint: \"https://www.foo.com\"}, nil\n  }\n```\n"},{"file":"adapters/acuityads/acuityads.go","start":28,"end":28,"message":"Scope of `AcuityAdsAdapter` is limited to this adapter package. Therefore `AcuityAdsAdapter` can be renamed to `adapter`. Refer following example.\n```\n  type adapter struct {\n    endpoint string\n  }\n  func  Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) {\n    return &adapter{endpoint: \"https://www.foo.com\"}, nil\n  }\n```\n"},{"file":"adapters/adgeneration/adgeneration.go","start":288,"end":288,"message":"Scope of `AdgenerationAdapter` is limited to this adapter package. Therefore `AdgenerationAdapter` can be renamed to `adapter`. Refer following example.\n```\n  type adapter struct {\n    endpoint string\n  }\n  func  Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) {\n    return &adapter{endpoint: \"https://www.foo.com\"}, nil\n  }\n```\n"},{"file":"adapters/adhese/adhese.go","start":278,"end":278,"message":"Scope of `AdheseAdapter` is limited to this adapter pa
...
{"file":"adapters/videoheroes/videoheroes.go","start":137,"end":137,"message":"Found incorrect assignment made to `Bid` Recommended to either create copy of `bid` and then assign it as `Bid`  ```\n  for _, seatBid := range response.SeatBid {\n    for _, bid := range seatBids.Bid {\n      ...\n      seatBid := bid\n      responseBid := &adapters.TypedBid{\n        Bid: &seatBid,\n        ...\n      }\n      ...\n    }\n  }\n``` Alternatively can make use of `seatBids.Bid` loop index as shown below  ```\n  for _, seatBid := range response.SeatBid {\n    for i, _ := range seatBids.Bid {\n      ...\n      responseBid := &adapters.TypedBid{\n        Bid: &seatBids.Bid[i],\n        ...\n      }\n      ...\n      ...\n    }\n  }\n```\n"}]

~/prebid-repos/prebid-server (add-semgrep-rules) »

formatting nits

.semgrep/adapter/type-bid-assignment.yml

Sonali-More-Xandr

LGTM!

It depends on adapter code maintainer whether to use utils for status check or whether to check using custom function. So removing no-content-status and bad-request-not-ok-status rules

mansinahar · 2023-06-26T13:57:34Z

.semgrep/adapter/builder-struct-name.yml

+    languages:
+      - go
+    message: >
+      Scope of `$BUILDER` is limited to this adapter package. Therefore `$BUILDER` can be renamed to `adapter`. Refer following example.


Could you please also add a comment about how using the adapter's actual name for the adapter struct also makes it redundant with the package name. Example, foo.Foo

03ee138 updates message

mansinahar · 2023-06-26T13:58:22Z

.semgrep/adapter/builder-struct-name.yml

+    languages:
+      - go
+    message: >
+      Scope of `$BUILDER` is limited to this adapter package. Therefore `$BUILDER` can be renamed to `adapter`. Refer following example.


Nitpick: Rewrite as Refer to the following example

mansinahar · 2023-06-26T14:01:43Z

.semgrep/adapter/type-bid-assignment.yml

+    languages:
+      - go
+    message: >
+      Found incorrect assignment made to `Bid` Consider using `seatBids.Bid` loop index as shown below  


I think it will be helpful to give the author some explanation of why their implementation is incorrect. Can you please provide some context in the description about how the loop variable gets overridden and that can cause unexpected behavior?

dc62fff adds the reasoning behind not assigning loop variable pointer value as bid

mansinahar · 2023-06-26T14:22:21Z

.semgrep/adapter/type-bid-assignment.yml

+                }
+                ...
+              } 
+          - pattern: >


Won't we need another case here as

- pattern: > for _, $BID := range ... { ... ... := append(..., &adapters.TypedBid{ $KEY: &$BID, ... }) ... }

we won't be reinitialising i.e := append inside loop

Ah ok! Yeah, you're right

mansinahar

@onkarvhanumante thanks for making the changes. I just have two minor comments about the messages.

mansinahar · 2023-06-29T18:43:33Z

.semgrep/adapter/type-bid-assignment.yml

+      Found incorrect assignment made to $KEY. $BID variable receives a new value in each iteration of range loop. Assigning the address of $BID `(&$BID)` to $KEY will result in a pointer that always points to the same memory address with the value of the last iteration. 
+      This can lead to unexpected behavior or incorrect results. Refer https://go.dev/play/p/9ZS1f-5h4qS
+
+      Consider using `seatBids.Bid` loop index as shown below  


Nitpick: Consider rewriting the last sentence as:
Consider using an index variable in the seatBids.Bid loop as shown below

mansinahar · 2023-06-29T18:49:49Z

.semgrep/adapter/builder-struct-name.yml

@@ -3,9 +3,22 @@ rules:
    languages:
      - go
    message: >
-      Scope of `$BUILDER` is limited to this adapter package. Therefore `$BUILDER` can be renamed to `adapter`. Refer to the following example.
+      Using the adapter name for the struct makes it redundant with the package name. 


I would recommend making this comment a little simpler such as:

You can call this simply "adapter", the $BUILDER identification is already supplied by the package name. As you have it, referencing your adapter from outside the package would be $BUILDER.$BUILDER which looks a little redundant. See example below:

(Stealing these words by Hans from an old adapter PR)

bd7aaff

thanks for the updated message

Sonali-More-Xandr · 2023-07-04T10:37:55Z

I see that the HTTP response checks were removed from this PR. I agree that the use of utils is optional and should not block the PR from merging. It is upto the author if they want to update the adapter to use the utils or not but it is better to offer them an option. I think we should keep a semgrep rule that will add a comment as optional and let the author make decision.
thoughts?

mansinahar · 2023-07-05T21:20:25Z

@Sonali-More-Xandr I don't have a very strong opinion on this one so if you both think it will be valuable to have that as a warning, I won't fight it :)

onkarvhanumante · 2023-07-06T07:31:43Z

I see that the HTTP response checks were removed from this PR. I agree that the use of utils is optional and should not block the PR from merging. It is upto the author if they want to update the adapter to use the utils or not but it is better to offer them an option. I think we should keep a semgrep rule that will add a comment as optional and let the author make decision. thoughts?

@Sonali-More-Xandr I don't have a very strong opinion on this one so if you both think it will be valuable to have that as a warning, I won't fight it :)

For now lets have 2 semgrep rules. We can add other in new PR

@akkapur

* Fix: deal tiers no longer ignored due to presence of tid (prebid#2829) * CAPT-787: GPP support for imds bidder. (prebid#2867) Co-authored-by: Timothy M. Ace <[email protected]> * Adsinteractive: change usersync endpoint to https (prebid#2861) Co-authored-by: Balint Vargha <[email protected]> * consumable adapter: add gpp support (prebid#2883) * feat: IX Bid Adapter - gpp support for user sync urls (prebid#2873) Co-authored-by: Chris Corbo <[email protected]> * fix: update links in readme (prebid#2888) authored by @akkapur * New Adapter: AIDEM (prebid#2824) Co-authored-by: AndreaC <[email protected]> Co-authored-by: Andrea Tumbarello <[email protected]> Co-authored-by: darkstar <[email protected]> * Improve Digital adapter: Set currency in bid response (prebid#2886) * Sharethrough: Support multiformat bid request impression (prebid#2866) * Triplelift Bid Adapter: Adding GPP Support (prebid#2887) * YahooAdvertising rebranding to Yahoo Ads. (prebid#2872) Co-authored-by: oath-jac <[email protected]> * IX: MultiImp Implementation (prebid#2779) Co-authored-by: Chris Corbo <[email protected]> Co-authored-by: Oronno Mamun <[email protected]> * Exchange unit test fix (prebid#2868) * Semgrep rules for adapters (prebid#2833) * IX: Remove glog statement (prebid#2909) * Activities framework (prebid#2844) * PWBID: Update Default Endpoint (prebid#2903) * script to run semgrep tests against adapter PRs (prebid#2907) authored by @onkarvhanumante * semgrep rule to detect undesirable package imports in adapter code (prebid#2911) * update package-import message (prebid#2913) authored by @onkarvhanumante * Bump google.golang.org/grpc from 1.46.2 to 1.53.0 (prebid#2905) --------- Co-authored-by: Brian Sardo <[email protected]> Co-authored-by: Timothy Ace <[email protected]> Co-authored-by: Timothy M. Ace <[email protected]> Co-authored-by: balintvargha <[email protected]> Co-authored-by: Balint Vargha <[email protected]> Co-authored-by: Jason Piros <[email protected]> Co-authored-by: ccorbo <[email protected]> Co-authored-by: Chris Corbo <[email protected]> Co-authored-by: Ankush <[email protected]> Co-authored-by: Giovanni Sollazzo <[email protected]> Co-authored-by: AndreaC <[email protected]> Co-authored-by: Andrea Tumbarello <[email protected]> Co-authored-by: darkstar <[email protected]> Co-authored-by: Jozef Bartek <[email protected]> Co-authored-by: Max Dupuis <[email protected]> Co-authored-by: Patrick Loughrey <[email protected]> Co-authored-by: radubarbos <[email protected]> Co-authored-by: oath-jac <[email protected]> Co-authored-by: Oronno Mamun <[email protected]> Co-authored-by: Veronika Solovei <[email protected]> Co-authored-by: Onkar Hanumante <[email protected]> Co-authored-by: Stephen Johnston <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

shahinrahbariasl · 2023-07-27T20:51:19Z

Hey, our gofmt job in our build pipeline started failing due to missing package keyword in some of these files. Is that intended? Shouldn't all go files have a package declaration?

onkarvhanumante added 4 commits June 9, 2023 12:48

Add rules for response status code checks

9a5b0ee

Add rule for adapter Bidder implementer name check

7fbdaba

Add rule for type bid assignment

6ac09c0

Update semgrep messages

3de7707

formatting nits

bsardo assigned mansinahar Jun 14, 2023

onkarvhanumante added the do not port label Jun 15, 2023

onkarvhanumante assigned Sonali-More-Xandr Jun 15, 2023

Sonali-More-Xandr reviewed Jun 16, 2023

View reviewed changes

.semgrep/adapter/type-bid-assignment.yml Outdated Show resolved Hide resolved

Sonali-More-Xandr reviewed Jun 16, 2023

View reviewed changes

.semgrep/adapter/type-bid-assignment.yml Outdated Show resolved Hide resolved

onkarvhanumante added 2 commits June 16, 2023 13:39

fix typo

e49d669

rename var

f5c43f0

Sonali-More-Xandr previously approved these changes Jun 16, 2023

View reviewed changes

onkarvhanumante added 4 commits June 21, 2023 16:17

Remove no-content-status and bad-request-not-ok-status rules

a4bf4f1

It depends on adapter code maintainer whether to use utils for status check or whether to check using custom function. So removing no-content-status and bad-request-not-ok-status rules

Update builder-struct-name rule

7e48ceb

Update type-bid-assignment rule

4f0e7b7

Update type-bid-assignment-check message

0c8d97d

onkarvhanumante dismissed Sonali-More-Xandr’s stale review via 0c8d97d June 21, 2023 11:38

mansinahar reviewed Jun 26, 2023

View reviewed changes

onkarvhanumante added 4 commits June 28, 2023 12:43

update builder-struct-name.yml message

f25485c

remove extra space

ec0ed3c

update type-bid-assignment-check message

dc62fff

update builder-struct-name message

03ee138

mansinahar reviewed Jun 29, 2023

View reviewed changes

onkarvhanumante added 2 commits June 30, 2023 12:50

update semgrep rules message

bd7aaff

minor nits

5a9293a

mansinahar approved these changes Jun 30, 2023

View reviewed changes

Sonali-More-Xandr approved these changes Jul 6, 2023

View reviewed changes

Sonali-More-Xandr merged commit f1787d4 into master Jul 6, 2023

onkarvhanumante mentioned this pull request Jul 6, 2023

script to run semgrep tests against adapter PRs #2907

Merged

Peiling-Ding pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jul 6, 2023

Semgrep rules for adapters (prebid#2833)

c9ae6ca

onkarvhanumante deleted the add-semgrep-rules branch July 10, 2023 09:57

oronno mentioned this pull request Jul 13, 2023

Gofmt failing for not defining package #2925

Closed

Peiling-Ding pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jul 14, 2023

Semgrep rules for adapters (prebid#2833)

cf5e688

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semgrep rules for adapters #2833

Semgrep rules for adapters #2833

onkarvhanumante commented Jun 9, 2023 •

edited

Loading

Sonali-More-Xandr left a comment

mansinahar Jun 26, 2023

onkarvhanumante Jun 28, 2023

mansinahar Jun 26, 2023

onkarvhanumante Jun 28, 2023

mansinahar Jun 26, 2023

onkarvhanumante Jun 28, 2023

mansinahar Jun 26, 2023

onkarvhanumante Jun 28, 2023 •

edited

Loading

mansinahar Jun 29, 2023

mansinahar left a comment

mansinahar Jun 29, 2023

onkarvhanumante Jun 30, 2023

mansinahar Jun 29, 2023 •

edited

Loading

onkarvhanumante Jun 30, 2023

Sonali-More-Xandr commented Jul 4, 2023

mansinahar commented Jul 5, 2023

onkarvhanumante commented Jul 6, 2023

shahinrahbariasl commented Jul 27, 2023

Semgrep rules for adapters #2833

Semgrep rules for adapters #2833

Conversation

onkarvhanumante commented Jun 9, 2023 • edited Loading

Sonali-More-Xandr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onkarvhanumante Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sonali-More-Xandr commented Jul 4, 2023

mansinahar commented Jul 5, 2023

onkarvhanumante commented Jul 6, 2023

shahinrahbariasl commented Jul 27, 2023

onkarvhanumante commented Jun 9, 2023 •

edited

Loading

onkarvhanumante Jun 28, 2023 •

edited

Loading

mansinahar Jun 29, 2023 •

edited

Loading