Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New transformer to convert ".js" to ".js?f=sxg" in script node values. #451

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
11c1e4c
Merge pull request #386 from ampproject/master
twifkak Jan 31, 2020
cc38c5f
Merge pull request #390 from ampproject/master
banaag Feb 4, 2020
bf061be
Create CODE_OF_CONDUCT.md (#410)
nainar Apr 27, 2020
47b5000
initial creation of transformer
xiexr151e Jul 10, 2020
9e02617
updated transformer after consultation
xiexr151e Jul 11, 2020
dff39de
transformer rename
xiexr151e Jul 11, 2020
b099fd1
.sxg.js to &f=sxg
xiexr151e Aug 11, 2020
81b97fb
use of go url api
xiexr151e Aug 11, 2020
81dbcc8
import error
xiexr151e Aug 11, 2020
c69061c
compiler errors
xiexr151e Aug 12, 2020
8a9d719
compiler errors w/ url
xiexr151e Aug 12, 2020
749ef85
correction in test
xiexr151e Aug 12, 2020
30a3227
merge
xiexr151e Aug 12, 2020
2c45701
merge conflict
xiexr151e Aug 13, 2020
1265589
correction to test
xiexr151e Aug 13, 2020
3b31c3b
merge
xiexr151e Aug 13, 2020
addc286
14 transformers present
xiexr151e Aug 13, 2020
0a355d4
minor semantic/syntactical changes
xiexr151e Aug 13, 2020
95cca88
headnode
xiexr151e Aug 13, 2020
fc36393
format
xiexr151e Aug 13, 2020
8bcdf45
next sibling
xiexr151e Aug 13, 2020
79c2296
additional test
xiexr151e Aug 13, 2020
e9295bf
revert next
xiexr151e Aug 13, 2020
ddb10fa
Rearranged import name?
xiexr151e Aug 13, 2020
74ecad2
wrap everything in head
xiexr151e Aug 13, 2020
4a977f5
Merge branch 'derek-sxg-url' of github.com:xiexr151e/amppackager into…
xiexr151e Aug 13, 2020
82b4ad3
firstchild
xiexr151e Aug 13, 2020
dc4c310
various suggestions
xiexr151e Aug 15, 2020
7eb2438
rename test
xiexr151e Aug 15, 2020
adcfd26
more error
xiexr151e Aug 15, 2020
2eefaba
test fix
xiexr151e Aug 15, 2020
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
1 change: 1 addition & 0 deletions CODE_OF_CONDUCT.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
See https://github.com/ampproject/meta/blob/master/CODE_OF_CONDUCT.md
xiexr151e marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 6 additions & 5 deletions transformer/transformer.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ import (
rpb "github.com/ampproject/amppackager/transformer/request"
"github.com/ampproject/amppackager/transformer/transformers"
"github.com/pkg/errors"
"golang.org/x/net/html/atom"
"golang.org/x/net/html"
"golang.org/x/net/html/atom"
)

// Transformer functions must be added here in order to be passed in from
Expand All @@ -44,6 +44,7 @@ var transformerFunctionMap = map[string]func(*transformers.Context) error{
"absoluteurl": transformers.AbsoluteURL,
"ampboilerplate": transformers.AMPBoilerplate,
"ampruntimecss": transformers.AMPRuntimeCSS,
"ampruntimejs": transformers.AMPRuntimeJS,
"linktag": transformers.LinkTag,
"nodecleanup": transformers.NodeCleanup,
"preloadimage": transformers.PreloadImage,
Expand Down Expand Up @@ -72,6 +73,7 @@ var configMap = map[rpb.Request_TransformersConfig][]func(*transformers.Context)
transformers.AMPRuntimeCSS,
transformers.TransformedIdentifier,
transformers.URLRewrite,
transformers.AMPRuntimeJS,
transformers.PreloadImage,
// ReorderHead should run after all transformers that modify the
// <head>, as they may do so without preserving the proper order.
Expand All @@ -90,7 +92,6 @@ var configMap = map[rpb.Request_TransformersConfig][]func(*transformers.Context)
// from an unnecessary number of fetches.
const maxPreloads = 20


// Override for tests.
var runTransformers = func(c *transformers.Context, fns []func(*transformers.Context) error) error {
// Invoke the configured transformers
Expand Down Expand Up @@ -224,11 +225,11 @@ func extractPreloads(dom *amphtml.DOM) []*rpb.Metadata_Preload {
// amp-script without an explicit max-age. This is 1 day, to parallel the
// security precautions put in place around service workers:
// https://dev.chromium.org/Home/chromium-security/security-faq/service-worker-security-faq#TOC-Do-Service-Workers-live-forever-
const defaultMaxAgeSeconds int32 = 86400 // number of seconds in a day
const defaultMaxAgeSeconds int32 = 86400 // number of seconds in a day

// maxMaxAgeSeconds is the max duration of an SXG, per
// https://wicg.github.io/webpackage/draft-yasskin-http-origin-signed-responses.html#signature-validity.
const maxMaxAgeSeconds int32 = 7*86400
const maxMaxAgeSeconds int32 = 7 * 86400

// computeMaxAgeSeconds returns the suggested max-age based on the presence of
// any inline <amp-script> tags on the page; callers should min() the return
Expand Down Expand Up @@ -317,7 +318,7 @@ func Process(r *rpb.Request) (string, *rpb.Metadata, error) {
return "", nil, err
}
metadata := rpb.Metadata{
Preloads: extractPreloads(context.DOM),
Preloads: extractPreloads(context.DOM),
MaxAgeSecs: computeMaxAgeSeconds(context.DOM),
}
return o.String(), &metadata, nil
Expand Down
2 changes: 1 addition & 1 deletion transformer/transformer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestProcess(t *testing.T) {
config rpb.Request_TransformersConfig
expectedLen int
}{
{rpb.Request_DEFAULT, 13},
{rpb.Request_DEFAULT, 14},
{rpb.Request_NONE, 0},
{rpb.Request_VALIDATION, 1},
{rpb.Request_CUSTOM, 0},
Expand Down
38 changes: 38 additions & 0 deletions transformer/transformers/ampruntimejs.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package transformers

import (
"net/url"
"strings"

"github.com/ampproject/amppackager/transformer/internal/amphtml"
"github.com/ampproject/amppackager/transformer/internal/htmlnode"
"golang.org/x/net/html"
"golang.org/x/net/html/atom"
)

// AMPRuntimeJS rewrites the value of src in script nodes, where applicable.
// If the value is of the form "*.js", replace it with "*.js?f=sxg".
func AMPRuntimeJS(e *Context) error {
for n := e.DOM.HeadNode.FirstChild; n != nil; n = n.NextSibling {
if n.Type == html.ElementNode && n.DataAtom == atom.Script {
src, ok := htmlnode.FindAttribute(n, "", "src")
if ok && strings.HasPrefix(src.Val, amphtml.AMPCacheRootURL) {
u, uerr := url.Parse(src.Val)
if uerr != nil {
continue
}
query, queryerr := url.ParseQuery(u.RawQuery)
if queryerr != nil {
continue
}
path := u.Path
if strings.HasSuffix(path, ".js") {
query.Set("f", "sxg")
u.RawQuery = query.Encode()
src.Val = u.String()
}
}
}
}
return nil
}
100 changes: 100 additions & 0 deletions transformer/transformers/ampruntimejs_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
package transformers_test

import (
"strings"
"testing"

"github.com/ampproject/amppackager/transformer/internal/amphtml"
tt "github.com/ampproject/amppackager/transformer/internal/testing"
"github.com/ampproject/amppackager/transformer/transformers"
"golang.org/x/net/html"
)

func TestAmpRuntimeJS(t *testing.T) {
tcs := []tt.TestCase{
{
Desc: "no script node",
Input: "<head></head><body></body>",
Expected: "<head></head><body></body>",
},
{
Desc: "no prefix",
Input: "<head><script src=main.js/></head>",
Expected: "<head><script src=main.js/></head>",
},
{
Desc: "no suffix",
Input: `<head><script src="https://cdn.ampproject.org"/></head>`,
Expected: `<head><script src="https://cdn.ampproject.org"/></head>`,
},
{
Desc: "transformation",
Input: `<head><script async src="https://cdn.ampproject.org/v0.js"></script></head>`,
Expected: `<head><script async src="https://cdn.ampproject.org/v0.js?f=sxg"></script></head>`,
},
{
Desc: "transform on two scripts",
Input: `<head><script async src="https://cdn.ampproject.org/foo.js"></script><script async src="https://cdn.ampproject.org/bar.js"></script></head>`,
Expected: `<head><script async src="https://cdn.ampproject.org/foo.js?f=sxg"></script><script async src="https://cdn.ampproject.org/bar.js?f=sxg"></script></head>`,
},
{
Desc: "skip one, transform one",
Input: `<head><script async src="https://cdn.ampproject.org/"></script><script async src="https://cdn.ampproject.org/bar.js"></script></head>`,
Expected: `<head><script async src="https://cdn.ampproject.org/"></script><script async src="https://cdn.ampproject.org/bar.js?f=sxg"></script></head>`,
},
{
Desc: "additional params exist",
Input: `<head><script async src="https://cdn.ampproject.org/v0.js?optin=beta"></script></head>`,
Expected: `<head><script async src="https://cdn.ampproject.org/v0.js?f=sxg&optin=beta"></script></head>`,
},
{
Desc: "existing f param",
Input: `<head><script async src="https://cdn.ampproject.org/v0.js?f=foo"></script></head>`,
Expected: `<head><script async src="https://cdn.ampproject.org/v0.js?f=sxg"></script></head>`,
},
{
Desc: "url escape parsing bug in query param",
Input: `<head><script async src="https://cdn.ampproject.org/v0.js?x=%"></script></head>`,
Expected: `<head><script async src="https://cdn.ampproject.org/v0.js?x=%"></script></head>`,
},
{
Desc: "url escape parsing bug",
Input: `<head><script async src="https://cdn.ampproject.org/v0.js%%"></script></head>`,
Expected: `<head><script async src="https://cdn.ampproject.org/v0.js%%"></script></head>`,
},
xiexr151e marked this conversation as resolved.
Show resolved Hide resolved
}

for _, tc := range tcs {
inputDoc, err := html.Parse(strings.NewReader(tc.Input))
if err != nil {
t.Errorf("%s: html.Parse failed %q", tc.Input, err)
continue
}
inputDOM, err := amphtml.NewDOM(inputDoc)
if err != nil {
t.Errorf("%s\namphtml.NewDOM for %s failed %q", tc.Desc, tc.Input, err)
continue
}
transformers.AMPRuntimeJS(&transformers.Context{DOM: inputDOM})
var input strings.Builder
if err := html.Render(&input, inputDoc); err != nil {
t.Errorf("%s: html.Render failed %q", tc.Input, err)
continue
}

expectedDoc, err := html.Parse(strings.NewReader(tc.Expected))
if err != nil {
t.Errorf("%s: html.Parse failed %q", tc.Expected, err)
continue
}
var expected strings.Builder
err = html.Render(&expected, expectedDoc)
if err != nil {
t.Errorf("%s: html.Render failed %q", tc.Expected, err)
continue
}
if input.String() != expected.String() {
t.Errorf("%s: Transform=\n%q\nwant=\n%q", tc.Desc, &input, &expected)
}
}
}