Bug 2067123 - improved handling of policy failures - #1402
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
PolicyFailures.getAll() currently returns a plain object keyed by policy names, which can introduce prototype-pollution edge cases with special keys like __proto__.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves Enterprise Policies failure reporting by tracking per-policy apply failures (including async failures) and surfacing them in about:policies, so administrators can see when a policy was only partially applied and why.
Changes:
- Added a shared
PolicyFailuresregistry plus helper functions to record failures per policy and describe preference type-mismatch failures. - Updated the policy engine and multiple policies/helpers to record failures without aborting subsequent operations, including handling async callback rejections.
- Updated about:policies UI to flag partially applied policies, add a dedicated policy-name column in the Errors tab, and added new xpcshell/browser coverage.
File summaries
| File | Description |
|---|---|
| toolkit/components/enterprisepolicies/PoliciesHelpers.sys.mjs | Introduces failure tracking (PolicyFailures), reportFailure, improved preference failure messaging, and plumbs policy names through helper APIs. |
| toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs | Records validation/callback failures per policy and captures async callback rejections; clears failures on reapply/reset. |
| browser/locales/en-US/browser/aboutPolicies.ftl | Adds the “Not fully applied” marker string for the Active tab. |
| browser/components/enterprisepolicies/tests/xpcshell/xpcshell.toml | Registers the new xpcshell test file. |
| browser/components/enterprisepolicies/tests/xpcshell/test_policy_failures.js | Adds xpcshell tests for failure recording across multiple policy types and async failure paths. |
| browser/components/enterprisepolicies/tests/browser/browser.toml | Registers the new browser test. |
| browser/components/enterprisepolicies/tests/browser/browser_policies_about_policies.js | Adds UI tests verifying the Active-tab marker and Errors-tab policy attribution/deduping. |
| browser/components/enterprisepolicies/Policies.sys.mjs | Switches multiple policy code paths to record failures per policy and to continue applying remaining entries. |
| browser/components/enterprisepolicies/helpers/WebsiteFilter.sys.mjs | Records WebsiteFilter failures into PolicyFailures. |
| browser/components/enterprisepolicies/helpers/ProxyPolicies.sys.mjs | Records Proxy policy failures into PolicyFailures. |
| browser/components/enterprisepolicies/helpers/BookmarksPolicies.sys.mjs | Records bookmark operation failures into PolicyFailures while continuing processing. |
| browser/components/enterprisepolicies/content/aboutPolicies.js | Flags partially-applied policies in Active tab and adds policy-name attribution in Errors tab using recorded failures. |
| browser/components/enterprisepolicies/content/aboutPolicies.html | Adds a policy-name header column to the Errors tab table. |
| browser/components/enterprisepolicies/content/aboutPolicies.css | Styles the partially-applied marker shown under the policy name. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * The failures of every policy, keyed by policy name. | ||
| */ | ||
| getAll() { | ||
| const failures = {}; |
There was a problem hiding this comment.
@bsmth maybe worth but should be a follow up on m-c as well
There was a problem hiding this comment.
Took a look into it and this isn't that reachable, it's possible an admin provides a policy called __proto__ to get this far, but there's no real impact of that. Worth fixing upstream in our next patch that touches this file.
There was a problem hiding this comment.
🔵 Needs a closer look
about:policies error attribution currently keys recorded failures by message text, which can misattribute or drop failures when different policies emit identical messages.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
browser/components/enterprisepolicies/content/aboutPolicies.js:303
generateErrors()currently uses the message string as a unique key (policyByMessage). If two policies record the same message text, later entries overwrite earlier ones and the Errors tab can misattribute or effectively drop a policy’s failure. Prefer rendering recordedPolicyFailuresdirectly as(policy, message)rows, then add any remaining console errors that weren’t already recorded.
toolkit/components/enterprisepolicies/PoliciesHelpers.sys.mjs:117reportFailure()always logs only the message string. Several updated call sites now interpolate exceptions into the string (e.g.${ex}), which loses the original exception object/stack in the console. Allow passing an optional exception/cause through tolazy.log.error()so stacks are preserved when available, while still recording the administrator-facing message.
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Description
Bugzilla: Bug-2067123
Porting across the m-c changes manually here becuase the merge in from upstream will cause conflicts.
Screenshots
Testing
Steps to verify changes:
{"policies": {"Preferences": {"browser.startup.page": "1", "browser.altClickSave": true}}}Expected result:
Policy names have their own column now, with a proper error.