Skip to content

Add fixed trusted weekly workflow identity foundation - #32

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/pert-weekly-trusted-workflow-identity-pr-a0
Closed

Add fixed trusted weekly workflow identity foundation#32
Pigbibi wants to merge 2 commits into
mainfrom
codex/pert-weekly-trusted-workflow-identity-pr-a0

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Scope

Pure PR-A0 foundation for the trusted weekly period-lock harness.

  • fixed code-reviewed repository/path/ref identity;
  • runtime reviewed workflow SHA is separate strict 40-hex evidence;
  • zero-override public identity API and strict canonical wire parser/serializer;
  • sanitized fail-closed errors for wrong identity, duplicate/noncanonical/unknown/missing wire data and malformed SHA;
  • characterization/adversarial tests and contract documentation.

Explicit non-scope

No period-bundle integration, workflow, permissions, Actions/artifact access,
producer, QAR, Pages, publisher, legacy, migration, or rerun behavior.
PR #31 remains closed/frozen evidence and is not reused.

Verification

  • focused identity pytest: 23 passed
  • full python3 -m pytest -q: 110 passed
  • uv sync --locked --extra test: passed
  • locked uv run --locked pytest -q: 110 passed
  • compileall: passed
  • diff-check: passed
  • ruff: unavailable

A1 bundle integration is blocked until this PR is merged and post-merge main CI succeeds.

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🤖 Codex PR Review

🚫 Merge blocked: active blocking history requires independent source-of-truth clearance

⚖️ Codex Review Arbitration

🚫 block: The prior security finding is not demonstrably fixed. TrustedWorkflowIdentity in src/political_event_tracking_research/trusted_workflow_identity.py is a @dataclass(frozen=True, slots=True), which blocks normal assignment but does not seal the object against object.__setattr__ on existing slot fields. TrustedWorkflowEvidence.__post_init__ only checks identity.repository/workflow_path/workflow_ref at construction time; after validate_trusted_workflow_identity() returns a valid TrustedWorkflowEvidence, a caller can still mutate evidence.identity in place and later code that reads evidence.identity.repository / workflow_path / workflow_ref would observe attacker-controlled values. The new serializer revalidates before emitting bytes, but the documented contract in docs/trusted_workflow_identity_contract.md says callers cannot provide another trust root, and the type itself still does not enforce that invariant for post-construction consumers. No current finding conflicts with the prior one because the current blocking list is empty.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Reliability in src/political_event_tracking_research/trusted_workflow_identity.py

serialize_trusted_workflow_evidence() assumes any exact-typed TrustedWorkflowEvidence instance has a valid identity object with the expected attributes. In Python this invariant can be bypassed with object.__new__/object.__setattr__, and the function will then raise an uncaught AttributeError when it reads evidence.identity.repository instead of returning a sanitized TrustedWorkflowIdentityError. That leaves a crash path in the public API despite the module contract promising fail-closed identity errors. (line 104)

Suggestion: Revalidate evidence.identity before dereferencing it, mirroring TrustedWorkflowEvidence.__post_init__: first check type(evidence.identity) is TrustedWorkflowIdentity, then compare the fixed fields, and convert missing/invalid attributes into trusted_workflow_identity_mismatch rather than letting AttributeError escape.


Review by Codex PR Review bot • PR

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 827598fde6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

class TrustedWorkflowIdentity:
"""Immutable fixed identity; construction is intentionally not public."""

__slots__ = ("_repository", "_workflow_path", "_workflow_ref")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Make the fixed identity object immutable

When any caller receives the singleton from trusted_workflow_identity(), the slots declared here are still writable (for example, identity._workflow_ref = "..."), and because the same singleton is returned for all callers, later code deriving repository/path/ref from that typed identity can see a caller-supplied trust root despite the zero-override contract. Add a rejecting __setattr__ after _create or otherwise freeze the singleton before this becomes the trust boundary.

Useful? React with 👍 / 👎.

Comment on lines +89 to +90
@dataclass(frozen=True, slots=True)
class TrustedWorkflowEvidence:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Hide the evidence constructor behind validation

If the later integration has a SHA from a miswired or untrusted workflow, it can still call TrustedWorkflowEvidence(trusted_workflow_identity(), sha) and serialize a canonical trusted wire object without ever comparing the runtime github.workflow_ref. Because this public dataclass generates an accepting constructor, it bypasses the only API that validates the workflow ref; keep construction internal or gate it with a private factory token so callers must use validate_trusted_workflow_identity().

Useful? React with 👍 / 👎.

Co-Authored-By: Codex <noreply@openai.com>
@Pigbibi

Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Concentrated closure applied at a1afd72571beaeb91b303cffd05d3b3e23d93e30.

  • Replaced mutable singleton trust with frozen fixed-value TrustedWorkflowIdentity; each call returns an independently immutable value and all boundaries re-check fixed repository/path/ref values.
  • TrustedWorkflowEvidence validates identity and reviewed SHA in its dataclass invariant, including direct construction and dataclasses.replace; forged identity values fail closed.
  • Added singleton mutation, direct constructor, replace, and forged identity/SHA regressions.

Pure PR-A0 scope remains unchanged: no bundle, workflow, permissions, Actions, artifact, producer, QAR, Pages, legacy, or migration integration.

@Pigbibi

Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

PERMANENT_FREEZE/HUMAN_REQUIRED per stop rule. Preserve branch/worktree/commits/comments/threads/checks/tests; no further code action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant