Preparing to work your ticket and create a pull request

1. Collect the requirements that pertain to your change. Are they formal or informal? Do you have security requirements? (protip: you do have security requirements)

2. Identify the appropriate stakeholders and SMEs before starting development. Look at who created the Jira issue, who has commented on it and who is watching it. These are likely the stakeholders. As you familiarize yourself with the task, see what part of the code base you will be touching, and identify who originally wrote the code, who frequently changes it, and who touched it the most. Also, if applicable, identify the component team lead. These players are likely your Subject Matter Experts (SMEs).

3. Involve the relevant players early in design. Avoid surprising SMEs and stakeholders when you submit your code. Make sure you are doing what the stakeholders want, and the make sure the SMEs buy into your implementation. Whenever relevant, have whiteboard sessions with architects. Make mockups of UIs (myBalsamiq) before development. Having buy-in early avoids major rewrites when going into a code review.
4. Analyze the security and privacy risks associated with your requirements.
5. Generate a threat model and analyze the attack surface. This step is easiest to do in a group. Consider using something like Google Groups to share things since we have a distributed project team. This is a great place to start involving the SMEs that you noted in step 2.
6. Make sure the tasks are appropriately broken down into user stories. Ensuring a successful and low-pain peer review starts in grooming sessions. In some cases, it could be worthwhile to recommend breaking a ticket up to extract the design from the implementation. For example, for a UI ticket, the mock-up be its own ticket and would get its own peer review.
7. Capture Artifacts and post them to your Jira Issue. Create an audit trail for any design decisions. Photos of whiteboards, a summary of in-person discussions, and links to relevant research, mock-ups, etc, should be added to the Jira Issue. Having this available is a resource to explain why you did what you did, and helps hold people accountable for how they influenced your decision. This makes it a lot easier to go back and understand the reasoning behind how things were done when reviewing.
8. Expose your development and get early feedback. In cases where you desire direction while in development, commit your work-in-progress and push it to your remote fork. Add the link that commit to your Jira ticket and get your target audience to check it out by tagging them or contacting them directly. They should be able to add comments to your commit within your fork, effectively treating it like a “pre-review” (view).
9. Verify that your code follows the security best practices:
   1. Are you validating all of your input?
   2. Are you adhering to the principle of least privilege?
   3. Are you sanitizing any data sent to other systems or sub-systems?
   4. Are any security policies that you're using set to deny by default? (whitelist as opposed to blacklist)
   5. Are you the only person that will be able to maintain this code? If so, consider refactoring or simplifying things.
10. Glance at git history and include people who made the last commit in areas you are now changing. This allows you to get more context and history as to why the existing implementation was done a particular way. It also gives the previous author a chance to point out any concerns.
11. Develop in the correct environment. Set up your development environment with the recommended for the current project. For example, develop against the recommended java version. Also, use the correct formatter. Often committed code has many changes that are non-functional, and show up because different developers are using different formatters. All development should be done using the formatter specified for that project. Ideally, the file already conforms to that format. Reviewing code with formatting changes mixed in amongst functional changes is a challenge, as it takes additional time to identify what modifications are relevant.
12. Verify that your threat models are correct and that you correctly identified the attack surface associated with the change. Try to tell a story using your implementation and verify that the model is correct.
13. If this is a new web application or endpoint, fuzz testing should be performed to verify that there are no new vulnerabilities. Consider using testing frameworks such as: https://github.com/zaproxy/zaproxy/wiki/Downloads or http://w3af.org/download

Code Review Guidelines

Set up the Code Review appropriately. It is up to the submitter to ensure the peer review is being seen by enough of the right people. Engage the right people during development and have a diligent self-review. When submitting the PR, ensure you have provided your commit correctly, and described the peer review appropriately, including tagging the ideal (most relevant, available, etc) stakeholders/SMEs. Identify an effective and complete way to hero what you contributed, something that exercises the code, proves the capability and looks for regressions.

During the review, the submitter should follow these guidelines:

• It is up to the submitter to ensure the peer review is being addressed in a timely fashion.
• Address comments quickly, whether they result in a change to the code or not. Get a synchronous conversation with all parties when a review results in dissenting comments from multiple reviewers.
• Ping reviewers who are slow to review.
•  Ensure the hero builder was successful, and the hero builder provides a good summary of the actions taken and results seen in the hero build process as a comment on your PR.
• Make sure a committer is aware that you believe the PR is successful and complete.

The Reviewer should adhere to these guidelines:

•  Be professional. Comments in reviews can be seen by externals, including customers. Write comments in a way that you would be comfortable with anyone reading it.
•  Reviews should go from best practices/naming conventions/copyright headers all the way to confirming/validating the architectural design matches expectations.
•  Leverage the Reviewer's Cheat Sheet (Reviewer's Cheat Sheet) for technical concepts to consider when doing a review. The developer should have these same concepts in mind when writing the code. Enforcing these concepts leads to better code and more educated developers. Note also, this cheat sheet is a wiki and should be updated by the community as appropriate.
• Asking for clarification in a comment is not a bad thing, confirming “Are we sure this will be never null?” or “Is this thread safe?” is acceptable.
•  Be clear about what comments you feel should require a change or explanation, and which others are observations or recommendations that don’t require action. Leverage git’s capability to specify the scope of your overall review, whether your comments require a change or at least require the committer to convince you, or if you’d be ok with the committer coming back and saying “because this” or “not changing that”.
•  Be timely reviewing code. Ideally, reviewers work together, consider others comments, and make the larger concerns within a code review a discussion that can be addressed in a near real time.
•  Carve out a regular portion of your day which can be dedicated to code reviews. Code reviews should not be something that are reserved for the end of the sprint or right before a release.