Resolve redirected-page links against the final URL - #74
Conversation
|
Thanks for your contribution and interest in Kage! Feel free to ping me when you are ready for a review. : ) |
|
Hi @tamnd — this is ready for a review when you have a moment. I've merged current main (v0.3.11) into the branch, so it's conflict-free and govulncheck is clean (x/text is v0.39.0 now). The workflow runs are waiting on your approval to start. Thanks! |
tamnd
left a comment
tamnd
left a comment
There was a problem hiding this comment.
There are three separate changes in here and they share no code. I would like to take two of them almost as is and talk about the third.
- The redirect and
<base href>resolve base is a real bug fix and I want it. I checked the ordering and it is correct:pageResolveBasereads the base beforeCleanTreeremoves the element. - The charset rewriting is right too, and it is a genuine contribution to #16.
- The active carrier removal is a product decision, not a bug fix, and as written it deletes every third party embed from every mirror we produce. That needs a flag and a maintainer decision before it ships.
Please split into three PRs. The first two can land this week.
One thing that is not in this PR and probably should be, since you are already in the charset code: saved pages still have no <!DOCTYPE html>, so they all render in quirks mode, which is where the charset meta you just corrected is least authoritative. Chrome's outerHTML of <html> never includes the doctype, html.Parse therefore builds no doctype node, and html.Render emits none. I verified it. Happy for that to be a fourth PR or for me to do it.
| // links that pointed at /old still resolve. Cross-host redirects leave the | ||
| // resolve base as the final location for relative refs; scope checks still | ||
| // use that absolute URL. | ||
| resolveBase := pageResolveBase(j.u, res.FinalURL, root) |
There was a problem hiding this comment.
This is the good part of the PR and I want it. Resolving against the post redirect URL fixes a real bug: today a page fetched at /old that redirects to /new/ resolves href="next" as /next instead of /new/next, which breaks links and produces 404 asset fetches across the whole mirror.
I checked the ordering and it is correct. You read the <base href> here, RewriteHTML consumes it, and only then does CleanTree delete the element. Worth keeping that dependency in mind if anyone reorders processPage later.
One case to name in the CHANGELOG: on a cross host redirect (ex.com/old to other.com/new) the base becomes the other host, so every relative reference resolves out of scope, gets left absolute, and the page saved under ex.com/old ends up with nothing local in it at all. Your comment acknowledges the mechanism but not that outcome. Arguably the right answer is to enqueue the final URL as its own page when it is in scope, but I am happy to leave that for later as long as it is written down.
| } | ||
|
|
||
| // documentBaseHref returns the first <base href> in document order, or "". | ||
| func documentBaseHref(root *html.Node) string { |
There was a problem hiding this comment.
This reimplements a first match tree walk that findElement in the sanitize package already does. Not blocking, but two nearly identical walkers in two packages is the kind of thing that drifts. A shared helper would be fine.
| removeAttr(n, "src") | ||
| rep.ActiveFramesRemoved++ | ||
| } | ||
| case strings.HasPrefix(low, "http://"), strings.HasPrefix(low, "https://"), strings.HasPrefix(low, "//"): |
There was a problem hiding this comment.
This is the change I cannot take as written.
Reading asset/html.go, atom.Iframe and atom.Frame go through rewriteAttr(n, "src", base, sink, pageOrAsset), and an out of scope iframe is deliberately left as an absolute URL to the live web. That is documented behaviour for off domain content. This branch then strips exactly those.
So on a typical page, every YouTube embed, Vimeo player, Google Map, CodePen, Twitter embed, Disqus thread and reCAPTCHA frame becomes an empty <iframe> with no src, which browsers render as a blank bordered box. An article that had a video in the middle of it now has a hole in it, and the reader has no idea anything was there.
I am not saying the policy is wrong. A live third party frame does phone home and run code when the saved page is opened online, and "inert snapshot" is the promise. But this is a change to what kage produces for nearly every real page on the web, and it arrives as an unannounced side effect of a PR titled "harden inert snapshots".
What I want before this lands:
- A flag.
--keep-remote-framesor the inverse, whichever default you argue for.--keep-noscriptand--all-asset-hostsalready establish that policy like this is user selectable. - A placeholder instead of a blank box, something like
<a href="https://...">Embedded content: youtube.com/...</a>. That keeps the information at zero risk. Silently blanking destroys it. - A line in the README and CHANGELOG in a user's words: third party embeds are removed from saved pages.
| if i := strings.IndexAny(path, "?#"); i >= 0 { | ||
| path = path[:i] | ||
| } | ||
| for _, ext := range []string{".html", ".htm", ".xhtml", ".svg", ".xml"} { |
There was a problem hiding this comment.
.svg in this list is too broad. <object data="diagram.svg"> and <embed src="logo.svg"> are ordinary, script free ways to place vector graphics, and they are common on documentation sites, which are our most likely target. Those illustrations now vanish entirely, and because the caller does RemoveChild rather than just dropping the reference, any fallback content nested inside the <object> goes with them.
The underlying concern is right: a raw .svg takes the asset path and is never sanitized, so an SVG with a <script> in it runs when framed. But the fix is at the wrong layer. SVG is XML and the existing tree walk applies almost unchanged, so sanitizing SVG assets and keeping the element is both safer and less destructive. Failing that, drop the reference and keep the element so the fallback survives.
| // carriers (HTML, SVG, or anything with a script-bearing data | ||
| // URL) so the inert-snapshot promise holds. | ||
| if neutralizePlugin(c, rep) { | ||
| n.RemoveChild(c) |
There was a problem hiding this comment.
Note the inconsistency between the two carriers: an unsafe <iframe> is kept as an empty shell a few lines up, while an unsafe <object> is deleted outright here. Two policies for the same class of problem, with no stated reason for the difference. Whichever you pick, please pick one.
| case atom.Iframe, atom.Frame: | ||
| neutralizeActiveFrame(c, rep) | ||
| case atom.Object, atom.Embed: | ||
| // Same-domain HTML/SVG served through object/embed executes |
There was a problem hiding this comment.
This comment says the inert snapshot promise holds, and it only half does.
processPage runs asset.RewriteHTML(root, resolveBase, sink) before sanitize.CleanTree(root, ...). So by the time this code deletes an <object data="thing.svg">, the sink has already queued thing.svg for download. The script bearing file gets fetched, written into _kage/, and packed into the ZIM. It is merely unreferenced.
A reader who opens the archive and navigates to the file directly still gets a live script, and that is trivially possible given kage serve generates directory listings and Kiwix has search. Right now the change pays the cost of a strict policy without getting its benefit.
Either do not ship the payload, or reconsider whether deleting the visible content was justified. I would rather resolve that one way or the other than sit in between.
| low := strings.ToLower(ref) | ||
| if strings.HasPrefix(low, "javascript:") { | ||
| rep.JSURLsNeutralized++ | ||
| rep.ActiveFramesRemoved++ |
There was a problem hiding this comment.
These two counters double charge for a single action, so the report numbers no longer partition the work done.
ActiveFramesRemoved is also counting five different things by now: a dropped srcdoc, a data URL, a remote frame, a removed <object>, and a removed <embed>, under a name that says frames. Worth getting right while it is cheap.
| CharsetAdded bool | ||
| ActiveFramesRemoved int | ||
| BaseTagsRemoved int | ||
| CharsetFixed bool // true when a charset meta was inserted or rewritten to utf-8 |
There was a problem hiding this comment.
Renaming an exported field is a breaking change for anyone importing github.com/tamnd/kage/sanitize. Nothing in this repo reads it so it costs us nothing internally, but please move it out of Fixed and into Changed in the CHANGELOG with the rename spelled out.
While we are here: I grepped, and no non test code reads CharsetAdded, CondCommentsRemoved, or any other counter. Strip and CleanTree return a Report that is always discarded, and this PR adds two more counters to it.
That is not your fault, but it is the moment to notice it. I would rather surface the report than keep growing it. A kage clone --verbose that printed "142 scripts stripped, 3 frames blanked, 1 charset rewritten" would have let the reporters of #16, #61 and #62 diagnose their own problems. If you would rather not take that on, say so and I will do it separately.
| // fixCharsetMetas rewrites existing charset declarations to utf-8. found is | ||
| // true when a declaration was already present (so the caller should not insert | ||
| // another); changed is true when a non-utf-8 value was rewritten in place. | ||
| func fixCharsetMetas(head *html.Node) (found, changed bool) { |
There was a problem hiding this comment.
This walks only the direct children of <head>, same as the old hasCharsetMeta, so no regression. But since the input is Chrome's serialised DOM, a <meta charset> in a malformed document can end up outside <head> entirely, and it will then be neither found nor fixed, leaving two contradictory declarations in the file. A findElement style descendant walk would be more robust. Low priority.
| if !strings.HasPrefix(low, "data:") { | ||
| return false | ||
| } | ||
| // Only the mediatype token before the first comma decides; the payload |
There was a problem hiding this comment.
Nice detail. Parsing only the mediatype token before the first comma, with a comment saying why, is exactly right. The payload really can contain text/html by chance in base64.
SihanTeng
changed the title
|
Thanks for the detailed review. I narrowed this PR to the redirect and document-base fix, added the cross-host outcome to the changelog, and kept Charset rewriting is now the separate #81. It preserves I removed all active frame, object, embed, and SVG changes from this branch. Before preparing that third PR, my proposed compatibility-first shape is an opt-in
|
|
One explicit answer to the report suggestion: I kept |
|
Follow-up quality pass:
|
Co-authored-by: SihanTeng <SihanTeng@users.noreply.github.com>
|
Stripping base href is a bigger win than the description gives it credit for. I checked on main and the live base survives into the saved file, so on any page using it every rewritten relative link re-roots back to the live site, which quietly defeats the whole mirror. Keeping target while dropping href is right. I squashed onto main to clear the changelog conflicts, and I am merging this with a follow up to come for the scope side: because SameSite is an exact host match, resolving against the final URL after an apex to www redirect puts every link out of scope and the crawl stops at one page. Assets still load because they go through SameRegistrableDomain, so it looks like success. |
61914a6 to
1a38ac0
Compare
urlx.SameSite matches hostnames exactly, so a seed that redirects apex to www (or the reverse) lands on a host the scope rejects. Resolving the page's relative links against that host put every one of them out of scope: they stayed absolute, nothing was enqueued, and the crawl saved the seed page and stopped. Assets go through SameRegistrableDomain and kept downloading, so the run ended with a complete-looking single page and no error. scopedResolveBase falls back to the document <base href>, and then to the enqueued URL, when the redirect target is out of scope. A redirect that genuinely leaves the site keeps the previous behaviour, since those links are out of scope either way and are re-fetched and re-redirected if they are reached on their own. Follow-up to #74.
2 participants
Summary
<base href>, then strip every basehrefafter rewriting while preservingtargetbehavior.Scope
This PR now contains only the accepted redirect/base-resolution fix. Charset rewriting moved to #81. Active frame, object, embed, and SVG policy changes were removed pending a separate maintainer decision.
Test plan
go test -count=1 ./clone/ ./sanitize/