Repository navigation
[3268 by Claude Opus 5.5] Terminate serialized RPM public-key files with a newline - #3269
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…n when the configured public key has none. The key text itself is unchanged, and only the files handed to the tools get the newline. All new tests fail on the original code and pass on the fix. The fix (`apps/desktop/scripts/rpm-signing.mjs`) - A new shared helper, `writeRpmPublicKeyFile(path, identity)`, checks the identity with `requireRpmIdentity` as before. It adds one LF only if the key doesn't already end with one, and writes the file with mode 0600. - Both places that wrote the key file now use it: - `withKey` (`release.asc`), which feeds `gpg --import` and `rpmkeys --import` in `verifyRpm`/`signRpm`. - The acceptance runner's `public.asc` that is mounted into the container as `/input/public.asc`. - Nothing else changed: the identity's `publicKey` string, the manifest comparison and all fingerprints are untouched. The fingerprint, exactly-one-key and RSA checks still run, as do signature parsing and temp-directory cleanup. The diagnostic stages are unchanged. Workflows, the container script, the Dockerfile, keys and secrets weren't edited. Tests - `rpm-signing.test.mjs`, without real tools: a key with no final newline, one with LF and one with CRLF all produce the right file bytes with mode 0600. The input string is never modified. Invalid identities (lowercase fingerprint, private key) are still rejected and no file is written. - `rpm-signing.test.mjs`, with real tools and disposable keys: this runs in the existing workflows' required-tools step, so no workflow change was needed. A key with its final newline stripped, and one with it intact, both go through `withKey`. Each written file is the exported key plus exactly one LF, with mode 0600. Each imports with `rpmkeys` into a fresh RPM database, giving exactly one key with the matching key ID, and the temporary directory is removed afterwards. A wrong fingerprint and two concatenated keys are still rejected. - `signed-rpm-acceptance-diagnostics.test.mjs`: the fake Docker now records the mounted `public.asc` bytes and mode. A new test runs the full acceptance flow with a key that lacks the final newline and one that has it. In both cases the mounted file ends with exactly one LF and has mode 0600, the identity is unchanged, evidence passes and the workspace is removed. The existing privacy and diagnostic tests still pass. Results: The new tests are the three named above, and all three failed when I swapped the original source back in. I then ran the seven related suites (`rpm-signing`, `linux-preview-release`, `linux-preview-workflow`, `signed-rpm-acceptance-evidence`, `signing-key-verification`, `signed-rpm-acceptance-diagnostics`, `signed-rpm-acceptance-context`) with `PROPR_REQUIRE_RPM_TESTS=1`. 68 passed and 0 failed. One test was skipped because it needs Linux root. The container has no gpg or rpm, so for the real-tool runs I extracted Debian 12's rpm 4.18 and GnuPG 2.2 into `/tmp` without root. This doesn't reproduce the openSUSE rejection itself. That still needs the planned operator check: the strict openSUSE key-trust comparison and the full protected acceptance run. `linux-preview-release.mjs:168` builds `propr-desktop-rpm.asc` for users with `print(..., end="")`, so that file also has no final newline. I left it alone because the issue rules out changing consumer trust, but it may hit the same `rpm --import` error on older RPM versions. No previews: nothing here is visible in the UI.
|
/review gpt-6-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
|
Please complete the same serialization fix in the generated RPM trust-bootstrap command in Preserve the raw manifest/identity publicKey string and fingerprint, the independent owner notice comparison, manual administrator import, signature verification and all installation gates. Only ensure the extracted tool-input Add a meaningful regression that executes the generated extraction command (or its extracted literal Python program) against a synthetic manifest with no-final-LF armor and already-terminated armor, then checks exact serialized bytes and unchanged manifest. Use disposable public keys for any real-tool test. Do not import into host trust or use owner secrets. Existing canonical-instruction validation must remain strict; update the canonical generated instructions and their tests together. Operator evidence: both existing production file-writing boundaries were independently exercised in the strict openSUSE image: no-LF input reaches actual RPM import and is rejected; already-terminated input imports successfully. The independent after-fix probe is ready. The old payload/product source remains frozen; these are release-control and consumer-instruction corrections only. No other changes are requested. |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR correctly addresses the visible objective by adding a final LF at both public-key file boundaries. The change is narrow and preserves existing validation and cleanup behavior. Ready to merge within the reviewed scope, conditional on pending checks completing successfully. ✅ Identity preservation — Serialization appends LF only when absent, leaving existing LF/CRLF endings and the configured identity string unchanged. This preserves the manifest’s exact-string identity contract. ✅ Consistent file handling — Both ✅ Focused regression coverage — Added tests cover serialized bytes, permissions, identity immutability, actual RPM imports, fingerprint mismatch, multiple-key rejection, and container-mounted input. This assessment is based on static inspection; no commands or tests were run. The supplied issue description is truncated, so requirements beyond its visible text could not be assessed. The authoritative checks report no failures and 13 pending checks. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to No suggestions. ScoreThe implementation satisfies the visible objective without a demonstrated regression and includes focused coverage of both affected paths. Pending CI results remain outstanding. 🤖 Review Details
🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit d5bca23 Summary of ChangesThe key-extraction command in the generated RPM install notes now writes a The fix ( python3 -c 'import json; k = json.load(open("linux-preview.json"))["rpmSigning"]["publicKey"]; print(k, end="" if k.endswith("\n") else "\n")' > propr-desktop-rpm.ascThat is the only line that changed in the instructions. The manifest's Tests
Results
For the real-tool runs I unpacked Debian 12's rpm 4.18 and GnuPG 2.2.40 into There are no previews because nothing here is visible in the UI. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…es now writes a `propr-desktop-rpm.asc` that ends with a newline. It adds one LF only when the key doesn't already end with one. The new tests fail on the old command and pass on the fix.
The key-extraction command in the generated RPM install notes now writes a `propr-desktop-rpm.asc` that ends with a newline. It adds one LF only when the key doesn't already end with one. The new tests fail on the old command and pass on the fix.
**The fix** (`apps/desktop/scripts/linux-preview-release.mjs:168`)
```sh
python3 -c 'import json; k = json.load(open("linux-preview.json"))["rpmSigning"]["publicKey"]; print(k, end="" if k.endswith("\n") else "\n")' > propr-desktop-rpm.asc
```
That is the only line that changed in the instructions. The manifest's `publicKey` and fingerprint are untouched, as are the owner-notice comparison, the manual `sudo rpm --import`, `rpmkeys --checksig` and the install commands. The checks that compare published instructions against the canonical text are unchanged and still require an exact match. The canonical text and the tests changed together.
**Tests**
- **`linux-preview-release.test.mjs`, no real tools:** the test takes the `python3` line from the generated notes and runs it with `sh -c` against a synthetic manifest. It checks the exact bytes in three cases:
- a key with no final newline gets exactly one LF added.
- a key ending in LF is copied unchanged.
- a key ending in CRLF is copied unchanged.
In every case `linux-preview.json` is byte-for-byte unchanged and the key string in memory is not modified.
- **`rpm-signing.test.mjs`, real tools and a disposable key:** the same generated command runs on a manifest holding the key with its final newline stripped, then on one with it intact. Each output file is the exported key plus exactly one LF. `gpg --show-keys` shows the expected fingerprint, and `rpmkeys --import` into a fresh, throwaway RPM database gives exactly one key with the matching key ID. Nothing touches the host's trust store and no owner secrets are used.
**Results**
- Both new tests failed when I put the original command back.
- With the fix, the seven related suites passed: 70 passed, 0 failed, with `PROPR_REQUIRE_RPM_TESTS=1`. The one skip is the existing test that needs Linux root.
- ESLint with `--max-warnings 0` gave no errors on the three changed files.
For the real-tool runs I unpacked Debian 12's rpm 4.18 and GnuPG 2.2.40 into `/tmp`, since the container has neither. That RPM version accepts the key either way, so locally the old command fails only on the byte check, not at import. The openSUSE rejection itself still depends on your after-fix check in the strict image.
There are no previews because nothing here is visible in the UI.
PR: #3269
Comment by: @integry (ID: 6275952881)
Model: Claude Opus 5.5
|
/review gpt-6-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR correctly confines newline normalization to public-key tool-input files, including the generated bootstrap command. It is merge-ready within the reviewed scope, conditional on pending checks completing successfully. ✅ Minimal serialization change — Adds one LF only when absent, preserving already-terminated input and the raw identity and manifest strings. ✅ Trust boundaries preserved — Retains identity validation, fingerprint and single-key checks, manual administrator import, signature verification, and existing file permissions. ✅ Meaningful regression coverage — Tests execute the generated extraction command, compare exact bytes and unchanged manifests, and exercise disposable RPM imports and acceptance-container input. This assessment is based on static review; no commands were run. The issue excerpt is truncated, so requirements beyond its visible text could not be assessed. The supplied diff was reviewed in full. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to No suggestions. ScoreThe focused implementation addresses the demonstrated serialization failure with strong coverage and no verified regression. Current checks report no failures, with 13 still pending. Score: 9/10 🤖 Review Details
🤖 Review by ProPR |
AI Implementation Summary
Closes #3268
Branch:
3268/claude-opus-5-5-terminate-serialized-rpm--20261019-1944-yfcCommits: ✅ Changes committed (0146125)
AI Processing Completed
Execution Details:
Summary:
RPM public-key files are now always written with a final newline, even when the configured public key has none. The key text itself is unchanged, and only the files handed to the tools get the newline. All new tests fail on the original code and pass on the fix.
The fix (
apps/desktop/scripts/rpm-signing.mjs)writeRpmPublicKeyFile(path, identity), checks the identity withrequireRpmIdentityas before. It adds one LF only if the key doesn't already end with one, and writes the file with mode 0600.withKey(release.asc), which feedsgpg --importandrpmkeys --importinverifyRpm/signRpm.public.ascthat is mounted into the container as/input/public.asc.publicKeystring, the manifest comparison and all fingerprints are untouched. The fingerprint, exactly-one-key and RSA checks still run, as do signature parsing and temp-directory cleanup. The diagnostic stages are unchanged. Workflows, the container script, the Dockerfile, keys and secrets weren't edited.Tests
rpm-signing.test.mjs, without real tools: a key with no final newline, one with LF and one with CRLF all produce the right file bytes with mode 0600. The input string is never modified. Invalid identities (lowercase fingerprint, private key) are still rejected and no file is written.rpm-signing.test.mjs, with real tools and disposable keys: this runs in the existing workflows' required-tools step, so no workflow change was needed. A key with its final newline stripped, and one with it intact, both go throughwithKey. Each written file is the exported key plus exactly one LF, with mode 0600. Each imports withrpmkeysinto a fresh RPM database, giving exactly one key with the matching key ID, and the temporary directory is removed afterwards. A wrong fingerprint and two concatenated keys are still rejected.signed-rpm-acceptance-diagnostics.test.mjs: the fake Docker now records the mountedpublic.ascbytes and mode. A new test runs the full acceptance flow with a key that lacks the final newline and one that has it. In both cases the mounted file ends with exactly one LF and has mode 0600, the identity is unchanged, evidence passes and the workspace is removed. The existing privacy and diagnostic tests still pass.Results: The new tests are the three named above, and all three failed when I swapped the original source back in. I then ran the seven related suites (
rpm-signing,linux-preview-release,linux-preview-workflow,signed-rpm-acceptance-evidence,signing-key-verification,signed-rpm-acceptance-diagnostics,signed-rpm-acceptance-context) withPROPR_REQUIRE_RPM_TESTS=1. 68 passed and 0 failed. One test was skipped because it needs Linux root.The container has no gpg or rpm, so for the real-tool runs I extracted Debian 12's rpm 4.18 and GnuPG 2.2 into
/tmpwithout root. This doesn't reproduce the openSUSE rejection itself. That still needs the planned operator check: the strict openSUSE key-trust comparison and the full protected acceptance run.linux-preview-release.mjs:168buildspropr-desktop-rpm.ascfor users withprint(..., end=""), so that file also has no final newline. I left it alone because the issue rules out changing consumer trust, but it may hit the samerpm --importerror on older RPM versions.No previews: nothing here is visible in the UI.
Detailed Logs:
09023e5d-ba38-4a16-84b6-0e9bdd07e32aLog files stored at:
/usr/src/app/logs/executions/issue-3268-2026-10-19T19-52-06-518Z-conversation.json/usr/src/app/logs/executions/issue-3268-2026-10-19T19-52-06-518Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3268.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!