Contributing Guidelines
Signing commits
Commits should be signed, as explained in the GitHub documentation. This helps verify commits proposed in a pull request are from the expected author.
Branching Strategy
Development for the upcoming release of SecureDrop takes place on develop
,
which is the default branch. If you want to contribute, you should branch
from and submit pull requests to develop
. If you want to install or audit
SecureDrop, you should use the latest tag that is not a release candidate (e.g.
0.6
not 0.6-rc1
).
Tip
After you have cloned the SecureDrop repository, you can run
git tag
locally to see all the tags. Alternatively, you can view them on
GitHub.
Automated Testing
When a pull request is submitted, we have Circle CI automatically run the SecureDrop test suites, which consist of:
Unit tests of the Python SecureDrop application code.
Functional tests that use Selenium to drive a web browser to verify the function of the application from the user’s perspective.
Tests of the system configuration state using testinfra.
Before a PR can be merged, these tests must all pass. If you modify the application code, you should verify the tests pass locally before submitting your PR. If you modify the server configuration, you should run the testinfra tests. Please denote in the checklist when you submit the PR that you have performed these checks locally.
Code Style
We use code linters to keep a consistent code quality and style. These linters also run in CI and will produce build failures. To avoid this, we have included a git pre-commit hook. You can install it with the following command run at the root of the repository:
ln -sf .githooks/pre-commit .git/hooks/pre-commit
Note
The code linters are installed automatically on the Development VM, but for the pre-commit hook to work, you will need to install the linting tools locally on your host machine. From the root of the repo you can run the following:
pip install --no-deps --require-hashes -r securedrop/requirements/python3/develop-requirements.txt
Python
All Python code should be flake8
compliant. You can run flake8
locally via:
make flake8
Shell
All Shell code (e.g. bash
, sh
) should be shellcheck
compliant. You can run shellcheck
locally via:
make shellcheck
For reference, consult the shellcheck wiki for detailed explanations of any reported violations.
HTML
HTML should be in compliance with
Google’s HTML style guide.
We use html-linter to lint
our HTML templates in securedrop/source_templates
and
securedrop/journalist_templates
. Run the HTML linting options we use via:
make html-lint
Accessibility
SecureDrop’s accessibility guidelines and tooling are a work in progress. At
a minimum, if you make changes involving images, make sure they have alt
attributes in accordance with the W3C’s “alt decision tree”, so that the
interfaces will be navigable by people using screen-readers. For more-involved
changes to the UIs, consult resources such as the A11y Project checklist.
If you have accessibility expertise to offer, the “a11y” label in GitHub is a great place to contribute.
YAML
The Ansible configuration is specified in YAML files, including variables,
tasks, and playbooks. All YAML files in the project should pass the
yamllint standards declared
in the .yamllint
file at the root of the repository.
Run the checks locally via:
make yamllint
Type Hints in Python code
By adding type hints/annotations in the Python code, we are making the codebase easier to maintain in the long run by explicitly specifying the expected input/output types of various functions.
Any pull request with Python code in SecureDrop should have corresponding type hints for all the functions. Type hints and function annotations are defined in PEP 484 and in PEP 3107. We also use the mypy tool in our CI to find bugs in our Python code.
If you are new to Python type hinting, please read the above mentioned PEP documents, and then go through the examples in the mypy documentation. Some type annotations are included as code comments due to SecureDrop being Python 2 only when they were added, but any annotation syntax supported in Python 3.5 is allowed (i.e. function but not variable annotations which were added in Python 3.6).
Example of Type Hint
import typing
# https://www.python.org/dev/peps/pep-0484/#runtime-or-type-checking
if typing.TYPE_CHECKING:
# flake8 can not understand type annotation yet.
# That is why all type annotation relative import
# statements has to be marked as noqa.
# https://flake8.pycqa.org/en/latest/user/error-codes.html?highlight=f401
from typing import Dict # noqa: F401
class Config(object):
def __init__(self):
# type: () -> None
self.NAMES = {} # type: Dict[str, str]
def add(self, a, b):
# type: (int, int) -> float
c = 10.5 # type: float
return a + b + c
def update(self, uid, Name):
# type: (int, str) -> None
"""
This method updates the name example.
"""
self.NAMES[uid] = Name
def main():
# type: () -> None
config = Config() # type: Config
config.add(2, 3)
config.update(223, "SD")
if __name__ == '__main__':
main()
The above example shows how to do a conditional import of Dict
class from
typing
module. typing.TYPE_CHECKING
will only be true when we use mypy
to check type annotations.
How to Use mypy?
make lint
already checks for any error using the mypy
tool. In case you want
to have a local installation, you can do that using your Python 3 virtualenv.
$ python3 -m venv ../.py3
$ source ../.py3/bin/activate
$ pip install mypy
$ mypy securedrop
Git History
We currently use an explicit merge strategy to merge feature branches into
develop
.
Note
It is generally good practice to maintain a clean git history by reducing the number of commits to a reasonable minimum. You can do this by squashing closely related commits through an interactive rebase once your PR is close to being merged.
If you would like a project maintainer to help you with squashing commits in a PR, please don’t hesitate to leave a comment requesting assistance.
Privileges
Note
The privilege escalation workflow is different for code maintainers and translation maintainers.
Dedicated contributors to SecureDrop will be granted extra privileges such as the right to push new branches or to merge pull requests. Any contributor with the right technical and social skills is entitled to ask. The people who have the power to grant such privileges are committed to do so in a transparent way as follows:
The contributor posts a message in the forum asking for privileges (review or merge, etc.).
After at least a week someone with permissions to grant such privilege reviews the thread and either:
grants the privilege if there are no objections from current maintainers and adds a message to the thread; or
explains what is expected from the contributor before they can be granted the privilege.
The thread is closed.
The privileges of a developer who has not been active for six months or more are revoked. They can apply again at any time.
Other Tips
To aid in review, please write clear commit messages and include a descriptive PR summary. We have a PR template that specifies the type of information you should include.
To maximize the chance that your PR is merged, please include the minimal changes to implement the feature or fix the bug.
If there is not an existing issue for the PR you are interested in submitting, you should submit an issue first or comment on an existing issue outlining how you intend to approach the problem.