Skip to content

fix: do not log raw json value in json validator error - #258

Open
vergiLgood1 wants to merge 1 commit into
af:mainfrom
vergiLgood1:fix/json-error-leaks-value
Open

vergiLgood1 wants to merge 1 commit into
af:mainfrom
vergiLgood1:fix/json-error-leaks-value

Conversation

@vergiLgood1

Copy link
Copy Markdown

Summary

The json() validator includes the entire raw value in its error message:

throw new EnvError(`Invalid json: "${x}"`)

Environment variables parsed as JSON often contain secrets — API tokens, service-account credentials, connection configs. When a JSON env var fails to parse, that raw value currently ends up in the console/log output via the error report.

This changes the error to "Invalid json", mirroring the change already made for the url validator in 1f67e8d ("Prevent logging of potentially sensitive data in url validator"):

// Not logging the json value as it could contain sensitive data
throw new EnvError(`Invalid json`)

Behavior change note: the error message text changes from Invalid json: "<value>" to Invalid json. No tests asserted the previous message, and debugging value content can still be done locally by inspecting the variable itself.

Changes

  • src/validators.ts — stop including the raw value in the error (same pattern as the url validator)
  • tests/validators.test.ts — regression test: an unparseable JSON value containing a secret produces "Invalid json" with no trace of the raw value

Verification

  • bun run ts ✅
  • bun run lint ✅
  • bun run test:ci ✅ (67 tests)
  • bun run coverage ✅ (functions 100%, lines 98.85%)

The raw value could contain sensitive data (API keys, tokens, service
credentials) and would end up in logs via the error output. Follows
1f67e8d, which made the same change for the url validator.
@af

af commented Oct 22, 2026

Copy link
Copy Markdown
Owner

Hi,

Thanks for taking a stab at this, I agree that secret handling is something that should be improved, but I don't think just handling this case in the json field is the way forward. Secrets are probably most commonly used in the str validator, so that one would also need to have the same safeguards.

I have been thinking (no implementation yet) about supporting a mask argument like str({ mask: 'last4' }), so that any logging of the corresponding data would log a masked value instead. Maybe it'd make sense to support it for all fields, not just str and json. I think that'd be possible using Proxy traps, but haven't confirmed yet. You'd get the protection, but still have a bit of inspectability for debugging.

Thoughts?

@vergiLgood1

Copy link
Copy Markdown
Author

Thanks for the thoughtful feedback!

I agree that addressing this only in json() would leave the same issue in other validators, especially str(). A more general masking approach could provide more consistent protection while preserving some debugging visibility.

The mask option sounds like a promising direction. I think it would also be worth considering how masking behaves across error messages and logging, particularly if implemented through proxies.

Would you be open to me exploring and implementing this more general masking approach?

@af

af commented Oct 23, 2026

Copy link
Copy Markdown
Owner

Sure, I would appreciate help with the R&D on that idea. It may require some iterations before we land on a shippable implementation, but definitely take a crack at it and we'll see how it looks/feels to use

@vergiLgood1

Copy link
Copy Markdown
Author

Thanks again for the direction on this. I gave the general masking idea a shot and opened a draft to show it: #260 (stacked on top of this one).

One thing I came across while exploring the Proxy route: Proxy traps only work on objects, not primitives (new Proxy('x', {}) throws), so it can't cover str, which is where you mentioned secrets mostly live. I ended up rendering masked values at the error-message boundary instead.

It's still a draft and I'm not at all sure it's the shape you had in mind, so I'd really welcome your thoughts whenever you get a chance. If you'd prefer to keep this PR focused on the json fix, I'm happy to leave the two separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants