Avoid disclosing config file contents in parse error messages#2261
Open
jiafatom wants to merge 4 commits into
Open
Avoid disclosing config file contents in parse error messages#2261jiafatom wants to merge 4 commits into
jiafatom wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request hardens GenAI config parsing error handling to avoid leaking contents of genai_config.json via exception messages, while still identifying the failing file path for diagnostics.
Changes:
- Sanitizes
ParseConfigparse-failure exceptions by removing the JSON parser’s raw message (which may echo file contents). - Adds a C API regression test ensuring parse errors mention
genai_config.jsonbut do not include a “secret” JSON key from the file.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/config.cpp | Replaces parse-error rethrow to omit the JSON parser’s message and only include a stable prefix + file path. |
| test/c_api_tests.cpp | Adds CAPITests.ConfigParseErrorDoesNotLeakContent to assert the error message does not echo config contents. |
When parsing genai_config.json fails, ParseConfig rethrew the raw JSON parser message, which embeds verbatim content from the file (such as JSON key names). If the config path is influenced by untrusted input, this could disclose contents of the targeted file through the error message. Keep the file path in the error so the failing file can still be identified, but no longer echo the parser's message content. Add CAPITests.ConfigParseErrorDoesNotLeakContent verifying the error names the file but does not include a key from the file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Use a unique per-test temp directory (timestamp suffix) to avoid collisions/flakiness across parallel runs; drop the initial remove_all. - Do not assign the unused OgaConfig::Create result, which could trip MSVC /W4 /WX unreferenced-local-variable warnings. - Assert the error contains the "Error encountered while parsing" prefix so the test actually exercises the parse-error path rather than an unrelated file-open failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
std::filesystem::remove_all can throw on transient filesystem/permission errors. Since this is only test cleanup, use the error_code overload so a cleanup failure cannot make the test flaky or fail for the wrong reason. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jiafatom
force-pushed
the
fix/config-parse-error-info-disclosure
branch
from
5ea0cb2 to
a14df15
Compare
Per reviewer feedback, retain the JSON parser's message (which identifies the failing key/token) in debug builds to aid diagnosis, while still omitting it in release builds to satisfy the MSRC no-content-disclosure requirement. Gate the test's no-leak assertion under NDEBUG accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Description
When
genai_config.jsonfails to parse,ParseConfig(insrc/config.cpp) rethrew the raw JSON parser message. That message can embed verbatim content from the file being parsed — for example, a JSON key name (Unknown value "<key>"). If the config path is influenced by untrusted input, this echoes contents of the targeted file back through the error string.Change
ParseConfig, on a JSON parse failure, keep the file path in the error message (so the failing file can still be identified) but no longer append the parser's raw message content.CAPITests.ConfigParseErrorDoesNotLeakContent(no model required): writes agenai_config.jsoncontaining an unknown key, callsOgaConfig::Create, and asserts the resulting error namesgenai_config.jsonand contains the parse-error prefix, but does not contain the key from the file. Uses a unique temp directory to avoid parallel-run collisions.Scope / non-goals
This intentionally does not alter path handling. Accepting an arbitrary filesystem path is the intended behavior of
OgaCreateConfig/OgaCreateModel; validating which paths are permissible is the responsibility of the calling application, and adding path/..//UNC restrictions here would break legitimate use cases (relative paths, models on network shares).Testing
Built and ran the affected tests:
Replaces #2258 (recreated as a non-fork branch so CI can run).