BCH-1341: Detect cross-SSP leverage drift, notify downstream, support re-attestation (Phase 5)#451
Conversation
… re-attestation (Phase 5) Extends SyncExportOffering to flip stale leverage links to drifted (on a content-changing version bump, offering deprecation/revocation, or its leveraged authorization being deleted), raising a downstream-only inherited-revoked risk and a leverage_drifted/leverage_revoked notification without ever exposing the upstream's own risk register or evidence. A new re-attestation endpoint is the sole path that clears drift and remediates the risk.
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds SSP export-offering status transitions, implementation-status-aware versioning, leverage drift risk handling, re-attestation, and per-owner email notifications. OpenAPI documentation, route wiring, worker registration, and unit/integration coverage are included. ChangesSSP leverage drift lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Publish
participant SyncExportOffering
participant DriftEngine
participant WorkerService
participant River
Client->>Publish: publish export offering
Publish->>SyncExportOffering: synchronize offering
SyncExportOffering->>DriftEngine: evaluate leverage drift
DriftEngine-->>Publish: return drifted links
Publish->>WorkerService: enqueue drift notifications
WorkerService->>River: insert owner jobs
River-->>Client: process notification asynchronously
sequenceDiagram
participant Client
participant ReAttest
participant SSPLeverageLink
participant Risk
Client->>ReAttest: attest drifted link
ReAttest->>SSPLeverageLink: recompute and activate link
ReAttest->>Risk: remediate drift risk
ReAttest-->>Client: return updated link
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Just let me know how I can help! (=^･ｪ･^=) ? ✅ Action performedSummary regeneration triggered. |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/docs.go`:
- Around line 38947-38954: Update the Go request struct corresponding to
oscal.updateOfferingStatusRequest, locating its status field, to mark it
required with binding:"required" and constrain it with the swaggo enum
annotation enums=deprecated,revoked. Then rerun swag init to regenerate the docs
artifacts, including the schema metadata in docs.go and the corresponding
swagger JSON/YAML files; do not edit generated files directly.
In `@docs/swagger.json`:
- Around line 38941-38948: Document the allowed values on the Status field of
the relevant Go request struct in ssp_export_offerings.go using the swaggo
enum/description annotation, matching validate()’s "deprecated" and "revoked"
constraints. Then rerun swag init to regenerate docs/swagger.json; do not edit
the generated schema directly.
In `@internal/api/handler/oscal/ssp_export_offerings.go`:
- Around line 739-743: Extract the duplicated drift-notification enqueue logic
into a shared helper near enqueueLeverageDriftNotifications, such as
enqueueLeverageDriftNotificationsAsync, accepting the request context, logger,
enqueuer, and drifted links. Have it return for empty links, detach cancellation
with context.WithoutCancel, apply the shared 10-second timeout, defer
cancellation, and invoke enqueueLeverageDriftNotifications. Replace the repeated
blocks in Publish, UpdateOfferingStatus, and
DeleteSystemImplementationLeveragedAuthorization with calls to this helper.
In `@internal/api/handler/oscal/ssp_leverage.go`:
- Around line 797-849: Prevent the TOCTOU race in the re-attestation transaction
by adding a drifted-status predicate to the SSPLeverageLink update in the
transaction callback. Update the Where clause to require both the link ID and
status relational.SSPLeverageStatusDrifted, then check RowsAffected and return
an appropriate conflict/not-found error when no row was updated, ensuring
concurrent requests or newly drifted links cannot be silently overwritten.
In `@internal/service/worker/leverage_notifications.go`:
- Around line 63-68: Replace the substring check in notificationKindForReason
with an explicit mapping of known reason strings to notification kinds, using a
switch or map; preserve the existing default behavior for unrecognized reasons
by returning leverageDriftedNotificationKind.
In `@internal/service/worker/service.go`:
- Line 1117: Remove the unused downstreamSSPID parameter from
Service.EnqueueLeverageDriftNotification and update the SSPJobEnqueuer interface
plus all callers and implementations to match, unless it is intentionally
required for future use; rely on the risk's SSPID field when constructing
LeverageDriftNotificationArgs.
🪄 Autofix (Beta)
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: 810e209c-08ee-40c9-ac95-85b122cc6837
📒 Files selected for processing (24)
docs/docs.godocs/swagger.jsondocs/swagger.yamlinternal/api/handler/oscal/api.gointernal/api/handler/oscal/dashboard_suggestions_integration_test.gointernal/api/handler/oscal/ssp_export_offering_status_test.gointernal/api/handler/oscal/ssp_export_offerings.gointernal/api/handler/oscal/ssp_export_offerings_unit_test.gointernal/api/handler/oscal/ssp_leverage.gointernal/api/handler/oscal/ssp_leverage_drift.gointernal/api/handler/oscal/ssp_leverage_drift_enqueue_test.gointernal/api/handler/oscal/ssp_leverage_drift_integration_test.gointernal/api/handler/oscal/ssp_leverage_drift_test.gointernal/api/handler/oscal/ssp_leverage_reattest_test.gointernal/api/handler/oscal/ssp_leverage_unit_test.gointernal/api/handler/oscal/system_security_plans.gointernal/api/handler/oscal/system_security_plans_leveraged_auth_drift_test.gointernal/authz/manifest.yamlinternal/service/relational/risks/models.gointernal/service/worker/jobs.gointernal/service/worker/leverage_notifications.gointernal/service/worker/leverage_notifications_test.gointernal/service/worker/risk_notifications.gointernal/service/worker/service.go
…ueue logic, a re-attestation race, and a fragile reason-string match Documents the two valid updateOfferingStatusRequest.Status values in swagger, extracts the repeated drift-notification enqueue block into one helper, closes a TOCTOU window where a stale re-attestation could silently overwrite a link that had already been cleared or re-drifted, replaces a substring match with an explicit reason-to-kind mapping, and drops an unused parameter from the notification enqueuer interface.
1 participant
Summary
SyncExportOfferingto flip staleSSPLeverageLinks todriftedon a content-changing version bump (now including a backing component'sImplementationStatusdowngrade), an offering deprecation/revocation (newPATCH .../export-offerings/:id/statusendpoint), or itsLeveragedAuthorizationbeing deleted ("lapsed").inherited-revokedrisk (linked viarisk_responsibility_linksand best-effortrisk_control_links) and enqueues aleverage_drifted/leverage_revokednotification to the risk's owner(s).POST .../leveraged-controls/:linkId/attest) is the sole path that clears drift: bumpsOfferingVersion, refreshesSatisfaction, and remediates the risk.Test plan
go build ./...,go vet ./...(incl.-tags integration),golangci-lint run ./...all cleango test ./...(unit) passesgo test -tags integration ./internal/api/handler/oscal/...passes (real Postgres via testcontainers), including the ticket's own flow: publish v1 → subscribe → downgrade a provided status → re-sync → link drifted + inherited-revoked risk + notification enqueued; re-attest → clearedmake swagdocs regeneratedSummary by CodeRabbit
New Features
Bug Fixes
Documentation