Sec Approval

Security Approval Skill

Dependencies and Bugzilla API key setup live in README.md. Point the user there if bmo-to-md or bmo-sec-approval --check-auth fails.

Scope: sec-approval is required only for a vulnerability in the parent process that is triggerable from a content process — typically a bug keyworded sec-high + csectype-sandbox-escape. A recent parent-process regression that has shipped only in Nightly is exempt when its regressing check-in is known and Beta and ESR are marked unaffected. All other security fixes can land without explicit sec-approval once their normal review and testing requirements are met. Most media bugs fall outside the gate because the affected code runs in the content, RDD, GMP, or Utility process. Phase 1 still applies to every security bug; Phase 2 Step 1 decides whether the questionnaire is needed.

This skill has two phases:

1. Compliance audit — verify the patch follows the Fixing Security Bugs guidelines. Violations are a hard gate: they must be resolved before proceeding.
2. Sec-approval questionnaire — determine whether sec-approval is required at all, and if so draft answers to the Security Approval questions.

Arguments: $0

Parse the arguments: the first token that looks like a number is the bug ID, and any token that contains a / (path separator) is the bug report path. Both are optional and may appear in either order.

Preliminary: Gather Patch Context

Retrieve the bug

Security bugs are private and cannot be fetched via the MCP tool mcp__moz__get_bugzilla_bug — it will always fail for sec-* bugs. Use the following priority order:

1. Bug report path provided: if the user supplied a path argument, read the summary file directly from that directory. The path points to a folder containing markdown files generated by bmo-to-md (look for summary.md or bug-<id>.md).
2. Bug ID provided but no path: use bmo-to-md to download the report:
   • Check that bmo-to-md is installed: run bmo-to-md --help. If not found, ask the user to install it: cargo install bmo-to-md (see https://github.com/padenot/bmo-to-md).
   • Check that a Bugzilla API key is configured (never read or print the key itself):

     python3 .claude/skills/sec-approval/bmo-sec-approval --check-auth
     

     If it fails, ask the user to store their key in ~/.config/bugzilla/config.toml (see Step 6, item 1 for instructions).
   • Download to a temp folder: bmo-to-md -o /tmp/bug-<id> -a <bug_id>
   • If bmo-to-md fails, stop and report to the user.
3. Neither provided: ask the user whether they have a local bug report (markdown files from bmo-to-md). If they provide a path, read it. If they provide a bug ID, go to step 2. If neither, proceed without bug context (the compliance audit can still run on the patch alone, but the questionnaire will need the user to supply missing information manually).
4. Read the summary to understand the vulnerability type, severity, and any existing comments. Note the sec-* keyword (sec-critical, sec-high, sec-moderate, sec-low) and the status-firefox* flags.

Inspect the local changes

Determine which VCS is in use and read the relevant commits:

# Check if using jj or git
jj --version 2>/dev/null && echo "jj" || echo "git"

If jj:

jj log -T builtin_log_detailed -r 'trunk()..@'
jj diff -r 'trunk()..@'

If git:

git log --oneline origin/main..HEAD
git diff origin/main..HEAD

Collect:

• Commit messages (check-in comments)
• All changed files
• The full diff

Phase 1: Compliance Audit — Fixing Security Bugs

Audit the patch against every rule from the Fixing Security Bugs guidelines. Work through each check below. For each one, report PASS or FAIL with specifics. If any check fails, present concrete fixes and ask the user to resolve them before moving to Phase 2.

Check 1: Commit Messages

Commit messages must not contain any of the following:

• Nature of the vulnerability (overflow, use-after-free, XSS, CSP bypass, null deref, race condition, etc.)
• Exploitation methods or triggering mechanisms
• Security-related trigger words: "security", "exploitable", "vulnerable", "vulnerability", "CVE", "attacker", "exploit", "malicious", "crash" (when crash implies a security issue)
• Security approver names
• Affected Firefox versions or components in a security context
• Any phrasing that makes the patched vulnerability obvious

If a commit message fails: suggest a generic rewrite. Examples:

• Bad: Fix use-after-free in MediaDecoder::Shutdown
• Good: Improve lifetime management in MediaDecoder
• Bad: Fix heap buffer overflow when parsing VP9
• Good: Add validation for buffer size in VP9 decoder
• Bad: Prevent XSS in content process
• Good: Strengthen input sanitization in content process

Omitting a detailed commit message entirely is acceptable — details should go in the private bug comment instead.

Check 2: Code Comments

Inline comments in the diff must not:

• Reveal the nature of the vulnerability or exploitation vectors
• Disclose that the change is security-related
• Mention the bug as a security issue
• Reference CVE numbers or sec-* keywords
• Include stack traces, ASAN output, or crash signatures
• Reference file paths or line numbers that pinpoint the flaw (e.g. // See H265.cpp:651)
• Provide reasoning that reveals the attack vector (e.g. // Without this check, an attacker could trigger X by doing Y)

If comments fail: suggest removing them or rewriting them as generic correctness/robustness comments. Security context belongs in the private bug, not in the source.

Check 3: Security-Revealing Identifiers

New or renamed variables, functions, classes, or constants in the diff must not reveal the vulnerability class or attack vector:

• Bad: fixUAFBuffer, preventOverflow, sanitizeXSSInput
• Good: buffer, validateSize, sanitizeInput
• Bad: kMaxAllocBeforeOOB, gNullDerefGuard
• Good: kMaxAlloc, gGuard

If identifiers fail: suggest neutral renames that describe the correctness purpose, not the security purpose. The vulnerability context belongs in the private bug.

Check 4: Test Cases

Tests included in the patch must not:

• Have filenames that hint at the vulnerability (e.g., test_uaf_in_foo.html, test_overflow_parser.js)
• Contain comments or assertions that describe the security nature of the fix
• Include exploit-like test content that demonstrates the attack vector

Test-landing policy. Tests are no longer withheld by default. The current Fixing Security Bugs guidance is that "the advent of AI tools has frequently made this precaution not worth the extra effort". Judge case by case:

• Default: land the tests with the fix, provided they are sanitized per the rules above.
• Split the tests into a separate commit only when the vulnerability is unusually hard to deduce from the patch, or unusually hard to trigger — i.e. when the test, not the diff, is what gives it away.
• If split and the bug shipped in a release: the tests land at least 4 weeks after the release containing the fix goes live. Track it either by cloning a security-sensitive "land tests for bug XXXXXXX" task bug (rated sec-other), or by setting in-testsuite to ? plus a whiteboard tag of the form [reminder-test YYYY-MM-DD] — Bugbot needinfos the assignee on that date; flip in-testsuite to + once the tests land.
• If the bug never shipped in a release (development-branch-only regression): tests can land immediately on all affected branches.

If tests fail the sanitization rules: suggest renaming or rewriting the test content. Deferring the tests is a judgment call, not an automatic requirement.

Check 5: Try Server / CI

Check whether the user has pushed (or plans to push) to Try:

• Best practice: do not push to Try at all; test locally instead.
• If a Try push is necessary: remind the user to:
  • Remove bug numbers from all commits in the Try push
  • Never push the bug's own vulnerability testcase; ideally push no tests
  • Never disclose the vulnerability nature or triggering methods in the Try push commit message or mozconfig
  • Fold the change in with unrelated work in the same area, so the push doesn't read as a security fix

Ask the user about their Try push status.

Check 6: Patch Obfuscation

Review whether the fix can be plausibly framed as a non-security change (performance improvement, correctness fix, code cleanup). The goal is to reduce the identifiability of the security fix:

• Can the fix be bundled with other unrelated work?
• Does the diff look like a pure correctness or robustness improvement rather than a targeted security patch?

This is advisory — report observations but do not block on it. The current process explicitly downgrades the value of obfuscation: AI tooling can analyze a fix, derive the root cause, and often build a proof of concept, so shipping the fix quickly is the primary protection. Obfuscation matters most for Try pushes (Check 5), where the patch is public before it lands.

Compliance Verdict

Present a summary table:

Then present the checklist for the user to confirm:

• Commit message is not security-revealing
• No security-revealing inline comments in the patch
• No security-revealing identifiers in the patch
• Tests are sanitized (split into a follow-up only if the test, not the diff, is what reveals the flaw)
• Not pushed to Try with bug number / security tests
• Bug is filed as restricted/sec-* on Bugzilla

If any check is FAIL: present the specific violations with suggested fixes. Ask the user if they want help fixing them now. Re-audit after fixes.

This is a hard gate. Ask the user to confirm all checklist items pass before proceeding. Do not proceed to Phase 2 until Checks 1–5 all pass and the user gives explicit approval to continue.

Phase 2: Security Approval Questionnaire

Step 1: Is sec-approval Required at All?

The gate is narrow: sec-approval is only required for a vulnerability in the parent process that is triggerable from a content process — typically a bug keyworded sec-high and csectype-sandbox-escape. Previously all sec-high bugs needed approval; that rule is now inverted.

Decide from the actual affected and triggering processes, not from severity or an IPC actor's name alone. sec-high by itself does not require approval, and csectype-sandbox-escape is supporting evidence rather than a substitute for checking the process boundary.

No sec-approval needed if any of these hold:

1. Rating is sec-low, sec-moderate, sec-other, or sec-want
2. The flaw only affects the content process, regardless of a sec-high rating
3. The flaw only affects some other non-parent process (GPU, RDD, GMP, Utility, Socket, …), regardless of a sec-high rating
4. It is a cross-process bug whose target process is not the parent — the correct keyword for that case is csectype-priv-escalation, not csectype-sandbox-escape. Fix the keyword if you see it used wrongly.
5. It is a parent-process bug but a recent unshipped regression on mozilla-central: a specific regressing check-in is identified, the ESR and Beta status flags are marked unaffected, and the vulnerability only shipped in Nightly builds

Work out which process the flaw actually runs in — for media code this is usually not the parent process:

• Decoding (dom/media/platforms/, dom/media/ipc/) runs in the RDD or Utility process; EME/CDM code runs in the GMP process; the playback pipeline mostly runs in the content process.
• Parent-process media code does exist: MediaManager / device enumeration, the *ProcessHost / *ProcessParent process-launch and IPC-bridging code, permission and pref plumbing, and code under toolkit/ or browser/.
• Careful with IPDL naming: a …Parent actor is the parent side of the protocol, which usually lives in the RDD/GPU/Utility process, not in the parent process — RDDParent and RemoteDecoderManagerParent run in the RDD process. Check where the actor is actually constructed.
• The case that does need approval: the parent process mishandling data or a message that a content process controls.

If the bug is unrated, rate it following the Client Severity Guidelines rather than defaulting to worst-case.

State the determination to the user with the reason, e.g. "No sec-approval required: sec-high, but the flaw is in the RDD process (csectype-priv-escalation), not the parent process." If sec-approval is not required, stop after reporting the Phase 1 audit and this determination. Do not draft the questionnaire or offer to set the flag unless the user explicitly asks for a draft anyway.

If the answer is genuinely unclear — an ambiguous process boundary, or an unrated bug that might reach the parent — the docs are explicit: request sec-approval anyway and move on. "Don't overthink it!"

Step 2: Answer the Questionnaire

Work through all questions systematically by examining the diff and commit messages gathered in the Preliminary step.

Q1: Patch Visibility

Question: "How easily could an exploit be constructed based on the patch?"

(The source docs phrase this as "How easily can the security issue be deduced from the patch?" — cover both senses: how visible the flaw is in the diff, and how much work remains to weaponize it.)

Analyze:

• Does the diff clearly show a specific memory safety fix (e.g., bounds check, null check, free ordering fix)?
• Are variable names, function names, or code structure self-explanatory about the vulnerability class?
• Could a malicious actor trivially write an exploit by reading the diff?

Rate as one of:

• Easy: The trigger is obvious from the patch and reproducible with standard web APIs alone; no special timing or heap work needed. A security researcher would immediately understand the vulnerability class and location.
• Moderate: The trigger is discoverable from the patch + public API knowledge, but reliable exploitation needs one additional factor (timing, heap shaping, or a non-default condition).
• Difficult: The trigger is non-obvious from the patch and requires independent discovery; OR reliable exploitation needs two or more independent factors (e.g. non-obvious event vector + deterministic GC timing + heap shaping). For race conditions, also use Difficult when the patch only fixes one side of the race — the other racing operation and the API sequence needed to reproduce the window are not disclosed by the patch.

Always open the answer with one of "Easy.", "Moderate.", or "Difficult.", followed by 2-3 sentences of justification. Base the answer on the patch itself — what the fix reveals about the vulnerable code path — and on whether an attacker could recreate the conditions using only publicly available web APIs.

Q2: Comments and Tests as Bulls-Eyes

Question: "Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?"

By this point, Phase 1 should have already caught and resolved any bulls-eyes. Confirm that:

• Commit messages are clean (verified in Check 1)
• Code comments are clean (verified in Check 2)
• Test files are sanitized, or split into a follow-up (verified in Check 4)

Report the current state — ideally "No, the patch has been reviewed for information leaks."

Q3: Affected Branches

Question: "Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?"

Fetch the current release calendar to determine which versions are on each channel:

WebFetch: https://whattrainisitnow.com/calendar/

Extract the current Nightly, Beta, Release, and ESR version numbers.

Check the current Firefox version from the local tree:

cat config/milestone.txt

Check status-firefox* flags from the bug (if fetched). If not available, inspect the code history to find when the vulnerable code was introduced:

If git:

git log --oneline --follow -S "<key_symbol>" -- <affected-file> | head -10

If jj:

jj log -r 'ancestors(trunk())' -T builtin_log_oneline -p -s -- <affected-file> | head -50

Cross-reference with the status-firefoxNN: affected/unaffected/fixed flags on the bug if available.

Verify reachability on each branch. The vulnerable code may exist on a branch but be unreachable if it is gated behind a preference or feature flag that is disabled on that branch. For each affected ESR and release branch:

1. Check the pref/flag value on the branch itself — do not assume the current trunk value applies to older branches:

   # Example: check a pref on each ESR branch
   git show upstream/esr115:<path-to-pref-file> | grep -A10 '<pref-name>'
   git show upstream/esr128:<path-to-pref-file> | grep -A10 '<pref-name>'
   git show upstream/esr140:<path-to-pref-file> | grep -A10 '<pref-name>'
   

2. Check that the vulnerable code exists on the branch — it may have been added after the branch point:

   git show upstream/esr115:<affected-file> 2>&1 | head -3
   

3. Determine when the feature was enabled — find the commit that changed the pref from disabled to enabled, and check which branches contain it:

   git log --all --oneline --grep='<enable-bug>' -- <pref-file>
   git branch --all --contains <enable-commit>
   

A branch is not affected if the pref is false or @IS_NIGHTLY_BUILD@ on release builds, even if the vulnerable code exists. Clearly state in the answer which branches are affected and which are not, with the reason (e.g., "ESR 128: not affected — WebCodecs disabled by default, value: @IS_NIGHTLY_BUILD@").

If no regression range is identified and no pref gate exists, assume the worst — all supported branches are affected.

Use this answer format: "Introduced in Firefox 118 (Bug XXXXXXX). Affects Nightly (150), Beta (149), Release (148), and ESR 140. ESR 128 and ESR 115 are not affected: the feature is disabled by default on those branches."

Q4: Regression Source

Question: "If not all supported branches, which bug introduced the flaw?"

Only needed if Q3 shows some branches are unaffected. Find the introducing commit:

If git:

git log --oneline -S "<key_symbol>" -- <affected-file> | head -10
git blame -L <line>,<line> <affected-file>

If jj:

jj log -r 'ancestors(trunk())' -T builtin_log_oneline -s -- <affected-file> | head -30
jj annotate <affected-file>

Report the bug number or commit that introduced the flaw. If the vulnerable code was introduced in one bug but only became reachable due to a later bug (e.g., a feature flag being enabled), report both:

• The bug that introduced the vulnerable code
• The bug that made it reachable (e.g., enabled the feature pref)

This distinction matters for determining which branches actually need a fix versus which are technically vulnerable but unexploitable.

Q5: Backport Status

Questions (two separate bullets in the output):

• "Do you have backports for the affected branches?"
• "If not, how different, hard to create, and risky will they be?"

Ask the user:

• Are backports already prepared?
• If not: review the diff complexity to assess backport risk:
  • Low risk: Small, self-contained change in code that hasn't diverged
  • Medium risk: Moderate change, code has some differences across branches
  • High risk: Large refactor, depends on other recent changes, or code has significantly diverged

Note: backports to ESR require separate approval — mention this if ESR is affected.

Q6: Regression Risk and Testing

Question: "How likely is this patch to cause regressions; how much testing does it need?"

Assess:

• Size: Number of lines changed
• Scope: How many call sites / how core is the changed component?
• Test coverage: Are there existing tests? Is a new test included?
• Change type: Pure addition (lower risk) vs. behavioral change (higher risk)

Rate as:

• Low: Minimal, well-tested, targeted fix with no API changes
• Moderate: Some risk, needs test run on affected platforms
• High: Significant change, needs thorough testing including edge cases

Q7: Landing Readiness

Question: "Is the patch ready to land after security approval is given?"

Answer Yes or No. Consider whether:

• The patch has been reviewed (r+ on Phabricator)
• CI/try results are green (or local testing is sufficient)
• Any blockers remain

Q8: Android Affected

Question: "Is Android affected?"

Answer Yes, No, or Unknown. Check whether:

• The affected code paths are shared with Android (GeckoView)
• The code is desktop-only (e.g., Windows-specific compositing, macOS-only widget code) or cross-platform
• If the code is in gfx/, dom/media/, or other shared directories, it is likely Android-affected

Step 3: Draft the Questionnaire

Generate the complete text using the same format that Bugzilla auto-generates for sec-approval requests. The comment will be posted with is_markdown: true so Markdown is rendered on Bugzilla. Use this exact format:

### Security Approval Request
* **How easily could an exploit be constructed based on the patch?**: <answer>
* **Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?**: <answer>
* **Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?**: <answer>
* **If not all supported branches, which bug introduced the flaw?**: <answer — or "N/A">
* **Do you have backports for the affected branches?**: <answer>
* **If not, how different, hard to create, and risky will they be?**: <answer>
* **How likely is this patch to cause regressions; how much testing does it need?**: <answer>
* **Is the patch ready to land after security approval is given?**: <Yes/No>
* **Is Android affected?**: <Yes/No/Unknown>

*Drafted with the assistance of Claude Code — reviewed and approved by the patch author.*

Keep answers factual, specific, and concise. Each answer follows the **: on the same line as a single flowing sentence or paragraph. Do not reveal more about the vulnerability than necessary.

Step 4: Present and Confirm

Present to the user:

1. The draft questionnaire text to copy into Bugzilla.
2. A reminder that Phase 1 compliance was already verified (re-show the summary table if fixes were applied during this session).

Ask the user if they want to revise any answer before finalizing.

Step 5: Generate Markdown File

After the user confirms they are satisfied with the questionnaire answers, generate a markdown file at the repository root named sec-approval-bug-<bug_id>.md (e.g., sec-approval-bug-1234567.md). If no bug ID is available, use sec-approval.md.

The file must use the exact same markdown format as the questionnaire drafted in Step 3 (the Bugzilla auto-generated format):

### Security Approval Request
* **How easily could an exploit be constructed based on the patch?**: <answer>
* **Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?**: <answer>
* **Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?**: <answer>
* **If not all supported branches, which bug introduced the flaw?**: <answer>
* **Do you have backports for the affected branches?**: <answer>
* **If not, how different, hard to create, and risky will they be?**: <answer>
* **How likely is this patch to cause regressions; how much testing does it need?**: <answer>
* **Is the patch ready to land after security approval is given?**: <answer>
* **Is Android affected?**: <answer>

*Drafted with the assistance of Claude Code — reviewed and approved by the patch author.*

Use the Write tool to create this file, then inform the user of the file path.

Step 6: Post to Bugzilla (Optional)

This step only applies when sec-approval is required — i.e. Step 1 concluded the flaw is in the parent process and triggerable from a content process (typically sec-high + csectype-sandbox-escape). Do NOT offer to post if any Step 1 exemption applies:

• sec-low, sec-moderate, sec-other, or sec-want: the patch can land directly.
• sec-high but content-process-only, or targeting a non-parent process (GPU, RDD, GMP, Utility, Socket, …): the patch can land directly.
• Recent unshipped Nightly-only regression with ESR and Beta marked unaffected: the patch can land directly.

For these cases, inform the user that no sec-approval request needs to be posted. If the user explicitly requested a questionnaire draft anyway, note that it is only for their records. Skip the rest of this step.

For parent-process sandbox escapes — and for genuinely ambiguous cases, where the guidance is to request approval rather than agonize over it — ask the user whether they want to post the questionnaire directly to Bugzilla and request sec-approval? on the attachment.

If the user agrees:

1. Check API key: run the auth check — never read or print the key itself:

   python3 .claude/skills/sec-approval/bmo-sec-approval --check-auth
   

   If it fails, offer the user two options and stop:

   • Persistent (recommended): store the key once for all future sessions:

     mkdir -p ~/.config/bugzilla && cat > ~/.config/bugzilla/config.toml << 'EOF'
     api_key = "YOUR_KEY"
     EOF
     chmod 600 ~/.config/bugzilla/config.toml
     

   • One-time: set it for the current session only:

     export BMO_API_KEY="YOUR_KEY"
     

   Tell the user to replace YOUR_KEY with their Bugzilla API key from https://bugzilla.mozilla.org/userprefs.cgi?tab=apikey. If they already have bmo-to-md configured, the key in ~/.config/bmo-to-md/config.toml is also picked up automatically.

2. Identify the attachment: the sec-approval flag must be set on the Phabricator attachment (the patch revision), not on the bug itself.

   First, extract Phabricator revision IDs from the local commits gathered in the Preliminary step:

   If git:

   git log origin/main..HEAD --format=%B | grep -oP 'Differential Revision:.*/(D\d+)' | sed 's|.*Differential Revision:.*/||'
   

   If jj:

   jj log -r 'trunk()..@' -T description | grep -oP 'Differential Revision:.*/(D\d+)' | sed 's|.*Differential Revision:.*/||'
   

   Then fetch all active Phabricator attachments from Bugzilla:

   python3 .claude/skills/sec-approval/bmo-sec-approval <bug_id> --list
   

   This prints each attachment with its attachment ID, Phabricator revision ID, summary, and existing flags.

   Cross-reference the revisions found in the commits with the attachments from Bugzilla. Present a table like:

   Revision	Attachment ID	Summary	Flags	In local commits?
   D290715	9560245	Bug 2022604 - ...		Yes
   D290800	9560300	Bug 2022604 - ...		No

   • If exactly one attachment matches a local commit revision, suggest it as the default. Ask the user to confirm.
   • If multiple match, ask the user to pick one.
   • If none match (e.g. patch not yet submitted to Phabricator), tell the user to submit the patch first, or let them provide an attachment ID manually.

3. Dry-run first: always run with --dry-run so the user can verify before posting:

   python3 .claude/skills/sec-approval/bmo-sec-approval \
       <bug_id> sec-approval-bug-<bug_id>.md --attachment <id> --dry-run
   

   Show the dry-run output to the user and ask for confirmation.

4. Post: after the user confirms, run without --dry-run:

   python3 .claude/skills/sec-approval/bmo-sec-approval \
       <bug_id> sec-approval-bug-<bug_id>.md --attachment <id>
   

5. Report: show the user the Bugzilla URL from the script output.

Tips

• Severity keywords: sec-critical, sec-high, sec-moderate, sec-low, sec-other, sec-want. Process keywords: csectype-sandbox-escape (target is the parent process) vs csectype-priv-escalation (cross-process, but the target is not the parent).
• The sec-approval gate now covers only parent-process vulnerabilities triggerable from a content process; everything else lands under normal review. Phase 1 hygiene still applies to every security bug.
• sec-audit / sec-want are the right home for discussion of code patterns or architectural limitations — keep that out of bugs filed by external reporters, since AI is good at turning a described pattern into new bugs.
• Backports to ESR require separate approval; mention this if ESR is affected
• If the patch is on an uplift request (not main/nightly), that changes the urgency and review process
• If you can't tell which process the flaw lives in, or the bug is unrated and might reach the parent, request sec-approval anyway — "Don't overthink it!"
• Contact the security team (needinfo, or #security on Slack — current sec-approvers are Dan Veditz and Tom Ritter) when uncertain