Summary:

Add base support for code safety checkers.

Review Request #11904 — Created Jan. 4, 2022 and submitted June 14, 2022, 4:52 p.m.

Information
Owner:	chipx86
Repository:	Review Board
Branch:	release-5.0.x
Bugs:
Depends On:
Blocks:	11907, 11906
Reviewers
Groups:	reviewboard
People:

Description

Review Board's focus historically has been to provide tools for
automatically or manually catching problems with code before it goes
into a product. The automatic checks come by way of extensions like
Review Bot, and these can only check code once it's been published.

There are issues that are best caught before code goes up for review,
and there are also issues that are best caught by default without
needing to install a tool like Review Bot.

Two examples would be credentials accidentally left in code and
so-called "Trojan Source" attacks (where code is either accidentally or
intentionally added to a file that displays one way to a user but
executes another way).

This begins laying the foundation for code safety checkers, which can
look for suspicious content in code before it's ready for review,
flagging issues that are found.

This will be used in the diff validation API and the diff viewer to
highlight any issues that are found. Specific code safety checkers will
be implemented in future changes.

Testing Done

Unit tests pass.

Tested this along with a code checker implementation and the upcoming
diff viewer updates.

Commits

Summary
	Add base support for code safety checkers.

Screenshots

Issues

Description	From	Last Updated
E501 line too long (94 > 79 characters)	reviewbot	Jan. 4, 2022, 7:45 p.m.
F401 'reviewboard.codesafety.checkers.trojan_source.TrojanSourceCodeSafetyChecker' imported but unused	reviewbot	Jan. 4, 2022, 7:45 p.m.
F811 redefinition of unused 'test_render_file_alert_html' from line 38	reviewbot	Jan. 4, 2022, 7:45 p.m.
E501 line too long (94 > 79 characters)	reviewbot	Jan. 4, 2022, 9:52 p.m.
Seems like "unicode" probably shouldn't be capitalized here?	david	May 23, 2022, 6:06 p.m.
And here.	david	May 23, 2022, 6:06 p.m.
Seems like an edit got messed up here.	david	May 23, 2022, 6:01 p.m.
E501 line too long (94 > 79 characters)	reviewbot	May 23, 2022, 6:08 p.m.
SafeString instead of SafeText?	maubin	June 8, 2022, 2:58 p.m.
SafeString instead of SafeText?	maubin	June 8, 2022, 2:58 p.m.
Just a thought--do we want to make this use an EntryPointRegistry instead?	david	June 1, 2022, 2:45 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/codesafety/checkers/base.py (Diff revision 1)
Show all issues
```
E501 line too long (94 > 79 characters)
```

reviewboard/codesafety/checkers/registry.py (Diff revision 1)

Show all issues

F401 'reviewboard.codesafety.checkers.trojan_source.TrojanSourceCodeSafetyChecker' imported but unused

reviewboard/codesafety/tests/test_base_code_safety_checker.py (Diff revision 1)
Show all issues
```
F811 redefinition of unused 'test_render_file_alert_html' from line 38
```

Change Summary:


Removed a duplicate unit test.
Removed an import that should have been in the next change in this series.

Commits:

	Summary
-		Add base support for code safety checkers.
+		Add base support for code safety checkers.

Diff:

Revision 2 (+750)

Show changes

	reviewboard/codesafety/__init__.py
	reviewboard/codesafety/checkers/__init__.py
	reviewboard/codesafety/checkers/base.py
	reviewboard/codesafety/checkers/registry.py
	reviewboard/codesafety/tests/__init__.py
	reviewboard/codesafety/tests/test_base_code_safety_checker.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/codesafety/checkers/base.py (Diff revision 2)
Show all issues
```
E501 line too long (94 > 79 characters)
```

reviewboard/codesafety/checkers/base.py (Diff revision 2)
Show all issues
```
Seems like "unicode" probably shouldn't be capitalized here?
```
reviewboard/codesafety/checkers/base.py (Diff revision 2)
Show all issues
```
And here.
```
reviewboard/codesafety/checkers/base.py (Diff revision 2)
Show all issues
```
Seems like an edit got messed up here.
```

Change Summary:

Updated for release-5.0.x:

Changed version numbers in docstrings
Changed docs to use Keys and str.
Removed six usage
Removed __future__ imports

Commits:

	Summary
-		Add base support for code safety checkers.
+		Add base support for code safety checkers.

Branch:

-	release-4.0.x
+	release-5.0.x

Diff:

Revision 3 (+730)

Show changes

	reviewboard/codesafety/__init__.py
	reviewboard/codesafety/checkers/__init__.py
	reviewboard/codesafety/checkers/base.py
	reviewboard/codesafety/checkers/registry.py
	reviewboard/codesafety/tests/__init__.py
	reviewboard/codesafety/tests/test_base_code_safety_checker.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/codesafety/checkers/base.py (Diff revision 3)
Show all issues
```
E501 line too long (94 > 79 characters)
```

Change Summary:

Update another docstring to use Keys.

Commits:

	Summary
-		Add base support for code safety checkers.
+		Add base support for code safety checkers.

Diff:

Revision 4 (+734)

Show changes

	reviewboard/codesafety/__init__.py
	reviewboard/codesafety/checkers/__init__.py
	reviewboard/codesafety/checkers/base.py
	reviewboard/codesafety/checkers/registry.py
	reviewboard/codesafety/tests/__init__.py
	reviewboard/codesafety/tests/test_base_code_safety_checker.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Changed ugettext_lazy to gettext_lazy.

Commits:

	Summary
-		Add base support for code safety checkers.
+		Add base support for code safety checkers.

Diff:

Revision 5 (+734)

Show changes

	reviewboard/codesafety/__init__.py
	reviewboard/codesafety/checkers/__init__.py
	reviewboard/codesafety/checkers/base.py
	reviewboard/codesafety/checkers/registry.py
	reviewboard/codesafety/tests/__init__.py
	reviewboard/codesafety/tests/test_base_code_safety_checker.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/codesafety/checkers/registry.py (Diff revision 5)

Show all issues

Just a thought--do we want to make this use an EntryPointRegistry instead?

chipx86 May 26, 2022, 10:43 a.m.

I think we should discourage entrypoints in favor of extensions where possible. Extensions give us better control all around and let us manage initialization order.

reviewboard/codesafety/checkers/base.py (Diff revision 5)
Show all issues
```
SafeString instead of SafeText?
```
reviewboard/codesafety/checkers/base.py (Diff revision 5)
Show all issues
```
SafeString instead of SafeText?
```

Change Summary:

Changed SafeText to SafeString in docs.

Commits:

	Summary
-		Add base support for code safety checkers.
+		Add base support for code safety checkers.

Diff:

Revision 6 (+734)

Show changes

	reviewboard/codesafety/__init__.py
	reviewboard/codesafety/checkers/__init__.py
	reviewboard/codesafety/checkers/base.py
	reviewboard/codesafety/checkers/registry.py
	reviewboard/codesafety/tests/__init__.py
	reviewboard/codesafety/tests/test_base_code_safety_checker.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (0547356)