Repository navigation
Conversation
- Whitespace-only keys (>=32 bytes included) rejected by the verifier, the durable verifier, and sign_acceptance. - A configured-but-invalid key (explicit or PERMIT_ACCEPTANCE_KEY: empty, under 32 bytes, whitespace) raises even with the demo flag; the demo fallback now applies only when no key is configured at all. - Demo key is an explicit --demo-key flag, off by default, on every entry point: server.py (mock incl. --db), demo.py, demos/*, python -m permit, trace.py. Without a valid key or the flag each exits non-zero naming the fix. CI/forge demo smokes opt in explicitly. - server.py --db --demo-key persists the generated key in a 0600 <db>.demo-key sidecar so it cannot rotate silently across restarts. - Stale ledger.py docstring fixed; sign_acceptance API change and the new key rules documented in README. - tests/test_issue53_acceptance_key.py: 29 tests covering bars 1-4 for every entry point; all 29 fail on 9809cc7, all pass here. Full suite: 428 passed, 1 skipped, plain and under -O.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 76317cf45a
ℹ️ 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".
| f"acceptance key from {source} is missing, empty, or under " | ||
| f"{MIN_ACCEPTANCE_KEY_BYTES} bytes (fail closed, issue #35/#53)" | ||
| ) | ||
| if not resolved.strip(): |
There was a problem hiding this comment.
When an explicit string key or PERMIT_ACCEPTANCE_KEY consists of Unicode whitespace such as 32 non-breaking spaces (U+00A0), _coerce_key first UTF-8-encodes it and this bytes.strip() check no longer recognizes the whitespace, so the trivially guessable key is accepted and can forge Type A acceptance signatures. Validate string keys with Unicode-aware str.strip() before encoding, while retaining the byte-level check for byte inputs.
Useful? React with 👍 / 👎.
| if _os.path.exists(p): | ||
| _os.chmod(p, 0o600) | ||
| raw = open(p, encoding="ascii").read().strip() |
There was a problem hiding this comment.
When two server.py --db <same-path> --demo-key processes start concurrently, the creator makes the sidecar visible before writing its contents, so the other process can enter this existing-file branch, read an empty/partial key, and exit with ValueError; alternatively, both can observe the file as absent and the O_EXCL loser exits with an uncaught FileExistsError. This breaks the documented second-writer durable setup, so creation should atomically publish complete contents and concurrent losers should retry loading the winning file.
Useful? React with 👍 / 👎.
| if args.db and "PERMIT_ACCEPTANCE_KEY" not in os.environ: | ||
| from settlement.verifier import load_or_create_demo_key_file | ||
| try: | ||
| key = load_or_create_demo_key_file(args.db + ".demo-key") |
There was a problem hiding this comment.
When a developer runs server.py --db ledger.db --demo-key from the checkout, this creates ledger.db.demo-key containing the complete acceptance-signing secret, but no *.demo-key pattern exists in .gitignore. The untracked file can therefore be accidentally added and committed, after which anyone with repository access can forge Type A acceptance signatures for that durable instance; add the generated sidecar suffix to the repository ignore rules.
Useful? React with 👍 / 👎.
1 participant
Closes the two bars REA's after-the-fact check of 9809cc7 (#44) failed, per the pass bar written in #53.
What changed
_validate_acceptance_keyrejects empty, under-32-byte, and whitespace-only keys (32+ bytes included) forReleaseVerifier,DurableReleaseVerifier, andsign_acceptance.PERMIT_ACCEPTANCE_KEYthat is invalid raises even with the demo flag on. The demo fallback now applies only when NO key is configured at all.--demo-key, off by default, onserver.py(mock incl.--db),demo.py,demos/*,python -m permit, andtrace.py. Without a valid key or the flag, each exits non-zero naming the fix. CI and forge demo smokes opt in explicitly (demo.py --demo-key,permit demo --demo-key).--dbpersistence (bar 4):server.py --db --demo-keypersists the generated key in a 0600<db>.demo-keysidecar (load_or_create_demo_key_file) so it cannot rotate silently across restarts. Documented in README.ledger.pydocstring fixed; README notes thesign_acceptanceAPI change (key argument required, verifier: remove DEMO_ACCEPTANCE_KEY fallback (fail closed on missing key) #35) and the new key rules. The README license badge already reads Apache-2.0 (docs: relicense under Apache-2.0 and add NOTICE #51), so no badge change was needed.Verification (bar 6)
tests/test_issue53_acceptance_key.py: 29 tests. All 29 fail on 9809cc7 and pass on this branch (verified in a pristine 9809cc7 worktree).python -O.python demo.py --demo-key(ledger VERIFIED),python trace.py --demo-key(TRACE COMPLETE), six-beat--replayfixture path with--demo-key(ledger VERIFIED, 26 receipts).Merge gate
Per #53, merging needs Marcus Richards's own yes plus an independent REA check. This PR is intentionally left unmerged.