A project name holding a slash crashes the app when you tap Next - #192
Merged
Merged
Conversation
A project name holding a slash, a backslash or a dot component is
refused by StoragePaths.isValidProjectName, so getTargetFile() and
getProfileFile() both return null. HTTrackActivity.unserialize() passed
that null straight to OptionsMapper.unserialize, which called
profile.exists() on it. Tapping Next crashed the app. This is Play's
top crash cluster: 21 users, 32 reports.
The read half of unserialize moves to ProfileFormat.rawFields, which
refuses a null file with the same IOException("no such profile") an
absent file already got. The caller in validatePane already swallows
that, and it still rejects the name a few lines later, so the
traversal defence is untouched.
The extraction is what makes the guard testable: OptionsMapper's own
static initializer builds android.util.Pair values, which the stub
android.jar refuses, so no unit test can call the static
unserialize. The nested ProfileFormat class initializes on its own.
onShowLogs had the same shape of bug and is fixed alongside.
Closes #185
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>
unserialize's local was called raw, but rawFields two lines above is what actually returns the undecoded values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The guard matched on the prefix "profile.", so re-introducing `new FileReader(profile)` passed it. It now counts every mention of the parameter in the block, reads the source with comments and string literals blanked, and fails loudly when its anchor moves. rawFields is one side of the decode boundary, so a value carrying %% and %09 pins that it hands the escapes back untouched. profileDecode is private and unreachable from a test, so the other side is pinned by source text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Both openers were noun phrases, and the test one had its qualifier so far from what it qualifies that the sentence did not survive one read. Also drop "stat'ing", which reads as a verb only if you already know the system call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
#193 rewrote onShowLogs to read the log through LogTail, and its version already returns early on a null file, so the one-token guard this branch added there is dropped. The OptionsMapper change and ProfileReadTest are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.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
1 participant
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.
Closes #185
Type a project name holding a slash, such as a pasted site address, then tap Next, and the app crashes.
StoragePaths.isValidProjectNamerefuses the name, sogetProfileFile()returns null, andOptionsMapper.unserializecallsexists()on it. Play counts 21 users and 32 reports.The read half of
unserializemoves toProfileFormat.rawFields, which refuses a null file with the sameIOException("no such profile")an absent file already got.validatePanealready catches that, and it still rejects the name a few lines later, so the path-traversal defence is unchanged.The move is what makes the guard testable:
OptionsMapper's static initializer buildsandroid.util.Pairvalues, which the stubandroid.jarrefuses.ProfileReadTestthrows a NullPointerException without the fix.