feat(api): v4.0.1 OpenAPI contract, contract-first source, and specs - #1
Open
nicky-mezzina-lhc wants to merge 50 commits into
Open
feat(api): v4.0.1 OpenAPI contract, contract-first source, and specs#1nicky-mezzina-lhc wants to merge 50 commits into
nicky-mezzina-lhc wants to merge 50 commits into
Conversation
…ent and dependency tracking, also add Makefile with lint command.
…guist-generated files
…es, adjust OpenAPI spec path, and set up CI workflow
…`nc3_testing_platform`, update references, and adjust tooling and CI configuration
…ion logic for `nc3_testing_platform`
…e UTC normalization for `Timestamp`
…or API authentication
…andling and integrate with FastAPI
nicky-mezzina-lhc
added
documentation
📝 WalkthroughWalkthroughAdded the NC3 Testing Platform v4.0.1 MVP foundation. The change defines API and data-model contracts, shared FastAPI infrastructure, scan workflows, resource routers, application assembly, OpenAPI export, documentation, and CI validation. ChangesNC3 Testing Platform MVP
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant ScanLaunchValidation
participant ScanRouter
participant EventStream
Client->>ScanLaunchValidation: Submit JSON or multipart scan launch
ScanLaunchValidation->>ScanRouter: Return validated launch and authentication state
ScanRouter-->>Client: Return queued ScanJobAccepted
Client->>EventStream: Request scan progress
EventStream-->>Client: Emit task, job, heartbeat, and terminal events
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
nicky-mezzina-lhc
requested review from
afrittellalhc
and removed request for
t0kubetsu
August 4, 2026 15:41
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 32-45: Add a test step to the CI workflow that runs pytest to
verify behavior. Insert a new step in the job after the existing Lint, Type
check, Validate OpenAPI spec, and drift guard steps that executes the test
suite. Start by implementing contract tests that verify each router mounts at
its documented path in the OpenAPI spec and that problem_responses produces the
declared HTTP status codes, ensuring handler behavior matches the contract as
handler logic replaces the stubs.
- Around line 21-22: Update the Checkout step using actions/checkout to pin the
action to commit SHA 08c6903cd8c0fde910a37f88322edcfb5dd907a8, retaining the
v5.0.0 comment, and add the persist-credentials input set to false.
In `@api/openapi.json`:
- Around line 8329-8341: Replace the hardcoded placeholder `openIdConnectUrl`
value in the OpenIdConnect security scheme with a reference to the identity
provider URL from core/config.py configuration. Add a top-level servers entry to
the OpenAPI document root that also sources its URL from configuration in
core/config.py. Update api/README.md to document that the committed contract
contains development placeholder values that are replaced at runtime or
deployment from configuration, so a generated client understands the distinction
between the contract-only state and the deployment-ready state.
In `@Makefile`:
- Around line 9-10: Align the lint target with CI by using the same scope in the
Makefile’s lint recipe and the CI Ruff command; prefer repository-root linting
by updating the CI step to check “.” and confirm Ruff exclusions cover generated
or vendored files.
In `@pyproject.toml`:
- Line 8: Update the export-openapi target in the Makefile to invoke the command
through uv run --frozen, preserving the existing export-openapi command and
ensuring dependency resolution uses the lockfile.
In `@src/nc3_testing_platform/core/config.py`:
- Line 19: Add validation for the RETENTION_EXTENSION_DAYS environment variable
to ensure it is greater than zero before creating the timedelta for
RETENTION_EXTENSION. Parse the value from os.getenv, validate that the integer
is positive, and raise an exception or handle the error appropriately if the
value is zero or negative to prevent negative retention extensions from making
scan data prematurely eligible for hard deletion.
In `@src/nc3_testing_platform/core/enums.py`:
- Around line 1-3: Update the module docstring describing the enums to clarify
that not every enum value maps to a PostgreSQL enum; specifically, state that
VerificationStatus.EXPIRED is API-computed and must not be persisted as a value
of the database verification_status enum.
In `@src/nc3_testing_platform/core/errors.py`:
- Around line 111-115: Update register_exception_handlers to register a fallback
handler for the base Exception class that returns ProblemDetail, alongside the
existing HTTP and validation handlers. Ensure unhandled application exceptions
are routed through the problem+json response path rather than FastAPI’s default
ServerErrorMiddleware response.
In `@src/nc3_testing_platform/core/security.py`:
- Around line 32-69: Update require_authentication and the OidcAuth/ApiKeyAuth
dependency flow so credentials are cryptographically and semantically validated
before returning successfully, rather than treating any non-empty header as
authenticated. Verify OIDC signatures, issuer, and expiry, and validate API keys
including revocation and required scopes; ensure every route using Authenticated
receives the verified principal and rejects invalid credentials with the
appropriate unauthorized/forbidden response.
In `@src/nc3_testing_platform/domains/admin/router.py`:
- Line 18: Move the shared ASSET_ID and ORGANIZATION_ID definitions from the
scans examples module into a neutral core examples module, then update the admin
router and scans-domain references to import them from the new location. Remove
the cross-domain import while preserving the existing identifier values and
usage.
- Around line 28-33: Define a distinct PlatformAdmin dependency that validates
the identity provider’s platform-administrator claim, rather than reusing
Authenticated/require_authentication alone. Apply PlatformAdmin to the
/audit-events route’s dependencies while preserving its existing authentication
and response configuration.
In `@src/nc3_testing_platform/domains/api_keys/router.py`:
- Line 42: Replace the "nc3_sk_live_" prefix in both the key_prefix parameter at
line 42 and the equivalent literal at line 80 with an obviously non-live marker
(such as a prefix ending in _test or _fake) to prevent secret scanners from
incorrectly flagging these fabricated sample values. Apply the same prefix
change to the examples list for ApiKeyCreated.secret in
domains/api_keys/schemas.py, then regenerate the OpenAPI contract using make
export-openapi.
- Around line 50-55: Add an MFA dependency alongside the existing Authenticated
dependency in all three API key routes (including the list_api_keys route shown
here and the other two routes). For the create_api_key route specifically, add
validation logic to enforce organization_admin role when the request body has
organization_key set to true. Update the problem_responses documentation in all
three route decorators to include any additional error codes needed for MFA or
authorization failures (such as a 422 response for missing MFA assurance or a
403 response for insufficient organization role).
In `@src/nc3_testing_platform/domains/assets/examples.py`:
- Around line 80-95: Update the DomainVerification fixture construction in the
example factory so failure_code and last_recheck_at are derived from the same
checked transition: only set a failure code when a check occurred and
verification failed, and set last_recheck_at when verification is checked,
including verified challenges. Preserve null values for new pending challenges.
In `@src/nc3_testing_platform/domains/assets/router.py`:
- Around line 225-227: Update the revoked-feed fixture construction around
sample_feed so revoked_at is set to a timestamp at or after feed.last_used_at,
preserving chronological validity before returning feed.
- Around line 195-210: Update create_asset_feed to set Cache-Control: no-store
on the response containing AssetFeedCreated before returning it, using the
endpoint’s response mechanism rather than caching plaintext token data. Verify
the cache middleware preserves this directive for the feed-creation response.
- Around line 125-140: Update create_verification to use a route-specific
dependency that validates current MFA assurance from the OIDC token’s acr or amr
claims before issuing a challenge. Do not rely solely on the existing
Authenticated dependency, which permits API keys and non-MFA tokens; preserve
the current response behavior after assurance validation succeeds.
- Around line 251-254: Update the successful feed Response in the relevant
router handler to include a Cache-Control: no-store header, ensuring
URL-token-authorized feed responses are not cached and revoked tokens reach the
documented 410 path.
- Around line 245-254: Update get_feed to resolve and validate the supplied
token against the stored token hash before returning the Atom response. Return
404 when no matching feed exists and 410 when the matched feed has revoked_at
set; only valid, non-revoked tokens should receive the existing 200 response.
In `@src/nc3_testing_platform/domains/assets/schemas.py`:
- Around line 53-60: Add a shared domain parser that validates and canonicalizes
IDNA names while rejecting schemes, paths, ports, trailing dots, invalid labels,
and non-domain strings. Apply it to both the AssetCreate.value input model and
the stored Asset.value model, and update create_asset to use the normalized
value so all asset-domain boundaries enforce the canonical format.
In `@src/nc3_testing_platform/domains/notifications/router.py`:
- Around line 84-97: Move the dynamic DELETE route for dismiss_notification
below the /webhook routes so the literal webhook path is matched by
delete_webhook before ResourceId validation. Add route coverage confirming
DELETE /notifications/webhook reaches delete_webhook while notification IDs
still reach dismiss_notification.
In `@src/nc3_testing_platform/domains/notifications/schemas.py`:
- Around line 12-17: Replace AnyHttpUrl with an Annotated[AnyUrl,
UrlConstraints(allowed_schemes=["https"], host_required=True)] type in both
OrganizationWebhook and OrganizationWebhookUpsert, updating imports accordingly.
Ensure webhook target validation accepts only HTTPS URLs with a required host.
In `@src/nc3_testing_platform/domains/reports/schemas.py`:
- Around line 27-47: Update the ReportRequest model validator
_exactly_one_source to reject any non-null technical_view when tier is not
ReportTier.TECHNICAL, while preserving the existing exactly-one source
validation and successful technical-tier behavior.
In `@src/nc3_testing_platform/domains/scans/dependencies.py`:
- Line 107: Update the media_type normalization in the request content-type
handling to apply lower-case conversion after stripping parameters and
whitespace, before comparison with the lower-case media type constants. Preserve
the existing parsing behavior and 415 handling for unsupported types.
- Around line 110-116: Update the multipart branch around ResolvedLaunch to
preserve the UploadFile object instead of converting form["file"] with str();
expose it through the appropriate UploadFile-typed field while keeping
FileScanLaunch.file documentation-only, and use request.form() as an async
context manager so the multipart resources are closed after processing.
In `@src/nc3_testing_platform/domains/scans/models.py`:
- Around line 10-15: Track the ORM layer work as a single issue covering
SQLAlchemy and migration-tooling setup, then the four listed data-model tasks:
ScanJob, ScanTask, ScanResult, and declarative-base placement.
In `@src/nc3_testing_platform/domains/scans/repository.py`:
- Around line 13-14: Correct the architecture reference in the TODO for
insert_job_with_tasks and the corresponding second TODO so neither points to the
absent scan-launch-and-upload-handling.md document; either add that document at
the referenced location or update both TODOs to cite the same existing
authoritative source.
In `@src/nc3_testing_platform/domains/scans/router.py`:
- Around line 256-264: Update cancel_scan to return examples.sample_job with
ScanJobStatus.CANCELED. Extend sample_job to accept an optional status_reason
parameter and provide the appropriate cancellation reason when constructing the
canceled job, while preserving the existing default reason for other callers.
- Around line 170-211: Update the get_scan, get_scan_results, and
stream_scan_events route decorators to add openapi_extra={"security":
ANONYMOUS_ALTERNATIVE} and replace problem_responses(401, 404) with
problem_responses(404). Preserve their existing handlers and guest-token
parameters.
- Around line 219-223: Update event_stream to an async generator returning
AsyncIterator[str], and change its iteration to the async form required by
_sample_events if applicable. Configure StreamingResponse with Cache-Control:
no-store and X-Accel-Buffering: no headers, and replace the Iterator import with
AsyncIterator.
In `@src/nc3_testing_platform/domains/scans/schemas.py`:
- Around line 358-360: Update the schema containing the modules field to enforce
both a maximum length equal to the module catalog size and unique module
entries. Before the scan task fan-out, deduplicate modules so repeated requests
cannot create duplicate tasks, while preserving the existing minimum-length
requirement.
- Around line 410-413: Update the GuestScanLaunch.target Field declaration to
enforce the schema boundary contract with min_length=1, max_length=253, and a
hostname pattern accepting canonical IDNA domains without trailing dots. Keep
the existing description and example, and ensure invalid unauthenticated launch
targets fail schema validation with 422 before persistence.
In `@src/nc3_testing_platform/domains/schedules/schemas.py`:
- Around line 40-41: Update the schedule schema fields recurrence_rule and
timezone, including their corresponding recurrence and timezone fields at the
other referenced locations, to validate values before persistence or execution.
Parse recurrence_rule as a valid RFC 5545 rule and reject malformed input;
validate timezone against known IANA timezone identifiers and reject unknown
values while preserving valid inputs.
- Around line 74-80: Update ScheduleUpdate and its merge/application flow to
distinguish omitted fields from explicitly provided null values. Ensure omitted
fields remain unchanged while null is rejected or otherwise prevented from being
assigned to the non-nullable Schedule fields modules, module_configuration,
recurrence_rule, timezone, and enabled; use Pydantic unset tracking such as
exclude_unset or an equivalent sentinel-based approach.
In `@src/nc3_testing_platform/tools/export_openapi.py`:
- Line 19: Update the DEST.write_text call in the OpenAPI export flow to
explicitly pass encoding="utf-8", preserving the existing JSON serialization and
trailing newline.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4ea6a23b-7dfb-4ede-b1af-1ad0f9ac1313
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
.editorconfig.gitattributes.github/workflows/ci.yml.python-versionMakefileREADME.mdapi/README.mdapi/openapi.jsondocs/reference/api-design-v4_0_1.mddocs/reference/data-model-v4_0_1.mdpyproject.tomlsrc/nc3_testing_platform/__init__.pysrc/nc3_testing_platform/core/__init__.pysrc/nc3_testing_platform/core/config.pysrc/nc3_testing_platform/core/enums.pysrc/nc3_testing_platform/core/errors.pysrc/nc3_testing_platform/core/openapi.pysrc/nc3_testing_platform/core/pagination.pysrc/nc3_testing_platform/core/schemas.pysrc/nc3_testing_platform/core/security.pysrc/nc3_testing_platform/domains/__init__.pysrc/nc3_testing_platform/domains/admin/__init__.pysrc/nc3_testing_platform/domains/admin/router.pysrc/nc3_testing_platform/domains/admin/schemas.pysrc/nc3_testing_platform/domains/api_keys/__init__.pysrc/nc3_testing_platform/domains/api_keys/router.pysrc/nc3_testing_platform/domains/api_keys/schemas.pysrc/nc3_testing_platform/domains/assets/__init__.pysrc/nc3_testing_platform/domains/assets/examples.pysrc/nc3_testing_platform/domains/assets/router.pysrc/nc3_testing_platform/domains/assets/schemas.pysrc/nc3_testing_platform/domains/findings/__init__.pysrc/nc3_testing_platform/domains/findings/router.pysrc/nc3_testing_platform/domains/health/__init__.pysrc/nc3_testing_platform/domains/health/router.pysrc/nc3_testing_platform/domains/notifications/__init__.pysrc/nc3_testing_platform/domains/notifications/router.pysrc/nc3_testing_platform/domains/notifications/schemas.pysrc/nc3_testing_platform/domains/org/__init__.pysrc/nc3_testing_platform/domains/org/router.pysrc/nc3_testing_platform/domains/org/schemas.pysrc/nc3_testing_platform/domains/reports/__init__.pysrc/nc3_testing_platform/domains/reports/router.pysrc/nc3_testing_platform/domains/reports/schemas.pysrc/nc3_testing_platform/domains/scans/__init__.pysrc/nc3_testing_platform/domains/scans/dependencies.pysrc/nc3_testing_platform/domains/scans/examples.pysrc/nc3_testing_platform/domains/scans/models.pysrc/nc3_testing_platform/domains/scans/repository.pysrc/nc3_testing_platform/domains/scans/router.pysrc/nc3_testing_platform/domains/scans/schemas.pysrc/nc3_testing_platform/domains/scans/service.pysrc/nc3_testing_platform/domains/schedules/__init__.pysrc/nc3_testing_platform/domains/schedules/router.pysrc/nc3_testing_platform/domains/schedules/schemas.pysrc/nc3_testing_platform/domains/statements/__init__.pysrc/nc3_testing_platform/domains/statements/router.pysrc/nc3_testing_platform/domains/statements/schemas.pysrc/nc3_testing_platform/main.pysrc/nc3_testing_platform/tools/__init__.pysrc/nc3_testing_platform/tools/export_openapi.py
Comment on lines
+358
to
+360
| modules: list[ScanModule] = Field( | ||
| min_length=1, description="One or more modules to run against the target." | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Bound the modules list.
modules has min_length=1 but no upper bound and no uniqueness rule. A caller can repeat the same module many times. Each entry fans out into tasks, so the request size controls the task count. Add max_length matching the module catalog size, and deduplicate before fan-out.
♻️ Proposed constraint
modules: list[ScanModule] = Field(
- min_length=1, description="One or more modules to run against the target."
+ min_length=1,
+ max_length=len(ScanModule),
+ description="One or more modules to run against the target.",
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nc3_testing_platform/domains/scans/schemas.py` around lines 358 - 360,
Update the schema containing the modules field to enforce both a maximum length
equal to the module catalog size and unique module entries. Before the scan task
fan-out, deduplicate modules so repeated requests cannot create duplicate tasks,
while preserving the existing minimum-length requirement.
Comment on lines
+40
to
+41
| recurrence_rule: str = Field(description=_RECURRENCE_DESCRIPTION) | ||
| timezone: str = Field(description=_TIMEZONE_DESCRIPTION) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Validate recurrence rules and timezone identifiers.
These fields accept every string. Values such as recurrence_rule="invalid" and timezone="Mars/Base" therefore satisfy the API contract.
Reject malformed RFC 5545 rules and unknown IANA timezone identifiers before a schedule reaches persistence or execution.
Also applies to: 62-63, 76-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nc3_testing_platform/domains/schedules/schemas.py` around lines 40 - 41,
Update the schedule schema fields recurrence_rule and timezone, including their
corresponding recurrence and timezone fields at the other referenced locations,
to validate values before persistence or execution. Parse recurrence_rule as a
valid RFC 5545 rule and reject malformed input; validate timezone against known
IANA timezone identifiers and reject unknown values while preserving valid
inputs.
…te README and Makefile for test integration
…eplace OpenAPI spec validation with pytest
t0kubetsu
requested review from
afrittellalhc
and removed request for
t0kubetsu
August 5, 2026 08:54
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nc3_testing_platform/domains/scans/schemas.py (1)
178-233: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the exactly-one target invariant in
ScanTaskandScanJob.All target fields default to
None, and neither model has a cross-field validator. Add a sharedmodel_validator(mode="after")or a discriminated union. Test each valid variant and invalid zero/multiple-target combinations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nc3_testing_platform/domains/scans/schemas.py` around lines 178 - 233, Add shared cross-field validation for the target fields in both ScanTask and ScanJob, enforcing that exactly one of target_asset_id, target_domain, and file_upload_id is set. Use a model_validator(mode="after") or equivalent discriminated-union approach, reject zero or multiple targets, and add tests covering each valid variant plus invalid combinations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/api-design-v4_0_1.md`:
- Line 15: Update the collection cursor specification to guarantee a monotonic
total ordering: include (created_at, id) in both ordering and cursor encoding,
or define and use a shared monotonic UUIDv7 generator across all writers with
concurrent-allocation and clock-rollback tests.
- Line 36: Update the Guest target and web-task contract in the API design
document to define egress controls before enabling unauthenticated web tasks:
validate resolved IPv4 and IPv6 destinations before every connection, prevent
DNS rebinding, restrict redirects, and specify the guest egress policy. Keep the
existing target canonicalization and rejection behavior intact.
- Line 149: Update the asset deletion documentation to cover all restricted
foreign-key blockers, including domain_verification.asset_id, schedule.asset_id,
and asset_feed.asset_id, and define the required transactional cleanup or
document their 409 behavior. Then regenerate api/openapi.json so the API
specification matches the updated DELETE contract.
In `@docs/reference/data-model-v4_0_1.md`:
- Line 783: Update the scan_job row constraint to enforce that guest jobs have
no organization and non-guest jobs have an organization, matching the
relationship documented on lines 416 and 420. Replace the current OR condition
with the corresponding mutually exclusive claim/organization predicate.
- Around line 789-790: Update the scan_task constraints in the data model so
every row with module = 'file' requires a non-null file_upload_id, while
retaining the existing rule that file_upload_id may only reference File tasks.
Preserve the classification and target_domain constraints, and express the
upload relationship as a bidirectional invariant.
- Around line 769-770: The domain_verification CHECK constraints currently
require verified_scope and verified_at to be null whenever status is pending,
conflicting with retained proof data during re-verification. Update the
constraints for domain_verification so both fields are required when status is
verified but may remain populated for pending challenges, preserving the
documented re-proving behavior.
In `@README.md`:
- Around line 51-53: Update the documented development routine in README.md to
include make test alongside make export-openapi and make lint. Keep the existing
guidance about committing api/openapi.json and validation behavior unchanged.
In `@src/nc3_testing_platform/core/schemas.py`:
- Around line 34-49: Update _parse_domain_name to pass the original value to
idna.encode, then remove exactly one trailing dot from the encoded result before
validation and return. Preserve acceptance of one trailing ASCII or Unicode
separator while allowing IDNA to reject additional separators such as
example.com..; add regression coverage for both separator forms and
maximum-length domains.
---
Outside diff comments:
In `@src/nc3_testing_platform/domains/scans/schemas.py`:
- Around line 178-233: Add shared cross-field validation for the target fields
in both ScanTask and ScanJob, enforcing that exactly one of target_asset_id,
target_domain, and file_upload_id is set. Use a model_validator(mode="after") or
equivalent discriminated-union approach, reject zero or multiple targets, and
add tests covering each valid variant plus invalid combinations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3086c04a-27b6-4254-9a64-37f6165e96fe
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/dependabot.yml.github/workflows/ci.ymlREADME.mdapi/openapi.jsondocs/reference/api-design-v4_0_1.mddocs/reference/data-model-v4_0_1.mdpyproject.tomlsrc/nc3_testing_platform/core/schemas.pysrc/nc3_testing_platform/domains/assets/router.pysrc/nc3_testing_platform/domains/assets/schemas.pysrc/nc3_testing_platform/domains/scans/schemas.pysrc/nc3_testing_platform/tools/export_openapi.pytests/test_openapi_export.py
| | `scan_job` | `source <> 'guest' OR claimed_at IS NOT NULL OR claim_token_hash IS NOT NULL` | an unclaimed guest job always holds the claim hash | | ||
| | `scan_job` | `(claimed_at IS NULL) = (claimed_by_user_id IS NULL)` | claiming records the actor and the time together | | ||
| | `scan_job` | `claimed_at IS NULL OR claim_token_hash IS NULL` | the hash is discarded on claim | | ||
| | `scan_job` | `organization_id IS NOT NULL OR source = 'guest'` | only guest jobs lack an organization | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce the claim and organization relationship in the row constraint.
The current condition allows an unclaimed guest job with an organization and a claimed guest job without one. This contradicts Lines 416 and 420 and can break tenant isolation.
Proposed constraint
- | `scan_job` | `organization_id IS NOT NULL OR source = 'guest'` | only guest jobs lack an organization |
+ | `scan_job` | `((source = 'guest' AND claimed_at IS NULL) = (organization_id IS NULL))` | organization is absent only before a guest claim |📝 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
| | `scan_job` | `organization_id IS NOT NULL OR source = 'guest'` | only guest jobs lack an organization | | |
| | `scan_job` | `((source = 'guest' AND claimed_at IS NULL) = (organization_id IS NULL))` | organization is absent only before a guest claim | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/data-model-v4_0_1.md` at line 783, Update the scan_job row
constraint to enforce that guest jobs have no organization and non-guest jobs
have an organization, matching the relationship documented on lines 416 and 420.
Replace the current OR condition with the corresponding mutually exclusive
claim/organization predicate.
Comment on lines
+51
to
+53
| The development routine after any change to a router or Pydantic schema is `make export-openapi && make lint`. `api/openapi.json` is the contract the frontend interfaces with; commit it alongside the change that alters it. | ||
|
|
||
| `make test` validates the generated document against OpenAPI 3.1 and fails if the committed file differs from it. CI runs the same command. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include make test in the documented development routine.
Line 51 instructs contributors to run only make export-openapi && make lint. Line 53 states that make test validates OpenAPI 3.1 and detects contract drift. Add make test to the routine so local instructions perform the required checks.
Proposed documentation fix
-The development routine after any change to a router or Pydantic schema is `make export-openapi && make lint`.
+The development routine after any change to a router or Pydantic schema is `make export-openapi && make lint && make test`.📝 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
| The development routine after any change to a router or Pydantic schema is `make export-openapi && make lint`. `api/openapi.json` is the contract the frontend interfaces with; commit it alongside the change that alters it. | |
| `make test` validates the generated document against OpenAPI 3.1 and fails if the committed file differs from it. CI runs the same command. | |
| The development routine after any change to a router or Pydantic schema is `make export-openapi && make lint && make test`. `api/openapi.json` is the contract the frontend interfaces with; commit it alongside the change that alters it. | |
| `make test` validates the generated document against OpenAPI 3.1 and fails if the committed file differs from it. CI runs the same command. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 51 - 53, Update the documented development routine in
README.md to include make test alongside make export-openapi and make lint. Keep
the existing guidance about committing api/openapi.json and validation behavior
unchanged.
t0kubetsu
requested changes
Aug 5, 2026
t0kubetsu
left a comment
t0kubetsu
left a comment
There was a problem hiding this comment.
Review — v4.0.1 API contract (PR #1)
Method (per the review-guidance page and the US #151 traceability comment): sectioned review with file:line anchors; findings classified as defect (internal inconsistency between repository artifacts), decision item (divergence from an external draft — for the task owner, not necessarily a change), or preference (optional). Traceability checked feature-scope-fit, not one-endpoint-per-requirement. The §4 Deferred table was screened before reporting any omission.
Validation (all green)
ruff check src clean · pytest 6/6 · openapi-spec-validator --schema 3.1 OK · drift guard reproduces the committed contract · pyright src 0 errors.
Re-run of the §3 preliminary verifications: endpoint inventory ↔ spec (56 operations, both directions) ✓ · exactly the seven declared anonymous operations ✓ · four findings filters ✓ · problem+json on every declared 4xx/5xx including all 52 declared 422s ✓ · retention rules consistent across schemas, examples, and both docs ✓ · enum claim ✗ (two mismatches — A4, A16 below).
A. Defects (internal inconsistencies — please fix in this PR)
High
- A1 — GDPR erasure deadlock via claimed guest jobs.
docs/reference/data-model-v4_0_1.md:781CHECK(claimed_at IS NULL) = (claimed_by_user_id IS NULL)contradicts:391claimed_by_user_id … ON DELETE SET NULL: erasing any user who ever claimed a guest scan fires SET NULL whileclaimed_atstays set, violating the CHECK and aborting theapp_userDELETE — breaking the 30-day erasure guarantee (:190). The temporal pattern is already handled correctly forsource = 'manual'at:805; this constraint needs the same one-way form (claimed_by_user_id IS NULL OR claimed_at IS NOT NULL). - A2 — Same defect class on invitations.
data-model-v4_0_1.md:767CHECK(accepted_at IS NULL) = (accepted_by_user_id IS NULL)vs:169accepted_by_user_id … ON DELETE SET NULL— erasing a user who accepted an invitation blocks the DELETE. - A3 — Re-verification is unexpressible in the single-row model.
data-model-v4_0_1.md:769-770biconditional CHECKs(status='verified') = (verified_scope IS NOT NULL)/(= verified_at IS NOT NULL)plus UNIQUEasset_id(:229, "current state only":257) cannot represent "still verified at the old scope while a new challenge is pending" — the lifecycle stated at:254andapi-design-v4_0_1.md:171. (Confirms the open CodeRabbit finding at data-model:770 — the spec must pick a model.) - A4 —
trend/TrendDirectionexist in the contract but in neither reference doc.api/openapi.jsonexposesScanResult.trendand aTrendDirectioncomponent (improving/unchanged/declining;core/enums.py:140-145,domains/scans/schemas.py:115-132,167), yetdata-model-v4_0_1.md§2.4/§8.1 andapi-design-v4_0_1.mdnever mention it. Also falsifies thecore/enums.py:3docstring claim that every enum mirrors a PostgreSQL enum. - A5 —
DomainNamecanonicalization bug (reproduced).core/schemas.py:49:'example.com..'canonicalizes to'example.com.'(trailing dot survives) and'example.com。'(U+3002) likewise yields'example.com.'— both violate the documented invariant and the asset-uniqueness guarantee. This is shipped validation logic, not a stub. (Backs the open CodeRabbit finding; regression tests for both inputs recommended.)
Medium
- A6 — Authenticated access to the guest scan reads is not expressible from the contract.
api-design-v4_0_1.md:9says a ScanJob is read through/scans"whether the caller is authenticated or a guest", butGET /scans/{scan_id},/results,/eventsdeclare no security scheme (:69/api/openapi.json), unlikePOST /scans([{OpenIdConnect},{ApiKey},{}],domains/scans/router.py:126-129). A generated client will never attach credentials to those reads, so an org member reading a claimed scan has no declared auth path. - A7 — Pagination guarantee falsified by three operations.
api-design-v4_0_1.md:15("Collection endpoints use cursor pagination", no exceptions) vs bare arrays onGET /scans/{scan_id}/results(domains/scans/router.py:188-201),GET /assets/{asset_id}/feeds(domains/assets/router.py:187-193),GET /statements(domains/statements/router.py:24-29). Either carve them out in the doc or returnPage[T]. - A8 — Claim↔organization invariant not row-enforced.
data-model-v4_0_1.md:420states the claimed job'sorganization_idis the claimant's org, but the only CHECK (:783) passes a claimed job with NULLorganization_id— despite §14's preamble (:763) that every row-expressible invariant is carried in DDL. A claim transaction that skipsorganization_idcommits cleanly and the job becomes invisible to org-scoped RLS history. (Confirms the open CodeRabbit finding.) - A9 — File-task/upload CHECK is one-way.
data-model-v4_0_1.md:790permits amodule='file'task withtarget_asset_id/target_domain, which no File test can execute (:361,:467-470). Should be biconditional, mirroring:789. (Confirms the open CodeRabbit finding.) - A10 — Undocumented asset-deletion blockers.
data-model-v4_0_1.md:221("referencing FKs restrict, never cascade or set null") makesdomain_verificationandasset_feedrows permanent blockers, whileapi-design-v4_0_1.md:149says DELETE 409s only "while scan history or discovered children reference the asset" — an asset that ever had a verification or feed is undeletable under the stated rules. (Confirms the open CodeRabbit finding.) - A11 — Media type compared case-sensitively.
domains/scans/dependencies.py:107— RFC 9110 media types are case-insensitive; this dispatcher is real shipped logic that selects thePOST /scansrequest schema. - A12 — Mock fixtures contradict the model's own rules (the live mock is a frontend deliverable):
domains/assets/examples.py:95(pending verification withfailure_codebut nulllast_recheck_at, violating the data model's own CHECK),domains/assets/router.py:229-230(feedlast_used_atafterrevoked_at),domains/scans/router.py:264(cancel returns a non-canceledsample_job()). - A13 — No fallback problem+json handler.
core/errors.pyclaims every error is aProblemDetail, but an unhandled exception returns FastAPI's default 500 — the stated contract doesn't hold at runtime. (Backs the open CodeRabbit finding.) - A14 —
technical_viewaccepted on a non-technical report.domains/reports/schemas.py:41-47validates source exclusivity but not the data model's own row CHECK (tier='technical' OR technical_view IS NULL). (Backs the open CodeRabbit finding.) - A15 — Unvalidated settings parsing.
core/config.py:8-19bareos.getenv+int()— a negativeRETENTION_EXTENSION_DAYSsilently movespurge_atbackwards; a non-numeric value crashes at import. (Backs the open CodeRabbit finding.)
Low
- A16 —
data-model-v4_0_1.md:63listsverification_statusaspending, verifiedwhilecore/enums.py:38-43and the contract carryexpired(API-computed per:252) — the §3 "every enum matches" claim and theenums.py:3docstring both need the exception stated. - A17 —
api-design-v4_0_1.md:212names report source fieldsscan_job_id/scan_task_id; the schema and data model both saysource_scan_job_id/source_scan_task_id(domains/reports/schemas.py:34-45, data-model:606-607). - A18 —
data-model-v4_0_1.md:523finding.external_referencesreads "Not null; default" with the default value missing (compare:497). - A19 —
GET /statementsis the only operation declaring a 500 (domains/statements/router.py:27) — no artifact explains the exception. - A20 — MFA gate on
POST /assets/{asset_id}/verificationasserted in code (domains/assets/router.py:140) but absent from api-design §5.1 (the API-key gate, by contrast, is stated in both). - A21 —
domains/scans/repository.py:14TODOs cite a nonexistent architecture document;README.md:53dev routine omitsmake test.
Contract-quality tightening (schema constraints a generated client can't recover otherwise)
claim_tokenfields are unconstrainedstr(domains/scans/schemas.py:446,dependencies.py:40-50) though the generation rule is exact (43-char base64url) — siblingsigning_secretalready usesmin_length.DomainNamerenders as a bare{"type":"string"}in the published schema (core/schemas.py:34-55) — the 253-char bound and IDNA shape live only in the server-side validator.- The documented "exactly one of" target invariant on
ScanJob/ScanTask(domains/scans/schemas.py:178-234, 236-317) has nomodel_validator, unlikeReportRequestwhich enforces its XOR. moduleshas nomax_length/uniqueness andstatement_responsesis unbounded (domains/scans/schemas.py:359-371) — a contract-level ceiling caps guest-triggered fan-out. (Backs the open CodeRabbit finding.)OrganizationWebhookUpsert.endpoint_urlshould be HTTPS-only now — tightening later is a breaking contract change. (Backs the open CodeRabbit finding.)AssetFeedCreated.feed_urlisstr, notAnyHttpUrl(domains/assets/schemas.py:188-191), unlikeendpoint_url.Cache-Control: no-storeon the three responses that carry plaintext secrets (API key, feed token, feed delivery) is cheap and belongs in the contract. (Backs the open CodeRabbit findings.)
B. Decision items (divergences from external drafts — for the task owner; reconciliation-log entries, not necessarily changes)
High
- B1 — No organization resource. v0.6 had
GET/PATCH /org; the contract has only members/invitations/webhook. No operation returns the caller's organization name (organization_nameexists only on the invitee-facingInvitationPreview;GET /accountreturns onlyorganization_id). The UI-05-01 org page has nothing to read.organization.settings/white-label are deferred (§4/§16) — the org identity read is not. - B2 — Dashboard aggregate (US #151.11) reversed without a deferral entry. No
/dashboardpath or domain. Findings and activity widgets are servable fromGET /findings/GET /scans; asset badges need per-asset calls; posture and warnings have no data source at all. If per-widget/frontend-composed is the intent, it needs a Deferred-table or reconciliation entry plus frontend guidance.
Medium
- B3 — Reports flow overturns the recorded 2026-07-29 architecture decision (202 + status resource + one-shot download + regeneration + SSE
report-ready): now synchronousPOST /reportsreturning document bytes, no report id in the response (api-design-v4_0_1.md:220acknowledges a download can't be matched to its metadata row), regeneration = re-POST, 409 afterpurge_at. Arguably simpler and "artefacts never stored" survives — but it reverses a dated decision and voids the frontend's async-report flow; needs explicit sign-off. - B4 — SSE contract rewritten. v0.6 envelope
{schema_version, job_id, seq, ts, type, data}+ 9 event types → 4 events (task/job/heartbeat/end), noseq/Last-Event-IDresume (snapshot refetch is the recovery), nopercent, noqueue_position, nodiscovery.progress. UI-01-03 stays servable (snapshot + terminal-task counting, guests included), but every planned SSE consumer changes. - B5 — Verification lifecycle drops
Suspended. US #151 / UI-06 lifecycle was Pending/Verified/Expired/Suspended; the contract haspending/verified/expiredandverifiedis terminal (api-design-v4_0_1.md:170-173). Not in the Deferred table. - B6 — Notification settings collapsed. v0.6
GET/PUT /notifications/settings(user AND org, distinct) → oneemail_notifications_enabledboolean onPATCH /account+ the singleton org webhook. Per-type/per-channel preferences are stranded until ≥4.1organization.settings. - B7 — Job status vocabulary.
succeeded/timed_out→completed/failed+status_reason;partialredefined (usable-results-after-termination);canceledadded. All v0.6 outcomes remain expressible; frontend mapping changes. - B8 — Retention moved per-scan. Org-level
GET /org/retention+ extension + deletion-confirmation →POST /scans/{scan_id}/retention/extend(no body, server-configured delta) +DELETE /scans/{scan_id}; the confirm-before-purge handshake became a 30-day notice. No org-wide retention overview. - B9 — Asset trends (UI-03-04) have no series contract.
ScanResult.trendis a one-step delta; a grade/severity time series requires oneGET /scans/{id}/resultsper historical job (N+1). Servable, but expensively. - B10 — Membership restructured.
POST/DELETE /org/members→ invitation flow +disable/enable; removal not modeled (one org for the life of the account).
Low
- B11 — Guest launch broadened from v0.6's single module to "one or more" (non-intrusive enforced); no adaptive-challenge negotiation surface reserved on
POST /scans(challenge left to standards per the deferral note). - B12 — Rate-limit surfacing (
RateLimit/Retry-After+ 429) declared only onPOST /scans; the v0.6 open questionGET /quotaresolved to nothing; the 5-min per-target cooldown is unsurfaced. - B13 —
schema_versiondeliberately narrowed to the three out-of-document payloads (scan results, webhooks, notificationdata) — coherent, documented atapi-design-v4_0_1.md:16. - B14 — Faithful renames/moves: API-key revoke as POST with row kept; feeds nested per-asset; guest scope folded into
/scans; GDPR export moved to a separate workflow (§12); no v4.0 test intrusive → all MFA/attestation/re-verification gates dormant by design.
C. Preferences (optional)
HealthStatus.status as free string vs enum (domains/health/router.py:14-29); redundant response_model on launch_scan (domains/scans/router.py:125); nc3_sk_live_ sample prefix permanently trips secret scanners; shared sample IDs imported across domain slices (domains/admin/router.py:18); SSE stub as sync def without no-store.
CodeRabbit thread disposition (44 threads)
- Verified fixed in tree or withdrawn — safe to resolve: checkout pinning, CI test step,
.invalidIdP URL (withdrawn), lint scope,AssetCreateDomainName, DELETE route shadowing, anonymous-op declaration (fixed differently: security removed + test pins the set), guest-target validation, UTF-8 export, error-content assertion. - Backed as fix-in-this-PR: the parser bug, fixture validity, media-type casing, fallback handler, the three data-model CHECK findings, asset-deletion gap, HTTPS-only webhook, report tier validator, modules bound.
- Defer with a single tracking issue (auth family): credential verification, admin claim, API-key MFA, verification MFA, feed-token validation — contract-only by design; the issue should gate any non-mock deployment.
- Defer to implementation: RRULE/timezone validation,
UploadFilehandling,ScheduleUpdatenull-vs-omission (support CodeRabbit's drafted follow-up issue), SSRF/egress controls (open a spec issue before scan workers exist). - Pushed back: UUIDv7 cursor ordering — a stable total order is what cursor pagination needs, and the unique id provides it.
Verdict: request changes — driven by the Section A defects (A1–A5 in particular). Section B needs reconciliation-log/sign-off decisions, not necessarily code changes. The deliverable is otherwise in strong shape: all CI gates green, the §3 mechanical claims held up on re-verification except the enum claim, and the restructurings (guest folding, statements generalization, invitation flow) are coherent and mostly well documented.
This was referenced Aug 5, 2026
…x` and add test scenarios. * Labels are split on all four Unicode full stops, and a root dot survives encoding. * Removing that dot before encoding would hide an empty label and let a non-ASCII separator reappear as a trailing dot.
…lose reverification gaps
…ceptance/claim relationships
|
Two additional findings from today's architecture gap pass (Docmost → Runtime & lifecycle views, 2026-08-05), classified per the review guidance: A21 — Password change referenced but not defined (internal inconsistency)
Context that raises the stakes: the architecture direction recorded today (Runtime & lifecycle views v0.2, IdP-agnostic pass) is that the platform's own auth layer is the identity provider by default, with an external OIDC IdP as an opt-in deployment. Under that model the whole credential family — registration, login/logout, password change, MFA factor enrollment/verification — moves into the platform's surface rather than being "IdP-owned, out of contract". Suggestion: fold this into the auth-family tracking issue (#4) with the default-IdP operation set enumerated, and either define the operations or record the deferral explicitly in §4 so the §13 / data-model §9.2 references stop dangling. A22 — Statement acceptance is write-only; the client cannot know what to prompt (requirement not expressible)§14 ( Suggestion: either a Related (already visible elsewhere, noted for the same thread): gated operations "consume current MFA assurance" (§2.1, §13) but no RFC 9457 problem |
…pendencies and update `uv.lock`
…57 compliance in error responses - Introduced `_unhandled_exception_handler` to standardize responses for uncaught exceptions. - Updated `register_exception_handlers` to include the new handler. - Added tests to validate RFC 9457 compliance and ensure exception details do not leak to clients.
…media types in `POST /scans` - Introduced tests to ensure JSON `Content-Type` handling is case-insensitive. - Added validation for rejecting unsupported media types as per RFC standards.
…update `uv.lock`
…dling to specify `source_scan_job_id` or `source_scan_task_id`.
…quirement in verification flow
…authentication support on operations - Implemented `OptionallyAuthenticated` in `core.security` for operations accommodating both authentication and claim tokens. - Applied the new dependency to scan-related endpoints for enhanced flexibility in access control. - Added extra OpenAPI metadata for anonymous access representation. - Introduced validation for `technical_view` restriction in `ReportRequest` schema.
…uest and owner access scenarios
…onal reference types
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/openapi.json`:
- Around line 213-221: Replace the empty security alternatives with the
ScanClaimToken security requirement in api/openapi.json at lines 213-221,
367-375, and 452-460, preserving the existing OpenIdConnect and ApiKey
alternatives. Define claim_token as the named guest credential for the OpenAPI
security scheme in docs/reference/api-design-v4_0_1.md at line 73.
- Line 1361: Update the OpenAPI 403 response definitions for this operation and
every operation requiring current MFA assurance to use a dedicated MFA-required
Problem Details contract with the stable type urn:nc3:problem:mfa-required
instead of only generic ProblemDetail. Define client-action fields only if they
are specified by the identity-provider step-up flow, and keep unrelated 403
responses unchanged.
In `@docs/reference/api-design-v4_0_1.md`:
- Around line 178-179: Update the MFA-gated verification operation documentation
and its corresponding OpenAPI security definition to remove ApiKey as an
authorization alternative; require identity-provider session or token assurance,
or an explicitly verifiable step-up credential, so static API keys cannot
satisfy current MFA assurance.
In `@docs/reference/data-model-v4_0_1.md`:
- Around line 262-269: Update the documented domain verification lifecycle to
define an atomic retry flow that removes or replaces an expired
domain_verification_challenge before creating a new challenge, accounting for
the unique asset_id constraint. Preserve the expired attempt in audit_event, and
keep the existing behavior for successful challenges and active challenge
replacement unchanged.
In `@src/nc3_testing_platform/domains/assets/examples.py`:
- Around line 91-103: Update sample_verification so sample_challenge is created
only when status is VerificationStatus.PENDING; keep challenge unset for
VERIFIED and EXPIRED statuses while preserving the existing checked argument for
pending verifications.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 536c9c7e-7ffb-46d4-b64f-313c71f23fb3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
api/openapi.jsondocs/reference/api-design-v4_0_1.mddocs/reference/data-model-v4_0_1.mdpyproject.tomlsrc/nc3_testing_platform/core/errors.pysrc/nc3_testing_platform/core/schemas.pysrc/nc3_testing_platform/core/security.pysrc/nc3_testing_platform/domains/api_keys/router.pysrc/nc3_testing_platform/domains/assets/examples.pysrc/nc3_testing_platform/domains/assets/router.pysrc/nc3_testing_platform/domains/assets/schemas.pysrc/nc3_testing_platform/domains/reports/schemas.pysrc/nc3_testing_platform/domains/scans/dependencies.pysrc/nc3_testing_platform/domains/scans/repository.pysrc/nc3_testing_platform/domains/scans/router.pytests/test_domain_name.pytests/test_error_contract.pytests/test_launch_dispatch.pytests/test_openapi_export.py
| "assets" | ||
| ], | ||
| "summary": "Start a verification challenge", | ||
| "description": "Issue a challenge at the requested coverage.\n\nRequires current MFA assurance, read from the OIDC token rather than from any\nstored flag — proving control of a domain is what later authorizes scanning it.\n\nOn an already-verified asset the response carries both the standing proof and\nthe new challenge, so coverage in force is never withdrawn while ownership is\nre-proven.", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 \
'current MFA assurance|mfa-required|mfa_required|403|ProblemDetail' \
docs/reference/api-design-v4_0_1.md \
api/openapi.json \
src/nc3_testing_platformRepository: NC3-TestingPlatform/testing-platform-backend
Length of output: 50401
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
spec = json.loads(Path("api/openapi.json").read_text())
paths = spec.get("paths", {})
print("MFA-related operations:")
for path, item in paths.items():
for method, op in item.items():
if method.lower() not in {"get", "post", "put", "patch", "delete", "head", "options", "trace"}:
continue
text = json.dumps(op).lower()
if "mfa" in text or "assurance" in text:
print(f"{method.upper()} {path}")
print(" operationId:", op.get("operationId"))
print(" security:", op.get("security"))
print(" responses:", sorted(op.get("responses", {}).keys()))
for code, response in op.get("responses", {}).items():
if code == "403":
print(" 403:", json.dumps(response, separators=(",", ":")))
print("\nProblemDetail schema:")
print(json.dumps(spec.get("components", {}).get("schemas", {}).get("ProblemDetail"), indent=2))
print("\nSchemas containing MFA-like fields:")
for name, schema in spec.get("components", {}).get("schemas", {}).items():
if "mfa" in json.dumps(schema).lower() or "assurance" in json.dumps(schema).lower():
print(name, json.dumps(schema, separators=(",", ":")))
PY
printf '\nDesign-document MFA/auth excerpts:\n'
rg -n -C 6 -i \
'MFA|assurance|step.?up|identity provider|problem.?detail|RFC 9457|client.action|redirect|reauth' \
docs/reference/api-design-v4_0_1.md \
docs README.md 2>/dev/null | head -n 300
printf '\nImplementation references:\n'
rg -n -C 4 -i \
'MFA|assurance|step.?up|ProblemDetail|problem\+json|oidc' \
src tests 2>/dev/null | head -n 400Repository: NC3-TestingPlatform/testing-platform-backend
Length of output: 50402
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' 'Relevant route declarations and MFA text:'
rg -n -C 12 \
'create_verification|create_api_key|launch_scan|current MFA assurance|require.*MFA|mfa' \
src/nc3_testing_platform/domains/assets/router.py \
src/nc3_testing_platform/domains/api_keys/router.py \
src/nc3_testing_platform/domains/scans/router.py \
src/nc3_testing_platform/core/security.py
printf '\n%s\n' 'All OpenAPI operation descriptions and response metadata containing MFA/security terms:'
python3 - <<'PY'
import json
from pathlib import Path
spec = json.loads(Path("api/openapi.json").read_text())
for path, item in spec["paths"].items():
for method, op in item.items():
if method not in {"get", "post", "put", "patch", "delete", "head", "options", "trace"}:
continue
blob = json.dumps(op).lower()
if any(term in blob for term in ("mfa", "assurance", "step-up", "step_up")):
print(f"{method.upper()} {path}")
print("description:", op.get("description", ""))
print("security:", op.get("security"))
for status, response in op.get("responses", {}).items():
if status == "403":
print("403:", json.dumps(response, indent=2))
print()
printf '\n%s\n' 'Tests that define or imply an MFA Problem Details contract:'
rg -n -C 8 -i \
'mfa|assurance|step.?up|403|ProblemDetail|problem type|urn:' \
tests src/nc3_testing_platform \
| grep -vE '(^|/)(openapi\.json|__pycache__|\.pyc)' \
| head -n 500Repository: NC3-TestingPlatform/testing-platform-backend
Length of output: 13792
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' 'MFA-related OpenAPI operations:'
python3 - <<'PY'
import json
from pathlib import Path
spec = json.loads(Path("api/openapi.json").read_text())
for path, item in spec["paths"].items():
for method, op in item.items():
if method not in {"get", "post", "put", "patch", "delete", "head", "options", "trace"}:
continue
blob = json.dumps(op).lower()
if any(term in blob for term in ("mfa", "assurance", "step-up", "step_up")):
print(f"{method.upper()} {path}")
print(" operationId:", op.get("operationId"))
print(" security:", op.get("security"))
print(" description:", op.get("description", "").replace("\n", " "))
print(" 403:", json.dumps(op.get("responses", {}).get("403")))
PY
printf '\n%s\n' 'API-key route declarations:'
sed -n '1,180p' src/nc3_testing_platform/domains/api_keys/router.py
printf '\n%s\n' 'Tests or contract text defining an MFA problem type or client-action fields:'
rg -n -C 5 -i \
'mfa-required|mfa_required|step.?up|assurance|client.?action|reauth|redirect|challenge|problem type|urn:' \
tests src docs/reference README.md \
| head -n 500Repository: NC3-TestingPlatform/testing-platform-backend
Length of output: 50402
Define a step-up MFA Problem Details type.
This operation requires current MFA assurance, but its 403 response references only the generic ProblemDetail schema. Add a stable type, such as urn:nc3:problem:mfa-required, and apply the same contract to all operations that can reject a request for missing MFA assurance. Define client-action fields only when the identity-provider step-up flow specifies them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/openapi.json` at line 1361, Update the OpenAPI 403 response definitions
for this operation and every operation requiring current MFA assurance to use a
dedicated MFA-required Problem Details contract with the stable type
urn:nc3:problem:mfa-required instead of only generic ProblemDetail. Define
client-action fields only if they are specified by the identity-provider step-up
flow, and keep unrelated 403 responses unchanged.
Comment on lines
+91
to
+103
| def sample_verification( | ||
| status: VerificationStatus = VerificationStatus.VERIFIED, | ||
| checked: bool = False, | ||
| ) -> DomainVerification: | ||
| """A zone-scoped verification in the given state.""" | ||
| verified = status == VerificationStatus.VERIFIED | ||
| return DomainVerification( | ||
| asset_id=ASSET_ID, | ||
| status=status, | ||
| verified_scope=VerificationScope.ZONE if verified else None, | ||
| verified_at=_T0 if verified else None, | ||
| challenge=None if verified else sample_challenge(checked=checked), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not attach an active challenge to an expired verification.
sample_verification() attaches sample_challenge() for every non-verified status. This makes VerificationStatus.EXPIRED contain an awaiting challenge, although an expired verification has no answerable challenge.
Create a challenge only for VerificationStatus.PENDING.
Proposed fix
- challenge=None if verified else sample_challenge(checked=checked),
+ challenge=(
+ sample_challenge(checked=checked)
+ if status is VerificationStatus.PENDING
+ else None
+ ),📝 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
| def sample_verification( | |
| status: VerificationStatus = VerificationStatus.VERIFIED, | |
| checked: bool = False, | |
| ) -> DomainVerification: | |
| """A zone-scoped verification in the given state.""" | |
| verified = status == VerificationStatus.VERIFIED | |
| return DomainVerification( | |
| asset_id=ASSET_ID, | |
| status=status, | |
| verified_scope=VerificationScope.ZONE if verified else None, | |
| verified_at=_T0 if verified else None, | |
| challenge=None if verified else sample_challenge(checked=checked), | |
| ) | |
| def sample_verification( | |
| status: VerificationStatus = VerificationStatus.VERIFIED, | |
| checked: bool = False, | |
| ) -> DomainVerification: | |
| """A zone-scoped verification in the given state.""" | |
| verified = status == VerificationStatus.VERIFIED | |
| return DomainVerification( | |
| asset_id=ASSET_ID, | |
| status=status, | |
| verified_scope=VerificationScope.ZONE if verified else None, | |
| verified_at=_T0 if verified else None, | |
| challenge=( | |
| sample_challenge(checked=checked) | |
| if status is VerificationStatus.PENDING | |
| else None | |
| ), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nc3_testing_platform/domains/assets/examples.py` around lines 91 - 103,
Update sample_verification so sample_challenge is created only when status is
VerificationStatus.PENDING; keep challenge unset for VERIFIED and EXPIRED
statuses while preserving the existing checked argument for pending
verifications.
Contributor
Author
|
This has become extremely time consuming, and unproductive. It was supposed to be concise docs and easy to scan through. But the time spent reading and reviewing cascading AI reviews could have brought us so much closer to an MVP. I took the time to address several points and pushed many fixes where it made sense, but it is best that I do not respond further – not even to present resolutions and reconciliation logs as it will just trigger more AI responses. We need human intervention and the more AI splatter occupies this thread, the longer the read time and cognitive load, and the further away are we from resolving this PR and implementing. At the base, the goal of architectural work is to reach general understanding of the data model and API design. If we stop at each "the specs say this but I don't see it in the model" because our favorite AI tool says so, we are not progressing. This was an interesting experiment, reviewing reviews of my own PR by myself, but I leave this to the team's discretion. Respectfully, |
… assurance constraints
…cation flows; also clarified enum documentation to specify computed vs. stored attributes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/api-design-v4_0_1.md`:
- Around line 179-180: Update the MFA assurance requirements near the OpenID
Connect authentication guidance and the failure response at the referenced
assurance rule to specify the exact claim or session state that proves current
MFA, including how freshness is determined. Define the corresponding RFC 9457
problem-detail type returned when assurance is missing or stale, distinguishing
it from ordinary forbidden responses.
In `@src/nc3_testing_platform/core/security.py`:
- Around line 77-91: Update require_oidc_token to validate the token’s MFA
assurance, preserving the existing 401 response for missing credentials and
returning a distinct RFC 9457 step-up-MFA problem for insufficient assurance.
Define the dedicated problem type and response schema, then register that
response alongside the existing responses on all three operations using
MfaGated.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f7d5d2f0-b835-470e-84f5-be1e9ab4b8cf
📒 Files selected for processing (7)
api/openapi.jsondocs/reference/api-design-v4_0_1.mddocs/reference/data-model-v4_0_1.mdsrc/nc3_testing_platform/core/enums.pysrc/nc3_testing_platform/core/security.pysrc/nc3_testing_platform/domains/api_keys/router.pysrc/nc3_testing_platform/domains/assets/router.py
Comment on lines
+179
to
+180
| - `POST .../verification` requires current MFA assurance. | ||
| - Current MFA assurance is read from the identity provider's session or token. An operation that requires it therefore declares only the OpenID Connect scheme: a platform API key carries no assurance. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Define the exact MFA assurance signal and failure contract.
OpenID Connect identifies the authentication scheme, but it does not define which claim or session state proves current MFA assurance. The contract also does not define the RFC 9457 type returned when assurance is missing or stale. Add both rules for Lines 179-180 and Line 344. Otherwise, implementations may check only the scheme, and clients cannot distinguish step-up MFA from an ordinary forbidden response.
Also applies to: 344-344
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/api-design-v4_0_1.md` around lines 179 - 180, Update the MFA
assurance requirements near the OpenID Connect authentication guidance and the
failure response at the referenced assurance rule to specify the exact claim or
session state that proves current MFA, including how freshness is determined.
Define the corresponding RFC 9457 problem-detail type returned when assurance is
missing or stale, distinguishing it from ordinary forbidden responses.
Comment on lines
+77
to
+91
| def require_oidc_token(oidc_token: OidcAuth) -> None: | ||
| """Rejects a caller without an OpenID Connect token. | ||
|
|
||
| Belongs on an operation that consumes current MFA assurance. | ||
| Assurance is read from the identity provider's token, so a platform API key cannot satisfy the gate and the operation declares only the OIDC scheme. | ||
| """ | ||
| if not oidc_token: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Present an OpenID Connect token carrying current MFA assurance.", | ||
| ) | ||
|
|
||
|
|
||
| # Attach as `dependencies=[MfaGated]` on an operation. | ||
| MfaGated = Depends(require_oidc_token) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in \
src/nc3_testing_platform/core/errors.py \
src/nc3_testing_platform/core/security.py \
src/nc3_testing_platform/domains/api_keys/router.py \
src/nc3_testing_platform/domains/assets/router.py \
api/openapi.json
do
printf '\n--- %s ---\n' "$file"
rg -n -C 4 \
'MfaGated|require_oidc_token|ProblemDetail|RFC.?9457|mfa|assurance|401|403|responses=' \
"$file" || true
doneRepository: NC3-TestingPlatform/testing-platform-backend
Length of output: 50400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- security.py ---'
cat -n src/nc3_testing_platform/core/security.py | sed -n '1,180p'
printf '%s\n' '--- errors.py ---'
cat -n src/nc3_testing_platform/core/errors.py | sed -n '1,230p'
printf '%s\n' '--- security and MFA references ---'
rg -n -i -C 3 'require_oidc_token|MfaGated|current MFA|assurance|acr|amr|oidc_token|OpenIdConnect' \
src tests 2>/dev/null || true
printf '%s\n' '--- targeted OpenAPI operations ---'
python3 - <<'PY'
import json
from pathlib import Path
doc = json.loads(Path("api/openapi.json").read_text())
for path, method in [
("/api/v1/api-keys", "post"),
("/api/v1/api-keys/{key_id}/revoke", "post"),
("/api/v1/assets/{asset_id}/verification", "post"),
]:
op = doc.get("paths", {}).get(path, {}).get(method)
print(path, method)
if op is None:
print(" NOT FOUND")
continue
print(" operationId:", op.get("operationId"))
print(" security:", op.get("security"))
print(" responses:", sorted(op.get("responses", {})))
for code, response in op.get("responses", {}).items():
schema = response.get("content", {}).get("application/problem+json", {}).get("schema")
print(" ", code, response.get("description"), schema)
PYRepository: NC3-TestingPlatform/testing-platform-backend
Length of output: 35511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository requirements and error conventions ---'
rg -n -i -C 3 \
'RFC 9457|problem type|problem detail|step[- ]up|MFA|multi[- ]factor|assurance|insufficient|Unauthorized|Forbidden|401|403|WWW-Authenticate|security requirement' \
README.md docs api src tests 2>/dev/null || true
printf '%s\n' '--- all error/problem declarations ---'
rg -n -C 4 \
'class .*Problem|ProblemDetail|problem_responses|HTTPException|status_code=status.HTTP_(UNAUTHORIZED|FORBIDDEN)|status_code=(401|403)' \
src tests 2>/dev/null || true
printf '%s\n' '--- route dependency and response inventory ---'
python3 - <<'PY'
from pathlib import Path
import re
for name in (
"src/nc3_testing_platform/domains/api_keys/router.py",
"src/nc3_testing_platform/domains/assets/router.py",
):
text = Path(name).read_text()
print(name)
for match in re.finditer(r'responses=problem_responses\(([^)]*)\).*?dependencies=\[([^\]]+)\]', text, re.S):
print(" ", "responses:", match.group(1).replace("\n", " "),
"dependencies:", match.group(2).replace("\n", " "))
PY
printf '%s\n' '--- direct semantic probe of require_oidc_token ---'
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("src/nc3_testing_platform/core/security.py")
tree = ast.parse(path.read_text())
fn = next(
node for node in tree.body
if isinstance(node, ast.FunctionDef) and node.name == "require_oidc_token"
)
if len(fn.body) != 2 or not isinstance(fn.body[1], ast.If):
raise SystemExit("unexpected function shape")
condition = ast.unparse(fn.body[1].test)
raise_call = fn.body[1].body[0]
keywords = {item.arg: ast.literal_eval(item.value) for item in raise_call.exc.keywords}
print("condition:", condition)
print("status_code:", keywords["status_code"])
print("detail:", keywords["detail"])
print("no assurance claim lookup:", not any(
isinstance(node, ast.Name) and node.id in {"acr", "amr", "assurance"}
for node in ast.walk(fn)
))
PYRepository: NC3-TestingPlatform/testing-platform-backend
Length of output: 50403
Add a distinct RFC 9457 problem for insufficient MFA assurance.
require_oidc_token accepts any non-empty OIDC value and does not inspect MFA assurance. Define a dedicated step-up-MFA problem type and response schema. Add it to the OpenAPI responses for the three MfaGated operations. Keep missing credentials separate from insufficient assurance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nc3_testing_platform/core/security.py` around lines 77 - 91, Update
require_oidc_token to validate the token’s MFA assurance, preserving the
existing 401 response for missing credentials and returning a distinct RFC 9457
step-up-MFA problem for insufficient assurance. Define the dedicated problem
type and response schema, then register that response alongside the existing
responses on all three operations using MfaGated.
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
2 participants
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.
Summary
Establishes the v4.0.1 API contract: a contract-first FastAPI source tree that generates
api/openapi.json, plus the API design and data model documents that specify it.Fixes #75
Type of Change
Changes Made
src/layout, one vertical slice per domain, sharedcore/for enums, errors, pagination, and security schemes.POST /scanspicks one of three request schemas by media type and access state.api/openapi.json, generated bymake export-openapi. 40 paths under/api/v1, plus two root health probes.Makefile, CI contract validation,.editorconfig.Test Plan
make test/pytest/ etc.)No pytest suite in this branch —
tests/is empty. CI still gates every push with four checks:ruff check srcpyright srcopenapi-spec-validator --schema 3.1 api/openapi.jsonmake lintpasses clean.make export-openapireproduces the committed contract.Security Checklist
npm audit/pip-audit)Every request and response is a declared Pydantic model.
pip-auditis not configured; direct dependencies arefastapi[standard]andpydantic[email].Screenshots / Output
Too large to show.
Reviewer Notes
The deliverable is the contract and the two reference documents. Handlers return stub data by design, so the server doubles as a frontend mock.
Summary by CodeRabbit
Summary by CodeRabbit