Skip to content

Fix: Change account disabled code #4899

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

Open
linux019 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix: Change account disabled code #4899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c4dc738
fix: change account blocked status to 403
Aug 6, 2026
680cb46
fix: change account blocked status to 403
Aug 6, 2026
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
2 changes: 1 addition & 1 deletion endpoints/openrtb2/amp_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
for _, er := range errL {
errCode := errortypes.ReadCode(er)
if errCode == errortypes.BlockedAppErrorCode || errCode == errortypes.AccountDisabledErrorCode {
httpStatus = http.StatusServiceUnavailable
httpStatus = http.StatusForbidden

@postindustria-code postindustria-code Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor consistency point: this site was left as an if / else if while auction.go and video_auction.go were converted to a switch in the same diff. That is not wrong — in fact this is the only one of the three that still short-circuits the loop correctly — but it does mean the three copies of this logic now differ in both style and behaviour.

Either convert all three or none; if you keep the switch elsewhere, this one is the reference for what the loop is supposed to do.

metricsStatus = metrics.RequestStatusBlockedApp
break
}
Expand Down
8 changes: 4 additions & 4 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1920,12 +1920,12 @@ func writeError(errs []error, w http.ResponseWriter, labels *metrics.Labels) boo
httpStatus := http.StatusBadRequest
metricsStatus := metrics.RequestStatusBadInput
for _, err := range errs {
erVal := errortypes.ReadCode(err)
if erVal == errortypes.BlockedAppErrorCode || erVal == errortypes.AccountDisabledErrorCode {
httpStatus = http.StatusServiceUnavailable
switch errortypes.ReadCode(err) {

@postindustria-code postindustria-code Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In Go, break inside a switch leaves the switch, not the enclosing for. The old if / else if version broke out of the loop on the first matching error; this one runs to completion, so the last matching error wins instead of the first. The three break statements below are now dead code (a Go switch does not fall through).

I measured it with the same probe on the base commit a5002538f and on 680cb46fe:

errors passed in base this PR
[AccountDisabled, MalformedAcct] 503 500
[MalformedAcct, AccountDisabled] 500 403
[BlockedApp, MalformedAcct] 503 500

To be fair: this is latent, not a live regression — GetAccount returns immediately on each of these codes (account/account.go:21, :48, :53, :67) and BlockedApp returns on the spot (auction.go:1226), so a slice never carries two of them today. But the guard is gone and go vet does not catch it, so the next change that accumulates errors flips the behaviour silently.

Either drop the redundant break statements, or make the short-circuit explicit if it was intentional:

loop:
	for _, err := range errs {
		switch errortypes.ReadCode(err) {
		case errortypes.BlockedAppErrorCode, errortypes.AccountDisabledErrorCode:
			httpStatus = http.StatusForbidden
			metricsStatus = metrics.RequestStatusBlockedApp
			break loop
		case errortypes.MalformedAcctErrorCode:
			httpStatus = http.StatusInternalServerError
			metricsStatus = metrics.RequestStatusAccountConfigErr
			break loop
		}
	}

case errortypes.BlockedAppErrorCode, errortypes.AccountDisabledErrorCode:
httpStatus = http.StatusForbidden
metricsStatus = metrics.RequestStatusBlockedApp
break
} else if erVal == errortypes.MalformedAcctErrorCode {
case errortypes.MalformedAcctErrorCode:
httpStatus = http.StatusInternalServerError
metricsStatus = metrics.RequestStatusAccountConfigErr
break
Expand Down
2 changes: 1 addition & 1 deletion endpoints/openrtb2/sample-requests/account-required/with-account/required-disabled-acct.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,6 @@
}
}
},
"expectedReturnCode": 503,
"expectedReturnCode": 403,
"expectedErrorMessage": "Invalid request: Prebid-server has disabled Account ID: disabled_acct, please reach out to the prebid server host.\n"
}
2 changes: 1 addition & 1 deletion endpoints/openrtb2/sample-requests/blocked/blocked-app.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,6 @@
}
}
},
"expectedReturnCode": 503,
"expectedReturnCode": 403,
"expectedErrorMessage": "Invalid request: Prebid-server does not process requests from App ID: spam_app\n"
}
10 changes: 5 additions & 5 deletions endpoints/openrtb2/video_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,16 +428,16 @@ func handleError(labels *metrics.Labels, w http.ResponseWriter, errL []error, vo
var errors string
var status int = http.StatusInternalServerError
for _, er := range errL {
erVal := errortypes.ReadCode(er)
if erVal == errortypes.BlockedAppErrorCode || erVal == errortypes.AccountDisabledErrorCode {
status = http.StatusServiceUnavailable
switch errortypes.ReadCode(er) {

@postindustria-code postindustria-code Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as in auction.go:1923 — break inside a switch no longer exits the for, so the last matching error wins rather than the first, and the three break statements here are dead code. Latent today (the codes are mutually exclusive by construction), but the short-circuit the old code had is gone.

Worth pinning whichever semantics you settle on with a test: TestHandleError only ever passes single-error slices, so neither the old nor the new precedence rule is covered. A case with []error{&errortypes.AccountDisabled{}, &errortypes.AcctRequired{}} would do it.

case errortypes.BlockedAppErrorCode, errortypes.AccountDisabledErrorCode:
status = http.StatusForbidden
labels.RequestStatus = metrics.RequestStatusBlockedApp
break
} else if erVal == errortypes.AcctRequiredErrorCode {
case errortypes.AcctRequiredErrorCode:
status = http.StatusBadRequest
labels.RequestStatus = metrics.RequestStatusBadInput
break
} else if erVal == errortypes.MalformedAcctErrorCode {
case errortypes.MalformedAcctErrorCode:
status = http.StatusInternalServerError
labels.RequestStatus = metrics.RequestStatusAccountConfigErr
break
Expand Down
6 changes: 3 additions & 3 deletions endpoints/openrtb2/video_auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,15 +813,15 @@ func TestHandleError(t *testing.T) {
giveErrors: []error{
&errortypes.AccountDisabled{},
},
wantCode: 503,
wantCode: http.StatusForbidden,
wantMetricsStatus: metrics.RequestStatusBlockedApp,
},
{
description: "Blocked app - return 503 with blocked metrics status",
description: "Blocked app - return 403 with blocked metrics status",
giveErrors: []error{
&errortypes.BlockedApp{},
},
wantCode: 503,
wantCode: http.StatusForbidden,
wantMetricsStatus: metrics.RequestStatusBlockedApp,
},
{
Expand Down
Loading