Email the requester when their access request is decided by maxkpower · Pull Request #8291 · bitwarden/server

maxkpower · 2026-08-31T14:14:06Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-42817

📔 Objective

Emails the requester when their pending access request is approved or denied. They are blocked on that answer and currently have nothing to watch but the web vault.

One mail class, two bodies. The approved body says access has not started and that the approval lapses if unused: approval records a verdict, and ActivateAccessRequestCommand is what mints the lease.
Sent to the requester only. The approver is the actor and is never a recipient.
The approver's comment is not included, for the same reason as the request reason: free text into an HTML body, and it may name the system. It stays on the request.

Third of four in a stack on pam/uat. Sits on the approver notification PR.

📸 Screenshots

github-actions · 2026-08-31T14:15:03Z

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed at c20bbb36d. Both findings outstanding at the last pass are fixed in 01eacdbd3: AccessRequestDecidedView.text.hbs now uses triple braces for OrganizationName, WindowStart, WindowEnd, and Url, so an org name containing & or ' no longer arrives HTML-escaped in the plain-text body, and RequesterMailNotifier's constructor now guards all five dependencies with ArgumentNullException, matching ApproverMailNotifier and AccessMailNotifier. The rest of the change re-verified clean: the FeatureFlagKeys.Pam guard sits ahead of the organization read, the catch-all logs ids only and cannot fail a decision the repository already committed, TryAddScoped matches the neighbouring registrations, the notifier reads only fields unaffected by the Action/ActionDate stamping that follows it, and the generated .html.hbs still matches its .mjml source with {{OrganizationName}} escaped in the HTML part.

Code Review Details

No findings.

Notes, not findings:

The four new Aikido Security findings on AccessRequestDecidedView.text.hbs:3,5,7,12,14,16 are false positives, and they are what the failing check reports. A .text.hbs is rendered into the plain-text body, where HTML escaping corrupts output rather than protecting it — this is exactly the defect fixed in 01eacdbd3. 34 existing .text.hbs templates in src/Core use triple braces for the same reason. The threads need an @AikidoSec ignore: reply to clear the check.
The older AccessRequestDecidedView.html.hbs:222 thread on {{{Url}}} is also correctly dismissed: Url is composed from IGlobalSettings.BaseServiceUri.VaultWithHash plus a Guid in PamAccessMailView, with no user-supplied segment.
ResolveWithDecisionAsync is a guarded UPDATE that returns nothing, so two approvers overlapping inside the read-to-update window would mail the requester a verdict that was never recorded. Still not raised as a finding: the window is milliseconds wide, the pre-existing EmitAsync(Outcome) and NotifyRequesterAsync calls beside it already share the property, and closing it means changing the repository signature across all four providers — a call for the stack, not this layer.
NotifyDecisionAsync is awaited inline in DecideAsync, adding mail latency to the decide request. Consistent with SubmitAccessRequestCommand.cs:275, which awaits NotifyPendingRequestAsync the same way, so this is the established convention rather than a regression.
NotBefore/NotAfter/WindowStart/WindowEnd are duplicated between AccessRequestDecidedView and AccessRequestPendingView while FormatWindow lives on the shared base. Read as a deliberate split — not every future PAM mail carries a window — so not flagged.
No decision mail on the auto-approval paths (SubmitAccessRequestCommand, RequestLeaseExtensionCommand). Read as intentional and consistent with the stated objective.
The stacked-PR relaxation was not applied: gh pr list --base is unavailable in this environment, so the upper layer could not be confirmed and everything was reviewed on its own terms. Nothing here needed it — every symbol this PR adds has a consumer inside the diff.
Run tests was still pending at review time, so the assessment rests on reading rather than a green test run.

codecov · 2026-08-31T16:54:10Z

Codecov Report

❌ Patch coverage is 91.52542% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.77%. Comparing base (560b397) to head (c20bbb3).

Files with missing lines	Patch %	Lines
...src/Services/Pam/Services/RequesterMailNotifier.cs	88.37%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@                          Coverage Diff                          @@
##           pam/PM-42817/mail-request-pending    #8291      +/-   ##
=====================================================================
+ Coverage                              64.75%   64.77%   +0.01%     
=====================================================================
  Files                                   2603     2605       +2     
  Lines                                 111994   112053      +59     
  Branches                               10046    10054       +8     
=====================================================================
+ Hits                                   72522    72581      +59     
+ Misses                                 37038    37035       -3     
- Partials                                2434     2437       +3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…r denied

…controls

Also trims the mail notification doc comments.

…il-request-decided

…ided text template

aikido-pr-checks · 2026-09-11T22:50:47Z

@@ -0,0 +1,19 @@
+{{#if Approved}}Your access request was approved
+
+An approver has approved your access request in {{{OrganizationName}}}. Your access has not started yet. Open the request and start it once the access window below opens.


Rendering unescaped input in handlebar/mustache template can lead to XSS attacks - high severity
Using {{{ }}} renders an unescaped string into the template. This means the developer is responsible for allowlisting all HTML strings that can be passed in.

Show fix

Remediation: Avoid {{{ }}} usage. Ignore this issue only if the HTML content you feed into it is static. Defend in depth against XSS attacks by using a strict CSP policy. If you must use HTML, sanitize it with a library such as DOMPurify.

_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}