Repository navigation
Redact credentials from the engine log line and vet every zip entry name - #247
Merged
Merged
Conversation
Two CodeQL alerts in the app's own Java, the only two outside the vendored engine. java/sensitive-log: CrawlRun logged the whole engine argv through printArray, and that argv carries user-typed text verbatim. Both the URL list and the proxy address accept "user:password@", which the engine honours, so logcat held the credentials. CrawlArgv.printable blanks the userinfo out for the log line while the engine still receives the real argv. java/zipslip: buildResourceFile extracted R.raw.resources entry by entry with no check on the entry name. The bundle is generated at build time and APK-signed, so it is not attacker-controlled, but extraction targets getExternalCacheDir() and the app holds MANAGE_EXTERNAL_STORAGE, so a ".." entry would land anywhere on shared storage. The guard is two lines and survives the next submodule bump. StoragePaths.isInsideRoot holds the canonical-prefix comparison that isWritable already carried inline, so the check has one definition rather than a second copy. Each of the four guards was mutation-tested: dropping the zip check, weakening isInsideRoot to a bare startsWith, reverting CrawlRun to printArray, and removing the redaction each turn a distinct test red. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The review found the first regex leaked two forms the engine accepts. An empty user, "http://:pw@host/", did not match because the pattern needed one or more characters before the colon. A password containing '@', "http://bob:p@ss@host/", matched only up to the first '@' and left the rest in the log, while the engine reads the LAST '@' as the end of the userinfo (jump_identification_const in htslib.c, and its own selftest pins that). Redaction is now structural rather than a per-field shape. An option keeps its text, the -O1 target keeps its path, and every other argument has each run up to its last '@' blanked. Over-redaction only costs log detail, so a query string that happens to look like userinfo goes too. -%X was the largest remaining secret: it carries a whole header line, so a user putting "Authorization: Bearer ..." in Other headers had it logged verbatim. Its value is now hidden whole, by option name rather than by value shape. printable became redacted, because the old name read as a join helper at the call site, which is what made reverting it to printArray look free. Mutation-tested: replaceAll weakened to replaceFirst, the userinfo end moved back to the first '@', the header value left visible, and the option skip dropped each turn a test red. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The test matched only the isInsideRoot call, so two mutants kept it green: the answer assigned and dropped, and the guard moved below the FileOutputStream it protects. Either leaves the predicate correct and the extraction open. It now matches the refusal and asserts the guard precedes the write. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
xroche
enabled auto-merge (squash)
October 13, 2026 18:18
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.
These are the only two CodeQL alerts outside the vendored engine.
CrawlRunlogged the whole engine argv. A URL and the proxy address are free text, and the engine takes a password in either.-%Xcarries a whole header line. So logcat held credentials. The log line now blanks every userinfo run and hides a header's value whole, and the engine still gets the real argv. It redacts to the last@, not the first, because the engine ends the userinfo there and a password may contain one.The resource bundle is generated at build time and APK-signed, so nobody else can plant a
..entry in it. Extraction still targetsgetExternalCacheDir()and the app holdsMANAGE_EXTERNAL_STORAGE, so the guard is worth its two lines.Both alerts survive the fix, measured on this PR's own run. CodeQL wants a constant prefix for the path check, and
java/sensitive-loghas no sanitizer for redaction. So each needs dismissing as a false positive once this lands.