feat: add bounded canonical identity validator - #84
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review✅ Merge allowed: No issues found Review by Codex PR Review bot • PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 650167b696
ℹ️ 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".
| raise IdentityError("invalid secret reference") | ||
| return {"kind": kind, "value": {"type": typ, "role": role, "position": position}} | ||
| item = _text(token["value"]) | ||
| if any(marker in item.lower() for marker in MARKERS): |
There was a problem hiding this comment.
When a legitimate identifier contains sk- as part of a normal word, such as task-handler or risk-score, this substring check rejects the payload as a reserved secret before the identifier regex runs. Since hyphenated identifiers are otherwise allowed, this can make valid contract anchors or predicates unusable whenever common names happen to contain one of these short prefixes; the marker check should require a credential-shaped token or at least a segment/start boundary rather than any substring occurrence.
Useful? React with 👍 / 👎.
| raise IdentityError("invalid object fields") | ||
| return value | ||
| def _text(value: Any, limit: int = 512) -> str: | ||
| if not isinstance(value, str) or len(value) > limit or len(value.encode()) > limit: |
There was a problem hiding this comment.
When this validator is fed JSON-decoded input containing an escaped unpaired surrogate, for example in scope.file or a token value, value.encode() raises UnicodeEncodeError before the control-character check can raise IdentityError. Callers that treat IdentityError as the closed validation failure will instead get an unexpected exception or 500 for malformed text, so the length check should reject surrogates before encoding or catch encoding errors and re-raise IdentityError.
Useful? React with 👍 / 👎.
| fields = {"schema", "canonicalizer_version", "scope", "anchors", "predicates", "required_behavior", "forbidden_behavior", "ordering_constraints", "contract_key", "behavior_digest", "fingerprint_v2"} | ||
| if not isinstance(record, dict) or set(record) != fields or any(not isinstance(record[name], str) or not DIGEST.fullmatch(record[name]) for name in fields - {"schema", "canonicalizer_version", "scope", "anchors", "predicates", "required_behavior", "forbidden_behavior", "ordering_constraints"}): | ||
| raise IdentityError("invalid verified record") | ||
| identity = validate_identity({name: record[name] for name in fields - {"contract_key", "behavior_digest", "fingerprint_v2"}}) |
There was a problem hiding this comment.
When a record contains fields that normalize to the canonical payload but are not themselves canonical, such as an uppercase scope.repo paired with the digest for the lowercase repo, verification still succeeds because this revalidates a normalized copy and only compares the digests. That means the verified-record boundary accepts multiple on-wire records for one fingerprint, so any caller that stores or compares the supplied record can treat bytes not actually covered by the fingerprint as verified; compare the record to identity.as_record() before returning.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fc8e7336f
ℹ️ 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".
| typ, role, position = _text(ref["type"], 32), _text(ref["role"], 32), ref["position"] | ||
| if not re.fullmatch(r"[a-z][a-z0-9_.-]{0,31}", typ) or not re.fullmatch(r"[a-z][a-z0-9_.-]{0,31}", role) or isinstance(position, bool) or not isinstance(position, int) or not 0 <= position <= 1024: | ||
| raise IdentityError("invalid secret reference") | ||
| return {"kind": kind, "value": {"type": typ, "role": role, "position": position}} |
There was a problem hiding this comment.
When a secret_ref is built from untrusted extraction output, the type or role fields can still carry a literal high-confidence token (for example role="sk-proj-1234567890abcdef" or a github_pat_... value) because this branch validates only the lowercase shape and returns before running SECRET_PATTERNS. That metadata is emitted unchanged by as_record() and covered by the digest, so the secret-safe reference path can still leak the secret; apply the same marker rejection to these fields before returning.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea19ccc849
ℹ️ 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".
| parts = repo.split("/") | ||
| if len(parts) != 2 or not OWNER.fullmatch(parts[0]) or "--" in parts[0] or not REPO.fullmatch(parts[1]) or parts[1] in {".", ".."}: | ||
| raise IdentityError("invalid repo") | ||
| path = _text(scope["file"], 1024) |
There was a problem hiding this comment.
When scope.file comes from untrusted extraction or repo metadata and contains a credential-shaped segment such as leaks/sk-proj-1234567890abcdef.py, this path is only normalized as text and then as_record() emits it unchanged, while the secret-marker guard is only applied to token values. That bypasses the validator's secret-safe boundary for verified records; apply the same SECRET_PATTERNS check to scope text before returning the canonical scope.
Useful? React with 👍 / 👎.
| re.compile(r"(?<![A-Za-z0-9_])(?:github_pat_|gh[pours]_)[A-Za-z0-9_]{8,}", re.I), | ||
| re.compile(r"(?<![A-Za-z0-9_])(?:AKIA|ASIA)[A-Z0-9]{16}(?![A-Za-z0-9_])"), | ||
| re.compile(r"(?<![A-Za-z0-9_])eyJ[A-Za-z0-9_-]+\.eyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+(?![A-Za-z0-9_])"), | ||
| re.compile(r"(?<![A-Za-z0-9_])sk-[A-Za-z0-9_-]{16,}(?![A-Za-z0-9_])"), |
There was a problem hiding this comment.
When an extracted identifier embeds a real token next to underscores, such as api_sk-proj-1234567890abcdef, the negative lookbehind/lookahead treats _ as part of the surrounding word so none of these patterns match, while the identifier grammar accepts underscores and the record is emitted unchanged. This leaves the token-value secret guard bypassable for common variable-name shapes; consider allowing _ to act as a segment boundary for credential-shaped substrings.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f0fc708ec
ℹ️ 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".
| IDENTIFIER = re.compile(r"^[A-Za-z_][A-Za-z0-9_.-]*(?:\(\))?$") | ||
| DIGEST = re.compile(r"^[0-9a-f]{64}$") | ||
| SECRET_PATTERNS = ( | ||
| re.compile(r"(?<![A-Za-z0-9])(?:github_pat_|gh[pours]_)[A-Za-z0-9_]{8,}", re.I), |
There was a problem hiding this comment.
Fresh evidence after the earlier boundary fix: this GitHub marker still requires only eight word characters after ghs_/github_pat_, so a valid identifier or file/repo segment such as ghs_database or github_pat_validator passes the identifier/path grammar but is rejected as a secret. Any contracts anchored on GitHub-token handling code will be unusable even though no credential-shaped token is present; require the real token length/structure (or another entropy check) before raising.
Useful? React with 👍 / 👎.
|
Superseded by merged PR #87. This PR remains unmerged; its implementation is not imported. |
1 participant
Summary
identifier (:: identifier)*anchorsScope
Fresh R1b replacement for frozen PR #83. No runtime, history, evidence, reviewer adapter, workflow, or R2 integration.
Validation
python3 -m unittest tests.test_canonical_typed_identity_r1b(5 passed)python3 -m unittest tests.test_run_codex_pr_review(54 passed)python3 -m unittest discover tests(625 passed, 1 skipped)python3 -m ruff check .python3 -m compileall -q service scripts testsactionlint .github/workflows/*.ymlgit diff --check origin/main...HEAD