fix(apigatewayv1): enforce Smithy input constraints + improve round-trip probe#1410
Conversation
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fakecloud-apigateway/src/validation.rs">
<violation number="1" location="crates/fakecloud-apigateway/src/validation.rs:326">
P1: `RejectDomainNameAccessAssociation` validates the wrong query parameter name (`domainNameArn`), so valid requests with `domainNameAccessAssociationArn` are rejected.</violation>
<violation number="2" location="crates/fakecloud-apigateway/src/validation.rs:358">
P1: `GetStages` is incorrectly validated as requiring `stageName`, causing valid `/restapis/{id}/stages` requests to return `BadRequestException`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
c82f674 to
ab1f6f5
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
7329683 to
849a0b1
Compare
|
@cubic-dev-ai re-review |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fakecloud-apigateway/src/validation.rs">
<violation number="1" location="crates/fakecloud-apigateway/src/validation.rs:262">
P2: `GetAuthorizer` is missing required `authorizerId` path validation because it shares constraints with `GetAuthorizers`.</violation>
<violation number="2" location="crates/fakecloud-apigateway/src/validation.rs:294">
P2: `DeleteDocumentationVersion` does not validate required `documentationVersion` path label.</violation>
<violation number="3" location="crates/fakecloud-apigateway/src/validation.rs:341">
P2: `DeleteModel` is missing required `modelName` path validation due shared constraints with `GetModels`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
849a0b1 to
b5a1b0e
Compare
|
@cubic-dev-ai review |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fakecloud-apigateway/src/validation.rs">
<violation number="1" location="crates/fakecloud-apigateway/src/validation.rs:94">
P2: `@required` is enforced too strictly: empty string/list/map values are rejected for every required body field, which can incorrectly fail valid Smithy requests that only require presence/non-null.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
d597f5c to
7d1b93c
Compare
|
@cubic-dev-ai review |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
7d1b93c to
19d3a48
Compare
|
@cubic-dev-ai review |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fakecloud-apigateway/src/service_rest_api_resources.rs">
<violation number="1" location="crates/fakecloud-apigateway/src/service_rest_api_resources.rs:23">
P1: The new global prevalidation call can incorrectly reject valid raw-payload import requests (ImportRestApi/ImportApiKeys) because validation expects a JSON `body` field instead of accepting HTTP payload bodies.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
| // body fields, missing/placeholder path labels, missing | ||
| // required query parameters, and invalid enum values before | ||
| // any per-handler logic runs. | ||
| crate::validation::prevalidate(action, req, ¶ms)?; |
There was a problem hiding this comment.
P1: The new global prevalidation call can incorrectly reject valid raw-payload import requests (ImportRestApi/ImportApiKeys) because validation expects a JSON body field instead of accepting HTTP payload bodies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-apigateway/src/service_rest_api_resources.rs, line 23:
<comment>The new global prevalidation call can incorrectly reject valid raw-payload import requests (ImportRestApi/ImportApiKeys) because validation expects a JSON `body` field instead of accepting HTTP payload bodies.</comment>
<file context>
@@ -15,6 +15,12 @@ impl ApiGatewayService {
+ // body fields, missing/placeholder path labels, missing
+ // required query parameters, and invalid enum values before
+ // any per-handler logic runs.
+ crate::validation::prevalidate(action, req, ¶ms)?;
match action {
"GetAccount" => self.get_account(req),
</file context>
…rip probe
apigatewayv1 prevalidation
- New crates/fakecloud-apigateway/src/validation.rs centralizes Smithy
required-field, required-path-label, required-query, enum, and
path-enum checks for all 36 control-plane operations that the
conformance probe negative-tests. Surfaces BadRequestException (the
declared error shape) for missing required body/path/query fields
and out-of-set enum values that handlers previously silently
accepted.
- Path-label values are URL-decoded before the placeholder check so
`{Name}` substrings sent literally by the probe (when it omits an
@httpLabel member) are recognised as "missing".
Round-trip probe improvements (crates/fakecloud-conformance)
- Forward every shared identifier from the writer's input into the
follow-up Get/Describe. Multi-segment REST resources (API Gateway's
`/restapis/{r}/resources/{x}/methods/{m}`) need the whole composite
key, not just `id_source_field`, otherwise the Get always 404s.
- Use the Create response to fill reader inputs whose names diverge
from the writer's (e.g. CreateModel.name -> GetModel.modelName,
CreateAuthorizer response.id -> GetAuthorizer.authorizerId). Match
by exact field name first, then by stripped suffix heuristics
(<Resource>Name -> response.name, <Resource>Id -> response.id).
- Isolate each round-trip variant under a unique resource identifier
(`rt-<echo_field>`) so concurrent variants targeting the same writer
can't clobber each other's stored state — prior behaviour produced
spurious echo drops when a sibling variant overwrote the round-trip's
PUT.
- Prefer the exact-suffix reader in `find_round_trip_pairs` so
`PutIntegrationResponse` pairs with `GetIntegrationResponse` instead
of the shorter-suffix `GetIntegration`.
Conformance: apigatewayv1 3255/3375 (96.4%) -> 3376/3376 (100%).
Workspace baseline: 85115/86666 (98.2%) -> 85258/86679 (98.4%).
19d3a48 to
d12c92d
Compare
- RejectDomainNameAccessAssociation now requires both domainNameAccessAssociationArn and domainNameArn as `@required` httpQuery members (Smithy declares both required). - GetStages no longer claims `stageName` as a path label — the operation only takes `restApiId` from the path. Keep the requirement on FlushStageCache / FlushStageAuthorizersCache where stageName truly is part of the URI. Conformance: still 3376/3376 (100%).
d12c92d to
3ae53c2
Compare
|
@cubic-dev-ai review |
@vieiralucas I have started the AI code review. It will take a few minutes to complete. |
Summary
apigatewayv1 prevalidation: new
crates/fakecloud-apigateway/src/validation.rscentralizes Smithy required-field, required-path-label, required-query, enum, and path-enum checks for all 36 control-plane operations the conformance probe negative-tests. SurfacesBadRequestExceptionfor missing/invalid inputs that handlers previously silently accepted.Round-trip probe improvements (
crates/fakecloud-conformance):CreateModel.name->GetModel.modelName,CreateAuthorizer response.id->GetAuthorizer.authorizerId).find_round_trip_pairs(PutIntegrationResponsepairs withGetIntegrationResponse, not the shorterGetIntegration).Conformance: apigatewayv1 3255/3375 (96.4%) -> 3376/3376 (100%).
Workspace baseline: 85115/86666 (98.2%) -> 85258/86679 (98.4%).
Test plan
cargo run -p fakecloud-conformance --release -- run --services apigatewayv1reports 0 failurescargo run -p fakecloud-conformance --release -- check --baseline conformance-baseline.jsonpasses (no regressions across other services)cargo clippy -p fakecloud-apigateway -p fakecloud-conformance --all-targets -- -D warningscargo test -p fakecloud-conformance --lib83/83 passcargo fmtSummary by cubic
Adds Smithy-aligned input validation to API Gateway v1 and strengthens the round-trip probe for multi-segment REST resources.
apigatewayv1now passes 100%, with a small workspace baseline bump.New Features
crates/fakecloud-apigateway: Centralized prevalidation for 36 control-plane ops (required body/path/query, enum, path-enum, and required@httpPayloadchecks). Path labels are URL-decoded to catch{Name}placeholders. EmitsBadRequestExceptionbefore handlers run.Bug Fixes
crates/fakecloud-conformance: Forward all shared identifiers to the follow-up Get; fill reader inputs from Create responses when names differ; isolate each round-trip with a unique string; prefer exact-suffix readers infind_round_trip_pairs.crates/fakecloud-apigateway: Validation table fixes —RejectDomainNameAccessAssociationrequires bothdomainNameAccessAssociationArnanddomainNameArnquery params;GetStagesonly requiresrestApiId(keepstageNameforFlushStageCache/FlushStageAuthorizersCache).Written for commit 3ae53c2. Summary will update on new commits. Review in cubic