bamaer commented on PR #8316:
URL: https://github.com/apache/hop/pull/8316#issuecomment-5654682256
Thanks Matt — all six addressed, and each one was worth catching.
**Empty `-r` (GPG.java:350)** — gone with fail-closed. A new
`addRecipient()` throws `HopException` when the user ID is empty, on all four
encrypting paths (`encryptFile`, `signAndEncryptFile`, `encrypt`,
`signAndEncrypt`). `signFile` keeps omitting `-r`, as you suggested. I went
with fail-closed over documenting because the silent-default-recipient case is
the one nobody notices until the wrong person can open the file. Checked the
blast radius: `PGPEncryptStream` already rejects an empty key name before it
reaches `encrypt()`, and every empty `<userid>` in the integration workflows
(0001, 0003, 0008) belongs to a `sign` action. The exception surfaces through
the existing catch in `ActionPGPEncryptFiles.encryptFile()`, so the action
still errors — same outcome as the old `-r ""`, with a message that says why.
New test `anEmptyUserIdIsRefusedOnEveryEncryptPath` also asserts GnuPG is never
started, and the encrypt paths now assert the recipient rather than only
`signFile`.
**Option injection (GPG.java:357)** — `--` now precedes every file operand.
I deliberately left `--output`'s value alone: it's an option *argument*, and
GnuPG already takes it literally (`--output -weird.out` works; a `--` there
would break the command). Confirmed the underlying problem is real rather than
theoretical on GnuPG 2.4.4/2.4.9: `gpg --batch --verify -weird.txt` → `invalid
option "-weird.txt"`, with `--` it opens the file. New test
`filenamesThatLookLikeOptionsAreMarkedAsOperands` covers `-o`, `--output`,
`--status-fd` and `-r` as filenames.
**Test blind spots (test:207)** — the recorder now does `cat > stdin.txt`,
and `assertPassphraseOnStdinOnly()` asserts all of: passphrase absent from
argv, `--passphrase-fd 0` present, `--pinentry-mode loopback` present, and
stdin equal to the passphrase. Applied to all four passphrase paths, not just
`decryptFile`. There's also a negative case asserting no `--passphrase-fd` and
empty stdin when no passphrase is given.
This one found a real bug, so thank you: `execGnuPG()` only closed the
child's stdin when there was something to write. A recorder that reads stdin
deadlocked against `waitFor()`. Stdin is now closed either way, which is what
GnuPG should have been seeing all along.
**`fileMode` javadoc (GPG.java:167)** — rewritten to describe the actual
branch ("when false, `--batch --armor` is prepended"), plus a note that
`inputStr` is the passphrase for every caller except `encrypt()`.
**`CONST_BATCH_YES` (GPG.java:50)** — removed; it had no references left
anywhere in the repo.
**Narration (test:230)** — trimmed. The class javadoc is now a pointer to
#8311, the "the one that matters" preamble and the "safe to log" aside are
gone. Kept the loopback/process-table *why* in `addPassPhraseFromStdin` and in
the passphrase assertion helper.
Verified end to end against GnuPG 2.4.9 with a throwaway keyring and a
`-weird.txt` operand: encrypt, decrypt with a loopback passphrase on stdin,
sign+encrypt, `-se`, `--sign`, `--clearsign` and `--verify` all work with the
terminator. `mvn clean install apache-rat:check` on `plugins/actions/pgpfiles`
and `plugins/transforms/pgp` is green — 27 tests, RAT and Spotless clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
