security: Tier 1 hardening (wrapper integrity, token redaction, log sanitization)#39
Open
MarketDataApp wants to merge 1 commit into
Open
security: Tier 1 hardening (wrapper integrity, token redaction, log sanitization)#39MarketDataApp wants to merge 1 commit into
MarketDataApp wants to merge 1 commit into
Conversation
…on, log sanitization) Addresses the Tier 1 findings from the SECURITY.md-scoped review: - Build integrity: pin the Gradle distribution checksum (distributionSha256Sum) and add gradle/actions/wrapper-validation to the PR and publish workflows so a tampered gradle-wrapper.jar cannot execute in CI (fixes the misleading "validates the wrapper jar hash" comment, which setup-gradle does not do). - Configuration.toString() redacts the API key via Tokens.redact instead of the record's default verbatim rendering (latent token leak if ever logged). - Sanitize untrusted API-response strings (errmsg, malformed date/timestamp cells) before embedding them in exception/log messages, preventing CR/LF log-forging and ANSI-escape terminal spoofing (new LogSafe helper). SHA-pinning of first-party actions was intentionally excluded (low value for GitHub/Gradle-owned actions; Dependabot + reputation suffice). Tests added for LogSafe and the Configuration.toString redaction.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
=========================================
Coverage 98.27% 98.28%
- Complexity 1043 1049 +6
=========================================
Files 123 124 +1
Lines 2606 2617 +11
Branches 301 304 +3
=========================================
+ Hits 2561 2572 +11
Partials 45 45
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
This was referenced Jul 2, 2026
Open
MarketDataDev03
approved these changes
Jul 6, 2026
| StringBuilder out = new StringBuilder(truncated.length()); | ||
| for (int i = 0; i < truncated.length(); i++) { | ||
| char c = truncated.charAt(i); | ||
| out.append(Character.isISOControl(c) ? ' ' : c); |
Collaborator
There was a problem hiding this comment.
NIT: Unfiltered non-ISO-control Unicode — LogSafe.sanitize doesn't neutralize U+2028/U+2029 (line/paragraph separators) or U+202E (RTL override); they survive and can break a line in some log viewers or enable bidi visual spoofing.
| if (raw == null) { | ||
| return ""; | ||
| } | ||
| String truncated = raw.length() > MAX_LEN ? raw.substring(0, MAX_LEN) + "…" : raw; |
Collaborator
There was a problem hiding this comment.
NIT: Truncation can split a surrogate pair — substring(0, MAX_LEN) may leave a lone surrogate when cutting; cosmetic (replacement char in the log), no security impact.
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
2 participants
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.
Tier 1 security hardening
Applies the Tier 1 findings from a security review scoped to
SECURITY.md(the five in-scope axes: credential handling, TLS, request injection, deserialization safety, supply-chain). Tier 1 = behavior-compatible for consumers, so per policy these are applied directly; a reviewer sign-off is still wanted here.Changes
1. Gradle build integrity (
gradle-wrapper.properties,pull-request.yml,publish.yml)distributionSha256Sum(verified againstservices.gradle.orgfor 9.6.1) so a tampered/MITM'd distribution zip is rejected on every./gradlewrun.gradle/actions/wrapper-validationto the PR gate and the publish gate so a tamperedgradle-wrapper.jarcan't execute — importantly, in the signed publish build that holds the GPG key + Central creds.setup-gradledoes not validate the wrapper jar hash by itself (the old comment claimed it did).2.
Configuration.toString()redacts the API key (Configuration.java)toString()prints every component verbatim, so any future in-packageLOGGER.info("config=" + config)would have leaked the raw token. Now redactsapiKeyviaTokens.redact; non-secret fields stay visible for diagnostics.3. Sanitize untrusted API-response strings before logging (
LogSafe.java+ 6 call sites)errmsgand malformed date/timestamp cells are attacker-influenced (untrusted response body). Copied verbatim intoParseErrormessages that consumers log, embedded CR/LF can forge log lines andESCcan spoof terminals. NewLogSafe.sanitizecollapses ISO control chars and caps length; applied inParallelArrays, the three envelope-error deserializers, and the threeMarketDataDatesparse paths.Deliberately excluded
actions/*,gradle/actions,codecov,dependabot); the value is low and Dependabot + reputation suffice. Discussed and dropped.Verification
./gradlew buildgreen: unit tests, Spotless, and JaCoCo coverage verification all pass.LogSafeTest(control-char/ANSI/truncation) and aConfiguration.toStringredaction test.Not in this PR — Tier 2 (need maintainer approval per policy)
Tracked separately (compatibility-affecting):
http://base-URL cleartext-token allowance, symbol path-segment..//traversal, unbounded response-body size cap, andRedirect.NEVER.