Provide inline documentation/explanations for common false positive security reports

[legoktm]

To attempt to head off some common security bug reports we get that aren't actually security issues or are already known.

Some commits have a bit more detail in the commit message.

Test plan

• rationales make sense

Checklist

This change accounts for:

• any required additional documentation
• any necessary AppArmor changes (added or removed application files)
• any impact on new SecureDrop installs and upgrades
• our dependency update policy

[Copilot]

🟡 Not ready to approve

Several added “security rationale” comments are misleading/overstated (and one contains a grammar error), so the documentation should be corrected to avoid institutionalizing inaccurate security guidance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR adds inline rationale comments across the SecureDrop codebase to preempt recurring false-positive security reports by documenting why certain patterns are intentional and what their security implications are.

Changes:

• Adds clarification comments around “lightweight” PGP header checks for pre-encrypted replies.
• Documents known session-cookie confidentiality limitations and links to the tracked issue.
• Adds security rationale comments around 2FA verification ordering, 2FA-token logging, and “unscoped” UUID lookups in the journalist API.

File summaries

File	Description
securedrop/store.py	Adds rationale that the PGP armor check is only an API-misuse guard, not full OpenPGP validation.
securedrop/source_app/session_manager.py	Documents that Flask session cookie contents are not encrypted and links to the existing tracking issue.
securedrop/models.py	Adds rationale for verifying 2FA before password to enforce single-use OTP behavior.
securedrop/journalist_app/main.py	Adds rationale for logging the 2FA token during login.
securedrop/journalist_app/api.py	Adds rationales explaining why certain UUID relationships aren’t validated and why that’s not considered IDOR in this app.

Review details

Suppressed comments (3)

securedrop/journalist_app/api.py:189

• Using "XXX" for this rationale reads like a temporary hack; elsewhere in the codebase similar security rationales use "NOTE". Also, the comment can be slightly more precise by stating that there is no object-level ACL among journalists.

        # XXX: even though we don't validate reply_uuid belongs to source_uuid, there's
        # no IDOR here because all journalists can see all sources/replies

securedrop/journalist_app/api.py:201

• The new comment uses "see+delete", which is harder to read than "view and delete", and "XXX" is inconsistent with existing security notes (typically "NOTE"). Consider rewording for clarity and to match the established style.

        # XXX: even though we don't validate submission_uuid belongs to source_uuid, there's
        # no IDOR here because all journalists can see+delete all sources/submissions

securedrop/journalist_app/api.py:260

• Using "XXX" for this rationale reads like a temporary hack; elsewhere in the codebase similar security rationales use "NOTE". Also, the comment can be slightly more precise by stating that there is no object-level ACL among journalists.

        # XXX: even though we don't validate reply_uuid belongs to source_uuid, there's
        # no IDOR here because all journalists can see all sources/replies

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.