Check that sensitive values can actually be redacted - #1054
Conversation
|
The changes in this PR will be included in the next version bump.
|
📦 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 | 9ef8eac | Commit Preview URL Branch Preview URL |
Sep 01 2026, 08:42 PM |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The proxy path drops the new acknowledgement before evaluating its startup warning, so the documented opt-out does not work there. The warning also recommends a setting that attached @proxy items override.
Reviewed changes in 264db5fa, covering the short-sensitive-value rule, decorator parsing and validation, proxy startup reporting, serialization, tests, docs, and release metadata.
- Validation warning: Sensitive non-empty values shorter than 12 characters now enter warn-state, with
allowShortValueas an acknowledgement. - Proxy diagnostics: Proxy startup detects short managed values and reports their keys before constructing the child view.
- Documentation and fixtures: The decorator and proxy docs explain collision risk, while incidental short test values were lengthened and OTP assertions permit warnings.
azure/gpt-5.6-sol ｜ 𝕏
…text Redaction is substring replacement with no token boundary, so it cannot tell a leaked secret from ordinary text that happens to match it. Nothing guarded against this: every sensitive value was registered for redaction regardless of length, so a short one (an org slug, an account id, a dev password) gets rewritten wherever it appears, corrupting console output and proxied responses. Adds an advisory validation warning surfaced by varlock load, plus @sensitive={allowShortValue=true} for secrets that are short by nature (an OTP, a PIN) where the collision risk is real but there is nothing to fix.
getItemSummary and explain both guarded redaction on _.isString, so a sensitive value that coerced to a number (PIN=987654) fell through and printed in cleartext, while its quoted equivalent was masked. Masking now follows the item's sensitivity rather than its inferred type. load.command already had a [REDACTED] fallback and was unaffected.
173d170 to
311a15b
Compare
theoephraim
changed the title
There was a problem hiding this comment.
Caution
The new getItemSummary() redaction remains bypassable for explicitly coerced sensitive values, which leaves the original secret visible in CLI output.
Reviewed changes since the prior Pullfrog review, including the rewritten proxy-warning scope and the new non-string display redaction:
- Proxy warning scope: Removed the separate proxy startup scanner and its proxy-specific remediation path.
- Typed display redaction: Added
redactSensitiveDisplayValue()to mask non-string sensitive values before rendering them. - CLI call sites: Applied the helper to resolved values in item summaries and to resolved and raw values in
explain. - Regression coverage: Added focused helper and item-summary tests for inferred numeric, string, boolean, empty, and public values.
- Release metadata: Expanded the changeset to include the non-string redaction fix.
azure/gpt-5.6-sol ｜ 𝕏
… sensitive, and reject sensitive numbers
theoephraim
changed the title
There was a problem hiding this comment.
Caution
The expanded validation still permits a sensitive composite value that runtime redaction cannot protect, and boolean sensitivity now disagrees between generated types and the resolved graph.
Reviewed changes since the prior Pullfrog review, including the expanded sensitive-value rules and the fix for coerced raw-value disclosure:
- Expanded validation: Added hard failures for extremely short values, explicit numeric sensitivity, explicit sensitivity on
@currentEnv, and public values containing sensitive strings. - Adjusted inferred sensitivity: Demoted booleans to non-sensitive and changed numbers inherited from
@defaultSensitivefrom errors to warnings. - Covered composite values: Added per-element length checks and shared recursive value collection.
- Closed CLI disclosure: Redacted the pre-coercion value in
getItemSummary()and added a regression test for that suffix. - Updated user guidance: Documented the new validation rules and adjusted affected fixtures.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review, covering the fixes for the two remaining sensitivity-model mismatches.
- Covered composite leaves: Added recursive detection of non-string leaves so explicitly sensitive composites fail and implicitly sensitive composites warn when runtime redaction cannot protect them.
- Aligned type generation: Mirrored boolean demotion in
getTypeGenInfo()while preserving the existing@proxysensitivity override. - Added regression coverage: Exercised numeric array and record validation plus generated metadata for an implicitly sensitive boolean.
- Updated documentation: Documented how mixed and numeric composite values follow scalar redaction rules.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
Important
The shared plugin helper can accept an advisory warning as a generic expected error, allowing plugin resolver regressions to pass unnoticed.
Reviewed changes since the prior Pullfrog review, covering the plugin fixture and shared test-helper adjustments.
- Lengthened Dashlane fixtures: Replaced three short secret values so the tests no longer trigger the new sensitive-value warning.
- Allowed advisory warnings: Filtered item warnings from successful
pluginTest()expectations while retaining exact resolved-value assertions and failures for non-warning errors.
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: |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review, covering the shared plugin test-helper correction.
- Filtered expected errors: Reused the non-warning error list for generic and specific expected-error assertions, preventing advisory warnings from satisfying an expected resolver failure.
- Preserved success checks: Continued rejecting non-warning errors and comparing successful resolved values exactly.
azure/gpt-5.6-sol ｜ 𝕏
…ior, and make the length floor absolute
There was a problem hiding this comment.
Important
The hard floor is implemented correctly, but the public option reference still promises behavior that the new commit explicitly removed.
Reviewed changes since the prior Pullfrog review, covering the revised boolean and minimum-length behavior.
- Preserved boolean metadata: Replaced automatic boolean demotion with errors for explicit sensitivity and warnings for inherited sensitivity, leaving static and dynamic behavior unchanged.
- Made the redaction floor absolute: Lowered the hard floor to three characters and prevented
allowShortValuefrom acknowledging one-character or two-character values. - Expanded regression coverage: Added assertions for the hard floor, the acknowledgement boundary, and boolean type-generation parity.
- Updated release guidance: Revised the changeset and most decorator guidance for the new behavior.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The previously reported
allowShortValuedocumentation mismatch remains open.
Reviewed changes since the prior Pullfrog review, covering the expanded @currentEnv validation behavior.
- Warned on inherited sensitivity: Added an advisory warning when
@defaultSensitivemakes the@currentEnvitem sensitive, while retaining the error for an explicit@sensitivedecorator. - Avoided duplicate diagnostics: Recorded type and
@currentEnvplausibility findings so the generic short-value warning does not stack on top of a more specific message. - Updated guidance: Aligned the decorator documentation and release note with the explicit versus inherited severity split.
- Added regression coverage: Verified explicit, inherited, and opted-out
@currentEnvsensitivity states.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
Important
The broadened containment check now reports collisions for scalar values that runtime redaction never registers.
Reviewed changes since the prior Pullfrog review, covering the expanded containment rule.
- Broadened collision detection: Matched sensitive values from three through eleven characters inside public values and reported those matches as advisory warnings rather than leak errors.
- Updated guidance: Revised the decorator reference and release note to explain the length-based severity split.
- Added regression coverage: Verified that an acknowledged short string inside a public value warns while unrelated public values remain valid.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The two previously reported concerns remain open.
Reviewed changes since the prior Pullfrog review, covering the containment-severity adjustment.
- Downgraded containment findings: Changed public values containing sensitive strings from length-dependent errors or warnings to advisory warnings at every supported match length, with updated tests, documentation, comments, and release notes.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
Important
The scalar collision false positive is fixed, but composite serialized values can still be rewritten without the promised containment warning.
Reviewed changes since the prior Pullfrog review, covering the leaf-classification consolidation and documentation cleanup.
- Separated leaf classes: Replaced the previous helpers with
collectLeaves(), which distinguishes redactable string leaves from non-string leaves. - Restricted containment candidates: Stopped treating sensitive scalar numbers and booleans as runtime redaction matches, with regression coverage for a numeric port inside a public URL.
- Clarified user guidance: Moved the full sensitive-value rules into the secrets guide and documented that
allowShortValuecannot suppress the hard floor. - Simplified diagnostics: Consolidated explicit and inherited plausibility reporting without changing sensitivity or dynamic metadata.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
Important
The new inherited-sensitivity warning is implemented and tested, but one public option description still states the previous behavior.
Reviewed changes since the prior Pullfrog review, covering the revised minimum-length severity policy.
- Relaxed inherited sensitivity: Changed one-character and two-character values inherited from
@defaultSensitivefrom errors to warnings while retaining errors for explicit@sensitive. - Preserved acknowledgement limits: Kept
allowShortValueunable to suppress the minimum-length diagnostic in either severity branch. - Updated primary guidance: Revised the secrets guide and release note to describe the explicit versus inherited split.
- Expanded regression coverage: Asserted that explicit short values fail, inherited short values warn, and acknowledged explicit short values still fail.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
Important
The composite containment fix is incomplete for public composites, and the changed diagnostics currently break the focused test suite.
Reviewed changes since the prior Pullfrog review, covering the serialized composite containment fix and related diagnostic wording.
- Matched serialized composites: Added each sensitive composite's process environment string to containment candidates, added regression coverage for a public scalar containing that form, and clarified non-string leaf diagnostics.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review, covering the final fixes for documentation, diagnostic coverage, and symmetric composite containment.
- Updated diagnostic coverage: Aligned both explicit and inherited non-string composite assertions with the revised messages.
- Corrected option guidance: Documented the explicit error versus inherited warning split for values under three characters.
- Matched public composite serialization: Included public composite process-environment strings in containment matching and added a regression test for identical sensitive and public numeric arrays.
azure/gpt-5.6-sol ｜ 𝕏
1 participant
Redaction is substring replacement over strings. Plenty of values can be marked sensitive without that protection ever meaningfully applying, and nothing checked for it. Because
@defaultSensitive=trueis the default, most of these get swept in without anyone writing@sensitiveat all.Rules
@sensitive@defaultSensitive@currentEnvitemThe one guarantee across all of it: nothing an item inherited from
@defaultSensitivecan fail a load in this release. Only an explicit@sensitivecan, and the escalations noted below belong in a breaking release.Builtin
VARLOCK_*items are exempt.@sensitive={allowShortValue=true}opts out of the under-12 warning, for secrets that are short by nature (a one-time code, a PIN) and cannot be lengthened. It does not opt out of anything else.Why each one lands where it does
Length. An
ACME_ACCOUNT=acmecorewrites everyacmecoin console output and proxied response bodies, corrupting content that was never a secret. Under 3 characters there is no output that survives the redaction at all, so that one cannot be acknowledged: the ack records that a collision risk was read and accepted, and at one or two characters it is not a risk, it is a certainty. It follows the same explicit/implicit split as the type rules below, so an inheritedAWS_REGION=uswarns rather than failing CI.Type rules split by who asked. An explicit
@sensitiveis a claim that the value is a secret, and for these types that claim is not being honored, so it fails.@defaultSensitive=trueis not a claim, it is what a hand-written schema inherits, and it sweeps in every port, timeout and feature flag in the file. Those warn instead.true/falsein logs and response bodies.collectSensitiveStringsonly collects string leaves, andenvStris serialized only for object values. Making it a string also keeps the leading zeros and precision past 2^53 that a number drops.@type=array(number)has a compositecoercedTypewhile every element is a number, and only string leaves are registered.@currentEnvitem drives conditional imports andforEnv()and is echoed by most tools in the stack, so it is a mode name rather than a secret.Whichever branch fires, it replaces the generic length message rather than stacking a second one on the same item.
Nothing here changes an item's sensitivity. Demoting a boolean to non-sensitive is the eventual answer, but sensitivity is what
@dynamicdefaults to, so demoting would also flip the item to@staticand make its value eligible to be inlined into a build. That is a silent runtime behavior change that no error would announce, so it belongs in a breaking release rather than here.Containment is reported on the non-sensitive item, since marking it sensitive is the fix. It is a warning for now and an error in a breaking release: it is the only rule here that depends on how two resolved values relate, so whether it fires depends on what a given machine's
.envcontains, and as a hard failure it could pass locally and fail in CI.The length of the match still picks the advice, because it is catching two things. A long value inside a public one is a secret genuinely carried into public output; that overlaps heavily with leak detection, which already fails at runtime once the containing value reaches a response, and it also covers paths leak detection misses (build-time inlining, generated types). A short value is more likely a collision than a leak, but that collision is exactly what makes redaction rewrite the containing value everywhere it appears. The short case fires even when
allowShortValuesilenced the generic short-value warning, because here the collision is confirmed rather than hypothetical.Also fixed
Two disclosure bugs in CLI output, both pre-existing on
main:getItemSummaryandexplainguarded redaction on_.isString, so a sensitive value that coerced to a number printed in cleartext while its quoted equivalent was masked. Masking now follows the item's sensitivity rather than its inferred type.getItemSummarystill printedresolvedRawValueunredacted in its< coerced from ...suffix, so a sensitive@type=arrayor@type=jsonitem showed its full pre-coercion value in cleartext.Notes
src/lib/sensitive-value.ts, since log redaction and proxy response scrubbing share the problem.["averylongsecret", "x"]puts"x"in the map even though the joined form is long. Runtime also registers a composite's serialized form, and injects public composites as that form, so the containment check includes it on both sides: a sensitivePORTS=[3000,3001]has no redactable leaves, but its joined3000,3001still rewrites a public value that contains it, including a publicarray(number)that serializes to the same string.validationErrors, so a warned item'svalidationStateflips fromvalidtowarn.isValidstill returns true, so nothing fails.@defaultSensitive=falseor longer values, rather than loosened assertions.scan.commandhas the same_.isStringassumption when collecting values to scan files for. That is a detection gap rather than a disclosure, and changing it would affect scan's false-positive rate, so it is left alone here.varlock load, which prints every item, but not onrunorproxy, wherecheckForConfigErrorsprints warn-state items only alongside failures. Reaching those paths needs a design that does not write to stderr on success, so it is out of scope here.