Resolve relative canonical paths if rewriting is disabled

robertknight · robertknight · commit 83a33e05410a · 2015-12-10T08:31:50.000Z
For Via, we want rel=canonical links to resolve to the same absolute URL as it did on the original page. For absolute URLs, no rewriting is necessary. If the original rel=canonical URL was relative however, it needs to be resolved relative to the original URL. See hypothesis/via#65 for context.
diff --git a/pywb/rewrite/html_rewriter.py b/pywb/rewrite/html_rewriter.py
@@ -5,7 +5,7 @@
 import re
 
 from HTMLParser import HTMLParser, HTMLParseError
-from urlparse import urlsplit, urlunsplit
+from urlparse import urljoin, urlsplit, urlunsplit
 
 from url_rewriter import UrlRewriter
 from regex_rewriters import JSRewriter, CSSRewriter
@@ -276,9 +276,18 @@ def _rewrite_tag_attrs(self, tag, tag_attrs):
             # special case: if rewrite_canon not set,
             # don't rewrite rel=canonical
             elif tag == 'link' and attr_name == 'href':
-                if (self.opts.get('rewrite_rel_canon', True) or
-                     not self.has_attr(tag_attrs, ('rel', 'canonical'))):
-                    rw_mod = handler.get(attr_name)
+                rw_mod = handler.get(attr_name)
+
+                if self.has_attr(tag_attrs, ('rel', 'canonical')):
+                    if self.opts.get('rewrite_rel_canon', True):
+                        attr_value = self._rewrite_url(attr_value, rw_mod)
+                    else:
+                        # resolve relative rel=canonical URLs so that they
+                        # refer to the same absolute URL as on the original
+                        # page (see https://github.com/hypothesis/via/issues/65
+                        # for context)
+                        attr_value = urljoin(self.orig_url, attr_value)
+                else:
                     attr_value = self._rewrite_url(attr_value, rw_mod)
 
             # special case: meta tag
diff --git a/pywb/rewrite/test/test_html_rewriter.py b/pywb/rewrite/test/test_html_rewriter.py
@@ -158,8 +158,12 @@
 <link rel="canonical" href="/web/20131226101010oe_/http://example.com/">
 
 # rel=canonical: no_rewrite
->>> parse('<link rel=canonical href="http://example.com/">', urlrewriter=no_base_canon_rewriter)
-<link rel="canonical" href="http://example.com/">
+>>> parse('<link rel=canonical href="http://example.com/canon/path">', urlrewriter=no_base_canon_rewriter)
+<link rel="canonical" href="http://example.com/canon/path">
+
+# rel=canonical: no_rewrite
+>>> parse('<link rel=canonical href="/relative/path">', urlrewriter=no_base_canon_rewriter)
+<link rel="canonical" href="http://example.com/relative/path">
 
 # doctype
 >>> parse('<!doctype html PUBLIC "public">')
@@ -210,26 +214,24 @@
 import pprint
 import urllib
 
-urlrewriter = UrlRewriter('20131226101010/http://example.com/some/path/index.html',
-                          '/web/',
-                          rewrite_opts=dict(punycode_links=False))
+ORIGINAL_URL = 'http://example.com/some/path/index.html'
+
+def new_rewriter(prefix='/web/', rewrite_opts=dict()):
+    PROXY_PATH = '20131226101010/{0}'.format(ORIGINAL_URL)
+    return UrlRewriter(PROXY_PATH, prefix, rewrite_opts=rewrite_opts)
 
-full_path_urlrewriter = UrlRewriter('20131226101010/http://example.com/some/path/index.html',
-                                   'http://localhost:80/web/',
-                                   rewrite_opts=dict(punycode_links=False))
+urlrewriter = new_rewriter(rewrite_opts=dict(punycode_links=False))
 
-urlrewriter_pencode = UrlRewriter('20131226101010/http://example.com/some/path/index.html',
-                                   '/web/',
-                                   rewrite_opts=dict(punycode_links=True))
+full_path_urlrewriter = new_rewriter(prefix='http://localhost:80/web/',
+                                     rewrite_opts=dict(punycode_links=False))
 
+urlrewriter_pencode = new_rewriter(rewrite_opts=dict(punycode_links=True))
 
-no_base_canon_rewriter = UrlRewriter('20131226101010/http://example.com/some/path/index.html',
-                                     '/web/',
-                                     rewrite_opts=dict(rewrite_rel_canon=False,
-                                                       rewrite_base=False))
+no_base_canon_rewriter = new_rewriter(rewrite_opts=dict(rewrite_rel_canon=False,
+                                                        rewrite_base=False))
 
 def parse(data, head_insert=None, urlrewriter=urlrewriter):
-    parser = HTMLRewriter(urlrewriter, head_insert = head_insert)
+    parser = HTMLRewriter(urlrewriter, head_insert = head_insert, url = ORIGINAL_URL)
 
     if isinstance(data, unicode):
         data = data.encode('utf-8')
