Skip to content

Commit

Permalink
fix: escape everything with modified QueryEscape
Browse files Browse the repository at this point in the history
This commit switches to using `QueryEscape` for escaping all components.
However, because `QueryEscape` escapes ` ` (space) to `+`, we actually
change that to a `%20`, that is the percent-encoded equivalent.

`QueryEscape` was built for HTTP Query parameters, and although there is
[some discussion](https://stackoverflow.com/questions/2678551/when-should-space-be-encoded-to-plus-or-20)
around it, escaping ` ` to a `+` is completely valid.
Sadly, other languages like Javascript don't handle that properly, so if
we simply used `QueryEscape`, the purl couldn't be parsed by other
implementations.
By using the universally supported `%20` instead, we restore
compatibility.
  • Loading branch information
tommyknows authored and shibumi committed Aug 12, 2023
1 parent 3587d8c commit dd78cab
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 11 deletions.
32 changes: 24 additions & 8 deletions packageurl.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,18 +200,23 @@ func (p *PackageURL) ToString() string {
Fragment: p.Subpath,
}

nameWithVersion := url.PathEscape(p.Name)
paths := []string{p.Type}
// we need to escape each segment by itself, so that we don't escape "/" in the namespace.
for _, segment := range strings.Split(p.Namespace, "/") {
if segment == "" {
continue
}
paths = append(paths, escape(segment))
}

nameWithVersion := escape(p.Name)
if p.Version != "" {
nameWithVersion += "@" + p.Version
nameWithVersion += "@" + escape(p.Version)
}

// we use JoinPath and EscapedPath as the behavior for "/" is only correct with that.
// We don't want to escape "/", but want to escape all other characters that are necessary.
u = u.JoinPath(p.Type, p.Namespace, nameWithVersion)
// write the actual path into the "Opaque" block, so that the generated string at the end is
// pkg:<path> and not pkg://<path>.
u.Opaque, u.Path = u.EscapedPath(), ""
paths = append(paths, nameWithVersion)

u.Opaque = strings.Join(paths, "/")
return u.String()
}

Expand Down Expand Up @@ -263,6 +268,17 @@ func FromString(purl string) (PackageURL, error) {
return pURL, validCustomRules(pURL)
}

// escape the given string in a purl-compatible way.
func escape(s string) string {
// for compatibility with other implementations and the purl-spec, we want to escape all
// characters, which is what "QueryEscape" does. The issue with QueryEscape is that it encodes
// " " (space) as "+", which is valid in a query, but invalid in a path (see
// https://stackoverflow.com/questions/2678551/when-should-space-be-encoded-to-plus-or-20) for
// context).
// To work around that, we replace the "+" signs with the path-compatible "%20".
return strings.ReplaceAll(url.QueryEscape(s), "+", "%20")
}

func separateNamespaceNameVersion(path string) (ns, name, version string, err error) {
name = path

Expand Down
6 changes: 3 additions & 3 deletions packageurl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -302,12 +302,12 @@ func TestQualifiersMapConversion(t *testing.T) {

func TestNameEscaping(t *testing.T) {
testCases := map[string]string{
"abc": "pkg:abc",
"ab/c": "pkg:ab%2Fc",
"abc": "pkg:deb/abc",
"ab/c": "pkg:deb/ab%2Fc",
}
for name, output := range testCases {
t.Run(name, func(t *testing.T) {
p := &packageurl.PackageURL{Name: name}
p := &packageurl.PackageURL{Type: "deb", Name: name}
if s := p.ToString(); s != output {
t.Fatalf("wrong escape. expected=%q, got=%q", output, s)
}
Expand Down

0 comments on commit dd78cab

Please sign in to comment.