Repository navigation
Add a browser login step before the crawl - #253
Merged
Merged
Conversation
A site behind a login form or a CAPTCHA cannot be mirrored today, because the engine has no way to pass the gate. The user now passes it in a WebView inside the app, and the crawl reuses that browser's session. "Log in with a browser" on the address pane opens the first address in a WebView. On "Use this login" the screen queries every host the user reached and writes a Netscape cookies.txt into private storage. That path goes to the existing CookiesFile field (-%K), and the WebView's exact User-Agent to UserID (-F). It also ticks Accept cookies, because cookie_load() never reads the file otherwise. It drops the connection count to 2 and appends -*logout*, so the crawl does not end its own session. The window opens https only. The app declares no cleartext permission, so a WebView will not load http at all, and a password does not belong on cleartext either. Verified against the pinned engine 67d3a174 rather than its documentation. A file in this format makes the engine send the cookie on every request. The same crawl without -%K sends none, which is the control. The #HttpOnly_ form that browser extensions export is skipped, so this writer never emits it. Verified on an emulator (API 36, WebView 133). CookieManager.getCookie returns an HttpOnly cookie where document.cookie does not. The written file is mode 600. The login survives a rotation and a background recreation. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
xroche
marked this pull request as draft
October 18, 2026 09:43
A record passes two engine gates. cookie_load() reads each field into a buffer, and cookie_add() then refuses a field the jar cannot hold. The first round took the buffer sizes, which are far larger. So a name over 256 bytes or a value over 1024 was written and parsed, then dropped with a warning no Android user sees. The screen counted it as saved. A session cookie of that size therefore left the crawl logged out while the app said it was logged in. BrowserLoginEngineLimitsTest now reads both gates out of the pinned engine and asserts our constants are the tighter one. Restoring the old values turns it red. The whole-line check went with them, because three capped fields cannot reach the engine's line_max. The same test proves that, rather than leaving a branch nothing can enter. The review found more, all of it smaller. The connection count only narrows now. Integer.parseInt accepts an Arabic-Indic digit, which setText() can put in the field, but the option mapper emits -c only for ASCII digits. So the old code echoed back a value the mapper then dropped, leaving the crawl on the engine's default of four. It now uses the mapper's own test and returns a canonical value. isClean() and the byte counting were reinvented: OptionValues already had hasControlCharacter() and fitsBytes(). startUrl() now calls CommandlineTokens.urlTokens() instead of repeating its split and its option-token filter. The WebView is detached before destroy(), which leaks the window otherwise. forget() no longer dereferences a null file, reachable because finish() in onCreate still runs onStart and onResume. The explicit EdgeToEdge call is gone, because HTTrackApplication already fits every activity's insets. split() returns a pair per cookie instead of a flat alternating list. BrowserLoginPolicy is package-private with no licence header, matching the nine other Policy classes. Two tests could not fail. The Accept-cookies guard read the source with string literals blanked, so it could not see "0" in place of "1". A byte-counting assertion compared a length with a limit it could not exceed. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xroche
changed the title
A browser login left its cookies.txt behind in private storage when the project was deleted. CleanupActivity walks the mirror root only. So the session outlived everything the user could see of that project, and the Forget action was the only way left to reach it. The path moves to StoragePaths.loginCookiesFile, beside the rest of the path math. The cleanup and the hand-off now derive it the same way, rather than each building it. It refuses a name that is not one valid segment, as before, and now has tests for that. Dropping the call from deleteProjects turns the new guard red. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
From httrack 67510a96 a -%K jar is rewritten through <file>.<pid>.tmp and the previous one is kept as <file>.bak. Deleting only the name we wrote would leave the session in that .bak. The previous commit closed exactly that leak, and the engine's own save reopens it. The pinned engine does not do this yet, so the new loop finds nothing today. It is cleanup of files the engine will create, not an option the engine has to understand, so it is safe ahead of the pin. StoragePaths.isLoginSibling decides which names go. A bare prefix test looks right and is wrong. A project may legitimately be called "A.txt", whose own login file is "A.txt.txt", and that starts with project "A"'s "A.txt". So the match is the name, its .bak, or its .<digits>.tmp, and nothing else. Loosening it to a prefix turns the new test red. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reason is httrack #1982. With -%K the engine now saves its jar back to that file, never to path_log/cookies.txt. A browser login therefore stops being written into the mirror directory. On Android that directory is shared storage, readable by other apps on purpose. The old behaviour therefore put a live session where anything on the device could read it. The bump also changes the cookie limits, and the writer follows. The two per-field caps in cookie_add are gone. In their place is COOKIE_MAX_PAIR of 4096 and one joint test. A name may not exceed it, and a value may not exceed what the name leaves. So BrowserLoginCookies now spends one budget across the pair rather than capping each field. Reading those numbers out of the pinned source is what made this safe. BrowserLoginEngineLimitsTest went red on the bump until it was updated, and putting 1024 back turns it red again. Seven other engine commits ride along. The ones that reach this app are #1980, which loads the #HttpOnly_ lines a browser export writes, #1981, which keeps a Set-Cookie to its own domain and honours expiry, and #1986, which lets -%K read a pasted document.cookie string. #1985 adds html/login.html, which the Help anchors can point at later. The rest are a guide image, a theme fix and a self-test. Checked before taking it: HTTRACK_VERSIONID is still "3.50.5", so the versionName prefix stays aligned and versionCode is untouched. No engine source was added, removed or renamed, so Android.mk's explicit list still resolves. The suite passes at 618 tests, including the 99 assertions that read the pinned engine for help anchors, Accept kinds, winprofile keys and option ceilings. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xroche
marked this pull request as ready for review
October 19, 2026 15:27
The second header line stopped the engine saving its jar back over our file, which is the behaviour the pin bump was taken for. cookie_load counts a comment it does not recognise as a line a rewrite would lose, and htscore then leaves the file unchanged. Our line was not on that list, so every crawl discarded the jar it had built. Measured against a binary from the pin, with one line changed and nothing else. The old line leaves the rotated cookie out of the file and writes no .bak. A recognised line produces both. Both header lines are now spellings cookie_known_comment accepts. A test reads that list out of the pinned source, so the next wording cannot break it in silence. A second leak, from the same review. The engine stages the .bak through a temporary of its own, so it can leave <file>.bak.<pid>.tmp, and that file holds a whole jar. The sibling predicate allowed one suffix, not two, so a deleted project kept its session there. The suffixes now stack. Deleting a project also no longer turns on our own delete succeeding. A directory removed from outside the app reports failure, and its session has to go too. A repeated cookie name is no longer counted as saved and then dropped. getCookie lists a name once per scope, most specific first, and a record flattens every scope onto the host we asked about. The engine keeps the last of a collision, so the broader scope used to win. The writer now keeps the browser's first and counts the rest. Three tests could not see a real bug. Dropping Pattern.quote stayed green, though a project name may hold a regex metacharacter, so "A+" would have matched another project's file. The loader's own field buffers went unpinned when the cap model changed. And nothing watched the header comment at all. Each of those now reds when its guard is removed. Three smaller findings. Two stacked javadoc blocks had left isValidProjectName undocumented. A comment had lost the cleartext fact it carried. And the name clause of the pair test could never refuse on its own, because Java does not wrap the subtraction the engine guards against. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous round deduplicated a repeated cookie name and kept the browser's first, on the stated ground that getCookie lists the most specific scope first. That ground is wrong. RFC 6265 orders equal-length paths by creation time, earliest first, and domain specificity is not a sort key at all. Every record we write uses path "/", so two scopes of one name sort by age, and a session set during the login sorts LAST. Keeping the first therefore kept the older cookie and dropped the one carrying the login. The engine agrees with the last, and says so where it reads the file: "the last one wins, as curl reads it". So the writer now keeps the last value per name. That is both the newer cookie and the record the engine would have ended up with. Deduplicating at all is still necessary, for a reason the previous round did not have. A repeated line increments the same refused counter an unknown comment does. Writing both lines would therefore suppress the jar save-back, exactly as the old header line did. A superseded value is no longer counted in dropped. Nothing is lost that the engine would have kept, so reporting it to the user as left out was misleading. Proved with the real writer rather than a crafted file. A harness calls BrowserLoginCookies.file, its bytes go to a crawl against a local server, and the server receives the newer value. The engine logs no warning about the file, writes the rotated cookie back into it, keeps a .bak, and leaves no copy in the mirror. The duplicate skip also no longer hides a mutating call inside a short-circuit, because the names are collapsed before the loop that writes them. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deleting a login took two same-shaped strings, the login name and a candidate name, and the call could transpose them without a compile error. Swapped, the predicate collapses to exact equality, so the file itself still goes and everything the engine left beside it is kept. An earlier round renamed the parameters, which documents the hazard without removing it. StoragePaths.savedLoginFiles now does the listing and the matching, so the only call passes a directory and a project name. That also makes the matching testable over real files, which nothing covered before. A temporary folder holds one project's four names, a neighbour's two and an unrelated file. The test names exactly which four come back. Four states a review found that passed anyway, now caught: The header test read the constant, so a second copy of the literal built elsewhere went unvetted. It reads what file() emits instead, and a stray comment line from anywhere reds it. Its window over the engine source is also narrowed to the array, because a comment in the same function could pose as the table. The duplicate key had no test with the same name on two hosts, so dropping the host from the key stayed green. No accepted pair had both sides loaded, so a check refusing exactly the budget when neither side was empty stayed green. The wiring test looked for a call to forgetLogin, not for the name it passes, so deleting the wrong project's login stayed green. One more stacked javadoc, the same defect the last commit fixed in the source. An inserted test had taken the comment belonging to the one below it. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deleting a project mid-copy never stuck. The crawl keeps writing the directory. The engine also recreates the cookies file it still holds when it winds down, so the saved login outlives its project. MirrorServer.stopAll stops serving, not copying. This is reachable on the ordinary path rather than a corner. The copy hands off to the job so it survives you leaving the app, which is the point of it. Coming back builds a fresh window with no saved state, so onCreate lands on the startup pane. attachToLiveCrawl then draws nothing there, because setProgressLines returns early when the progress pane's views are absent. That screen shows no sign a copy is running, and its Cleanup button was enabled on having any project at all. CleanupTitlePolicy.opens now decides, and the delete flow waits for the copy. The "..." picker does not, because choosing a project removes nothing. The check sits in onCleanup as well as on the button's enabled state, since performClick ignores enabled, which this file already notes elsewhere. Older than the browser login, and wider: without one, the delete still half fails and the user is not told. The login file is what made it visible. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard asked only whether a crawl was running, so it still let the delete through in the case that matters most. A connectivity stop empties the session slot and re-arms the job, so nothing is running and the copy is still coming back. Delete the project then, and the job rebuilds the tree and the engine rewrites the cookies file it holds. The app already answers this question in three places, at waitsForNetwork, attachToLiveCrawl and scheduleMirrorJob, and all three carry the pending term. This one now matches them, which is the point: one definition of a copy in progress rather than two. Two terms went the other way. The picker flag was dead. Both callers passed the same value, and the picker never consulted the policy at all, so opensDelete now names the one flow it decides. The session term was redundant. hasLiveRunner opens with that same test, then adds the window where a fragment exists and its crawl has not reached the slot yet. A mutation run found that one: dropping hasLiveRunner changed no test, which is what a term carrying nothing looks like. One limitation this inherits rather than adds. A job left pending offline blocks the delete, and the startup pane offers no route to Stop, so the only way out is connectivity. Starting a copy has refused on the same term and with the same dead end since the job was introduced. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Inverting the guard passed all 627 tests. The delete would then be refused when it was safe, and allowed while a copy ran. That is the one thing the guard exists to stop, and nothing caught it. The clause was a leading "!" on a policy call, which is the cheapest thing in the file to get backwards. So the policy answers the refusal instead. refusesDelete returns true when the flow must not open, and its guard reads as a plain call with no negation. The button's enabled state still negates it, because it asks the opposite question. That site is cosmetic anyway, since performClick ignores enabled, which is why the guard exists separately. The wiring test now pins the whole call at both sites rather than a prefix. A grep cannot otherwise see argument order, and two booleans of the same type transpose in silence. Transposing them, dropping either site, or inverting the policy each reds a test now. Two prose fixes. A doc-comment summary was a noun phrase, so it is a question. The new javadoc wrapped its parameter lines where the method above it keeps each on one, which cost three lines and no content. One correction to the previous message, which is pushed and cannot be amended. It says the performClick note lives in this file. It lives in CleanupActivity. Signed-off-by: Xavier Roche <xroche@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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
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.
The address pane gains a "Log in with a browser" button. It opens the first address in a WebView, and "Use this login" saves that browser's cookies and its exact User-Agent for the copy.
The engine reads a cookies file only when it accepts cookies. So the hand-off ticks Accept cookies as well as filling Cookies file and Browser identity. It also drops the connection count to 2 and appends
-*logout*, so a logged-in copy does not flood the site or end its own session.The window opens https only, and raises an http address to https, because a password does not belong on cleartext. Google accounts refuse an embedded WebView, and a site behind bot protection can still refuse the copy. The screen says both.
Three decisions, recorded here in place of a design note: