fix(SITES-44243): server-derive executedBy and add executedByUser audit trail#2445
Open
sandsinh wants to merge 15 commits into
Open
fix(SITES-44243): server-derive executedBy and add executedByUser audit trail#2445sandsinh wants to merge 15 commits into
sandsinh wants to merge 15 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This PR will trigger a patch release when merged. |
…utedByName Co-Authored-By: Claude <noreply@anthropic.com>
Adds two test cases to close the coverage gap on src/controllers/fixes.js: the early return when no fix has executedBy, and the outer catch that logs a warning when the IMS lookup throws synchronously. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sandsinh
changed the title
…d harden enrichment - Strip client-supplied executedBy from safeFixData unconditionally in createFixes so it cannot pass through when callerUserId is unresolvable (residual security gap) - Extract static #resolveCallerId(context) helper (user_id ?? sub ?? email) used by both createFixes and patchFix, closing the email-fallback gap for IMS-authenticated callers - patchFix now returns 400 with a descriptive message when executedBy intent signal is sent but no caller identity can be resolved (was silently producing a misleading 400) - Add IMS ID format validation in #enrichFixesWithUserNames before calling getImsAdminProfile, blocking pre-fix attacker-supplied legacy values - Redact IMS user IDs in warning logs; use null instead of dash/empty placeholders - Add executedByUser to OpenAPI Fix response schema (readOnly, list-endpoint-only note) - Fix stale TrialUser comment in fix.js DTO - Add comments to getByStatus and getByID explaining intentional enrichment omission - Tests: strip-on-no-authInfo, sub fallback (createFixes + patchFix), 400-on-unresolvable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The enrichment filter only allowed named account type suffixes (AdobeID, AdobeOrg, etc.) but IMS user IDs stored via the sub claim use a 24-char hex org ID suffix (e.g. B90A1...@9bcd5ee55f43b6f30a494212), causing executedByUser to be silently omitted for all real-world fixes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Conflicts: # src/controllers/fixes.js
getByID was missing the #enrichFixesWithUserNames call, producing an inconsistent API surface where the same fix returned executedByUser via getAllForOpportunity but not via the single-fetch endpoint. Added enrichment and a matching test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
executedByUseraudit-trail field onGET /sites/:siteId/opportunities/:opportunityId/fixes— resolves each fix'sexecutedByIMS user ID to{ firstName, lastName, email }via the IMS admin profile API at read time.executedByIMS user ID and exfiltrate that user's profile via the GET endpoint.executedByis now server-derived from the authenticated caller's JWT (profile.user_id ?? profile.sub); client-supplied values are treated as intent signals only and overridden increateFixes/patchFix.executedByis removed from theFixUpdateOpenAPI schema.docs/specs/anddocs/plans/, and unit tests covering enrichment happy path, IMS lookup failure, missingimsClient, emptyexecutedByset, and synchronous throw inside thePromise.allSettledmapping.Test plan
npm test— 107 unit tests pass (incl. 5 enrichment cases intest/controllers/fixes.test.js).npm run docs:lint— OpenAPI validates withexecutedByremoved fromFixUpdate.PATCH /sites/:siteId/opportunities/:opportunityId/fixes/:fixIdwith{ "executedBy": "<attacker-ims-id>", "status": "DEPLOYED" }— verify the storedexecutedByequals the JWT caller'suser_id, not the supplied attacker value.POST /sites/:siteId/opportunities/:opportunityId/fixeswith[{ "executedBy": "<attacker-ims-id>", ... }]— verify the same JWT override on every created fix.GET /sites/:siteId/opportunities/:opportunityId/fixes— verify fixes with server-mintedexecutedByare enriched withexecutedByUser: { firstName, lastName, email }; legacy/malformed values silently skip enrichment.🤖 Generated with Claude Code