Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ All notable changes to kage are recorded here. The format follows
directly on its article and keeps that article's title metadata ([#62](https://github.com/tamnd/kage/issues/62)).
- Non-UTF-8 `<meta charset>` and Content-Type charset declarations are
rewritten to `utf-8`, matching the encoding kage writes to disk ([#16](https://github.com/tamnd/kage/issues/16)).
- Relative links on redirected pages resolve against the browser's final URL
and the document's first `<base href>`, while the page remains saved under
the URL that was originally discovered. Consumed `href` attributes are
removed from every `<base>` so they cannot re-root rewritten links when the
saved page opens, while `target` behavior is preserved.
After a cross-host redirect,
relative references resolve to the other host and remain absolute when that
host is outside the crawl scope, so those resources are not localised.
- `--resume` picks an interrupted crawl back up instead of doing nothing ([#36](https://github.com/tamnd/kage/issues/36)).
`state.json` persisted only the visited set, and the frontier was rebuilt purely by re-rendering pages and following their links, which resume exists to avoid.
So a resumed run found its seed already visited, `enqueuePage` turned it down, nothing was queued, and the run printed `pages 0` and exited successfully with most of the site still missing.
Expand Down
59 changes: 58 additions & 1 deletion clone/cloner.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/tamnd/kage/sanitize"
"github.com/tamnd/kage/urlx"
"golang.org/x/net/html"
"golang.org/x/net/html/atom"
"golang.org/x/time/rate"
)

Expand Down Expand Up @@ -315,6 +316,13 @@ func (c *Cloner) processPage(ctx context.Context, j pageItem) {
return
}

// Resolve references against the post-redirect URL (and any <base href>),
// but keep writing the page under the discovered URL so existing offline
// 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)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


localFile := urlx.LocalPath(c.seedHost, j.u, urlx.Page, c.cfg.Reserved)
fileDir := urlx.Dir(localFile)

Expand All @@ -337,7 +345,7 @@ func (c *Cloner) processPage(ctx context.Context, j pageItem) {
}
}

asset.RewriteHTML(root, j.u, sink)
asset.RewriteHTML(root, resolveBase, sink)
sanitize.CleanTree(root, sanitize.Options{
KeepNoscript: c.cfg.KeepNoscript,
MobileReadable: c.cfg.MobileReadable,
Expand Down Expand Up @@ -367,6 +375,55 @@ func (c *Cloner) waitForCrawlDelay(ctx context.Context) bool {
return c.crawlLimiter.Wait(ctx) == nil
}

// pageResolveBase picks the URL against which relative references on a rendered
// page should resolve. Preference order:
// 1. A document <base href> (the live page's own base);
// 2. The browser's final URL after redirects;
// 3. The URL that was enqueued.
//
// The page is still written under the enqueued URL so offline links discovered
// as /old keep working when the server redirected /old → /new.
func pageResolveBase(enqueued *url.URL, finalURL string, root *html.Node) *url.URL {
base := enqueued
if finalURL != "" {
if u, err := url.Parse(finalURL); err == nil && u.Scheme != "" && u.Host != "" {
// Drop fragment; keep query/path as the browser shows them.
u.Fragment = ""
base = u
}
}
if href := documentBaseHref(root); href != "" {
if u, err := urlx.Normalize(base, href); err == nil {
return u
}
}
return base
}

// documentBaseHref returns the first <base href> in document order, or "".
func documentBaseHref(root *html.Node) string {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var found string
var walk func(*html.Node)
walk = func(n *html.Node) {
if found != "" || n == nil {
return
}
if n.Type == html.ElementNode && n.DataAtom == atom.Base {
for _, a := range n.Attr {
if strings.EqualFold(a.Key, "href") && strings.TrimSpace(a.Val) != "" {
found = strings.TrimSpace(a.Val)
return
}
}
}
for c := n.FirstChild; c != nil && found == ""; c = c.NextSibling {
walk(c)
}
}
walk(root)
return found
}

// processAsset downloads one asset, rewriting CSS references on the way, and
// writes it to its deterministic local path.
func (c *Cloner) processAsset(ctx context.Context, j assetItem) {
Expand Down
45 changes: 45 additions & 0 deletions clone/resolve_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package clone

import (
"net/url"
"strings"
"testing"

"golang.org/x/net/html"
)

func TestPageResolveBaseUsesFinalURL(t *testing.T) {
enqueued, _ := url.Parse("https://ex.com/old")
root, err := html.Parse(strings.NewReader(`<html><head></head><body><a href="next">n</a></body></html>`))
if err != nil {
t.Fatal(err)
}
base := pageResolveBase(enqueued, "https://ex.com/new/", root)
if base.String() != "https://ex.com/new/" {
t.Fatalf("resolve base = %q, want final URL", base)
}
}

func TestPageResolveBasePrefersDocumentBase(t *testing.T) {
enqueued, _ := url.Parse("https://ex.com/page")
root, err := html.Parse(strings.NewReader(
`<html><head><base href="https://ex.com/dir/"></head><body></body></html>`))
if err != nil {
t.Fatal(err)
}
base := pageResolveBase(enqueued, "https://ex.com/page", root)
if base.String() != "https://ex.com/dir/" {
t.Fatalf("resolve base = %q, want document <base>", base)
}
}

func TestDocumentBaseHref(t *testing.T) {
root, err := html.Parse(strings.NewReader(
`<html><head><base href="/subdir/"><base href="https://ignored.example/"></head></html>`))
if err != nil {
t.Fatal(err)
}
if got := documentBaseHref(root); got != "/subdir/" {
t.Fatalf("documentBaseHref = %q, want first base", got)
}
}
5 changes: 5 additions & 0 deletions docs/content/reference/release-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ The authoritative, commit-level history lives in [`CHANGELOG.md`](https://github
single-page archives still open directly on their article ([#62](https://github.com/tamnd/kage/issues/62)).
- **Saved pages declare their real encoding.** Non-UTF-8 charset metadata is
rewritten to UTF-8, matching the bytes kage writes to disk ([#16](https://github.com/tamnd/kage/issues/16)).
- **Redirects resolve correctly.** Relative links use the post-redirect URL and
the document's first `<base href>`. After rewriting, every base `href` is
removed so it cannot affect the saved page, while a base `target` is
preserved. On a cross-host redirect, references to an out-of-scope
destination remain absolute rather than being localised.

## v0.3.11

Expand Down
24 changes: 24 additions & 0 deletions sanitize/sanitize.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ type Report struct {
MetaRefreshRemoved int
DeadLinksRemoved int
CondCommentsRemoved int
BaseHrefsRemoved int
CharsetAdded bool
CharsetRewritten bool
}
Expand Down Expand Up @@ -110,6 +111,15 @@ func clean(n *html.Node, opts Options, rep *Report) {
}
if c.Type == html.ElementNode {
switch c.DataAtom {
case atom.Base:
// Link rewriting has already consumed the document base. Remove every
// href so a later base cannot become active when an earlier one is
// removed, but preserve target because it controls browsing contexts.
rep.BaseHrefsRemoved += stripBaseHrefs(c)
if len(c.Attr) == 0 {
n.RemoveChild(c)
continue
}
case atom.Script:
n.RemoveChild(c)
rep.ScriptsRemoved++
Expand Down Expand Up @@ -143,6 +153,20 @@ func clean(n *html.Node, opts Options, rep *Report) {
}
}

func stripBaseHrefs(n *html.Node) int {
kept := n.Attr[:0]
removed := 0
for _, a := range n.Attr {
if strings.EqualFold(a.Key, "href") {
removed++
continue
}
kept = append(kept, a)
}
n.Attr = kept
return removed
}

// stripHandlers removes every on* event-handler attribute from n.
func stripHandlers(n *html.Node, rep *Report) {
kept := n.Attr[:0]
Expand Down
37 changes: 37 additions & 0 deletions sanitize/sanitize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,43 @@ func TestKeepMetaRefreshPlain(t *testing.T) {
}
}

func TestBaseHrefRemovedAfterRewrite(t *testing.T) {
cases := []struct {
name string
bases string
wantBase bool
wantTarget bool
wantRemoved int
}{
{name: "href only", bases: `<base href="https://example.com/live/">`, wantRemoved: 1},
{name: "target only", bases: `<base target="_blank">`, wantBase: true, wantTarget: true},
{name: "href and target", bases: `<base href="https://example.com/live/" target="_blank">`, wantBase: true, wantTarget: true, wantRemoved: 1},
{name: "all hrefs", bases: `<base href="https://example.com/one/"><base href="https://example.com/two/" target="_blank">`, wantBase: true, wantTarget: true, wantRemoved: 2},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
in := `<html><head>` + tc.bases + `</head><body><a href="saved.html">saved</a></body></html>`
out, rep, err := Strip([]byte(in), Options{})
if err != nil {
t.Fatal(err)
}
s := strings.ToLower(string(out))
if got := strings.Contains(s, "<base"); got != tc.wantBase {
t.Errorf("base presence = %v, want %v:\n%s", got, tc.wantBase, out)
}
if got := strings.Contains(s, `target="_blank"`); got != tc.wantTarget {
t.Errorf("base target presence = %v, want %v:\n%s", got, tc.wantTarget, out)
}
if strings.Contains(s, "example.com/") {
t.Errorf("base href survived:\n%s", out)
}
if rep.BaseHrefsRemoved != tc.wantRemoved {
t.Errorf("BaseHrefsRemoved = %d, want %d", rep.BaseHrefsRemoved, tc.wantRemoved)
}
})
}
}

func TestCharsetAddedWhenMissing(t *testing.T) {
// A page whose source declared its charset only in the HTTP header has no
// <meta charset>. The saved file must gain one so a reader does not fall back
Expand Down