Skip to content

Commit 6a12a42

Browse files
julieqiujulieqiu
committed
internal/frontend: remove tab=doc query param
Previously, requests to pkg.go.dev/<path> were redirected to pkg.go.dev/<path>?tab=doc. The behavior is now the opposite: requests to pkg.go.dev/<path>?tab=doc will redirect to pkg.go.dev/<path>. Requests to pkg.go.dev/<path> will stay there. Additionally, requests to pkg.go.dev/<path> will always show the documentation tab, regardless of whether the package is redistributable. Previously, users were shown the overview tab when a package is not redistributable. Fixes golang/go#37351 Change-Id: Ic7ccbbb840cf04511d419f06eb7fb40ac57f68be Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/254745 Trust: Julie Qiu <[email protected]> Reviewed-by: Jonathan Amsterdam <[email protected]>
1 parent dd6f888 commit 6a12a42

File tree

8 files changed

+91
-82
lines changed

8 files changed

+91
-82
lines changed

‎content/static/html/pages/details.tmpl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,8 @@
101101
<a role="tab"
102102
{{if .Disabled}}
103103
aria-disabled="true"
104+
{{else if eq .Name "doc"}}
105+
href="{{$header.URL}}"
104106
{{else}}
105107
href="{{$header.URL}}?tab={{.Name}}"
106108
{{end}}

‎internal/frontend/badge.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ func (s *Server) badgeHandler(w http.ResponseWriter, r *http.Request) {
2626
return
2727
}
2828

29-
// The user may input a fully qualified URL (https://pkg.go.dev/net/http?tab=doc)
29+
// The user may input a fully qualified URL (https://pkg.go.dev/net/http)
3030
// or just a pathname (net/http). Using url.Parse we handle both cases.
3131
inputURL := r.URL.Query().Get("path")
3232
parsedURL, _ := url.Parse(inputURL)

‎internal/frontend/legacy_package.go

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -86,26 +86,19 @@ func (s *Server) legacyServePackagePageWithPackage(w http.ResponseWriter, r *htt
8686
return fmt.Errorf("creating package header for %s@%s: %v", pkg.Path, pkg.Version, err)
8787
}
8888

89-
tab := r.FormValue("tab")
90-
settings, ok := packageTabLookup[tab]
91-
if !ok {
92-
var tab string
93-
if pkg.LegacyPackage.IsRedistributable {
94-
tab = tabDoc
95-
} else {
96-
tab = tabOverview
97-
}
98-
http.Redirect(w, r, fmt.Sprintf(r.URL.Path+"?tab=%s", tab), http.StatusFound)
89+
settings, err := packageSettings(r.FormValue("tab"))
90+
if err != nil {
91+
http.Redirect(w, r, r.URL.Path, http.StatusFound)
9992
return nil
10093
}
10194
canShowDetails := pkg.LegacyPackage.IsRedistributable || settings.AlwaysShowDetails
10295

10396
var details interface{}
10497
if canShowDetails {
10598
var err error
106-
details, err = legacyFetchDetailsForPackage(r, tab, ds, pkg)
99+
details, err = legacyFetchDetailsForPackage(r, settings.Name, ds, pkg)
107100
if err != nil {
108-
return fmt.Errorf("fetching page for %q: %v", tab, err)
101+
return fmt.Errorf("fetching page for %q: %v", settings.Name, err)
109102
}
110103
}
111104

@@ -120,7 +113,7 @@ func (s *Server) legacyServePackagePageWithPackage(w http.ResponseWriter, r *htt
120113
page := &DetailsPage{
121114
basePage: s.newBasePage(r, packageHTMLTitle(pkg.Path, pkg.Name)),
122115
Name: pageName,
123-
Settings: settings,
116+
Settings: *settings,
124117
Header: pkgHeader,
125118
Breadcrumb: breadcrumbPath(pkgHeader.Path, pkgHeader.Module.ModulePath,
126119
pkgHeader.Module.LinkVersion),
@@ -134,7 +127,7 @@ func (s *Server) legacyServePackagePageWithPackage(w http.ResponseWriter, r *htt
134127
linkVersion(pkg.Version, pkg.ModulePath),
135128
),
136129
}
137-
page.basePage.AllowWideContent = tab == tabDoc
130+
page.basePage.AllowWideContent = settings.Name == tabDoc
138131
s.servePage(r.Context(), w, settings.TemplateName, page)
139132
return nil
140133
}

‎internal/frontend/package.go

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -63,26 +63,19 @@ func (s *Server) servePackagePage(ctx context.Context,
6363
return fmt.Errorf("creating package header for %s@%s: %v", um.Path, um.Version, err)
6464
}
6565

66-
tab := r.FormValue("tab")
67-
settings, ok := packageTabLookup[tab]
68-
if !ok {
69-
var tab string
70-
if um.IsRedistributable {
71-
tab = tabDoc
72-
} else {
73-
tab = tabOverview
74-
}
75-
http.Redirect(w, r, fmt.Sprintf(r.URL.Path+"?tab=%s", tab), http.StatusFound)
66+
settings, err := packageSettings(r.FormValue("tab"))
67+
if err != nil {
68+
http.Redirect(w, r, r.URL.Path, http.StatusFound)
7669
return nil
7770
}
7871
canShowDetails := um.IsRedistributable || settings.AlwaysShowDetails
7972

8073
var details interface{}
8174
if canShowDetails {
8275
var err error
83-
details, err = fetchDetailsForPackage(r, tab, ds, um)
76+
details, err = fetchDetailsForPackage(r, settings.Name, ds, um)
8477
if err != nil {
85-
return fmt.Errorf("fetching page for %q: %v", tab, err)
78+
return fmt.Errorf("fetching page for %q: %v", settings.Name, err)
8679
}
8780
}
8881
var (
@@ -96,7 +89,7 @@ func (s *Server) servePackagePage(ctx context.Context,
9689
page := &DetailsPage{
9790
basePage: s.newBasePage(r, packageHTMLTitle(um.Path, um.Name)),
9891
Name: pageName,
99-
Settings: settings,
92+
Settings: *settings,
10093
Header: pkgHeader,
10194
Breadcrumb: breadcrumbPath(pkgHeader.Path, pkgHeader.Module.ModulePath,
10295
pkgHeader.Module.LinkVersion),
@@ -109,7 +102,27 @@ func (s *Server) servePackagePage(ctx context.Context,
109102
pkgHeader.Module.ModulePath,
110103
pkgHeader.Module.LinkVersion),
111104
}
112-
page.basePage.AllowWideContent = tab == tabDoc
105+
page.basePage.AllowWideContent = settings.Name == tabDoc
113106
s.servePage(ctx, w, settings.TemplateName, page)
114107
return nil
115108
}
109+
110+
// packageSettings returns the TabSettings corresponding to tab.
111+
// If tab is not a valid tab from packageTabLookup or tab=doc, an error will be
112+
// returned and the user will be redirected to /<path> outside of this
113+
// function. If tab is the empty string, the user will be shown the
114+
// documentation page.
115+
func packageSettings(tab string) (*TabSettings, error) {
116+
if tab == tabDoc {
117+
// Redirect "/<path>?tab=doc" to "/<path>".
118+
return nil, derrors.NotFound
119+
}
120+
if tab == "" {
121+
tab = tabDoc
122+
}
123+
settings, ok := packageTabLookup[tab]
124+
if !ok {
125+
return nil, derrors.NotFound
126+
}
127+
return &settings, nil
128+
}

‎internal/frontend/server_test.go

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -396,7 +396,7 @@ func serverTestCases() []serverTestCase {
396396
},
397397
{
398398
name: "package default",
399-
urlPath: fmt.Sprintf("/%s?tab=doc", sample.PackagePath),
399+
urlPath: fmt.Sprintf("/%s", sample.PackagePath),
400400
wantStatusCode: http.StatusOK,
401401
want: in("",
402402
pagecheck.PackageHeader(pkgV100, unversioned),
@@ -408,9 +408,9 @@ func serverTestCases() []serverTestCase {
408408
},
409409
{
410410
name: "package default redirect",
411-
urlPath: fmt.Sprintf("/%s", sample.PackagePath),
411+
urlPath: fmt.Sprintf("/%s?tab=doc", sample.PackagePath),
412412
wantStatusCode: http.StatusFound,
413-
wantLocation: "/" + sample.ModulePath + "/foo?tab=doc",
413+
wantLocation: "/" + sample.ModulePath + "/foo",
414414
},
415415
{
416416
name: "package default nonredistributable",
@@ -421,7 +421,7 @@ func serverTestCases() []serverTestCase {
421421
},
422422
{
423423
name: "package at version default",
424-
urlPath: fmt.Sprintf("/%s@%s/%s?tab=doc", sample.ModulePath, sample.VersionString, sample.Suffix),
424+
urlPath: fmt.Sprintf("/%s@%s/%s", sample.ModulePath, sample.VersionString, sample.Suffix),
425425
wantStatusCode: http.StatusOK,
426426
want: in("",
427427
pagecheck.PackageHeader(pkgV100, versioned),
@@ -437,31 +437,24 @@ func serverTestCases() []serverTestCase {
437437
},
438438
{
439439
name: "package at version doc tab",
440-
urlPath: fmt.Sprintf("/%s@%s/%s?tab=doc", sample.ModulePath, "v0.9.0", sample.Suffix),
440+
urlPath: fmt.Sprintf("/%s@%s/%s", sample.ModulePath, "v0.9.0", sample.Suffix),
441441
wantStatusCode: http.StatusOK,
442442
want: in("",
443443
pagecheck.PackageHeader(pkgV090, versioned),
444444
in(".Documentation", text(`This is the documentation HTML`))),
445445
},
446-
{
447-
name: "package at version doc with hacked up links",
448-
urlPath: "/" + sample.ModulePath + "/foo/directory/hello?tab=doc",
449-
wantStatusCode: http.StatusOK,
450-
want: in(".Documentation",
451-
in("a", href("/io?tab=doc#Writer"), text("io.Writer"))),
452-
},
453446
{
454447
name: "package at version doc tab nonredistributable",
455448
// For a non-redistributable package, the doc tab will not show the doc.
456-
urlPath: "/github.com/[email protected]/bar?tab=doc",
449+
urlPath: "/github.com/[email protected]/bar",
457450
wantStatusCode: http.StatusOK,
458451
want: in("",
459452
pagecheck.PackageHeader(pkgNonRedist, versioned),
460453
in(".DetailsContent", text(`not displayed due to license restrictions`))),
461454
},
462455
{
463456
name: "package at version doc tab, no doc",
464-
urlPath: "/github.com/pseudo@" + pseudoVersion + "/dir/baz?tab=doc",
457+
urlPath: "/github.com/pseudo@" + pseudoVersion + "/dir/baz",
465458
wantStatusCode: http.StatusOK,
466459
want: in("", text("No documentation available")),
467460
},
@@ -779,13 +772,13 @@ func serverTestCases() []serverTestCase {
779772
},
780773
{
781774
name: "cmd go package page",
782-
urlPath: "/cmd/go?tab=doc",
775+
urlPath: "/cmd/go",
783776
wantStatusCode: http.StatusOK,
784777
want: pagecheck.PackageHeader(cmdGo, unversioned),
785778
},
786779
{
787780
name: "cmd go package page at version",
788-
urlPath: "/cmd/[email protected]?tab=doc",
781+
urlPath: "/cmd/[email protected]",
789782
wantStatusCode: http.StatusOK,
790783
want: pagecheck.PackageHeader(cmdGo, versioned),
791784
},
@@ -811,7 +804,7 @@ func serverTestCases() []serverTestCase {
811804
},
812805
{
813806
name: "stdlib no shortcut (net/http)",
814-
urlPath: "/net/http?tab=doc",
807+
urlPath: "/net/http",
815808
wantStatusCode: http.StatusOK,
816809
want: pagecheck.ModuleHeader(netHttp, unversioned),
817810
},
@@ -841,7 +834,7 @@ func serverTestCases() []serverTestCase {
841834
},
842835
{
843836
name: "stdlib shortcut (net/http) strip args",
844-
urlPath: "/[email protected]?tab=doc",
837+
urlPath: "/[email protected]",
845838
wantStatusCode: http.StatusFound,
846839
wantLocation: "/net/http",
847840
requiredExperiments: experiment.NewSet(internal.ExperimentUsePathInfo),
@@ -855,7 +848,7 @@ func serverTestCases() []serverTestCase {
855848
},
856849
{
857850
name: "stdlib shortcut with args and trailing slash",
858-
urlPath: "/[email protected]/?tab=doc",
851+
urlPath: "/[email protected]/",
859852
wantStatusCode: http.StatusFound,
860853
wantLocation: "/net/http",
861854
requiredExperiments: experiment.NewSet(internal.ExperimentUsePathInfo),

‎internal/middleware/godoc_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,16 +42,16 @@ func TestGodocURL(t *testing.T) {
4242
},
4343
{
4444
desc: "Strip utm_source, set temporary cookie, and redirect",
45-
path: "/cloud.google.com/go/storage?tab=doc&utm_source=godoc",
45+
path: "/cloud.google.com/go/storage?utm_source=godoc",
4646
code: http.StatusFound,
4747
headers: map[string]string{
48-
"Location": "/cloud.google.com/go/storage?tab=doc",
48+
"Location": "/cloud.google.com/go/storage",
4949
"Set-Cookie": "tmp-from-godoc=1; SameSite=Lax",
5050
},
5151
},
5252
{
5353
desc: "Delete temporary cookie; godoc URL should be set",
54-
path: "/cloud.google.com/go/storage?tab=doc",
54+
path: "/cloud.google.com/go/storage",
5555
cookies: map[string]string{
5656
"tmp-from-godoc": "1",
5757
},
@@ -106,7 +106,7 @@ func TestGodoc(t *testing.T) {
106106
from, to string
107107
}{
108108
{
109-
from: "https://pkg.go.dev/cloud.google.com/go/storage?tab=doc",
109+
from: "https://pkg.go.dev/cloud.google.com/go/storage",
110110
to: "https://godoc.org/cloud.google.com/go/storage?utm_source=backtogodoc",
111111
},
112112
{

‎internal/postgres/details.go

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -308,18 +308,16 @@ func scanModuleInfo(scan func(dest ...interface{}) error) (*internal.ModuleInfo,
308308

309309
// convertDocumentation takes a string that was read from the database and
310310
// converts it to a safehtml.HTML.
311-
func convertDocumentation(doc string) safehtml.HTML {
312-
if addDocQueryParam {
313-
doc = hackUpDocumentation(doc)
314-
}
315-
// We trust the data in our database and the transformation done by hackUpDocumentation.
311+
//
312+
// It rewrites documentation links by stripping the /pkg path prefix. It
313+
// preserves the safety of its argument. That is, if docHTML is safe
314+
// from XSS attacks, so is replaceDocumentationLinks(docHTML).
315+
func convertDocumentation(docHTML string) safehtml.HTML {
316+
doc := removePkgPrefix(docHTML)
317+
// We trust the data in our database and the transformation done by removePkgPrefix.
316318
return uncheckedconversions.HTMLFromStringKnownToSatisfyTypeContract(doc)
317319
}
318320

319-
// addDocQueryParam controls whether to use a regexp replacement to append
320-
// ?tab=doc to urls linking to package identifiers within the documentation.
321-
const addDocQueryParam = true
322-
323321
// packageLinkRegexp matches cross-package identifier links that have been
324322
// generated by the dochtml package. At the time this hack was added, these
325323
// links are all constructed to have either the form
@@ -329,12 +327,13 @@ const addDocQueryParam = true
329327
//
330328
// The packageLinkRegexp mutates these links as follows:
331329
// - remove the now unnecessary '/pkg' path prefix
332-
// - add an explicit ?tab=doc after the path.
333330
var packageLinkRegexp = regexp.MustCompile(`(<a href="/)pkg/([^?#"]+)((?:#[^"]*)?">.*?</a>)`)
334331

335-
// hackUpDocumentation rewrites anchor hrefs to add a tab=doc query parameter.
336-
// It preserves the safety of its argument. That is, if docHTML is safe
337-
// from XSS attacks, so is hackUpDocumentation(docHTML).
338-
func hackUpDocumentation(docHTML string) string {
339-
return packageLinkRegexp.ReplaceAllString(docHTML, `$1$2?tab=doc$3`)
332+
// removePkgPrefix removes the /pkg path prefix from links in docHTML.
333+
// See documentation for packageLinkRegexp for explanation and
334+
// TestRemovePkgPrefix for examples. It preserves the safety of its argument.
335+
// That is, if docHTML is safe from XSS attacks, so is
336+
// removePkgPrefix(docHTML).
337+
func removePkgPrefix(docHTML string) string {
338+
return packageLinkRegexp.ReplaceAllString(docHTML, `$1$2$3`)
340339
}

‎internal/postgres/details_test.go

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -393,36 +393,45 @@ func TestJSONBScanner(t *testing.T) {
393393
}
394394
}
395395

396-
func TestHackUpDocumentation(t *testing.T) {
396+
func TestRemovePkgPrefix(t *testing.T) {
397397
tests := []struct {
398398
body string
399399
want string
400400
}{
401-
{"nothing burger", "nothing burger"},
402-
{`<a href="/pkg/foo">foo</a>`, `<a href="/foo?tab=doc">foo</a>`},
403-
{`<a href="/pkg/foo"`, `<a href="/pkg/foo"`},
401+
// Cases where /pkg is expected to be removed.
402+
{`<a href="/pkg/foo">foo</a>`, `<a href="/foo">foo</a>`},
404403
{
405404
`<a href="/pkg/foo"><a href="/pkg/bar">bar</a></a>`,
406-
`<a href="/foo?tab=doc"><a href="/pkg/bar">bar</a></a>`,
405+
`<a href="/foo"><a href="/pkg/bar">bar</a></a>`,
407406
},
408407
{
409408
`<a href="/pkg/foo">foo</a>
410409
<a href="/pkg/bar">bar</a>`,
411-
`<a href="/foo?tab=doc">foo</a>
412-
<a href="/bar?tab=doc">bar</a>`,
410+
`<a href="/foo">foo</a>
411+
<a href="/bar">bar</a>`,
413412
},
414-
{`<ahref="/pkg/foo">foo</a>`, `<ahref="/pkg/foo">foo</a>`},
415-
{`<allhref="/pkg/foo">foo</a>`, `<allhref="/pkg/foo">foo</a>`},
416-
{`<a nothref="/pkg/foo">foo</a>`, `<a nothref="/pkg/foo">foo</a>`},
417-
{`<a href="/pkg/foo#identifier">foo</a>`, `<a href="/foo?tab=doc#identifier">foo</a>`},
418-
{`<a href="#identifier">foo</a>`, `<a href="#identifier">foo</a>`},
413+
{`<a href="/pkg/foo#identifier">foo</a>`, `<a href="/foo#identifier">foo</a>`},
419414
{`<span id="Indirect.Type"></span>func (in <a href="#Indirect">Indirect</a>) Type() <a href="/pkg/reflect">reflect</a>.<a href="/pkg/reflect#Type">Type</a>`,
420-
`<span id="Indirect.Type"></span>func (in <a href="#Indirect">Indirect</a>) Type() <a href="/reflect?tab=doc">reflect</a>.<a href="/reflect?tab=doc#Type">Type</a>`},
415+
`<span id="Indirect.Type"></span>func (in <a href="#Indirect">Indirect</a>) Type() <a href="/reflect">reflect</a>.<a href="/reflect#Type">Type</a>`},
421416
}
422417

423418
for _, test := range tests {
424-
if got := hackUpDocumentation(test.body); got != test.want {
425-
t.Errorf("hackUpDocumentation(%s) = %s, want %s", test.body, got, test.want)
419+
if got := removePkgPrefix(test.body); got != test.want {
420+
t.Errorf("removePkgPrefix(%s) = %s, want %s", test.body, got, test.want)
421+
}
422+
}
423+
424+
// Cases where no change is expected.
425+
for _, test := range []string{
426+
"nothing burger",
427+
`<ahref="/pkg/foo">foo</a>`,
428+
`<allhref="/pkg/foo">foo</a>`,
429+
`<a nothref="/pkg/foo">foo</a>`,
430+
`<a href="/pkg/foo"`,
431+
`<a href="#identifier">foo</a>`,
432+
} {
433+
if got := removePkgPrefix(test); got != test {
434+
t.Errorf("removePkgPrefix(%s) = %s, want %s", test, got, test)
426435
}
427436
}
428437
}

0 commit comments

Comments
 (0)