[Social Work] - Update NPDB Categories for Adverse Actions - #1789
Open
landonshumway-ia wants to merge 12 commits into
Open
[Social Work] - Update NPDB Categories for Adverse Actions#1789landonshumway-ia wants to merge 12 commits into
landonshumway-ia wants to merge 12 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThe pull request expands Social Work adverse-action contracts and public responses, changes non-production OpenSearch topology, reduces PII exposure in logs across applications, masks email addresses, disables debug logging by default, and updates related examples, fixtures, and tests. ChangesSensitive logging and email masking
Social Work adverse-action categories and public responses
Non-production search topology
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderClient
participant PublicProviderLookup
participant ProviderDataStore
participant PublicResponseSchema
ProviderClient->>PublicProviderLookup: request provider data
PublicProviderLookup->>ProviderDataStore: load licenses, privileges, and adverse actions
ProviderDataStore-->>PublicProviderLookup: return provider records
PublicProviderLookup->>PublicResponseSchema: serialize sanitized adverseActions
PublicResponseSchema-->>ProviderClient: return public provider response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
landonshumway-ia
marked this pull request as ready for review
July 29, 2026 15:52
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/social-work-app/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json (1)
32-48: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPublish the non-empty array constraint.
The runtime schema rejects
[], but this request schema permits an empty array because it lacks"minItems": 1. Generated clients can therefore send a payload the API rejects. Update the API-schema generation/model and regenerate artifacts.🤖 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 `@backend/social-work-app/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json` around lines 32 - 48, Update the model or API-schema generation source for clinicalPrivilegeActionCategories to require at least one item by setting the array constraint to minItems 1, then regenerate the affected schema artifacts so the published request schema rejects empty arrays.backend/social-work-app/docs/internal/api-specification/latest-oas30.json (1)
4759-4949: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRegenerate public provider GET docs — both are stale after the new
adverseActionsfield was added.LicensePublicResponseSchemaandPrivilegePublicResponseSchemanow serialize a sanitizedadverseActionslist (verified correct in thePUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.jsontest snapshot), but the generated API docs for/v1/public/compacts/{compact}/providers/{providerId}were not regenerated to match, leaving one shared documentation gap across two artifacts.
backend/social-work-app/docs/internal/api-specification/latest-oas30.json#L4759-L4949: addadverseActions(using theAdverseActionPublicResponseSchemashape) to both thelicensesandprivilegesitem definitions ofTestALicenI8ObIK8aJtpE.backend/social-work-app/docs/internal/postman/postman-collection.json#L4062-L4165: regenerate/update the/v1/public/compacts/:compact/providers/:providerIdexample response body to includeadverseActionsentries inlicenses/privileges, matching the pattern already present in the general provider GET example (line 1280).🤖 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 `@backend/social-work-app/docs/internal/api-specification/latest-oas30.json` around lines 4759 - 4949, Regenerate the public provider GET documentation to include the sanitized adverseActions field in both license and privilege responses. In backend/social-work-app/docs/internal/api-specification/latest-oas30.json lines 4759-4949, add adverseActions using the AdverseActionPublicResponseSchema shape to both licenses and privileges item definitions under TestALicenI8ObIK8aJtpE; in backend/social-work-app/docs/internal/postman/postman-collection.json lines 4062-4165, update the provider response example to include matching adverseActions entries, following the existing general provider GET example.
🧹 Nitpick comments (3)
backend/social-work-app/tests/app/test_search_persistent_stack.py (1)
139-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert replicas and zone awareness as well.
This only guards
InstanceCount; add synthesized-template assertions for three-AZ zone awareness andnumberOfReplicas: 2on the index-manager custom resource to cover the full non-prod topology contract.🤖 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 `@backend/social-work-app/tests/app/test_search_persistent_stack.py` around lines 139 - 145, Extend the sandbox OpenSearch assertions in test_search_persistent_stack.py to verify three-AZ zone awareness on the AWS::OpenSearchService::Domain, alongside the existing InstanceCount check. Also assert that the index-manager custom resource sets numberOfReplicas to 2, using the synthesized template’s corresponding resource properties.backend/social-work-app/stacks/search_persistent_stack/provider_search_domain.py (1)
316-328: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUpdate the non-prod CDK-Nag justifications.
This three-node configuration invalidates the “single-node non-prod/test” rationale still used for OS4 and OS7 suppressions. Describe the actual no-dedicated-master/no-standby cost trade-off so the generated compliance evidence remains accurate.
🤖 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 `@backend/social-work-app/stacks/search_persistent_stack/provider_search_domain.py` around lines 316 - 328, Update the CDK-Nag suppression justifications associated with OS4 and OS7 in the non-production capacity configuration to remove the outdated single-node/test rationale. Document that the cluster uses three data nodes without dedicated master nodes or multi-AZ standby as an intentional cost trade-off, while preserving the existing suppression behavior.backend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.ts (1)
67-80: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd focused tests for both email-masking helpers.
The duplicated privacy-sensitive implementations should share coverage for valid addresses, malformed inputs, and recipient arrays.
backend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.ts#L67-L80: add unit tests formaskEmailandmaskEmails.backend/social-work-app/lambdas/nodejs/lib/email/base-email-service.ts#L67-L80: add equivalent unit tests for the Social Work implementation.🤖 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 `@backend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.ts` around lines 67 - 80, Add focused unit tests for maskEmail and maskEmails in both backend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.ts:67-80 and backend/social-work-app/lambdas/nodejs/lib/email/base-email-service.ts:67-80. Cover valid addresses, malformed inputs, and arrays of recipients, verifying both implementations produce the expected masked output and preserve privacy behavior.
🤖 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
`@backend/social-work-app/docs/search-internal/postman/postman-collection.json`:
- Line 468: Replace every impossible calendar value in the response body’s
format-date fields with valid ISO 8601 calendar dates, including invalid
day/month combinations such as November 31 and February 31. Preserve the
response schema and placeholder fields, and ensure all date examples represent
real calendar days.
In `@backend/social-work-app/stacks/api_stack/v1_api/api_model.py`:
- Around line 787-797: Reject empty NPDB category arrays by adding min_items=1
to the outer category-array JsonSchema in api_model.py. Regenerate the
corresponding schema snapshots with "minItems": 1 in
backend/social-work-app/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json
(39-55), PATCH_LICENSE_INVESTIGATION_REQUEST_SCHEMA.json (54-70), and
PATCH_PRIVILEGE_INVESTIGATION_REQUEST_SCHEMA.json (40-56); no other changes are
needed.
In `@backend/social-work-app/stacks/search_persistent_stack/index_manager.py`:
- Around line 18-22: Update OpenSearchIndexManager.on_update to apply the
configured numberOfReplicas to existing indexes, including indexes whose aliases
already exist and therefore bypass create_provider_index_with_alias(). Reuse the
existing index and alias management symbols to update settings in place,
preserving existing data and the current creation behavior.
---
Outside diff comments:
In `@backend/social-work-app/docs/internal/api-specification/latest-oas30.json`:
- Around line 4759-4949: Regenerate the public provider GET documentation to
include the sanitized adverseActions field in both license and privilege
responses. In
backend/social-work-app/docs/internal/api-specification/latest-oas30.json lines
4759-4949, add adverseActions using the AdverseActionPublicResponseSchema shape
to both licenses and privileges item definitions under TestALicenI8ObIK8aJtpE;
in backend/social-work-app/docs/internal/postman/postman-collection.json lines
4062-4165, update the provider response example to include matching
adverseActions entries, following the existing general provider GET example.
In
`@backend/social-work-app/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json`:
- Around line 32-48: Update the model or API-schema generation source for
clinicalPrivilegeActionCategories to require at least one item by setting the
array constraint to minItems 1, then regenerate the affected schema artifacts so
the published request schema rejects empty arrays.
---
Nitpick comments:
In `@backend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.ts`:
- Around line 67-80: Add focused unit tests for maskEmail and maskEmails in both
backend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.ts:67-80 and
backend/social-work-app/lambdas/nodejs/lib/email/base-email-service.ts:67-80.
Cover valid addresses, malformed inputs, and arrays of recipients, verifying
both implementations produce the expected masked output and preserve privacy
behavior.
In
`@backend/social-work-app/stacks/search_persistent_stack/provider_search_domain.py`:
- Around line 316-328: Update the CDK-Nag suppression justifications associated
with OS4 and OS7 in the non-production capacity configuration to remove the
outdated single-node/test rationale. Document that the cluster uses three data
nodes without dedicated master nodes or multi-AZ standby as an intentional cost
trade-off, while preserving the existing suppression behavior.
In `@backend/social-work-app/tests/app/test_search_persistent_stack.py`:
- Around line 139-145: Extend the sandbox OpenSearch assertions in
test_search_persistent_stack.py to verify three-AZ zone awareness on the
AWS::OpenSearchService::Domain, alongside the existing InstanceCount check. Also
assert that the index-manager custom resource sets numberOfReplicas to 2, using
the synthesized template’s corresponding resource properties.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e44f03d-7418-44d7-81d2-de88c23586db
⛔ Files ignored due to path filters (3)
backend/compact-connect/lambdas/nodejs/yarn.lockis excluded by!**/yarn.lock,!**/*.lockbackend/cosmetology-app/lambdas/nodejs/yarn.lockis excluded by!**/yarn.lock,!**/*.lockbackend/social-work-app/lambdas/nodejs/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (68)
backend/common-cdk/common_constructs/stack.pybackend/compact-connect/lambdas/nodejs/cognito-emails/lambda.tsbackend/compact-connect/lambdas/nodejs/lib/email/base-email-service.tsbackend/compact-connect/lambdas/nodejs/lib/email/ingest-event-email-service.tsbackend/compact-connect/lambdas/nodejs/package.jsonbackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/user_client.pybackend/compact-connect/lambdas/python/common/cc_common/signature_auth.pybackend/compact-connect/lambdas/python/common/cc_common/utils.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.pybackend/compact-connect/lambdas/python/search/handlers/populate_provider_documents.pybackend/compact-connect/lambdas/python/staff-user-pre-token/main.pybackend/compact-connect/stacks/persistent_stack/staff_users.pybackend/cosmetology-app/lambdas/nodejs/cognito-emails/lambda.tsbackend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.tsbackend/cosmetology-app/lambdas/nodejs/lib/email/ingest-event-email-service.tsbackend/cosmetology-app/lambdas/nodejs/package.jsonbackend/cosmetology-app/lambdas/python/common/cc_common/data_model/data_client.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/user_client.pybackend/cosmetology-app/lambdas/python/common/cc_common/signature_auth.pybackend/cosmetology-app/lambdas/python/common/cc_common/utils.pybackend/cosmetology-app/lambdas/python/provider-data-v1/handlers/bulk_upload.pybackend/cosmetology-app/lambdas/python/search/handlers/populate_provider_documents.pybackend/cosmetology-app/lambdas/python/staff-user-pre-token/main.pybackend/cosmetology-app/stacks/persistent_stack/staff_users.pybackend/social-work-app/docs/internal/api-specification/latest-oas30.jsonbackend/social-work-app/docs/internal/postman/postman-collection.jsonbackend/social-work-app/docs/search-internal/api-specification/latest-oas30.jsonbackend/social-work-app/docs/search-internal/postman/postman-collection.jsonbackend/social-work-app/lambdas/nodejs/cognito-emails/lambda.tsbackend/social-work-app/lambdas/nodejs/lib/email/base-email-service.tsbackend/social-work-app/lambdas/nodejs/lib/email/ingest-event-email-service.tsbackend/social-work-app/lambdas/nodejs/package.jsonbackend/social-work-app/lambdas/python/common/cc_common/data_model/data_client.pybackend/social-work-app/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/social-work-app/lambdas/python/common/cc_common/data_model/schema/common.pybackend/social-work-app/lambdas/python/common/cc_common/data_model/schema/license/api.pybackend/social-work-app/lambdas/python/common/cc_common/data_model/schema/privilege/api.pybackend/social-work-app/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/social-work-app/lambdas/python/common/cc_common/data_model/user_client.pybackend/social-work-app/lambdas/python/common/cc_common/signature_auth.pybackend/social-work-app/lambdas/python/common/cc_common/utils.pybackend/social-work-app/lambdas/python/common/common_test/test_constants.pybackend/social-work-app/lambdas/python/common/tests/resources/api/adverse-action-post.jsonbackend/social-work-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.pybackend/social-work-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_investigation.pybackend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.pybackend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/social-work-app/lambdas/python/search/handlers/populate_provider_documents.pybackend/social-work-app/lambdas/python/search/tests/function/test_public_search_providers.pybackend/social-work-app/lambdas/python/staff-user-pre-token/main.pybackend/social-work-app/stacks/api_stack/v1_api/api_model.pybackend/social-work-app/stacks/persistent_stack/staff_users.pybackend/social-work-app/stacks/search_api_stack/v1_api/api_model.pybackend/social-work-app/stacks/search_persistent_stack/index_manager.pybackend/social-work-app/stacks/search_persistent_stack/provider_search_domain.pybackend/social-work-app/tests/app/test_search_persistent_stack.pybackend/social-work-app/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/PATCH_LICENSE_INVESTIGATION_REQUEST_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/PATCH_PRIVILEGE_INVESTIGATION_REQUEST_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/PUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.jsonbackend/social-work-app/tests/smoke/encumbrance_smoke_tests.pybackend/social-work-app/tests/smoke/investigation_smoke_tests.py
landonshumway-ia
requested review from
ChiefStief and
jlkravitz
and removed request for
ChiefStief
July 29, 2026 17:34
Collaborator
Author
|
@jlkravitz This is ready for your review. I have verified that debug logs are now not emitted with the env var set to false. Please perform another scan to verify we are not logging any sensitive fields in non-debug logs. Thanks |
jlkravitz
reviewed
Jul 31, 2026
jlkravitz
left a comment
jlkravitz
left a comment
Collaborator
There was a problem hiding this comment.
Looks good! A few findings from claude below...
| Sev | Location | Level | What's logged |
|---|---|---|---|
| High | common/cc_common/email_service_client.py:110 (compact-connect); :99 and :105 (cosmetology, social-work) |
ERROR |
Full notification payload — specificEmails (provider's email) + templateVariables with providerFirstName/providerLastName. Fires on any email-lambda failure or FunctionError. |
| Med | lambdas/nodejs/lib/email/email-notification-service.ts:351, 408, 441, 469 |
INFO ×4 |
providerEmail, recipients, providerEmail, oldEmailAddress. This PR made maskEmail() public but applied it only in ingest-event-email-service.ts and cognito-emails/lambda.ts — same class hierarchy, so it's a drop-in this.maskEmail(...). |
| Med | cognito-backup/handlers/cognito_backup.py:108, :171 — ×3 apps |
ERROR |
username= — Cognito Username is the email here (user_client.py:312, 326, 334, 400, 407, 436). This lambda iterates the whole user pool, so one systemic S3 failure enumerates every user's email. Suggest sub. |
| Med | provider-data-v1/handlers/provider_s3_events.py:12 and :18 |
INFO ×2 |
S3 key embedding the user-supplied filename (provider_users.py:181) — typically self-identifying (JSmith_DD214.pdf), revealing name + military-affiliation status. Both lines need fixing; :18 logs the key separately as s3_url. |
| Med | provider-data-v1/handlers/licenses.py:103 (compact-connect), :94 (cosmetology), :98 (social-work) |
ERROR |
failed_license_numbers= (built at cc_common/utils.py:907). Fires on SQS batch-send failure, dumping up to a full batch. Suggest count + failure codes. |
| Med | provider-data-v1/handlers/provider_users.py:396 |
WARNING |
new_email= on the AliasExistsException branch. compact + provider_id are already on the line, so this links a pseudonymous ID to a real address. |
| Low | common/cc_common/data_model/transaction_client.py:263 |
ERROR |
matching_privilege_records= — raw items. Mostly pseudonymous, but PrivilegeRecordSchema nests DeactivationDetailsSchema.deactivatedByStaffUserName (schema/privilege/record.py:55) — unverified whether populated on this path. |
| Low | custom-resources/handlers/ses_email_identity_verification_handler.py:29 — ×3 apps |
INFO |
Full CFN custom-resource event incl. presigned ResponseURL (AWSAccessKeyId + X-Amz-Signature). Not PII, but 8.7 also covers "sensitive authentication data that could assist an attacker." Deploy-time only. |
jlkravitz
reviewed
Jul 31, 2026
Collaborator
Author
|
@jlkravitz Are license numbers actually considered sensitive without other PII? Wouldn't that be more akin to the providerId, an id that tech support can use to lookup an individual without exposing their PII in the logs? |
Collaborator
|
@landonshumway-ia Sorry, I missed that one in my scan. I agree that license numbers are not PII since they are public. |
Contributor
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
`@backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py`:
- Around line 104-120: Remove the full payload DEBUG log in the exception path
of the email service client, or replace it with an explicitly allowlisted
diagnostic payload that excludes verificationCode, recoveryToken, email
addresses, provider names, and other credentials or PII. Keep the existing
non-PII ERROR fields and exception handling unchanged.
In
`@backend/compact-connect/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py`:
- Around line 29-32: Remove the full-event DEBUG logging from the SES email
identity verification handler, or sanitize the event to exclude ResponseURL and
other credentials/tokens before logging, across the handlers at
backend/compact-connect/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py
lines 29-32,
backend/cosmetology-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py
lines 29-32, and
backend/social-work-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py
lines 29-32; keep only an allowlisted, non-sensitive event subset in the ses
email identity verification handler.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50e69234-9433-45ef-9a54-c84de7a41be9
📒 Files selected for processing (19)
backend/common-cdk/common_constructs/stack.pybackend/compact-connect/lambdas/python/cognito-backup/handlers/cognito_backup.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.pybackend/compact-connect/lambdas/python/common/cc_common/email_service_client.pybackend/compact-connect/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_s3_events.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.pybackend/cosmetology-app/lambdas/python/cognito-backup/handlers/cognito_backup.pybackend/cosmetology-app/lambdas/python/common/cc_common/email_service_client.pybackend/cosmetology-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.pybackend/social-work-app/lambdas/python/cognito-backup/handlers/cognito_backup.pybackend/social-work-app/lambdas/python/common/cc_common/email_service_client.pybackend/social-work-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.pybackend/social-work-app/stacks/api_stack/v1_api/api_model.pybackend/social-work-app/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/PATCH_LICENSE_INVESTIGATION_REQUEST_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/PATCH_PRIVILEGE_INVESTIGATION_REQUEST_SCHEMA.jsonbackend/social-work-app/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json
💤 Files with no reviewable changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/common-cdk/common_constructs/stack.py
- backend/social-work-app/tests/resources/snapshots/PATCH_PRIVILEGE_INVESTIGATION_REQUEST_SCHEMA.json
- backend/social-work-app/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json
- backend/social-work-app/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/social-work-app/stacks/api_stack/v1_api/api_model.py
- backend/social-work-app/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/social-work-app/tests/resources/snapshots/PATCH_LICENSE_INVESTIGATION_REQUEST_SCHEMA.json
Comment on lines
+104
to
+120
| self._logger.error(error_message, template=payload.get('template')) | ||
| raise CCInternalException(error_message) | ||
|
|
||
| return response | ||
| except Exception as e: | ||
| error_message = f'Error invoking email notification service lambda: {str(e)}' | ||
| self._logger.error(error_message, payload=payload, exception=str(e)) | ||
| # payload may contain PII (specificEmails, templateVariables with provider names), so it is only | ||
| # logged at DEBUG level; the non-PII fields below are sufficient for triage at ERROR level | ||
| self._logger.error( | ||
| error_message, | ||
| template=payload.get('template'), | ||
| compact=payload.get('compact'), | ||
| jurisdiction=payload.get('jurisdiction'), | ||
| recipient_type=payload.get('recipientType'), | ||
| exception=str(e), | ||
| ) | ||
| self._logger.debug('Email notification service invocation payload', payload=payload) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log recovery credentials in the complete DEBUG payload.
send_provider_email_verification_code passes verificationCode, and send_provider_account_recovery_confirmation_email passes recoveryToken. This debug log records the entire payload after an invocation failure. DEBUG=false is not a sufficient control for bearer credentials. Remove the payload log or pass an allowlisted payload that excludes tokens, codes, email addresses, and provider names.
Suggested fix
- self._logger.debug('Email notification service invocation payload', payload=payload)Based on learnings, redact credentials, tokens, and PII before diagnostic logging.
📝 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
| self._logger.error(error_message, template=payload.get('template')) | |
| raise CCInternalException(error_message) | |
| return response | |
| except Exception as e: | |
| error_message = f'Error invoking email notification service lambda: {str(e)}' | |
| self._logger.error(error_message, payload=payload, exception=str(e)) | |
| # payload may contain PII (specificEmails, templateVariables with provider names), so it is only | |
| # logged at DEBUG level; the non-PII fields below are sufficient for triage at ERROR level | |
| self._logger.error( | |
| error_message, | |
| template=payload.get('template'), | |
| compact=payload.get('compact'), | |
| jurisdiction=payload.get('jurisdiction'), | |
| recipient_type=payload.get('recipientType'), | |
| exception=str(e), | |
| ) | |
| self._logger.debug('Email notification service invocation payload', payload=payload) | |
| self._logger.error(error_message, template=payload.get('template')) | |
| raise CCInternalException(error_message) | |
| return response | |
| except Exception as e: | |
| error_message = f'Error invoking email notification service lambda: {str(e)}' | |
| # payload may contain PII (specificEmails, templateVariables with provider names), so it is only | |
| # logged at DEBUG level; the non-PII fields below are sufficient for triage at ERROR level | |
| self._logger.error( | |
| error_message, | |
| template=payload.get('template'), | |
| compact=payload.get('compact'), | |
| jurisdiction=payload.get('jurisdiction'), | |
| recipient_type=payload.get('recipientType'), | |
| exception=str(e), | |
| ) |
🤖 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
`@backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py`
around lines 104 - 120, Remove the full payload DEBUG log in the exception path
of the email service client, or replace it with an explicitly allowlisted
diagnostic payload that excludes verificationCode, recoveryToken, email
addresses, provider names, and other credentials or PII. Keep the existing
non-PII ERROR fields and exception handling unchanged.
Source: Learnings
Comment on lines
+29
to
+32
| # The full event includes a pre-signed 'ResponseURL' (with an access key id and signature) used to signal | ||
| # CloudFormation, so it is only logged at DEBUG level. | ||
| logger.info('Entering SES email identity verification handler', request_type=event.get('RequestType')) | ||
| logger.debug('SES email identity verification handler event', event=json.dumps(event)) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove pre-signed CloudFormation URLs from DEBUG logs.
Each handler serializes the complete event. The event contains the pre-signed ResponseURL, which includes credentials. Log only an allowlisted subset of the event.
backend/compact-connect/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py#L29-L32: removeResponseURLbefore logging, or remove the full-event DEBUG log.backend/cosmetology-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py#L29-L32: removeResponseURLbefore logging, or remove the full-event DEBUG log.backend/social-work-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py#L29-L32: removeResponseURLbefore logging, or remove the full-event DEBUG log.
Based on learnings, redact credentials and tokens instead of relying only on a debug-level threshold.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 31-31: use jsonify instead of json.dumps for JSON output
Context: json.dumps(event)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
📍 Affects 3 files
backend/compact-connect/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py#L29-L32(this comment)backend/cosmetology-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py#L29-L32backend/social-work-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py#L29-L32
🤖 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
`@backend/compact-connect/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py`
around lines 29 - 32, Remove the full-event DEBUG logging from the SES email
identity verification handler, or sanitize the event to exclude ResponseURL and
other credentials/tokens before logging, across the handlers at
backend/compact-connect/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py
lines 29-32,
backend/cosmetology-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py
lines 29-32, and
backend/social-work-app/lambdas/python/custom-resources/handlers/ses_email_identity_verification_handler.py
lines 29-32; keep only an allowlisted, non-sensitive event subset in the ses
email identity verification handler.
Source: Learnings
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.
This updates the list of NPDB categories and disciplinary action types to be used by the Social Work API. It also updates the API schema to support accepting a list of multiple NPDB categories for one encumbrance.
In addition, this updates the public provider details endpoint for Social Work to return a subset of the adverse action data so that the frontend can display which records are under discipline, as well as when they were encumbered and when encumbrances were lifted.
This also updates the Social Work non-prod OpenSearch configuration to use 3 data nodes instead of 1 to avoid test/demo outages in those environments.
Finally, as part of enhancing our security posture, this sets the DEBUG env var to false in all environments and removes or masks PII in all non-debug level logs from all three backend APIs (JCC, Cosmetology, and Social Work).
Requirements List
Testing List
yarn test:unit:allshould run without errors or warningsyarn serveshould run without errors or warningsyarn buildshould run without errors or warningsbackend/compact-connect/tests/unit/test_api.pyrun compact-connect/bin/download_oas30.pyCloses #1786 #1760 #1706 #1742
Summary by CodeRabbit
New Features
Bug Fixes