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

IP scrubber #2990

Merged
merged 21 commits into from
Sep 5, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
107 changes: 107 additions & 0 deletions .github/workflows/adapter-code-coverage.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
name: Adapter code coverage
on:
pull_request_target:
paths: ["adapters/*/*.go"]
permissions:
pull-requests: write
contents: write
jobs:
run-coverage:
runs-on: ubuntu-latest
steps:
- name: Install Go
uses: actions/setup-go@v4
with:
go-version: 1.20.5

- name: Checkout pull request branch
uses: actions/checkout@v3
with:
fetch-depth: 0
ref: ${{github.event.pull_request.head.ref}}
repository: ${{github.event.pull_request.head.repo.full_name}}

- name: Get adapter directories
id: get_directories
uses: actions/github-script@v6
with:
result-encoding: string
script: |
const utils = require('./.github/workflows/helpers/pull-request-utils.js')
function directoryExtractor(filepath) {
// extract directory name from filepath of the form adapters/<adapter-name>/*.go
if (filepath.startsWith("adapters/") && filepath.split("/").length > 2) {
return filepath.split("/")[1]
}
return ""
}
const helper = utils.diffHelper({github, context})
const files = await helper.getDirectories(directoryExtractor)
return files.length == 0 ? "" : JSON.stringify(files);

- name: Run coverage tests
id: run_coverage
if: ${{ steps.get_directories.outputs.result }} != ""
run: |
directories=$(echo '${{ steps.get_directories.outputs.result }}' | jq -r '.[]')
go mod download

# create a temporary directory to store the coverage output
temp_dir=$(mktemp -d)
touch ${temp_dir}/coverage_output.txt

# generate coverage for adapter
cd ./adapters
for directory in $directories; do
cd $directory
coverage_profile_path="${PWD}/${directory}.out"
go test -coverprofile="${coverage_profile_path}"
go tool cover -html="${coverage_profile_path}" -o "${temp_dir}/${directory}.html"
go tool cover -func="${coverage_profile_path}" -o "${temp_dir}/${directory}.txt"
cd ..
done
echo "coverage_dir=${temp_dir}" >> $GITHUB_OUTPUT

# remove pull request branch files
cd ..
rm -f -r ./*

- name: Checkout coverage-preview branch
uses: actions/checkout@v3
with:
fetch-depth: 0
ref: coverage-preview
repository: prebid/prebid-server

- name: Commit coverage files to coverage-preview branch
if: ${{ steps.run_coverage.outputs.coverage_dir }} != ""
id: commit_coverage
run: |
directory=.github/preview/${{ github.run_id }}_$(date +%s)
mkdir -p $directory
cp -r ${{ steps.run_coverage.outputs.coverage_dir }}/*.html ./$directory
git config --global user.name "github-actions[bot]"
git config --global user.email "github-actions[bot]@users.noreply.github.com"
git add $directory/*
git commit -m 'Add coverage files'
git push origin coverage-preview
echo "remote_coverage_preview_dir=${directory}" >> $GITHUB_OUTPUT

- name: Checkout master branch
if: ${{ steps.get_directories.outputs.result }} != ""
run: git checkout master

- name: Add coverage summary to pull request
if: ${{ steps.run_coverage.outputs.coverage_dir }} != "" && ${{ steps.commit_coverage.outputs.remote_coverage_preview_dir }} != ""
uses: actions/github-script@v6
with:
script: |
const utils = require('./.github/workflows/helpers/pull-request-utils.js')
const helper = utils.coverageHelper({
github, context,
headSha: '${{ github.event.pull_request.head.sha }}',
tmpCoverageDir: '${{ steps.run_coverage.outputs.coverage_dir }}',
remoteCoverageDir: '${{ steps.commit_coverage.outputs.remote_coverage_preview_dir }}'
})
const adapterDirectories = JSON.parse('${{ steps.get_directories.outputs.result }}')
await helper.AddCoverageSummary(adapterDirectories)
77 changes: 77 additions & 0 deletions .github/workflows/helpers/pull-request-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,29 @@ class diffHelper {
}
return nonScannedCommitsDiff
}

/*
Retrieves a list of directories from GitHub pull request files
@param {Function} directoryExtractor - The function used to extract the directory name from the filename
@returns {Array} An array of unique directory names
*/
async getDirectories(directoryExtractor = () => "") {
const { data } = await this.github.rest.pulls.listFiles({
owner: this.owner,
repo: this.repo,
pull_number: this.pullRequestNumber,
per_page: resultSize,
})

const directories = []
for (const { filename } of data) {
const directory = directoryExtractor(filename)
if (directory != "" && !directories.includes(directory)) {
directories.push(directory)
}
}
return directories
}
}

class semgrepHelper {
Expand Down Expand Up @@ -331,7 +354,61 @@ class semgrepHelper {
}
}

class coverageHelper {
constructor(input) {
this.owner = input.context.repo.owner
this.repo = input.context.repo.repo
this.github = input.github
this.pullRequestNumber = input.context.payload.pull_request.number
this.headSha = input.headSha
this.previewBaseURL = `https://htmlpreview.github.io/?https://github.com/${this.owner}/${this.repo}/coverage-preview/${input.remoteCoverageDir}`
this.tmpCoverDir = input.tmpCoverageDir
}

/*
Adds a code coverage summary along with heatmap links and coverage data on pull request as comment
@param {Array} directories - directory for which coverage summary will be added
*/
async AddCoverageSummary(directories = []) {
const fs = require("fs")
const path = require("path")
const { promisify } = require("util")
const readFileAsync = promisify(fs.readFile)

let body = "## Code coverage summary \n"
body += "Note: \n"
body +=
"- Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors \n"
body += `- Coverage summary encompasses all commits leading up to the latest one, ${this.headSha} \n`

for (const directory of directories) {
let url = `${this.previewBaseURL}/${directory}.html`
try {
const textFilePath = path.join(this.tmpCoverDir, `${directory}.txt`)
const data = await readFileAsync(textFilePath, "utf8")

body += `#### ${directory} \n`
body += `Refer [here](${url}) for heat map coverage report \n`
body += "\`\`\` \n"
body += data
body += "\n \`\`\` \n"
} catch (err) {
console.error(err)
return
}
}

await this.github.rest.issues.createComment({
owner: this.owner,
repo: this.repo,
issue_number: this.pullRequestNumber,
body: body,
})
}
}

module.exports = {
diffHelper: (input) => new diffHelper(input),
semgrepHelper: (input) => new semgrepHelper(input),
coverageHelper: (input) => new coverageHelper(input),
}
4 changes: 3 additions & 1 deletion .semgrep/adapter/parse-bid-type-check.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
rules:
- id: parse-bid-type-check
message: The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to $ORTBTYPE. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the [MType](https://github.com/prebid/openrtb/blob/main/openrtb2/bid.go#L334) field in the response to accurately determine the media type for the impression.
message: >
Prebid server expects the media type to be explicitly set in the adapter response.
Therefore, recommends implementing a pattern where the adapter server sets the [MType](https://github.com/prebid/openrtb/blob/main/openrtb2/bid.go#L334) field in the response to accurately determine the media type for the impression.
languages:
- go
severity: WARNING
Expand Down
18 changes: 11 additions & 7 deletions account/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ import (
"context"
"encoding/json"
"fmt"

"github.com/buger/jsonparser"
"github.com/prebid/go-gdpr/consentconstants"
"github.com/prebid/prebid-server/config"
"github.com/prebid/prebid-server/errortypes"
"github.com/prebid/prebid-server/metrics"
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/stored_requests"
"github.com/prebid/prebid-server/util/iputil"
jsonpatch "gopkg.in/evanphx/json-patch.v4"
)

Expand Down Expand Up @@ -103,14 +103,18 @@ func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_r
return nil, errs
}

errs = account.Privacy.IPv6Config.Validate(errs)
if len(errs) > 0 {
return nil, errs
var ipv6Errs []error
ipv6Errs = account.Privacy.IPv6Config.Validate(ipv6Errs)
if len(ipv6Errs) > 0 {
account.Privacy.IPv6Config.AnonKeepBits = iputil.IPv6BitSize
// record invalid ipv6 metric
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you commented asking if this can be done in a separate PR, if so can this comment be removed?

}
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

errs = account.Privacy.IPv4Config.Validate(errs)
if len(errs) > 0 {
return nil, errs
var ipv4Errs []error
ipv4Errs = account.Privacy.IPv4Config.Validate(ipv4Errs)
if len(ipv4Errs) > 0 {
account.Privacy.IPv4Config.AnonKeepBits = iputil.IPv4BitSize
// record invalid ipv4 metric
Copy link
Contributor

Choose a reason for hiding this comment

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

Same point about removing metric related comment

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Could simplify the syntax...

if errs := account.Privacy.IPv6Config.Validate(nil); len(errs) > 0 {
	account.Privacy.IPv6Config.AnonKeepBits = iputil.IPv6BitSize
}

if errs := account.Privacy.IPv4Config.Validate(nil); len(errs) > 0 {
	account.Privacy.IPv4Config.AnonKeepBits = iputil.IPv4BitSize
}

Copy link
Contributor

Choose a reason for hiding this comment

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

These lines show as having test coverage, but there doesn't seem to an assertion on setting their default values. I made a type locally where IPv4Config.AnonKeepBits = iputil.IPv6BitSize which is setting the ipv6 value for the ipv4 default and this mistake wasn't caught by a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is tested in a general GetAccount test.
I added extra flag to assert IP config for account id "invalid_acct_ipv6_ipv4".
I also realized I set both ipv6 and ipv4 to max value, instead of default mask bits. I added 2 new constants:

IPv4DefaultMaskingBitSize = 24
IPv6DefaultMaskingBitSize = 56

For the simplified syntax I added new variables for errors to show explicitly we don't want to add IP config validation errors to errs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And also tested this update end to end including test with publisher account id.


// set the value of events.enabled field based on deprecated events_enabled field and ensure backward compatibility
Expand Down
17 changes: 9 additions & 8 deletions account/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package account
import (
"context"
"encoding/json"
"errors"
"fmt"
"testing"

Expand All @@ -19,12 +18,13 @@ import (
)

var mockAccountData = map[string]json.RawMessage{
"valid_acct": json.RawMessage(`{"disabled":false}`),
"invalid_acct": json.RawMessage(`{"disabled":false, "privacy": {"ipv6": {"anon-keep-bits": -32}}}`),
"disabled_acct": json.RawMessage(`{"disabled":true}`),
"malformed_acct": json.RawMessage(`{"disabled":"invalid type"}`),
"gdpr_channel_enabled_acct": json.RawMessage(`{"disabled":false,"gdpr":{"channel_enabled":{"amp":true}}}`),
"ccpa_channel_enabled_acct": json.RawMessage(`{"disabled":false,"ccpa":{"channel_enabled":{"amp":true}}}`),
"valid_acct": json.RawMessage(`{"disabled":false}`),
"invalid_acct_ipv6": json.RawMessage(`{"disabled":false, "privacy": {"ipv6": {"anon-keep-bits": -32}, "ipv4": {"anon-keep-bits": 16}}}`),
"invalid_acct_ipv4": json.RawMessage(`{"disabled":false, "privacy": {"ipv4": {"anon-keep-bits": -32}, "ipv6": {"anon-keep-bits": 32}}}`),
"disabled_acct": json.RawMessage(`{"disabled":true}`),
"malformed_acct": json.RawMessage(`{"disabled":"invalid type"}`),
"gdpr_channel_enabled_acct": json.RawMessage(`{"disabled":false,"gdpr":{"channel_enabled":{"amp":true}}}`),
"ccpa_channel_enabled_acct": json.RawMessage(`{"disabled":false,"ccpa":{"channel_enabled":{"amp":true}}}`),
"gdpr_channel_enabled_deprecated_purpose_acct": json.RawMessage(`{"disabled":false,"gdpr":{"purpose1":{"enforce_purpose":"full"}, "channel_enabled":{"amp":true}}}`),
"gdpr_deprecated_purpose1": json.RawMessage(`{"disabled":false,"gdpr":{"purpose1":{"enforce_purpose":"full"}}}`),
"gdpr_deprecated_purpose2": json.RawMessage(`{"disabled":false,"gdpr":{"purpose2":{"enforce_purpose":"full"}}}`),
Expand Down Expand Up @@ -80,7 +80,8 @@ func TestGetAccount(t *testing.T) {
{accountID: "valid_acct", required: false, disabled: true, err: nil},
{accountID: "valid_acct", required: true, disabled: true, err: nil},

{accountID: "invalid_acct", required: true, disabled: false, err: errors.New("left mask bits cannot exceed 128 in ipv6 address, or be less than 0")},
{accountID: "invalid_acct_ipv6", required: true, disabled: false, err: nil},
{accountID: "invalid_acct_ipv4", required: true, disabled: false, err: nil},

// pubID given and matches a host account explicitly disabled (Disabled: true on account json)
{accountID: "disabled_acct", required: false, disabled: false, err: &errortypes.BlacklistedAcct{}},
Expand Down
2 changes: 0 additions & 2 deletions adapters/adhese/adhese.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"strings"
"text/template"

"github.com/golang/glog"
"github.com/prebid/openrtb/v19/openrtb2"
"github.com/prebid/prebid-server/adapters"
"github.com/prebid/prebid-server/config"
Expand Down Expand Up @@ -201,7 +200,6 @@ func (a *AdheseAdapter) MakeBids(internalRequest *openrtb2.BidRequest, externalR
func convertAdheseBid(adheseBid AdheseBid, adheseExt AdheseExt, adheseOriginData AdheseOriginData) openrtb2.BidResponse {
adheseExtJson, err := json.Marshal(adheseOriginData)
if err != nil {
glog.Error(fmt.Sprintf("Unable to parse adhese Origin Data as JSON due to %v", err))
adheseExtJson = make([]byte, 0)
}
return openrtb2.BidResponse{
Expand Down
3 changes: 2 additions & 1 deletion adapters/adkernel/adkernel.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ func (adapter *adkernelAdapter) MakeBids(internalRequest *openrtb2.BidRequest, e
newBadServerResponseError(fmt.Sprintf("Bad server response: %d", err)),
}
}

if len(bidResp.SeatBid) != 1 {
return nil, []error{
newBadServerResponseError(fmt.Sprintf("Invalid SeatBids count: %d", len(bidResp.SeatBid))),
Expand All @@ -228,7 +229,7 @@ func (adapter *adkernelAdapter) MakeBids(internalRequest *openrtb2.BidRequest, e

seatBid := bidResp.SeatBid[0]
bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(bidResp.SeatBid[0].Bid))

bidResponse.Currency = bidResp.Cur
for i := 0; i < len(seatBid.Bid); i++ {
bid := seatBid.Bid[i]
bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
},
"user": {
"buyeruid": "A-38327932832"
}
},
"cur": ["TYR"]
},

"httpCalls": [
Expand All @@ -45,7 +46,8 @@
},
"user": {
"buyeruid": "A-38327932832"
}
},
"cur": ["TYR"]
}
},
"mockResponse": {
Expand All @@ -67,15 +69,16 @@
"w": 300
}]
}],
"bidid": "wehM-93KGr0"
"bidid": "wehM-93KGr0",
"cur": "TYR"
}
}
}
],

"expectedBidResponses": [
{
"currency": "USD",
"currency": "TYR",
"bids": [
{
"bid": {
Expand Down
Loading