sec-approval

name	sec-approval
description	Help prepare a Firefox security approval request by analyzing local commits/changes and drafting answers to the sec-approval questionnaire. Use when setting sec-approval? on a Bugzilla bug.
argument-hint	[bug-id] [path-to-bug-report]
allowed-tools	["Bash(git:*)","Bash(jj:*)","Bash(searchfox-cli:*)","Bash(bmo-to-md:*)","Bash(python3:*)","Read","Grep","Glob","AskUserQuestion","WebFetch","Write"]

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.

This skill has two phases:

1. Compliance audit — verify the patch strictly follows the Fixing Security Bugs guidelines. Violations are a hard gate: they must be resolved before proceeding.
2. Sec-approval questionnaire — 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

Additionally, verify the test-landing policy:

• If the bug affects released branches: tests should generally not be landed with the fix. They should land in a follow-up at least 4 weeks after the release containing the fix ships. Suggest creating a cloned "task" bug (also security-sensitive) to track the deferred test landing, or setting the in-testsuite flag to ?.
• If the bug is a development-branch-only regression (never shipped in a release): tests may land immediately.

If tests fail: suggest renaming, sanitizing test content, or splitting tests into a deferred follow-up.

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:
  • Get informal sec-approval first
  • Remove bug numbers from all commits in the Try push
  • Exclude vulnerability test cases entirely
  • Never disclose the vulnerability nature or triggering methods in the Try push commit message or mozconfig

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.

Compliance Verdict

Present a summary table:

Check	Status	Details
Commit messages	PASS/FAIL	...
Code comments	PASS/FAIL	...
Identifiers	PASS/FAIL	...
Test cases	PASS/FAIL	...
Try server	PASS/FAIL/N/A	...
Patch obfuscation	Advisory	...

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 don't paint a bulls-eye (or are deferred to follow-up)
• 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: Check for Automatic Approval Eligibility

Before drafting the questionnaire, check if the patch qualifies for automatic approval (i.e., no explicit sec-approval needed):

1. Bug has severity sec-low, sec-moderate, or sec-other/sec-want
2. OR the bug is a recent unshipped regression — a specific regressing commit is identified, the developer has marked ESR and Beta status flags as unaffected, and the vulnerability only shipped in Nightly builds

If either condition is met, inform the user they may be able to land without explicit approval. Ask if they still want to prepare the questionnaire (useful for sec-high/sec-critical, or when in doubt).

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?"

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 clean or deferred (verified in Check 3)

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. Do NOT offer to post if the bug qualifies for automatic approval (see Step 1):

• sec-low, sec-moderate, sec-other, or sec-want: no sec-approval needed — the patch can land directly.
• Recent unshipped Nightly-only regression with ESR and Beta marked unaffected: no sec-approval needed.

For these cases, inform the user the questionnaire file is available for their records but does not need to be posted. Skip the rest of this step.

For sec-high, sec-critical, or unrated bugs (assume worst-case): 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

• Security keywords in Firefox: sec-critical, sec-high, sec-moderate, sec-low, sec-other, sec-want
• The security team is primarily concerned about sec-high and sec-critical bugs that land before a public release
• 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
• When in doubt, assume worst-case severity and request sec-approval
• Contact the security team (needinfo) when uncertain about any guideline