diff options
Diffstat (limited to 'docs/bug-mgmt/processes')
| -rw-r--r-- | docs/bug-mgmt/processes/accessibility-review.md | 100 | ||||
| -rw-r--r-- | docs/bug-mgmt/processes/doc-requests.rst | 39 | ||||
| -rw-r--r-- | docs/bug-mgmt/processes/fixing-security-bugs.rst | 217 | ||||
| -rw-r--r-- | docs/bug-mgmt/processes/labels.rst | 155 | ||||
| -rw-r--r-- | docs/bug-mgmt/processes/regressions.rst | 64 | ||||
| -rw-r--r-- | docs/bug-mgmt/processes/security-approval.rst | 192 | ||||
| -rw-r--r-- | docs/bug-mgmt/processes/shared-bug-queues.rst | 34 |
7 files changed, 801 insertions, 0 deletions
diff --git a/docs/bug-mgmt/processes/accessibility-review.md b/docs/bug-mgmt/processes/accessibility-review.md new file mode 100644 index 0000000000..3358defd7e --- /dev/null +++ b/docs/bug-mgmt/processes/accessibility-review.md @@ -0,0 +1,100 @@ +# Accessibility Review + +## Introduction +At Mozilla, accessibility is a fundamental part of our mission to ensure the +internet is "open and accessible to all," helping to empower people, regardless +of their abilities, to contribute to the common good. Accessibility Review is a +service provided by the Mozilla Accessibility team to review features and +changes to ensure they are accessible to and inclusive of people with +disabilities. + +## Do I Need Accessibility Review? +You should consider requesting accessibility review if you aren't certain +whether your change is accessible to people with disabilities. Accessibility +review is optional, but it is strongly encouraged if you are introducing new +user interface or are significantly redesigning existing user interface. +Review should be requested both on the design side _and_ on the engineering side. + +## When Should I Request Accessibility Review? +Generally, it's best to request accessibility review as early as possible, even +during the product requirements or UI design stage. Particularly for more +complex user interfaces, accessibility is much easier when incorporated into the +design, rather than attempting to retro-fit accessibility after the +implementation is well underway. + +The accessibility team has developed the [Mozilla Accessibility Release +Guidelines](https://wiki.mozilla.org/Accessibility/Guidelines) which outline +what is needed to make user interfaces accessible. To make accessibility review +faster, you may wish to try to verify and implement these guidelines prior to +requesting accessibility review. + +For design reviews, please allow at least a week between review request and expected-engineering-handoff. The deadline for engineering review requests is Friday of the first week of nightly builds for the release in which the feature/change is expected to ship. +This is the same date as the PI Request deadline. + +## Requesting Design Review +Design review should be requested using the Accessibility Review Request +workflow in the #accessibility slack channel. To access this workflow, click on +the workflow link in the #accessibility slack channel header or in the links +sidebar. You can also type `/accessibility design review request` from any +channel or DM. Then, fill in the information requested. + +Please complete the following self-review tasks **before requesting review**. Note: Some of the following links require SSO authentication. + +- **Perform a contrast audit**: Using a [figma plugin that audits contrast](https://www.figma.com/community/plugin/748533339900865323/Contrast), check your designs for color contrast sufficiency. Your designs should be at least "AA" rated in order to pass accessibility review. "AAA" is even better! If there are particular components that are difficult to adjust to meet "AA" standards, make a note in the figma file and the a11y team will provide specific guidance during review. +- **Add focus order and role annotations**: Focus order annotations describe the behaviour a keyboard user should expect when navigating your design. Generally this follows the reading order. Note that we only care about focusable elements here (ie. links, buttons, inputs, etc.). +Role annotations help screen readers and other assistive technologies identify the "kind" of component they're navigating through. These mappings expose semantic information to the user. You can find a [list of common roles here](https://www.codeinwp.com/blog/wai-aria-roles/). You may want to use a [figma plugin that annotates focus and roles](https://www.figma.com/community/plugin/731310036968334777/A11y---Focus-Order) for this process. You do not need to annotate every view in your design, pick those with the largest amount of new content. +- **Create Windows High-Contrast Mode (HCM) mockups**: Our designs should be accessible to users [running HCM](https://docs.google.com/document/d/1El3XJiAdA5gFcG7H9iI1dNmLbht0hXfi_oKBZPWx3t0/edit). You can read more about [how HCM affects color selection](https://firefox-source-docs.mozilla.org/accessible/ColorsAndHighContrastMode.html), and [how to design for HCM](https://wiki.mozilla.org/Accessibility/Design_Guide). Using the ["Night Sky" HCM palette](https://www.figma.com/file/XQrEePCCJebjlVBQwNggQ6/Pro-Client-Accessibility-Reviews?node-id=25%3A3848), translate your designs into High Contrast. Remember, it's important we use these colors **semantically**, not based on a desire for a particular aesthetic. Colors are labelled according to their uses -- `Background` for page background, `Button Text` for button or control text, `Selected Item Background` for backgrounds of selected or active items, etc.. You do not need to mock up every view in your design, pick those with the largest amount of new content. You can find [examples of previous HCM mock ups here](https://www.figma.com/file/XQrEePCCJebjlVBQwNggQ6/Accessibility). +Where possible, we should rely on SVG's and PNG's for image content to increase adaptability. + +## Requesting Engineering Review +For an engineering-focused review, you submit a review request by setting the a11y-review flag to "requested" +on a bug in Bugzilla and filling in the template that appears in the comment +field. For features spanning several bugs, you may wish to file a new, dedicated +bug for the accessibility review. Otherwise, particularly for smaller changes, +you may do this on an existing bug. Note that if you file a new bug, you will +need to submit the bug and then edit it to set the flag. + +Once the review is done, the accessibility team will set the a11y-review +flag depending on the outcome: + +- passed: Either no changes are required, or some changes are required but the + accessibility team does not believe it is necessary to review or verify + those changes prior to shipping. Generally, a review will not be passed if + there are outstanding s2 or certain high impact s3 accessibility defects + which should block the feature or change from shipping. However, despite + their high severity, some s2 defects are trivial to fix (e.g. missing + accessibility labels), so the accessibility team may elect to pass the + review on the condition that these are fixed promptly. +- changes required: Changes are required and the accessibility team will need to + review or verify those changes before determining whether the accessibility + of the feature or change is acceptable for shipping. It is the + responsibility of the requesting engineering team to re-request + accessibility review once changes are made to address the accessibility + team's initial round of feedback. + +## Phabricator Review Group +There is also an +[accessibility-frontend-reviewers](https://phabricator.services.mozilla.com/tag/accessibility-frontend-reviewers/) +group on Phabricator. This should generally only be used if: + +1. The patch is a change requested as part of an accessibility review as + described above; or +2. There is a very specific aspect of accessibility implemented in the patch and + you would like the accessibility team to confirm whether it has been + implemented correctly. + +If you would instead like the accessibility team to assess **whether a feature +or change is sufficiently accessible**, please request an accessibility +**engineering** review as described above. Without this, the accessibility team +will not have sufficient context or understanding of the change or how to test +it, which is necessary to thoroughly assess its accessibility. + +## Questions? +If you have any questions, please don't hesitate to contact the Accessibility +team: + +* \#accessibility on + [Matrix](https://matrix.to/#/!jmuErVonajdNMbgdeY:mozilla.org?via=mozilla.org&via=matrix.org) + or [Slack](https://mozilla.slack.com/archives/C4E0W8B8E) +* Email: accessibility@mozilla.com +* Please avoid reaching out to individual team members directly -- containing review requests and questions in these channels helps us balance our workload. Thank you! diff --git a/docs/bug-mgmt/processes/doc-requests.rst b/docs/bug-mgmt/processes/doc-requests.rst new file mode 100644 index 0000000000..dc390a27cb --- /dev/null +++ b/docs/bug-mgmt/processes/doc-requests.rst @@ -0,0 +1,39 @@ +User documentation requests +=========================== + +If you are working on a change (bugfix, enhancement, or feature) which +would benefit from user-facing documentation, please use the +``user-doc-firefox`` flag to request it. + +This flag can be modified by anyone with ``EDITBUGS`` privileges. + +The default value of the flag is ``---``. + +If the bug needs user-facing documentation, set the flag to +``docs-needed``. This flag will be monitored by the support.mozilla.org +(SUMO) team. + +Once the docs are ready to be published, set the flag to +``docs-completed``. + +If it’s determined that documentation is not need after setting the flag +to ``docs-needed``, update the flag to ``none-needed`` so we know that +it’s been reviewed. + +Summary +------- + +=========== == ============== +From To +=========== == ============== +— to none-needed +— to docs-needed +docs-needed to none-needed +docs-needed to docs-completed +=========== == ============== + +Notes +----- + +A flag is used instead of the old keywords because flags can be +restricted to a subset of products and components. diff --git a/docs/bug-mgmt/processes/fixing-security-bugs.rst b/docs/bug-mgmt/processes/fixing-security-bugs.rst new file mode 100644 index 0000000000..e07853ac79 --- /dev/null +++ b/docs/bug-mgmt/processes/fixing-security-bugs.rst @@ -0,0 +1,217 @@ +Fixing Security Bugs +==================== + +A bug has been reported as security-sensitive in Bugzilla and received a +security rating. + +If this bug is private - which is most likely for a reported security +bug - **the process for patching is slightly different than the usual +process for fixing a bug**. + +Here are security guidelines to follow if you’re involved in reviewing, +testing and landing a security patch. See +:ref:`Security Bug Approval Process` +for more details about how to request sec-approval and land the patch. + +Keeping private information private +----------------------------------- + +A security-sensitive bug in Bugzilla means that all information about +the bug except its ID number are hidden. This includes the title, +comments, reporter, assignee and CC’d people. + +A security-sensitive bug usually remains private until a fix is shipped +in a new release, **and after a certain amount of time to ensure that a +maximum number of users updated their version of Firefox**. Bugs are +usually made public after 6 months and a couple of releases. + +From the moment a security bug has been privately reported to the moment +a fix is shipped and the bug is set public, all information about that +bug needs to be handled carefully in order to avoid an unmitigated +vulnerability becoming known and exploited before we release a +fix (0-day). + +During a normal process, information about the nature of bug can be +accessed through: + +- Bug comments (Bugzilla, GitHub issue) +- Commit message (visible on Bugzilla, tree check-ins and test servers) +- Code comments +- Test cases +- Bug content can potentially be discussed on public IRC/Slack channels + and mailing list emails. + +When patching for a security bug, you’ll need to be mindful about what +type of information you share and where. + +In commit messages +~~~~~~~~~~~~~~~~~~ + +People are watching code check-ins, so we want to avoid sharing +information which would disclose or help finding a vulnerability too +easily before we shipped the fix to our users. This includes: + +- The **nature of the vulnerability** (overflow, use-after-free, XSS, + CSP bypass...) +- **Ways to trigger and exploit that vulnerability** + - In commit messages, code comments and test cases. +- The fact that a bug / commit is security-related: + + - **Trigger words** in the commit message or code comments such as + "security", "exploitable", or the nature of a security vulnerability + (overflow, use-after-free…) + - **Security approver’s name** in a commit message. +- The Firefox versions and components affected by the vulnerability. +- Patches with an obvious fix. + +In Bugzilla and other public channels +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In addition to commits, you’ll need to be mindful of not disclosing +sensitive information about the bug in public places, such as Bugzilla: + +- Mention the bugs in comment of the private bug instead. +- Do not comment sensitive information in public related bugs. +- Also be careful about who you give bug access to: **double check + before CC’ing the wrong person or alias**. +- As of recently, you may now add public bugs in the “duplicate”, + “depends on”, “blocks”, “regression”, “regressed by”, or “see also” section. + Bugzilla will only reveal those relationships to people with ``editbugs`` + permission or access to the security bug. + +On IRC, Slack channels, GitHub issues, mailing lists: If you need to +discuss about a security bug, use a private channel (protected with a +password or with proper right access management) + +During Development +------------------ + +Testing security bugs +~~~~~~~~~~~~~~~~~~~~~ + +Pushing to Try servers requires Level 1 Commit access but **content +viewing is publicly accessible**. + +As much as possible, **do not push to Try servers**. Testing should be +done locally before checkin in order to prevent public disclosing of the +bug. + +Because of the public visibility, pushing to Try has all the same concerns +as committing the patch. Please heed the concerns in the +:ref:`landing-your-patch` section before thinking about it, and check with +the security team for an informal "sec-approval" before doing so. + +**Do not push the bug's own vulnerability testcase to Try.** + +If you need to push to Try servers, make sure your tests don’t disclose +what the vulnerability is about or how to trigger it. Do not mention +anywhere it is security related. + +Obfuscating a security patch +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If your security patch looks obvious because of the code it contains +(e.g. a one-line fix), or if you really need to push to Try servers, +**consider integrating your security-related patch to non-security work +in the same area**. And/or pretend it is related to something else, like +some performance improvement or a correctness fix. **Definitely don't +include the bug number in the commit message.** This will help making +the security issue less easily identifiable. (The absolute ban against +"Security through Obscurity" is in relation to cryptographic systems. In +other situations you still can't *rely* on obscurity but it can +sometimes buy you a little time. In this context we need to get the +fixes into the hands of our users faster than attackers can weaponize +and deploy attacks and a little extra time can help.) + +Requesting sec-approval +~~~~~~~~~~~~~~~~~~~~~~~ + +See :ref:`Security Bug Approval Process` +for more details + +.. _landing-your-patch: + +Landing your patch (with or without sec-approval) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Before asking for sec-approval or landing, ensure your patch does not disclose +information about the security vulnerability unnecessarily. Specifically: + +#. The patch commit message and its contents should not mention security, + security bugs, or sec-approvers. + Note that you can alter the commit message directly in phabricator, + if that's the only thing you need to do - you don't need to amend your + local commit and re-push it. + While comprehensive commit messages are generally encouraged; they + should be omitted for security bugs and instead be posted in the bug + (which will eventually become public.) +#. Separate out tests into a separate commit. + **Do not land tests when landing the patch. Remember we don’t want + to 0-day ourselves!** This includes when pushing to try. + + - Tests should only be checked in later, after an official Firefox + release that contains the fix has been live for at least + four weeks. For example, if Firefox 53 + contains a security issue that affects the world and that issue is + fixed in 54, tests for this fix should not be checked in + until four weeks after 54 goes live. + + The exception to this is if there is a security issue that doesn't + affect any release branches, only mozilla-central and/or other + development branches. Since the security problem was never + released to the world, once the bug is fixed in all affected + places, tests can be checked in to the various branches. + - There are two main techniques for remembering to check in the + tests later: + + a. clone the sec bug into a separate "task" bug **that is also + in a security-sensitive group to ensure it's not publicly visible** + called something like "land tests for bug xxxxx" and assign to + yourself. It should get a "sec-other" keyword rating. + + Tip: In phabricator, you can change the bug linked to + a commit with tests if the tests were already separate, while keeping + the previously granted review, meaning you can just land the patch + when ready, rather than having your reviewer and you have to remember + what this was about a month or two down the line. + b. Or, set the "in-testsuite" flag to "?", and later set it to "+" + when the tests get checked in. + + +Landing tests +~~~~~~~~~~~~~ + +Tests can be landed **once the release containing fixes has been live +at least 4 weeks**. + +The exception is if a security issue has never been shipped in a release +build and has been fixed in all development branches. + +Making a security bug public +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This is the responsibility of the security management team. + +Essentials +---------- + +- **Do not disclose any information about the vulnerability before a + release with a fix has gone live for enough time for users to update + their software**. + + - This includes code comments, commit messages, tests, public + communication channels. + +- If any doubt: '''request sec-approval? ''' +- If any doubt: **needinfo security folks**. +- **If there’s no rating, assume the worst and treat the bug as + sec-critical**. + +Documentation & Contacts +------------------------ + +- :ref:`Normal process for submitting a patch <How to submit a patch>` +- `How to file a security bug <https://wiki.mozilla.org/Security/Fileabug>`__ +- `Handling Mozilla security bugs (policy) <https://www.mozilla.org/en-US/about/governance/policies/security-group/bugs/>`__ +- :ref:`Security Bug Approval Process` +- `Contacting the Security team(s) at Mozilla: <https://wiki.mozilla.org/Security>`__ diff --git a/docs/bug-mgmt/processes/labels.rst b/docs/bug-mgmt/processes/labels.rst new file mode 100644 index 0000000000..60c1bbeb2b --- /dev/null +++ b/docs/bug-mgmt/processes/labels.rst @@ -0,0 +1,155 @@ +GitHub Metadata Recommendations +=============================== + +To have better consistency with code and task tracking among Mozilla +Central, Bugzilla, and GitHub, we request that you use a common set of +labels in your projects. Benefits of improved consistency in our +conventions include: + +- Consistency makes measurement of processes simpler across the + organization +- Consistency makes it easier to write reusable process tools +- Consistency increases clarity for those than need to work across + different repositories and bug trackers +- Consistency reduces friction around engineering mobility between + projects + +We recommend creating sets of labels in your project to do this. + +Bug types +--------- + +In Bugzilla bugs are distinguished by type: ``defect``, ``enhancement``, +and ``tasks``. Use a label to make this distinction in your project. + +Statuses +-------- + +Bugs in GitHub issues have two states: closed and open. Bugzilla has a +richer set of states. + +When you close a bug, add a label indicating `the +resolution <https://wiki.mozilla.org/BMO/UserGuide/BugStatuses#Resolutions>`__. + +- ``fixed`` + + - A change set for the bug has been landed in Mozilla-Central + - A GitHub issue could be closed, but the change set has not + landed so it would be still considered open from the + Bugzilla point of view + +- ``invalid`` + + - The problem described is not a bug. + +- ``incomplete`` + + - The problem is vaguely described with no steps to reproduce, or is + a support request. + +- ``wontfix`` + + - The problem described is a bug which will never be fixed. + +- ``duplicate`` + + - The problem is a duplicate of an existing bug. Be sure to link the + bug this is a duplicate of. + +- ``worksforme`` + + - All attempts at reproducing this bug were futile, and reading the + code produces no clues as to why the described behavior would + occur. + +Severities (Required) +--------------------- + +The triage process for Firefox bugs in Bugzilla requires a non default +value of a bug's :ref:`Severity (definitions) <Defect Severity>`. + +Release Status Flags +-------------------- + +Open Firefox bugs may also have :ref:`status flags <Release Status Flags>` +(``status_firefoxNN``) set for Nightly, Beta, Release, or ESR. + +Priorities +---------- + +Firefox projects in Bugzilla can use the :ref:`priority field <Priority Definitions>` +to indicate when a bug will be worked on. + +Keywords +-------- + +In GitHub issues metadata is either a label or the bug’s open/closed +state. + +Some Bugzilla metadata behaves like labels, but you need to be careful +with how you use it in order not to confuse QA. + +Regressions +~~~~~~~~~~~ + +In Bugzilla, the ``regression`` keyword indicates a regression in +existing behavior introduced by a code change. + +When a bug is labeled as a regression in GitHub does it imply the +regression is in the code module in GitHub, or the module that’s landed +in Mozilla Central? Using the label ``regression-internal`` will signal +QA that the regression is internal to your development cycle, and not +one introduced into the Mozilla Central tree. + +If it is not clear which pull request caused the regression, add the +``regressionwindow-wanted`` label. + +Other Keywords +~~~~~~~~~~~~~~ + +Other useful labels include ``enhancement`` to distinguish feature +requests, and ``good first issue`` to signal to contributors (`along +with adequate +documentation <http://blog.humphd.org/why-good-first-bugs-often-arent/>`__.) + +Summary +------- + +To represent Bugzilla fields, use labels following this scheme. + +- Bug types + + - ``defect``, ``enhancement``, ``task`` + +- Resolution statuses + + - ``invalid``, ``duplicate``, ``incomplete``, ``worksforme``, + ``wontfix`` + +- Regressions + + - ``regression``, ``regressionwindow-wanted``, + ``regression-internal`` + + +- :ref:`Severity <Defect Severity>` (required) + + - ``S1``, ``S2``, ``S3``, ``S4``, ``N/A`` (reserved for bugs + of type ``task`` or ``enhancement``) + +- :ref:`Status flags <Release Status Flags>` + + - ``status_firefoxNN:<status>`` + (example ``status_firefox77:affected``) + +- :ref:`Priority <Priority Definitions>` + + - ``P1``, ``P2``, ``P3``, ``P5`` + +- Other keywords + + - ``good first bug``, ``perf``, &etc. + + +You may already have a set of tags, so do an edit to convert them +or use `the GitHub settings app <https://github.com/probot/settings>`__. diff --git a/docs/bug-mgmt/processes/regressions.rst b/docs/bug-mgmt/processes/regressions.rst new file mode 100644 index 0000000000..e603902623 --- /dev/null +++ b/docs/bug-mgmt/processes/regressions.rst @@ -0,0 +1,64 @@ +How to Mark Regressions +======================= + +Regressions +----------- + +For regression bugs in Mozilla-Central, our policy is to tag the bug as +a regression, identify the commits which caused the regression, then +mark the bugs associated with those commits as causing the regression. + +What is a regression? +--------------------- + +A regression is a bug (in our scheme a ``defect``) introduced by a +`changeset <https://en.wikipedia.org/wiki/Changeset>`__. + +- Bug 101 *fixes* Bug 100 with Change Set A +- Bug 102 *reported which describes previously correct behavior now not + happening* +- Bug 102 *investigated and found to be introduced by Change Set A* + +Marking a Regression Bug +------------------------ + +These things are true about regressions: + +- **Bug Type** is ``defect`` +- **Keywords** include ``regression`` +- **Status_FirefoxNN** is ``affected`` for each version (in current + nightly, beta, and release) of Firefox in which the bug was found +- The bug’s description covers previously working behavior which is no + longer working + +Until the change set which caused the regression has been found through +`mozregression <https://mozilla.github.io/mozregression/>`__ or another +bisection tool, the bug should also have the ``regressionwindow-wanted`` +keyword. + +Once the change set which caused the regression has been identified, +remove the ``regressionwindow-wanted`` keyword and set the **Regressed +By** field to the id of the bug associated with the change set. + +Setting the **Regressed By** field will update the **Regresses** field +in the other bug. + +Set a needinfo for the author of the regressing patch asking them to fix +or revert the regression. + +Previous Method +--------------- + +Previously we over-loaded the **Blocks** and **Blocked By** fields to +track the regression, setting **Blocks** to the id of the bug associated +with the change set causing the regression, and using the +``regression``, ``regressionwindow-wanted`` keywords and the status +flags as described above. + +This made it difficult to understand what was a dependency and what was +a regression when looking at dependency trees in Bugzilla. + +FAQs +---- + +*To be written* diff --git a/docs/bug-mgmt/processes/security-approval.rst b/docs/bug-mgmt/processes/security-approval.rst new file mode 100644 index 0000000000..a4c29730e7 --- /dev/null +++ b/docs/bug-mgmt/processes/security-approval.rst @@ -0,0 +1,192 @@ +Security Bug Approval Process +============================= + +How to fix a core-security bug in Firefox - developer guidelines +---------------------------------------------------------------- + +Follow these security guidelines if you’re involved in reviewing, +testing and landing a security patch: +:ref:`Fixing Security Bugs`. + +Purpose: don't 0-day ourselves +------------------------------ + +People watch our check-ins. They may be able to start exploiting our +users before we can get an update out to them if + +- the patch is an obvious security fix (bounds check, kungFuDeathGrip, + etc.) +- the check-in comment says "security fix" or includes trigger words + like "exploitable", "vulnerable", "overflow", "injection", "use after + free", etc. +- comments in the code mention those types of things or how someone + could abuse the bug +- the check-in contains testcases that show exactly how to trigger the + vulnerability + +Principle: assume the worst +--------------------------- + +- If there's no rating we assume it could be critical +- If we don't know the regression range we assume it needs porting to + all supported branches + +Process for Security Bugs (Developer Perspective) +------------------------------------------------- + +Filing / Managing Bugs +~~~~~~~~~~~~~~~~~~~~~~ + +- Try whenever possible to file security bugs marked as such when + filing, instead of filing them as open bugs and then closing later. + This is not always possible, but attention to this, especially when + filing from crash-stats, is helpful. +- It is _ok_ to link security bugs to non-security bugs with Blocks, + Depends, Regressions, or See Also. Users with the editbugs permission + will be able to see the reference, but not view a restricted bug or + its summary. Users without the permission will not be able to see the link. + For critical severity bugs where even that seems problematic, consider + mentioning the bug in a comment on the security bug instead. We can always + fill in the links later after the fix has shipped. + +Developing the Patch +~~~~~~~~~~~~~~~~~~~~ + +- Comments in the code should not mention a security issue is being + fixed. Don’t paint a picture or an arrow pointing to security issues + any more than the code changes already do. +- Do not push to Try servers if possible: this exposes the security + issues for these critical and high rated bugs to public viewing. In + an ideal case, testing of patches is done locally before final + check-in to mozilla-central. +- If pushing to Try servers is necessary, **do not include the bug + number in the patch**. Ideally, do not include tests in the push as + the tests can illustrate the exact nature of the security problem + frequently. +- If you must push to Try servers, with or without tests, try to + obfuscate what this patch is for. Try to push it with other, + non-security work, in the same area. + +Request review of the patch in the same process as normal. After the +patch has been reviewed you will request sec-approval as needed. See +:ref:`Fixing Security Bugs` +for more examples/details of these points. + +Preparing the patch for landing +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See :ref:`Fixing Security Bugs` +for more details. + +On Requesting sec-approval +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +For security bugs with no sec- severity rating assume the worst and +follow the rules for sec-critical. During the sec-approval process we +will notice it has not been rated and rate it during the process. + +Core-security bug fixes can be landed by a developer without any +explicit approval if: + +| **A)** The bug has a sec-low, sec-moderate, sec-other, or sec-want + rating. +| **or** +| **B)** The bug is a recent regression on mozilla-central. This means + +- A specific regressing check-in has been identified +- The developer can (**and has**) marked the status flags for ESR and + Beta as "unaffected" +- We have not shipped this vulnerability in anything other than a + nightly build + +If it meets the above criteria, developers do not need to ask for sec-approval. + +In all other cases, developers should ask for sec-approval. +Set the sec-approval flag to '?' on the patch when it is ready to be landed. +You will find these flags in Bugzilla using the "Details" links in the +Bugzilla attachment table (not directly on phabricator at time of writing). + +If developers are unsure about a bug and it has a patch ready, just +request sec-approval anyway and move on. Don't overthink it! + +An automatic nomination comment will be added to bugzilla when +sec-approval is set to '?'. The questions in this need to be filled out +as best as possible when sec-approval is requested for the patch. + +It is as follows (courtesy of Dan Veditz):: + + [Security approval request comment] + How easily can the security issue be deduced from the patch? + Do comments in the patch, the check-in comment, or tests included in + the patch paint a bulls-eye on the security problem? + Which older supported branches are affected by this flaw? + If not all supported branches, which bug introduced the flaw? + Do you have backports for the affected branches? If not, how + different, hard to create, and risky will they be? + How likely is this patch to cause regressions; how much testing does + it need? + +This is similar to the ESR approval nomination form and is meant to help +us evaluate the risks around approving the patch for checkin. + +When the bug is approved for landing, the sec-approval flag will be set +to '+' with a comment from the approver to land the patch. At that +point, land it according to instructions provided.. + +This will allow us to control when we can land security bugs without +exposing them too early and to make sure they get landed on the various +branches. + +If you have any questions or are unsure about anything in this document +contact us on Slack in the #security channel or the current +sec-approvers Dan Veditz and Tom Ritter. + +Process for Security Bugs (sec-approver Perspective) +---------------------------------------------------- + +The security assurance team and release management will have their own +process for approving bugs: + +#. The Security assurance team goes through sec-approval ? bugs daily + and approves low risk fixes for central (if early in cycle). + Developers can also ping the Security Assurance Team (specifically + Tom Ritter & Dan Veditz) in #security on Slack when important. + + #. If a bug lacks a security-rating one should be assigned - possibly + in coordination with the (other member of) the Security Assurance + Team + +#. Security team marks tracking flags to ? for all affected versions + when approved for central. (This allows release management to decide + whether to uplift to branches just like always.) +#. Weekly security/release management triage meeting goes through + sec-approval + and ? bugs where beta and ESR is affected, ? bugs with + higher risk (sec-high and sec-critical), or ? bugs near end of cycle. + +Options for sec-approval including a logical combination of the +following: + +- Separate out the test and comments in the code into a followup commit + we will commit later. +- Remove the commit message and place it in the bug or comments in a + followup commit. +- Please land it bundled in with another commit +- Land today +- Land today, land the tests after +- Land closer to the release date +- Land in Nightly to assess stability +- Land today and request uplift to all branches +- Request uplift to all branches and we'll land as close to shipping as + permitted +- Chemspill time + +The decision process for which of these to choose is perceived risk on +multiple axes: + +- ease of exploitation +- reverse engineering risk +- stability risk + +The most common choice is: not much stability risk, not an immediate +reverse engineering risk, moderate to high difficulty of exploitation: +"land whenever". diff --git a/docs/bug-mgmt/processes/shared-bug-queues.rst b/docs/bug-mgmt/processes/shared-bug-queues.rst new file mode 100644 index 0000000000..dc2df9bbf9 --- /dev/null +++ b/docs/bug-mgmt/processes/shared-bug-queues.rst @@ -0,0 +1,34 @@ +Shared Bug Queues +================= + +Reviewers for change sets can be suggested at the product and component +level, but only the person who has been asked to review code will be +notified. + +Realizing that Bugzilla users can *watch* other users, `Chris +Cooper <https://mozillians.org/en-US/u/coop/>`__ came up with the idea +of having `a shared reviews alias for review +requests <http://coopcoopbware.tumblr.com/post/170952242320/experiments-in-productivity-the-shared-bug-queue>`__. + +If you want to watch a particular part of the tree in Mozilla Central, +then `use the Herald +tool <https://phabricator.services.mozilla.com/book/phabricator/article/herald/>`__. + +Process +------- + +1. Create a new bugzilla.mozilla.com account for an address which can + receive mail. + Use the ``name+extension@domain.tld`` trick such as + ``jmozillian+reviews@mozilla.com`` to create a unique address +2. Respond to the email sent by Bugzilla and set a password on the + account +3. `Open a bug <https://mzl.la/2Mg8Sli>`__ to convert the account to a + bot and make it the shared review queue for your component +4. BMO administrator updates the email address of the new account to the + ``@mozilla.bugs`` address +5. BMO administrator updates the default reviewer for the component + requested and sets it to the shared review account +6. Reviewers `follow the shared review account in + bugzilla <https://bugzilla.mozilla.org/userprefs.cgi?tab=email>`__ +7. Reviewers get notified when shared review account is ``r?``\ ed |