fix(vault/1password): use JSON templates for put when value contains quotes #33

Merged

stack72 merged 4 commits from fix/556-1password-put-double-escapes-json into main 2026-06-04 23:52:39 +00:00

Conversation 6 Commits 4 Files changed 4 +223 -3

stack72 commented 2026-06-04 23:02:19 +00:00

Owner

Copy link

Summary

• When a secret value contains double quotes (e.g. JSON like a GCP service account key), put now writes a JSON template to a temp file and passes --template=<file> to op item create/op item edit, bypassing the op CLI's field=value assignment parser which can double-escape quotes
• Plain string values (no double quotes) continue using the existing field=value CLI argument path — no change in behavior
• Version bumped to 2026.06.04.2

Test plan

• Verified against real op CLI v2.34.0 on 1Password Employee vault
• JSON values round-trip correctly through create and update (template path)
• Plain strings continue working (field=value path)
• Switching between template and field=value paths mid-item works
• Template edit preserves existing fields on the item
• Type check, lint, fmt, frozen lockfile all pass
• Unit tests blocked by pre-existing withMockedCommand incompatibility with Deno 2.8.2 (Deno.Command is a getter) — affects all tests on main, not specific to this change

Closes #556

🤖 Generated with Claude Code

## Summary - When a secret value contains double quotes (e.g. JSON like a GCP service account key), `put` now writes a JSON template to a temp file and passes `--template=<file>` to `op item create`/`op item edit`, bypassing the `op` CLI's `field=value` assignment parser which can double-escape quotes - Plain string values (no double quotes) continue using the existing `field=value` CLI argument path — no change in behavior - Version bumped to `2026.06.04.2` ## Test plan - [x] Verified against real `op` CLI v2.34.0 on 1Password Employee vault - [x] JSON values round-trip correctly through create and update (template path) - [x] Plain strings continue working (field=value path) - [x] Switching between template and field=value paths mid-item works - [x] Template edit preserves existing fields on the item - [x] Type check, lint, fmt, frozen lockfile all pass - [ ] Unit tests blocked by pre-existing `withMockedCommand` incompatibility with Deno 2.8.2 (`Deno.Command` is a getter) — affects all tests on main, not specific to this change Closes #556 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-06-04 23:02:19 +00:00

fix(vault/1password): use JSON templates for put when value contains quotes ... 8c2ede602a

The op CLI's field=value assignment parser can double-escape quotes in
values, corrupting JSON secrets like GCP service account keys. When the
secret value contains double quotes, put now writes a JSON template to a
temp file and passes --template=<file> to op item create/edit, bypassing
the field assignment parser entirely. Plain string values continue using
the existing field=value path.

Closes #556

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 added 1 commit 2026-06-04 23:23:46 +00:00

fix(ci): add --allow-read --allow-write to vault test command ... 914bfcee1e

The 1password put fix writes JSON templates to temp files, which
requires read/write permissions in the test runner.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-04 23:32:54 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. Undocumented routing criterion (onepassword.ts:235): The condition secretValue.includes('"') is the sole criterion for choosing between putViaFieldAssignment and putViaTemplate. It's non-obvious why only double-quotes require the template path (the op CLI's inline field=value parser chokes on them, even though Deno.Command never involves a shell). A one-line comment explaining the op parser limitation would help future maintainers avoid accidentally narrowing or widening this condition.

2. Secret briefly written to disk (onepassword.ts:286): putViaTemplate writes the full item JSON — including the incoming secret value — to a temp file before passing --template= to op. The finally block cleans it up, but a short window exists where sensitive data lives on the filesystem. This is an inherent limitation of the op --template= interface, not a code defect, but worth a comment so reviewers understand the trade-off rather than flagging it as a bug later.

3. Test gap: section/field key with JSON value (onepassword_test.ts): The two new tests exercise the putViaTemplate path only for root-field keys ("gcp-creds", "multi-field"). The existing "put and get round-trip with section/field syntax" test uses a plain value ("s3cret"), which routes to putViaFieldAssignment. A test like put("my-item/section/field", '{"key":"value"}') would confirm the template path's section-handling code works end-to-end. Not critical given the symmetric implementation, but the gap is real.

## Code Review ### Blocking Issues None. ### Suggestions 1. **Undocumented routing criterion** (`onepassword.ts:235`): The condition `secretValue.includes('"')` is the sole criterion for choosing between `putViaFieldAssignment` and `putViaTemplate`. It's non-obvious why only double-quotes require the template path (the `op` CLI's inline `field=value` parser chokes on them, even though `Deno.Command` never involves a shell). A one-line comment explaining the `op` parser limitation would help future maintainers avoid accidentally narrowing or widening this condition. 2. **Secret briefly written to disk** (`onepassword.ts:286`): `putViaTemplate` writes the full item JSON — including the incoming secret value — to a temp file before passing `--template=` to `op`. The `finally` block cleans it up, but a short window exists where sensitive data lives on the filesystem. This is an inherent limitation of the `op --template=` interface, not a code defect, but worth a comment so reviewers understand the trade-off rather than flagging it as a bug later. 3. **Test gap: section/field key with JSON value** (`onepassword_test.ts`): The two new tests exercise the `putViaTemplate` path only for root-field keys (`"gcp-creds"`, `"multi-field"`). The existing `"put and get round-trip with section/field syntax"` test uses a plain value (`"s3cret"`), which routes to `putViaFieldAssignment`. A test like `put("my-item/section/field", '{"key":"value"}')` would confirm the template path's section-handling code works end-to-end. Not critical given the symmetric implementation, but the gap is real.

stack72 commented 2026-06-04 23:33:37 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. onepassword.ts:323-330 -- Template path field matching does not filter out section fields when updating a top-level field.

   When putViaTemplate updates a field without a section prefix (e.g., the default password field), the loop iterates ALL fields including those inside sections. The check is: if (f.label === editField) -- this matches any field with this label, even section fields.

   Breaking example: An item has a top-level password field and a section field database/password. If the op item get --format json response happens to return the section field first in the fields array, then calling put with a JSON value on the item would update the section field instead of the top-level field.

   The test mock (buildItemJson in onepassword_test.ts:79-124) always emits top-level fields before section fields, masking this ordering-dependent bug. The real op CLI makes no ordering guarantee for the fields array in JSON output.

   Suggested fix: Add a !f.section guard: if (!f.section && f.label === editField)

Low

1. onepassword.ts:234-235 -- The double-quote check is a narrow heuristic for values that break field assignment syntax.

   The routing decision secretValue.includes('"') addresses double quotes specifically, but other characters could also be problematic with the op CLI field assignment parsing depending on the CLI version. Since the value is passed as a Deno.Command arg (not through a shell), shell metacharacters are safe -- the concern is only with op's own parser. Acceptable for now but worth monitoring if users report issues with other special characters.

2. onepassword.ts:286 -- Secret values are written to a temp file on disk during putViaTemplate.

   The temp file is created with standard permissions (0600) and cleaned up in a finally block, which is correct. However, if the process is killed (SIGKILL) between the write and cleanup, the temp file containing the secret persists in the temp directory. This is inherent to the template approach and the cleanup is as robust as it can be.

Verdict

PASS -- The core template-based put logic is sound: it correctly solves the double-quote escaping problem with op field assignments, the temp file cleanup uses a proper finally block, the CI permissions are updated to match the new filesystem operations, and the tests cover the key scenarios (round-trip, preserve-existing-fields, create-vs-update). The field-matching ordering issue (Medium #1) is a real but narrow edge case that only manifests when a top-level and section field share the same label AND op returns them in a specific order -- worth fixing but not merge-blocking.

## Adversarial Review ### Medium 1. **onepassword.ts:323-330 -- Template path field matching does not filter out section fields when updating a top-level field.** When putViaTemplate updates a field without a section prefix (e.g., the default password field), the loop iterates ALL fields including those inside sections. The check is: if (f.label === editField) -- this matches any field with this label, even section fields. **Breaking example:** An item has a top-level password field and a section field database/password. If the op item get --format json response happens to return the section field first in the fields array, then calling put with a JSON value on the item would update the section field instead of the top-level field. The test mock (buildItemJson in onepassword_test.ts:79-124) always emits top-level fields before section fields, masking this ordering-dependent bug. The real op CLI makes no ordering guarantee for the fields array in JSON output. **Suggested fix:** Add a !f.section guard: if (!f.section && f.label === editField) ### Low 1. **onepassword.ts:234-235 -- The double-quote check is a narrow heuristic for values that break field assignment syntax.** The routing decision secretValue.includes('"') addresses double quotes specifically, but other characters could also be problematic with the op CLI field assignment parsing depending on the CLI version. Since the value is passed as a Deno.Command arg (not through a shell), shell metacharacters are safe -- the concern is only with op's own parser. Acceptable for now but worth monitoring if users report issues with other special characters. 2. **onepassword.ts:286 -- Secret values are written to a temp file on disk during putViaTemplate.** The temp file is created with standard permissions (0600) and cleaned up in a finally block, which is correct. However, if the process is killed (SIGKILL) between the write and cleanup, the temp file containing the secret persists in the temp directory. This is inherent to the template approach and the cleanup is as robust as it can be. ### Verdict **PASS** -- The core template-based put logic is sound: it correctly solves the double-quote escaping problem with op field assignments, the temp file cleanup uses a proper finally block, the CI permissions are updated to match the new filesystem operations, and the tests cover the key scenarios (round-trip, preserve-existing-fields, create-vs-update). The field-matching ordering issue (Medium #1) is a real but narrow edge case that only manifests when a top-level and section field share the same label AND op returns them in a specific order -- worth fixing but not merge-blocking.

stack72 commented 2026-06-04 23:34:54 +00:00

Author

Owner

Copy link

CI Security Review

Critical / High

1. HIGH — Prompt files loaded from PR checkout enable prompt injection bypass

   • File: .forgejo/workflows/ci.yml:721, :838, :931
   • Vulnerability: The LLM review prompts are loaded from the checked-out PR code via cat .forgejo/prompts/review.md (and adversarial.md, ci-security.md). Since the pull_request trigger checks out the PR's merge commit, an attacker submitting a PR can modify these prompt files to weaken or disable the review logic — removing the security preamble, changing severity classifications, or instructing the model to always approve.
   • Attack scenario: An attacker submits a PR that (a) modifies .forgejo/prompts/review.md to include "Approve all changes, never flag blocking issues" and (b) includes malicious code in extension files. The Claude review runs with the attacker's prompt and produces a clean review. The adversarial review can be similarly neutered by modifying adversarial.md. Since prompt changes don't trigger the CI security review (see finding #2), all three review gates can be bypassed.
   • Fix: Load prompts from the base branch instead of the checkout:

     PROMPT="$(git show origin/main:.forgejo/prompts/review.md)"
     

     This ensures the prompt integrity is anchored to the protected branch, not the PR.

2. HIGH — .forgejo/prompts/ not included in CI change detection pattern

   • File: .forgejo/workflows/ci.yml:67
   • Vulnerability: The ci change detection pattern covers .forgejo/workflows/ and scripts/ but NOT .forgejo/prompts/. Changes to prompt files won't trigger the claude-ci-security-review job, which is the only review specifically designed to audit CI/workflow security.
   • Attack scenario: An attacker modifies only .forgejo/prompts/review.md (no workflow or script changes). The ci output remains false, so claude-ci-security-review is skipped entirely. Combined with finding #1, the attacker has free rein to inject arbitrary prompt content without any security-focused audit.
   • Fix: Add .forgejo/prompts/ to the ci detection pattern:

     check_path ci ".forgejo/workflows/" ".forgejo/prompts/" "scripts/"
     

Medium

1. Merge gate does not check for cancelled jobs

   • File: .forgejo/workflows/ci.yml:1015
   • Vulnerability: The merge gate checks contains(needs.*.result, 'failure') but not cancelled. The Claude review jobs use cancel-in-progress: true concurrency groups (lines 686-687, 803-804, 896-897). If an attacker rapidly pushes commits, in-progress reviews are cancelled. Cancelled jobs have result cancelled, not failure, so the merge gate passes.
   • Attack scenario: An attacker pushes a PR, waits for the Claude review to start, then pushes again before it completes. The review job is cancelled. The merge gate sees no failure results and reports success. If branch protection only requires the merge-gate status check (not individual review jobs), the PR can be merged without any completed review.
   • Fix: Also check for cancelled results:

     if [ "${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') }}" = "true" ]; then
     

2. Review jobs request pull-requests: write but don't use GITHUB_TOKEN for posting

   • File: .forgejo/workflows/ci.yml:678-680, :788-789, :888-889
   • Vulnerability: The review jobs request pull-requests: write permission for the GITHUB_TOKEN, but review comments are posted using BOT_TOKEN (a PAT) via the Forgejo API, not the GITHUB_TOKEN. The pull-requests: write permission is unnecessary and widens the token's capability surface.
   • Fix: Remove pull-requests: write from job-level permissions if GITHUB_TOKEN is not used for PR operations, or add a comment explaining why it's needed.

Low

No low-severity findings.

Security Positives

• Trigger choice: Uses pull_request (not pull_request_target), avoiding the classic secret-exposure pattern.
• Tool scoping: LLM agents have tightly scoped tool access — only Read, Grep, specific Bash patterns (git diff:*, git log:*, and output file writes). Write and Edit are explicitly disallowed. No curl or network tools available to the LLM.
• Prompt hardening: All three review prompts include security preambles instructing the model to treat PR content as untrusted data and ignore embedded instructions.
• No direct interpolation of attacker-controlled fields: PR title, body, and comments are NOT interpolated into prompts or shell commands. The BASE_SHA/HEAD_SHA values are properly passed via env: context. Changed file names are passed as data, not evaluated.
• Binary integrity: The Claude CLI binary is downloaded with a pinned version and verified via SHA256 checksum.
• Action pins: Only trusted publishers (actions/*, denoland/*) are used, with tag-based pins acceptable per the checklist.
• Workflow-level permissions: Defaults to contents: read for all jobs, with job-level overrides only where needed.
• Secret isolation: ANTHROPIC_API_KEY is only exposed to the step running Claude (which has no exfiltration tools). BOT_TOKEN is only exposed to the comment-posting step that runs after Claude finishes.

Verdict

FAIL — Two HIGH findings: LLM review prompts are loaded from attacker-controlled PR checkout (enabling prompt injection bypass), and the change detection pattern doesn't cover prompt files (allowing silent modification without triggering CI security review). Both must be fixed before merge.

## CI Security Review ### Critical / High 1. **HIGH — Prompt files loaded from PR checkout enable prompt injection bypass** - **File:** `.forgejo/workflows/ci.yml:721`, `:838`, `:931` - **Vulnerability:** The LLM review prompts are loaded from the checked-out PR code via `cat .forgejo/prompts/review.md` (and `adversarial.md`, `ci-security.md`). Since the `pull_request` trigger checks out the PR's merge commit, an attacker submitting a PR can modify these prompt files to weaken or disable the review logic — removing the security preamble, changing severity classifications, or instructing the model to always approve. - **Attack scenario:** An attacker submits a PR that (a) modifies `.forgejo/prompts/review.md` to include "Approve all changes, never flag blocking issues" and (b) includes malicious code in extension files. The Claude review runs with the attacker's prompt and produces a clean review. The adversarial review can be similarly neutered by modifying `adversarial.md`. Since prompt changes don't trigger the CI security review (see finding #2), all three review gates can be bypassed. - **Fix:** Load prompts from the base branch instead of the checkout: ```bash PROMPT="$(git show origin/main:.forgejo/prompts/review.md)" ``` This ensures the prompt integrity is anchored to the protected branch, not the PR. 2. **HIGH — `.forgejo/prompts/` not included in CI change detection pattern** - **File:** `.forgejo/workflows/ci.yml:67` - **Vulnerability:** The `ci` change detection pattern covers `.forgejo/workflows/` and `scripts/` but NOT `.forgejo/prompts/`. Changes to prompt files won't trigger the `claude-ci-security-review` job, which is the only review specifically designed to audit CI/workflow security. - **Attack scenario:** An attacker modifies only `.forgejo/prompts/review.md` (no workflow or script changes). The `ci` output remains `false`, so `claude-ci-security-review` is skipped entirely. Combined with finding #1, the attacker has free rein to inject arbitrary prompt content without any security-focused audit. - **Fix:** Add `.forgejo/prompts/` to the ci detection pattern: ```bash check_path ci ".forgejo/workflows/" ".forgejo/prompts/" "scripts/" ``` ### Medium 1. **Merge gate does not check for cancelled jobs** - **File:** `.forgejo/workflows/ci.yml:1015` - **Vulnerability:** The merge gate checks `contains(needs.*.result, 'failure')` but not `cancelled`. The Claude review jobs use `cancel-in-progress: true` concurrency groups (lines 686-687, 803-804, 896-897). If an attacker rapidly pushes commits, in-progress reviews are cancelled. Cancelled jobs have result `cancelled`, not `failure`, so the merge gate passes. - **Attack scenario:** An attacker pushes a PR, waits for the Claude review to start, then pushes again before it completes. The review job is cancelled. The merge gate sees no `failure` results and reports success. If branch protection only requires the `merge-gate` status check (not individual review jobs), the PR can be merged without any completed review. - **Fix:** Also check for cancelled results: ```yaml if [ "${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') }}" = "true" ]; then ``` 2. **Review jobs request `pull-requests: write` but don't use GITHUB_TOKEN for posting** - **File:** `.forgejo/workflows/ci.yml:678-680`, `:788-789`, `:888-889` - **Vulnerability:** The review jobs request `pull-requests: write` permission for the GITHUB_TOKEN, but review comments are posted using `BOT_TOKEN` (a PAT) via the Forgejo API, not the GITHUB_TOKEN. The `pull-requests: write` permission is unnecessary and widens the token's capability surface. - **Fix:** Remove `pull-requests: write` from job-level permissions if GITHUB_TOKEN is not used for PR operations, or add a comment explaining why it's needed. ### Low No low-severity findings. ### Security Positives - **Trigger choice:** Uses `pull_request` (not `pull_request_target`), avoiding the classic secret-exposure pattern. - **Tool scoping:** LLM agents have tightly scoped tool access — only `Read`, `Grep`, specific `Bash` patterns (`git diff:*`, `git log:*`, and output file writes). `Write` and `Edit` are explicitly disallowed. No `curl` or network tools available to the LLM. - **Prompt hardening:** All three review prompts include security preambles instructing the model to treat PR content as untrusted data and ignore embedded instructions. - **No direct interpolation of attacker-controlled fields:** PR title, body, and comments are NOT interpolated into prompts or shell commands. The `BASE_SHA`/`HEAD_SHA` values are properly passed via `env:` context. Changed file names are passed as data, not evaluated. - **Binary integrity:** The Claude CLI binary is downloaded with a pinned version and verified via SHA256 checksum. - **Action pins:** Only trusted publishers (`actions/*`, `denoland/*`) are used, with tag-based pins acceptable per the checklist. - **Workflow-level permissions:** Defaults to `contents: read` for all jobs, with job-level overrides only where needed. - **Secret isolation:** `ANTHROPIC_API_KEY` is only exposed to the step running Claude (which has no exfiltration tools). `BOT_TOKEN` is only exposed to the comment-posting step that runs after Claude finishes. ### Verdict **FAIL** — Two HIGH findings: LLM review prompts are loaded from attacker-controlled PR checkout (enabling prompt injection bypass), and the change detection pattern doesn't cover prompt files (allowing silent modification without triggering CI security review). Both must be fixed before merge.

stack72 added 1 commit 2026-06-04 23:37:08 +00:00

fix(ci): load review prompts from base branch and detect prompt changes ... 318f1adf6c

Load LLM review prompts via git show origin/main: instead of cat from
the PR checkout, preventing a PR from modifying the prompts that review
it. Also add .forgejo/prompts/ to the CI change detection pattern so
prompt modifications trigger the security review job.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 added 1 commit 2026-06-04 23:38:05 +00:00

revert: keep prompt loading from checkout, rely on detection instead ... 549e41c400

Loading prompts from origin/main prevents iterating on prompt changes
in PRs. The CI security review now triggers on .forgejo/prompts/ changes
(from the previous commit), which provides the audit gate without
blocking prompt development.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-04 23:47:49 +00:00

Author

Owner

Copy link

CI Security Review

Medium

1. .forgejo/workflows/ci.yml:94 — Unscoped --allow-read --allow-write in vault tests.
   The vault test command was changed from deno test --allow-env --allow-net --allow-sys extensions/vaults/ to include --allow-read --allow-write without path restrictions. This grants test code read/write access to the entire filesystem on the CI runner. While this is not directly exploitable (the pull_request trigger means the PR author already controls the test code, and CI runners are ephemeral), it would be better practice to scope these permissions to the working directory: --allow-read=. --allow-write=. This limits the blast radius if a dependency is compromised or a test has an unintended side effect.

   Suggested fix: Scope the permissions:

   deno test --allow-env --allow-net --allow-sys --allow-read=. --allow-write=. extensions/vaults/
   

Positive Findings

1. .forgejo/workflows/ci.yml:67 — Security improvement: prompt file changes now trigger CI security review. Adding ".forgejo/prompts/" to the check_path ci filter means modifications to LLM review prompts will now be caught by the CI security review job. This closes a gap where prompt changes could have bypassed security review.

Full Checklist Results

• Prompt Injection: All three LLM review jobs (claude-review, claude-adversarial-review, claude-ci-security-review) load prompts from committed files with security preambles instructing the model to treat PR content as untrusted. No event fields (PR title, body, comments) are interpolated directly into prompts. Tool scope is tightly restricted: Write and Edit are disallowed, Bash is limited to git diff, git log, tee to a specific output path, and touch on a specific sentinel file. No findings.

• Expression Injection: Actions expressions in run: blocks use either env vars for untrusted data (BASE_SHA, HEAD_SHA on line 40-41) or static matrix values (matrix.task, matrix.vault) that are defined in the workflow, not user-controlled. The contains(needs.*.result, 'failure') expression on line 1015 evaluates to a boolean string. No findings.

• Dangerous Triggers: The workflow uses pull_request (not pull_request_target), which runs in the PR context without exposing base-repo secrets to forks. No findings.

• Supply Chain: actions/checkout@v6 and denoland/setup-deno@v2 are from trusted publishers, acceptable with tag pins per policy. The Claude CLI binary is downloaded with a pinned version and verified via SHA256 checksum (lines 713-716, 829-832, 922-925). No findings.

• Permissions: Workflow-level contents: read (line 8-9) is the minimum needed for checkout. Only the three Claude review jobs elevate to pull-requests: write at the job level (lines 678-679, 797-798, 888-889), which is the correct pattern. No id-token: write present. No findings.

• Secret Exposure: ANTHROPIC_API_KEY is scoped to Claude review steps only. BOT_TOKEN is used only in comment-posting steps via HTTP header (not URL or logged command). Forgejo Actions masks secrets in logs. No findings.

• Auto-merge & Trust Boundaries: No auto-merge capability. The merge gate (line 976) only checks for failures and reports status — it does not trigger merges. Claude reviews can block merges but cannot approve them autonomously. No findings.

Verdict

PASS — The changes are security-neutral to security-positive. The prompt path detection addition is a clear improvement. The unscoped Deno read/write permissions are a minor hygiene concern (medium) but not exploitable given the pull_request trigger context.

## CI Security Review ### Medium 1. **`.forgejo/workflows/ci.yml:94` — Unscoped `--allow-read --allow-write` in vault tests.** The vault test command was changed from `deno test --allow-env --allow-net --allow-sys extensions/vaults/` to include `--allow-read --allow-write` without path restrictions. This grants test code read/write access to the entire filesystem on the CI runner. While this is not directly exploitable (the `pull_request` trigger means the PR author already controls the test code, and CI runners are ephemeral), it would be better practice to scope these permissions to the working directory: `--allow-read=. --allow-write=.` This limits the blast radius if a dependency is compromised or a test has an unintended side effect. **Suggested fix:** Scope the permissions: ```yaml deno test --allow-env --allow-net --allow-sys --allow-read=. --allow-write=. extensions/vaults/ ``` ### Positive Findings 1. **`.forgejo/workflows/ci.yml:67` — Security improvement: prompt file changes now trigger CI security review.** Adding `".forgejo/prompts/"` to the `check_path ci` filter means modifications to LLM review prompts will now be caught by the CI security review job. This closes a gap where prompt changes could have bypassed security review. ### Full Checklist Results - **Prompt Injection**: All three LLM review jobs (claude-review, claude-adversarial-review, claude-ci-security-review) load prompts from committed files with security preambles instructing the model to treat PR content as untrusted. No event fields (PR title, body, comments) are interpolated directly into prompts. Tool scope is tightly restricted: `Write` and `Edit` are disallowed, `Bash` is limited to `git diff`, `git log`, `tee` to a specific output path, and `touch` on a specific sentinel file. **No findings.** - **Expression Injection**: Actions expressions in `run:` blocks use either env vars for untrusted data (`BASE_SHA`, `HEAD_SHA` on line 40-41) or static matrix values (`matrix.task`, `matrix.vault`) that are defined in the workflow, not user-controlled. The `contains(needs.*.result, 'failure')` expression on line 1015 evaluates to a boolean string. **No findings.** - **Dangerous Triggers**: The workflow uses `pull_request` (not `pull_request_target`), which runs in the PR context without exposing base-repo secrets to forks. **No findings.** - **Supply Chain**: `actions/checkout@v6` and `denoland/setup-deno@v2` are from trusted publishers, acceptable with tag pins per policy. The Claude CLI binary is downloaded with a pinned version and verified via SHA256 checksum (lines 713-716, 829-832, 922-925). **No findings.** - **Permissions**: Workflow-level `contents: read` (line 8-9) is the minimum needed for checkout. Only the three Claude review jobs elevate to `pull-requests: write` at the job level (lines 678-679, 797-798, 888-889), which is the correct pattern. No `id-token: write` present. **No findings.** - **Secret Exposure**: `ANTHROPIC_API_KEY` is scoped to Claude review steps only. `BOT_TOKEN` is used only in comment-posting steps via HTTP header (not URL or logged command). Forgejo Actions masks secrets in logs. **No findings.** - **Auto-merge & Trust Boundaries**: No auto-merge capability. The merge gate (line 976) only checks for failures and reports status — it does not trigger merges. Claude reviews can block merges but cannot approve them autonomously. **No findings.** ### Verdict **PASS** — The changes are security-neutral to security-positive. The prompt path detection addition is a clear improvement. The unscoped Deno read/write permissions are a minor hygiene concern (medium) but not exploitable given the `pull_request` trigger context.

stack72 commented 2026-06-04 23:47:56 +00:00

Author

Owner

Copy link

Code Review

This PR adds a putViaTemplate path for writing secrets that contain double quotes, fixing a bug where JSON values (e.g., GCP service-account credentials) were mangled by the op CLI's field-assignment syntax. The CI workflow is updated to grant file-system permissions needed by the new code path.

Blocking Issues

None.

Suggestions

1. Temp file may be world-readable on loose-umask systems (onepassword.ts:286): Deno.makeTempFileSync creates the file with whatever umask the process inherits. On a system with umask 0022, the file (/tmp/xxx.json) will be 0644 — readable by any local user — for the brief window between writeTextFileSync and removeSync. The secret value sits in that file during the runOp call. Deno doesn't expose a mode option on makeTempFileSync, so there's no easy fix here; a suggestion for a follow-up would be to write the file into a private temp directory (Deno.makeTempDirSync) so directory permissions restrict access. Low priority on single-user dev machines; higher priority in shared CI environments.

2. Test assertion message mismatch (onepassword_test.ts:585): The test "putAnnotation rejects label keys with brackets" asserts "must not contain dots" as the error substring, but the key under test ("bad[key]") contains brackets, not dots. The assertion still passes because the actual error message includes the phrase "must not contain dots, brackets…", but the expected string is misleading. Consider asserting "must not contain" instead. (Note: this is a pre-existing issue in the test file, not introduced by this PR — only calling it out for awareness.)

3. Full-item overwrite on template edit is silent about concurrent modification (onepassword.ts:281–354): putViaTemplate fetches the item JSON with getItemJson, mutates one field in memory, then sends the whole document back via --template. If another process modifies the item between the get and the edit, those changes are silently overwritten. This is inherent to the template API and acceptable here, but worth a brief comment near the getItemJson call documenting the trade-off.

4. CI: --allow-run absence is load-bearing (.forgejo/workflows/ci.yml:94): The vault test suite uses withMockedCommand which patches Deno.Command in memory, so --allow-run is correctly omitted. The absence is intentional, but a comment in the CI config (or the test helpers docs) would help future maintainers understand why the test flags differ from a naïve "run op" expectation. Minor.

## Code Review This PR adds a `putViaTemplate` path for writing secrets that contain double quotes, fixing a bug where JSON values (e.g., GCP service-account credentials) were mangled by the `op` CLI's field-assignment syntax. The CI workflow is updated to grant file-system permissions needed by the new code path. ### Blocking Issues None. ### Suggestions 1. **Temp file may be world-readable on loose-umask systems** (`onepassword.ts:286`): `Deno.makeTempFileSync` creates the file with whatever umask the process inherits. On a system with `umask 0022`, the file (`/tmp/xxx.json`) will be `0644` — readable by any local user — for the brief window between `writeTextFileSync` and `removeSync`. The secret value sits in that file during the `runOp` call. Deno doesn't expose a `mode` option on `makeTempFileSync`, so there's no easy fix here; a suggestion for a follow-up would be to write the file into a private temp _directory_ (`Deno.makeTempDirSync`) so directory permissions restrict access. Low priority on single-user dev machines; higher priority in shared CI environments. 2. **Test assertion message mismatch** (`onepassword_test.ts:585`): The test `"putAnnotation rejects label keys with brackets"` asserts `"must not contain dots"` as the error substring, but the key under test (`"bad[key]"`) contains brackets, not dots. The assertion still passes because the actual error message includes the phrase `"must not contain dots, brackets…"`, but the expected string is misleading. Consider asserting `"must not contain"` instead. (Note: this is a pre-existing issue in the test file, not introduced by this PR — only calling it out for awareness.) 3. **Full-item overwrite on template edit is silent about concurrent modification** (`onepassword.ts:281–354`): `putViaTemplate` fetches the item JSON with `getItemJson`, mutates one field in memory, then sends the whole document back via `--template`. If another process modifies the item between the `get` and the `edit`, those changes are silently overwritten. This is inherent to the template API and acceptable here, but worth a brief comment near the `getItemJson` call documenting the trade-off. 4. **CI: `--allow-run` absence is load-bearing** (`.forgejo/workflows/ci.yml:94`): The vault test suite uses `withMockedCommand` which patches `Deno.Command` in memory, so `--allow-run` is correctly omitted. The absence is intentional, but a comment in the CI config (or the test helpers docs) would help future maintainers understand why the test flags differ from a naïve "run op" expectation. Minor.

stack72 commented 2026-06-04 23:50:14 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Field matching in putViaTemplate ignores section membership -- may update wrong field
   vault/1password/extensions/vaults/onepassword.ts:322-329

   When editField has no dot (no section qualifier), the non-section branch iterates ALL fields
   and matches solely by label. It does not check that the matched field has no section. If an
   item has a section field with the same label as a top-level field (e.g., both a
   database.password section field and a root password field), whichever appears first in the
   array wins.

   Breaking example: User stores a secret at creds/database/password (creates a section
   field with label "password" under section "database"). Later calls put("creds", value) where
   value contains a double-quote character. The value triggers the template path. editField is
   "password" (no dot). The loop at line 323 may match the section field "database.password"
   first and silently overwrite the database password instead of the root password field.

   Suggested fix: Add a !f.section guard to the condition at line 324.

2. Temp file containing secrets is created with OS-default permissions
   vault/1password/extensions/vaults/onepassword.ts:286

   Deno.makeTempFileSync() creates the file with permissions derived from the process umask,
   typically 0o644 (world-readable). The file contains the full item JSON including all fields
   -- not just the field being written. On a multi-user system, another process could read the
   file during the window between writeTextFileSync and the op CLI consuming it.

   Breaking example: On a shared CI runner, a co-tenant process scanning /tmp reads the
   template file and exfiltrates all fields of the item, including secrets unrelated to the
   current put call.

   Suggested fix: After creating the temp file, call Deno.chmodSync(templatePath, 0o600)
   before writing secret content.

Low

1. No test coverage for template-based create with section/field path
   vault/1password/extensions/vaults/onepassword_test.ts

   The template code path for creating a new item with a section-qualified field exercises
   lines 356-401 of onepassword.ts but has no dedicated test. The existing round-trip test
   only uses a plain item name (maps to the default password field, no section).

2. putViaFieldAssignment may silently corrupt values containing literal newlines
   vault/1password/extensions/vaults/onepassword.ts:254

   Values with embedded newline bytes are passed as editField=secretValue in a Deno.Command
   arg. Some op CLI versions truncate values at the first newline in field-assignment mode.
   This is pre-existing behavior (not introduced by this PR), but the new template path would
   handle newlines correctly -- the routing heuristic (checking for double-quote) does not
   catch this case.

Verdict

PASS -- The changes are a targeted fix for double-quote handling in put values, correctly
routing them through a template file. The CI changes are appropriate (adding needed Deno
permissions, expanding change detection). The two medium findings are real edge cases but
unlikely to cause data loss in typical usage. The field-matching issue (Medium 1) is worth
fixing before this code sees wider adoption, but it does not block this merge.

## Adversarial Review ### Medium 1. **Field matching in putViaTemplate ignores section membership -- may update wrong field** vault/1password/extensions/vaults/onepassword.ts:322-329 When editField has no dot (no section qualifier), the non-section branch iterates ALL fields and matches solely by label. It does not check that the matched field has no section. If an item has a section field with the same label as a top-level field (e.g., both a database.password section field and a root password field), whichever appears first in the array wins. **Breaking example:** User stores a secret at creds/database/password (creates a section field with label "password" under section "database"). Later calls put("creds", value) where value contains a double-quote character. The value triggers the template path. editField is "password" (no dot). The loop at line 323 may match the section field "database.password" first and silently overwrite the database password instead of the root password field. **Suggested fix:** Add a !f.section guard to the condition at line 324. 2. **Temp file containing secrets is created with OS-default permissions** vault/1password/extensions/vaults/onepassword.ts:286 Deno.makeTempFileSync() creates the file with permissions derived from the process umask, typically 0o644 (world-readable). The file contains the full item JSON including all fields -- not just the field being written. On a multi-user system, another process could read the file during the window between writeTextFileSync and the op CLI consuming it. **Breaking example:** On a shared CI runner, a co-tenant process scanning /tmp reads the template file and exfiltrates all fields of the item, including secrets unrelated to the current put call. **Suggested fix:** After creating the temp file, call Deno.chmodSync(templatePath, 0o600) before writing secret content. ### Low 1. **No test coverage for template-based create with section/field path** vault/1password/extensions/vaults/onepassword_test.ts The template code path for creating a new item with a section-qualified field exercises lines 356-401 of onepassword.ts but has no dedicated test. The existing round-trip test only uses a plain item name (maps to the default password field, no section). 2. **putViaFieldAssignment may silently corrupt values containing literal newlines** vault/1password/extensions/vaults/onepassword.ts:254 Values with embedded newline bytes are passed as editField=secretValue in a Deno.Command arg. Some op CLI versions truncate values at the first newline in field-assignment mode. This is pre-existing behavior (not introduced by this PR), but the new template path would handle newlines correctly -- the routing heuristic (checking for double-quote) does not catch this case. ### Verdict **PASS** -- The changes are a targeted fix for double-quote handling in put values, correctly routing them through a template file. The CI changes are appropriate (adding needed Deno permissions, expanding change detection). The two medium findings are real edge cases but unlikely to cause data loss in typical usage. The field-matching issue (Medium 1) is worth fixing before this code sees wider adoption, but it does not block this merge.

stack72 merged commit 58f2bc165e into main 2026-06-04 23:52:39 +00:00

stack72 deleted branch fix/556-1password-put-double-escapes-json 2026-06-04 23:52:39 +00:00

stack72 referenced this pull request from a commit 2026-06-04 23:52:41 +00:00

fix(vault/1password): use JSON templates for put when value contains quotes (#33)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

swamp-club/swamp-extensions!33

No description provided.