Skip inert placeholders instead of blocking, and give each substitution target its own budget - #1046
Merged
Merged
Conversation
…cking A matched rule's placeholder appearing in a surface the rule has no substitution targets on (typically the request body under the header-only default) previously blocked the whole request with blocked-location. That bricked 'varlock proxy run -- claude': once the agent quotes its own placeholder, every subsequent request carries it in the transcript body. Blocking added no protection since the proxy never substitutes there and an unswapped placeholder is inert. Now such occurrences are carried through unsubstituted, the request is forwarded, and a new carried-placeholder audit event records the item key and where the placeholder sat. Substitution is scoped per surface (per header, path, query, body) so carried occurrences stay literal. Fail-closed blocking is kept where it still protects: - occurrences at allowed targets exceeding maxOccurrences (the cap now counts only allowed-target occurrences, not carried ones) - off-path/off-param occurrences within a body/query surface the rule targets via body:<path>/query:<param> (blind replace can't skip them) - the route-mismatch helpful-failure block, unchanged
Contributor
|
The changes in this PR will be included in the next version bump.
|
Contributor
📦 Bundle size
dist/ only; native binaries are versioned separately and not counted here. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
varlock-website | 6199b45 | Commit Preview URL Branch Preview URL |
Aug 30 2026, 08:51 PM |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
The per-surface rewrite has one cross-item placeholder collision that can substitute a real secret into text that should remain carried.
Reviewed changes across the credential-proxy guard, request rewriting, audit output, tests, docs, and release note.
- Surface-scoped substitution: Untargeted placeholder occurrences are carried unchanged while targeted occurrences remain subject to placement and cardinality guards.
- Audit visibility: Carried placeholders are attached to runtime activity, persisted as correlated audit events, and rendered by the CLI.
- Regression coverage: Unit and TLS tests cover carried body/header/path/query occurrences, retained fail-closed cases, occurrence limits, and the Claude transcript scenario.
- User-facing contract: Proxy guides and decorator reference now describe carry-through behavior and the remaining blocking cases.
azure/gpt-5.6-sol ｜ 𝕏
varlock
@varlock/native-helper-darwin
@varlock/native-helper-linux-arm64
@varlock/native-helper-linux-x64
@varlock/native-helper-win32-x64
commit: |
'Skipped' states what the proxy did (substitution skipped this occurrence) and needs no design context to parse; 'carried' only made sense inside the design discussion. Renames the audit line type, the skippedPlaceholders activity field, the guard result field, the live log marker, and the docs wording. No behavior change.
Contributor
There was a problem hiding this comment.
Important
The terminology-only delta introduces no new issues, but the existing overlapping-placeholder finding remains unresolved, so this PR is not yet ready to merge.
Reviewed changes since the prior Pullfrog review covered the terminology rename throughout the proxy feature.
- Renamed skipped occurrences: Replaced the
carriedterminology withskippedacross runtime result types, activity payloads, audit event names, CLI output, tests, docs, and the release note. - Preserved behavior: Kept the substitution guard and per-surface rewriting behavior unchanged; the focused proxy suites pass all 79 tests.
azure/gpt-5.6-sol ｜ 𝕏
The per-surface rewrite filtered the item list down to the surface's own targets before replacing, which defeated the longest-placeholder-first protection when two placeholders overlap. With a body-targeted 'vlk_x' and a header-only 'vlk_x_1', a body containing only 'vlk_x_1' was classified as skipped but came out as 'REAL_SHORT_1': bytes the guard promised to leave inert were rewritten, and the wrong secret was spliced in. Overlapping placeholders are reachable in practice, since ensureUnique resolves a collision by appending '_1' and explicit @Placeholder values can overlap freely. Replace the per-surface blind replace with a single leftmost-longest scan (substitutePlaceholdersInSurface) that matches against every managed placeholder but only swaps the keys allowed in that surface. A shorter placeholder can no longer match inside its longer sibling, and non-allowed matches are re-emitted verbatim. Matched regions are never rescanned, so a real value containing another placeholder's text is also left alone (the old split/join chain would have rewritten it). Reported by pullfrog on #1046.
Contributor
There was a problem hiding this comment.
Important
The output corruption is fixed, but guard classification still disagrees with the new matcher for overlapping placeholders and can reject requests whose occurrences would all be skipped.
Reviewed changes since the prior Pullfrog review covered the overlapping-placeholder fix and its regression coverage.
- Preserved skipped placeholders: Added a global leftmost-longest surface matcher that leaves disallowed longer placeholders unchanged while substituting independently occurring allowed placeholders.
- Prevented replacement rescans: Ensured inserted real values are not scanned again for other placeholder text.
- Expanded regression coverage: Added focused unit cases and a TLS end-to-end case for placeholders that overlap across substitution surfaces.
azure/gpt-5.6-sol ｜ 𝕏
The global occurrence cap was redundant with substituteIn and pointed users the wrong way. An author who wrote substituteIn=[header:authorization, body:signature] had already declared both places legitimate, then got blocked anyway until they also set maxOccurrences=2 (our own docs shipped that example). The escape hatch was 'raise the number', which loosens every other spot at once, the same corrosive-remediation shape as body:*. Each substituteIn target is now worth exactly one substitution per request, so listing a target is what grants it an occurrence. Two named targets get one each with nothing to configure; a repeat at the SAME target still blocks, since that is where the which-copy-is-real ambiguity lives. The bare header target stays a single target covering all headers, so two headers under the default still block, and the fix is to name them (substituteIn=[header:authorization, header:x-api-key]), which tightens the rule instead of loosening it. Occurrences attribute to the broadest target that allows them, so declaring both header and header:x cannot split one header's copies across two targets and double the budget. maxOccurrences is removed from the schema and now raises a SchemaError naming the replacement (checked before the unknown-option sweep so the message explains rather than just rejecting), in @Proxy(...) and in rules=[{...}] entries alike.
theoephraim
changed the title
Contributor
There was a problem hiding this comment.
Important
No new issues were found in the incremental delta, but the existing open guard-classification finding remains unresolved and must be addressed before merging.
Reviewed changes since the prior Pullfrog review covered the replacement of the global occurrence cap with per-target substitution budgets.
- Removed
maxOccurrences: Rejected the removed option with migration guidance in top-level and nested proxy rules, and updated the public types and documentation. - Added per-target budgets: Granted one substitution to each distinct
substituteIntarget while preventing overlapping broad and narrow targets from multiplying the budget. - Expanded regression coverage: Added unit and TLS cases for same-target repetition, separately named targets, broad-target precedence, and schema migration errors.
azure/gpt-5.6-sol ｜ 𝕏
checkSubstitutionGuards counted each item's placeholder with an
independent substring search while substitution matched
leftmost-longest across all placeholders. The two disagreed whenever one
placeholder contains another, and the guard was the stricter of the
pair, so it blocked requests in which nothing would have been
substituted at all: with body-targeted 'vlk_x' and header-only
'vlk_x_1', a body of {note: vlk_x_1, other: vlk_x_1} was rejected as an
off-path body occurrence of 'vlk_x' even though substitution leaves both
values untouched. It also mislabeled the audit, reporting the shorter
item as injected and never recording the longer one as skipped.
Extract findPlaceholderMatches as the single source of truth for where
placeholders are, and drive both the guard and
substitutePlaceholdersInSurface from it. Each surface is tokenized once
against every managed placeholder and the guard reads per-item counts
out of that, so occurrence caps, location violations, injectedKeys and
skipped metadata all describe exactly what substitution will do. The
runtime now passes the full managed list, not just the injected items.
Tokenizing per surface instead of per item also drops the repeated body
parse and query parse that ran once per injected item.
Reported by pullfrog on #1046.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review covered the guard-classification repair for overlapping placeholders.
- Unified placeholder matching: Reused the global leftmost-longest matcher for guard classification and substitution so both paths assign identical token boundaries.
- Corrected guard accounting: Derived occurrence budgets, location checks, injected keys, and skipped metadata from per-surface matches against every managed placeholder.
- Added targeted regressions: Covered longer skipped placeholders in body and header surfaces, independent shorter matches, off-path classification, and violation attribution.
azure/gpt-5.6-sol ｜ 𝕏
The section had grown to explain the same rationale three times (the mail-API attack in the intro, again under substituteIn, again under the body rules) and spread the per-target budget over three paragraphs. Cut it by a third with no loss of substance: every target, the full header denylist, body path syntax, the body:* caveat, both fail-closed cases, and the audit event and links are all still there.
Contributor
There was a problem hiding this comment.
ℹ️ No critical issues. One documentation accuracy fix is suggested inline.
Reviewed changes since the prior Pullfrog review covered the substitution-surface documentation rewrite.
- Tightened the substitution guide: Consolidated the skipped-placeholder, body-path, per-target budget, and remaining fail-closed behavior into a shorter explanation.
- Aligned reference wording: Simplified the decorator and auditing references while preserving the public configuration contract.
azure/gpt-5.6-sol ｜ 𝕏
proxy-tls.test.ts is described as covering the cert-trust, CONNECT and streaming mechanics that plain-http tests can't reach, but substitution behavior had grown to 8 of its 15 tests and roughly half its lines. Those tests live there only because the proxy refuses to inject into a cleartext connection, so anything that substitutes a secret needs the MITM harness. That's a reason to share the harness, not to file the tests under TLS. Extract the harness (stub upstream + CONNECT tunnel + raw send/read) into mitm-test-harness.ts, following the existing env-graph test/helpers convention, and move the substitution tests to proxy-substitution.test.ts. proxy-tls.test.ts keeps the transport and TLS invariants plus the two policy tests. No test bodies changed; the DNS-poison case now mints its wrong-host leaf from harness.upstreamCa(). The harness is not a build entry, so it stays out of dist.
The docs promised one event for every skipped occurrence. The audit log emits one line per item per request, listing the parts of the request its placeholder appeared in, so two copies in the body are one line with locations=[body], not two lines. Reported by pullfrog on #1046.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review covered the audit-cardinality documentation correction.
- Corrected skipped-event cardinality: Described
skipped-placeholderlogging as one aggregated event per skipped item and request, with all affected request locations recorded on that event. - Aligned auditing guidance: Updated both the substitution-surface guide and audit reference to match the runtime and audit tests.
azure/gpt-5.6-sol ｜ 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes to the proxy's substitution-surface guard: stop blocking requests where blocking bought nothing, and replace the global occurrence cap with a per-target budget.
1. Skip inert placeholders instead of blocking
The guard previously blocked any request where a matched rule's placeholder appeared in a surface the rule doesn't substitute in (typically the body under the header-only default), with a `blocked-location` 403. This bricked `varlock proxy run -- claude`: the moment the agent quotes its own placeholder (echoes the env var, reads a schema containing it), every subsequent API request carries it in the transcript body and gets blocked until `/clear`. The same shape hits any agent flow that echoes seen text into a request body on an allowed host (posting an issue quoting the schema, telemetry uploads, webhooks). The block added no protection: substitution never happens in an untargeted surface, and an unswapped placeholder is inert.
Those occurrences are now skipped: forwarded unsubstituted, with a `skipped-placeholder` audit event recording the item key and where the placeholder sat (also shown as `skipped: KEY (body)` in the live request log), so probing stays visible.
Substitution is scoped per surface (per header, URL path, query, body). Each surface swaps only the items whose targets cover it, while still matching against every managed placeholder, so an overlapping placeholder can't be clobbered from inside its longer sibling (`ensureUnique` resolves a collision by appending `_1`, and explicit `@placeholder` values can overlap freely). Skipped occurrences reach the upstream byte-for-byte unchanged.
2.
maxOccurrencesremoved, replaced by one substitution per targetThe global cap was redundant with
substituteInand pointed users the wrong way. An author who wrotesubstituteIn=["header:authorization", "body:signature"]had already declared both places legitimate, then got blocked anyway until they also setmaxOccurrences=2(our own docs shipped exactly that example). Its escape hatch was "raise the number," which loosens every other spot at once, the same corrosive-remediation shape asbody:*.Now each target listed in
substituteInis worth exactly one substitution per request, so listing a target is what grants it an occurrence:[header:authorization, body:signature]with one occurrence in each: allowed, nothing to configure.[header]with the placeholder inauthorizationandx-evil: still blocked. The bareheadertarget is a single target covering every header, so both copies bill to it, which is exactly where the which-copy-is-real ambiguity lives.[header:authorization, header:x-api-key]), so the remediation tightens the rule instead of loosening it.[body:*]with many copies: still blocked.Occurrences attribute to the broadest target that allows them, so declaring both
headerandheader:xcan't split one header's copies across two targets and double the budget.Setting
maxOccurrencesis now aSchemaErrornaming the replacement, checked before the unknown-option sweep so the message explains rather than just rejecting, in both@proxy(...)andrules=[{...}]entries.Blocking that stays
body:<path>/query:<param>, since the per-surface replace can't skip a stray occurrence without re-serializing the body or query.blocked-uninjected), unchanged.Since the default targets are header-only and body substitution is always opt-in (
body:<path>/body:*), the body is never a swap surface unless explicitly widened, which is what makes skipping safe by construction.Tests
proxy-substitution.test.ts): body echo under header-only default forwards with header substituted and body bytes untouched; off-path body occurrence with abody:<path>target still blocks; same-target duplication across two headers blocks; two named targets substitute in one request; overlapping placeholders on different surfaces round-trip without corruption (verified to fail against the pre-fix implementation); a simulated Claude transcript request round-trips with the skipped audit record.maxOccurrencesrejected with migration guidance, at the top level and inside arulesentry.Docs updated (proxy rules guide substitution-surface section, auditing section, decorator reference).
The end-to-end substitution tests moved out of
proxy-tls.test.tsinto their own file, sharing the MITM harness now extracted tomitm-test-harness.ts. They had grown to over half of a file meant for TLS transport mechanics, and were only there because injecting a secret requires a TLS connection. No test bodies changed in the move.