Conversation
WalkthroughImplements bid screening for provider-group evaluation via a new ScreenBid function and ScreenBidParams/Result types; replaces Validate-based APIs with BidScreening/GatewayClient across service and gateway layers; adds dry-run reservation support in cluster inventory; updates mocks, tests, and dependency versions. Changes
Sequence DiagramsequenceDiagram
participant Client as Gateway Client
participant Service as Provider Service
participant ScreenBid as Bidengine<br/>ScreenBid
participant Attrs as Provider Attr Service
participant Cluster as Cluster<br/>Reserve
participant Pricing as Pricing<br/>Engine
Client->>Service: BidScreening(req)
Service->>Service: Validate GroupSpec
Service->>ScreenBid: ScreenBid(group, params)
ScreenBid->>Attrs: Fetch provider/auditor attributes
Attrs-->>ScreenBid: Attributes / Signatures
ScreenBid->>ScreenBid: Check attrs, resources, volumes, hostnames, auditor sigs
ScreenBid-->>Service: ScreenBidResult
alt Screening Passed
Service->>Cluster: Reserve(group, DryRun=true)
Cluster-->>Service: Reservation (allocated resources)
Service->>Pricing: Calculate price(from reservation)
Pricing-->>Service: Price
Service-->>Client: BidScreeningResponse(Passed=true, Price)
else Screening Failed
Service-->>Client: BidScreeningResponse(Passed=false, Reasons)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
bidengine/order.go (1)
519-539: Propagate the order context into screening instead ofcontext.Background().The
runfunction creates a lifecycle context at line 175, butshouldBiddoesn't accept or pass it toScreenBid. Line 527 usescontext.Background(), bypassing the order's context for cancellation and timeout behavior. UpdateshouldBidto acceptctxfrom its caller and use it in theScreenBidcall.Proposed refactor
-func (o *order) shouldBid(group *dtypes.Group) (bool, error) { +func (o *order) shouldBid(ctx context.Context, group *dtypes.Group) (bool, error) {- shouldBidCh = runner.Do(func() runner.Result { - return runner.NewResult(o.shouldBid(group)) + shouldBidCh = runner.Do(func() runner.Result { + return runner.NewResult(o.shouldBid(ctx, group)) })- result, err := ScreenBid(context.Background(), group, params) + result, err := ScreenBid(ctx, group, params)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bidengine/order.go` around lines 519 - 539, The screening call currently uses context.Background() which bypasses the order lifecycle context; change the shouldBid function signature to accept a context.Context (e.g., ctx) and replace the context.Background() call with that ctx when calling ScreenBid; update the caller(s) (notably run which creates the lifecycle context) to pass its ctx into shouldBid, and ensure signatures and any tests/uses of shouldBid and ScreenBidParams are updated accordingly so cancellation/timeouts propagate correctly.gateway/rest/router_test.go (2)
492-498: Mock setup is unnecessary for unauthorized test.The mock
BidScreeningexpectation at Line 498 will never be invoked because the request is unauthorized (emptyauthTypesat Line 491) and returns HTTP 401 before reaching the handler logic that callsBidScreening. This doesn't cause test failures, but the mock setup is dead code.🧹 Suggested cleanup
func TestRouteValidateUnauthorized(t *testing.T) { runRouterTest(t, []routerTestAuth{}, func(test *routerTest, hdr http.Header) { - price := testutil.AkashDecCoin(t, 200) - screenResp := &providerv1.BidScreeningResponse{ - Passed: true, - Price: &price, - } - - test.pclient.On("BidScreening", mock.Anything, mock.Anything).Return(screenResp, nil) - uri, err := apclient.MakeURI(test.host, apclient.ValidatePath())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/rest/router_test.go` around lines 492 - 498, Remove the unnecessary mock setup for BidScreening in this unauthorized test: the test sets authTypes empty so the request returns 401 before router calls the client, therefore the test.pclient.On("BidScreening", ...) expectation (and variables screenResp/price if only used for that) is dead code; either delete the test.pclient.On call and its related screenResp/price setup from this test, or move that setup into the authorized test that exercises the BidScreening call.
549-549: Mock will not be invoked due to early return on empty body.Similar to the unauthorized test, this mock setup won't be called because the handler returns HTTP 400 for empty body before invoking
BidScreening. The test correctly expectsStatusBadRequest, so the mock is unnecessary.🧹 Suggested cleanup
func TestRouteValidateFailsEmptyBody(t *testing.T) { runRouterTest(t, []routerTestAuth{routerTestAuthCert, routerTestAuthJWT}, func(test *routerTest, hdr http.Header) { - test.pclient.On("BidScreening", mock.Anything, mock.Anything).Return((*providerv1.BidScreeningResponse)(nil), errGeneric) - uri, err := apclient.MakeURI(test.host, apclient.ValidatePath())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/rest/router_test.go` at line 549, The mock setup test.pclient.On("BidScreening", mock.Anything, mock.Anything).Return((*providerv1.BidScreeningResponse)(nil), errGeneric) is dead because the handler returns HTTP 400 on an empty request body before calling BidScreening; remove this mock expectation from the test (or alternatively, if you intended to exercise BidScreening, provide a non-empty valid request body) so the test only asserts StatusBadRequest without relying on a mock invocation.bidengine/screening_test.go (1)
141-161: Consider a more robust assertion for validation errors.The check at Lines 153-159 determines if a reason is a validation error by comparing string length against a hardcoded prefix length. This is fragile and could break if the prefix changes. Consider using
strings.HasPrefixinstead.♻️ Suggested improvement
+ "strings" ... - hasValidationError := false - for _, reason := range result.Reasons { - if len(reason) > len("group validation error: ") { - hasValidationError = true - break - } - } - require.True(t, hasValidationError, "expected a group validation error reason") + hasValidationError := false + for _, reason := range result.Reasons { + if strings.HasPrefix(reason, "group validation error:") { + hasValidationError = true + break + } + } + require.True(t, hasValidationError, "expected a group validation error reason")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bidengine/screening_test.go` around lines 141 - 161, The test TestScreenBid_FailsValidateBasic currently detects a group validation error by comparing reason string lengths; update it to robustly check each result.Reasons entry with strings.HasPrefix(reason, "group validation error: ") instead, importing the "strings" package, and assert that at least one reason matches that prefix (e.g., set hasValidationError when prefix matches and require.True afterward) so it no longer depends on hardcoded length checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bidengine/screening.go`:
- Around line 39-55: Add defensive nil checks at the start of ScreenBid to avoid
panics: validate that the incoming pointers group, params.Provider, and
params.AttrService are non-nil and return a descriptive error if any are nil
before calling group.GroupSpec.MatchAttributes, params.Attributes.SubsetOf, or
params.AttrService.GetAttributes; ensure you reference the same error messages
consistently and bail out early from ScreenBid when any required input is
missing.
In `@operator/inventory/node-discovery_test.go`:
- Around line 17-20: The helper newKubeNode currently accepts an unused varying
parameter name which triggers golangci-lint unparam; change its signature to
remove the name parameter (e.g., newKubeNode(gpuCount int64)) and set a fixed
node name inside the function, then update all test call sites that currently
pass a name to only pass the gpuCount argument (or none if defaulting), ensuring
references to newKubeNode in tests are adjusted accordingly so the helper
compiles and linter warnings disappear.
In `@service.go`:
- Around line 272-273: The owner value obtained via ownerFromCtx(ctx) must be
checked for missing/empty before performing hostname-based screening; update the
code that builds the availability check request (the block using Owner:
ownerFromCtx(ctx) and Hostnames: req.GetHostnames()) to first call
ownerFromCtx(ctx), return an explicit failure response (with a clear reason like
"missing owner in context") if the owner is absent, and only proceed to
construct/submit the hostname screening request when the owner is present so
hostname checks do not produce misleading availability reasons.
- Around line 299-305: The precheck pricing request (priceReq of type
bidengine.Request) is missing Owner and PricePrecision causing inconsistent
pricing; update the construction in service.go (where priceReq is built before
calling s.config.BidPricingStrategy.CalculatePrice) to set Owner using
ownerFromCtx(ctx) and set PricePrecision to DefaultPricePrecision so the request
matches the production path (see Request/priceReq and bidengine.Request usage).
---
Nitpick comments:
In `@bidengine/order.go`:
- Around line 519-539: The screening call currently uses context.Background()
which bypasses the order lifecycle context; change the shouldBid function
signature to accept a context.Context (e.g., ctx) and replace the
context.Background() call with that ctx when calling ScreenBid; update the
caller(s) (notably run which creates the lifecycle context) to pass its ctx into
shouldBid, and ensure signatures and any tests/uses of shouldBid and
ScreenBidParams are updated accordingly so cancellation/timeouts propagate
correctly.
In `@bidengine/screening_test.go`:
- Around line 141-161: The test TestScreenBid_FailsValidateBasic currently
detects a group validation error by comparing reason string lengths; update it
to robustly check each result.Reasons entry with strings.HasPrefix(reason,
"group validation error: ") instead, importing the "strings" package, and assert
that at least one reason matches that prefix (e.g., set hasValidationError when
prefix matches and require.True afterward) so it no longer depends on hardcoded
length checks.
In `@gateway/rest/router_test.go`:
- Around line 492-498: Remove the unnecessary mock setup for BidScreening in
this unauthorized test: the test sets authTypes empty so the request returns 401
before router calls the client, therefore the test.pclient.On("BidScreening",
...) expectation (and variables screenResp/price if only used for that) is dead
code; either delete the test.pclient.On call and its related screenResp/price
setup from this test, or move that setup into the authorized test that exercises
the BidScreening call.
- Line 549: The mock setup test.pclient.On("BidScreening", mock.Anything,
mock.Anything).Return((*providerv1.BidScreeningResponse)(nil), errGeneric) is
dead because the handler returns HTTP 400 on an empty request body before
calling BidScreening; remove this mock expectation from the test (or
alternatively, if you intended to exercise BidScreening, provide a non-empty
valid request body) so the test only asserts StatusBadRequest without relying on
a mock invocation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
.mockery.yamlbidengine/order.gobidengine/order_test.gobidengine/pricing_test.gobidengine/screening.gobidengine/screening_test.gobidengine/service.gocluster/inventory.gocluster/service.gogateway/grpc/server.gogateway/rest/integration_test.gogateway/rest/router.gogateway/rest/router_test.gogo.modmocks/client/Client_mock.gomocks/client/GatewayClient_mock.gomocks/cluster/Cluster_mock.gomocks/cluster/Service_mock.gooperator/inventory/node-discovery.gooperator/inventory/node-discovery_test.goservice.go
Comment on lines
+39
to
+55
| func ScreenBid(_ context.Context, group *dtypes.Group, params ScreenBidParams) (ScreenBidResult, error) { | ||
| var reasons []string | ||
|
|
||
| // Check 1: provider attribute matching | ||
| if !group.GroupSpec.MatchAttributes(params.Provider.Attributes) { | ||
| reasons = append(reasons, "incompatible provider attributes") | ||
| } | ||
|
|
||
| // Check 2: order attribute matching | ||
| if !params.Attributes.SubsetOf(group.GroupSpec.Requirements.Attributes) { | ||
| reasons = append(reasons, "incompatible order attributes") | ||
| } | ||
|
|
||
| // Check 3: resource capability matching | ||
| attr, err := params.AttrService.GetAttributes() | ||
| if err != nil { | ||
| return ScreenBidResult{}, fmt.Errorf("fetching provider attributes: %w", err) |
There was a problem hiding this comment.
Add defensive nil preconditions for required inputs.
This path can panic if group, params.Provider, or params.AttrService is nil. Return an error instead of dereferencing immediately.
Proposed fix
func ScreenBid(_ context.Context, group *dtypes.Group, params ScreenBidParams) (ScreenBidResult, error) {
+ if group == nil {
+ return ScreenBidResult{}, fmt.Errorf("group is nil")
+ }
+ if params.Provider == nil {
+ return ScreenBidResult{}, fmt.Errorf("provider is nil")
+ }
+ if params.AttrService == nil {
+ return ScreenBidResult{}, fmt.Errorf("attribute service is nil")
+ }
+
var reasons []string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func ScreenBid(_ context.Context, group *dtypes.Group, params ScreenBidParams) (ScreenBidResult, error) { | |
| var reasons []string | |
| // Check 1: provider attribute matching | |
| if !group.GroupSpec.MatchAttributes(params.Provider.Attributes) { | |
| reasons = append(reasons, "incompatible provider attributes") | |
| } | |
| // Check 2: order attribute matching | |
| if !params.Attributes.SubsetOf(group.GroupSpec.Requirements.Attributes) { | |
| reasons = append(reasons, "incompatible order attributes") | |
| } | |
| // Check 3: resource capability matching | |
| attr, err := params.AttrService.GetAttributes() | |
| if err != nil { | |
| return ScreenBidResult{}, fmt.Errorf("fetching provider attributes: %w", err) | |
| func ScreenBid(_ context.Context, group *dtypes.Group, params ScreenBidParams) (ScreenBidResult, error) { | |
| if group == nil { | |
| return ScreenBidResult{}, fmt.Errorf("group is nil") | |
| } | |
| if params.Provider == nil { | |
| return ScreenBidResult{}, fmt.Errorf("provider is nil") | |
| } | |
| if params.AttrService == nil { | |
| return ScreenBidResult{}, fmt.Errorf("attribute service is nil") | |
| } | |
| var reasons []string | |
| // Check 1: provider attribute matching | |
| if !group.GroupSpec.MatchAttributes(params.Provider.Attributes) { | |
| reasons = append(reasons, "incompatible provider attributes") | |
| } | |
| // Check 2: order attribute matching | |
| if !params.Attributes.SubsetOf(group.GroupSpec.Requirements.Attributes) { | |
| reasons = append(reasons, "incompatible order attributes") | |
| } | |
| // Check 3: resource capability matching | |
| attr, err := params.AttrService.GetAttributes() | |
| if err != nil { | |
| return ScreenBidResult{}, fmt.Errorf("fetching provider attributes: %w", err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bidengine/screening.go` around lines 39 - 55, Add defensive nil checks at the
start of ScreenBid to avoid panics: validate that the incoming pointers group,
params.Provider, and params.AttrService are non-nil and return a descriptive
error if any are nil before calling group.GroupSpec.MatchAttributes,
params.Attributes.SubsetOf, or params.AttrService.GetAttributes; ensure you
reference the same error messages consistently and bail out early from ScreenBid
when any required input is missing.
Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bidengine/screening.go (1)
39-55:⚠️ Potential issue | 🟠 MajorAdd defensive nil preconditions for required inputs.
The function can panic if
group,params.Provider, orparams.AttrServiceis nil. These are dereferenced at lines 43, 43, and 53 respectively without validation.🛡️ Proposed fix
func ScreenBid(_ context.Context, group *dtypes.Group, params ScreenBidParams) (ScreenBidResult, error) { + if group == nil { + return ScreenBidResult{}, fmt.Errorf("group is nil") + } + if params.Provider == nil { + return ScreenBidResult{}, fmt.Errorf("provider is nil") + } + if params.AttrService == nil { + return ScreenBidResult{}, fmt.Errorf("attribute service is nil") + } + var reasons []string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bidengine/screening.go` around lines 39 - 55, ScreenBid currently dereferences group, params.Provider and params.AttrService without checks which can panic; add defensive nil preconditions at the start of ScreenBid to validate group != nil, params.Provider != nil and params.AttrService != nil and return a descriptive error if any are nil (e.g., "nil group", "nil provider in params", "nil AttrService in params"); update callers/tests if needed and ensure subsequent calls to group.GroupSpec.MatchAttributes, params.Attributes.SubsetOf and params.AttrService.GetAttributes only occur after these validations so error paths are consistent and informative.
🧹 Nitpick comments (2)
gateway/rest/router_test.go (1)
456-487: Add a happy-path test usingproviderv1.BidScreeningRequestbody.Current coverage exercises the legacy bare
GroupSpecpayload, but not the new envelope parsing path introduced in the handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/rest/router_test.go` around lines 456 - 487, Update the TestRouteValidateOK test to exercise the envelope parsing path by sending a JSON-encoded providerv1.BidScreeningRequest instead of the bare GroupSpec: construct a providerv1.BidScreeningRequest whose GroupSpec field contains the testutil.GroupSpec value, marshal that request to JSON and use it as the HTTP body for the GET to apclient.ValidatePath(), keep the existing test.pclient.On("BidScreening", ...) mock return (screenResp) and headers/content-type handling the same so the handler receives and routes the enveloped payload through the BidScreening call.bidengine/screening.go (1)
34-34: UnusedLogfield inScreenBidParams.The
Logfield is declared but never used within theScreenBidfunction. Consider removing it if not needed, or add logging for screening steps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bidengine/screening.go` at line 34, The ScreenBidParams struct contains an unused Log field; update the code so either remove the Log field from ScreenBidParams if no per-call logger is required, or wire it into the screening flow by using that logger inside ScreenBid (e.g., reference ScreenBidParams.Log within the ScreenBid function to emit screening start/decision/error logs). Locate the struct definition for ScreenBidParams and the ScreenBid function and perform the corresponding removal or replacement of any package-level logger usages with the provided params.Log to ensure the field is actually used or eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bidengine/order.go`:
- Around line 519-525: The ScreenBidParams struct is missing fields used by
ScreenBid's hostname availability check; update the params literal in order.go
to include HostnameService, Owner, and Hostnames so shouldBid doesn't bypass the
precheck. Populate HostnameService from the instance that provides hostname
checks (e.g. o.hostnameService or the relevant service on o.pass), set Owner
from the session or config (e.g. o.session.Owner() or o.cfg.Owner), and set
Hostnames from the config/session (e.g. o.cfg.Hostnames or
o.session.Hostnames()); ensure the added fields match ScreenBidParams' types.
In `@gateway/rest/router.go`:
- Around line 467-480: The fallback accepts malformed envelopes because unknown
top-level fields get ignored and json.Unmarshal into gspec yields a zero-value
GroupSpec; fix by first decoding data into a map[string]json.RawMessage to
detect which top-level keys exist (or alternatively use json.Decoder with
DisallowUnknownFields), then only allow the fallback path when the payload
contains the expected "groupSpec" key (or known BidScreeningRequest keys) —
update the logic around variables screenReq, gspec and data so you return
http.StatusUnprocessableEntity for payloads that contain no recognized top-level
fields instead of silently treating them as an empty GroupSpec.
---
Duplicate comments:
In `@bidengine/screening.go`:
- Around line 39-55: ScreenBid currently dereferences group, params.Provider and
params.AttrService without checks which can panic; add defensive nil
preconditions at the start of ScreenBid to validate group != nil,
params.Provider != nil and params.AttrService != nil and return a descriptive
error if any are nil (e.g., "nil group", "nil provider in params", "nil
AttrService in params"); update callers/tests if needed and ensure subsequent
calls to group.GroupSpec.MatchAttributes, params.Attributes.SubsetOf and
params.AttrService.GetAttributes only occur after these validations so error
paths are consistent and informative.
---
Nitpick comments:
In `@bidengine/screening.go`:
- Line 34: The ScreenBidParams struct contains an unused Log field; update the
code so either remove the Log field from ScreenBidParams if no per-call logger
is required, or wire it into the screening flow by using that logger inside
ScreenBid (e.g., reference ScreenBidParams.Log within the ScreenBid function to
emit screening start/decision/error logs). Locate the struct definition for
ScreenBidParams and the ScreenBid function and perform the corresponding removal
or replacement of any package-level logger usages with the provided params.Log
to ensure the field is actually used or eliminated.
In `@gateway/rest/router_test.go`:
- Around line 456-487: Update the TestRouteValidateOK test to exercise the
envelope parsing path by sending a JSON-encoded providerv1.BidScreeningRequest
instead of the bare GroupSpec: construct a providerv1.BidScreeningRequest whose
GroupSpec field contains the testutil.GroupSpec value, marshal that request to
JSON and use it as the HTTP body for the GET to apclient.ValidatePath(), keep
the existing test.pclient.On("BidScreening", ...) mock return (screenResp) and
headers/content-type handling the same so the handler receives and routes the
enveloped payload through the BidScreening call.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
.mockery.yamlbidengine/order.gobidengine/order_test.gobidengine/pricing_test.gobidengine/screening.gobidengine/screening_test.gobidengine/service.gocluster/inventory.gocluster/service.gogateway/grpc/server.gogateway/rest/integration_test.gogateway/rest/router.gogateway/rest/router_test.gogo.modmocks/client/Client_mock.gomocks/client/GatewayClient_mock.gomocks/cluster/Cluster_mock.gomocks/cluster/Service_mock.gooperator/inventory/node-discovery.gooperator/inventory/node-discovery_test.goservice.go
✅ Files skipped from review due to trivial changes (1)
- bidengine/screening_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cluster/service.go
- .mockery.yaml
- operator/inventory/node-discovery.go
- bidengine/pricing_test.go
- bidengine/service.go
Comment on lines
+519
to
525
| params := ScreenBidParams{ | ||
| Provider: o.session.Provider(), | ||
| Attributes: o.cfg.Attributes, | ||
| MaxGroupVolumes: o.cfg.MaxGroupVolumes, | ||
| AttrService: o.pass, | ||
| Log: o.log, | ||
| } |
There was a problem hiding this comment.
Populate full ScreenBidParams to avoid skipping hostname precheck.
shouldBid does not pass HostnameService, Owner, or Hostnames, so ScreenBid’s hostname availability check is effectively disabled on this path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bidengine/order.go` around lines 519 - 525, The ScreenBidParams struct is
missing fields used by ScreenBid's hostname availability check; update the
params literal in order.go to include HostnameService, Owner, and Hostnames so
shouldBid doesn't bypass the precheck. Populate HostnameService from the
instance that provides hostname checks (e.g. o.hostnameService or the relevant
service on o.pass), set Owner from the session or config (e.g. o.session.Owner()
or o.cfg.Owner), and set Hostnames from the config/session (e.g. o.cfg.Hostnames
or o.session.Hostnames()); ensure the added fields match ScreenBidParams' types.
Comment on lines
+467
to
480
| // Try BidScreeningRequest format first, fall back to bare GroupSpec | ||
| var screenReq providerv1.BidScreeningRequest | ||
| var gspec dvbeta.GroupSpec | ||
|
|
||
| if err := json.Unmarshal(data, &gspec); err != nil { | ||
| http.Error(w, err.Error(), http.StatusUnprocessableEntity) | ||
| return | ||
| if err := json.Unmarshal(data, &screenReq); err == nil && screenReq.GroupSpec != nil && screenReq.GroupSpec.Name != "" { | ||
| // parsed as BidScreeningRequest | ||
| } else { | ||
| // Backward compatible: try bare GroupSpec | ||
| if err := json.Unmarshal(data, &gspec); err != nil { | ||
| http.Error(w, err.Error(), http.StatusUnprocessableEntity) | ||
| return | ||
| } | ||
| screenReq.GroupSpec = &gspec | ||
| } |
There was a problem hiding this comment.
Fallback parsing can accept malformed request envelopes as empty specs.
Unknown top-level fields are ignored during fallback unmarshal, so invalid envelope payloads can slip through as zero-value GroupSpec and hit BidScreening instead of returning 422.
Proposed hardening
- if err := json.Unmarshal(data, &screenReq); err == nil && screenReq.GroupSpec != nil && screenReq.GroupSpec.Name != "" {
+ if err := json.Unmarshal(data, &screenReq); err == nil && screenReq.GroupSpec != nil {
// parsed as BidScreeningRequest
} else {
// Backward compatible: try bare GroupSpec
- if err := json.Unmarshal(data, &gspec); err != nil {
+ if err := json.Unmarshal(data, &gspec); err != nil || gspec.Name == "" {
http.Error(w, err.Error(), http.StatusUnprocessableEntity)
return
}
screenReq.GroupSpec = &gspec
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway/rest/router.go` around lines 467 - 480, The fallback accepts
malformed envelopes because unknown top-level fields get ignored and
json.Unmarshal into gspec yields a zero-value GroupSpec; fix by first decoding
data into a map[string]json.RawMessage to detect which top-level keys exist (or
alternatively use json.Decoder with DisallowUnknownFields), then only allow the
fallback path when the payload contains the expected "groupSpec" key (or known
BidScreeningRequest keys) — update the logic around variables screenReq, gspec
and data so you return http.StatusUnprocessableEntity for payloads that contain
no recognized top-level fields instead of silently treating them as an empty
GroupSpec.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.