Skip to content

Conversation

@cfm
Copy link
Member

@cfm cfm commented Nov 14, 2025

Closes #7682 with two out of its three goals:

  1. Cover more dataclasses with __post_init__() validation
  2. Mark input dataclasses as read-only via frozen
  3. Add some generics (despite /api/v2/data: handle batched events from clients #7672 (comment)) to explicitly mark untrusted values as tainted until sanitized

Overall I much prefer the declarative schema-based approach that the app has taken with Zod. Since we've decided to avoid further such dependencies on the server side, however, this has to be done separately and imperatively like this.

Test plan

  • The app works as expected.
  • The app's server tests pass without modification.

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

@cfm cfm self-assigned this Nov 14, 2025
@cfm cfm added this to SecureDrop Nov 14, 2025
@cfm cfm changed the base branch from develop to 7639-apiv2-minor-versioning November 14, 2025 01:49
@cfm cfm moved this to In Progress in SecureDrop Nov 14, 2025
@cfm cfm requested a review from Copilot November 14, 2025 01:50
Copilot finished reviewing on behalf of cfm November 14, 2025 01:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances type safety and validation in the API v2 implementation by making dataclasses frozen (immutable), adding comprehensive validation in __post_init__ methods, and updating tests to properly instantiate immutable Event objects instead of mutating existing ones.

  • Adds validation for version strings (must be 64 hex digits), UUIDs, and event IDs
  • Makes Target, EventData, Event, EventResult, and BatchRequest dataclasses frozen to prevent mutation
  • Updates test cases to create new Event instances instead of modifying existing ones

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
securedrop/journalist_app/api2/types.py Adds VERSION_LEN constant, implements __post_init__ validation for all dataclasses, makes dataclasses frozen, and enhances type annotations
securedrop/tests/test_journalist_api2.py Updates tests to create new Event instances rather than mutating existing ones, and uses VERSION_LEN constant for test data

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cfm cfm force-pushed the 7682-stricter-typing branch from 8f717d0 to ae351ff Compare November 14, 2025 01:54
@cfm cfm requested a review from Copilot November 14, 2025 01:55
Copilot finished reviewing on behalf of cfm November 14, 2025 01:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cfm cfm force-pushed the 7682-stricter-typing branch 2 times, most recently from 6533bc5 to b4f274e Compare November 14, 2025 02:30
@cfm cfm force-pushed the 7639-apiv2-minor-versioning branch from 88d5991 to e888fef Compare November 14, 2025 17:51
@cfm cfm force-pushed the 7682-stricter-typing branch from b4f274e to 3eb21db Compare November 14, 2025 21:05
@cfm cfm changed the title API v2: further tighten typing and validation feat(api2): validate and freeze dataclasses Nov 14, 2025
@cfm cfm marked this pull request as ready for review November 14, 2025 21:12
@cfm cfm requested a review from a team as a code owner November 14, 2025 21:12
@cfm cfm moved this from In Progress to Ready For Review in SecureDrop Nov 14, 2025
@cfm cfm removed their assignment Nov 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready For Review

Development

Successfully merging this pull request may close these issues.

API v2: further tighten typing and validation

2 participants