test: unencoded subpath cannot contain . or ..

[jeremylong]

Add test cases to check for invalid . and .. segments in the subpath.

[jkowalleck]

see also: https://github.com/package-url/purl-spec/pull/394/files#r1954552460

[matt-phylum]

I'm wondering if this is actually a good idea. I can agree that the subpath should not contain . or .. segments, and I would prefer if implementations didn't need path normalization logic, but I don't think parsers and formatters should validate that the subpath does not contain . or .. segments. The problem is that if you observe a PURL pkg:golang/google.golang.org/genproto@abcdedf#/googleapis/../api/annotations/ it cannot be understood at all if the parser rejects the .., and it cannot be canonicalized or otherwise parsed and then written back out if the formatter rejects the ... It's syntactically valid and at least the type name version have an clear, unambiguous meaning, but because of the .. everything is thrown away.

[jkowalleck]

[...] but I don't think parsers and formatters should validate that the subpath does not contain . or .. segments.

why? i mean, this is already part of the spec:

• spec

  purl-spec/PURL-SPECIFICATION.rst

  Line 212 in 65d98c4

  - must not be any of '..' or '.'

• generate

  purl-spec/PURL-SPECIFICATION.rst

  Line 322 in 65d98c4

  - Discard empty, '.' and '..' segments

• parse

  purl-spec/PURL-SPECIFICATION.rst

  Line 346 in 65d98c4

  - Discard any '.' or '..' segment from that split

The problem is that if you observe a PURL pkg:golang/google.golang.org/genproto@abcdedf#/googleapis/../api/annotations/ it cannot be understood at all if the parser rejects the .., and it cannot be canonicalized or otherwise parsed and then written back out if the formatter rejects the ... It's syntactically valid and at least the type name version have an clear, unambiguous meaning, but because of the .. everything is thrown away.

per spec, the .. and . are to be discarded, rest is to be kept untouched. to attempt of path-resolution or anything. foo/../bar becomes foo/bar. (not bar)

[matt-phylum]

Then these tests are wrong, right? They have "is_invalid": true, and they include the . and .. in the parsed subpaths.

[jkowalleck]

lets see how the different test scenarios behave:

• passing - parsing the test canonical purl then re-building a purl from these parsed components should return the test canonical purl. there si no way o get this self-fulfilling prophecy
• passed, as the . is expected to be stripped - parsing the test purl should return the components parsed from the test canonical purl
• passing, as the . is expected to be stripped - parsing the test purl then re-building a purl from these parsed components should return the test canonical purl
• passing - building a purl from the test components should return the test canonical purl

[matt-phylum]

I think also it also fails because the parse succeeds without an error, but is_invalid indicates that an error is expected.

[jkowalleck]

revisited the tests. you are right - all test are passing.
will set the is_invalid from true to false

[jkowalleck]

lets see how the different test scenarios behave:

• passing - parsing the test canonical purl then re-building a purl from these parsed components should return the test canonical purl. there si no way o get this self-fulfilling prophecy failing, anyway.
• passed, as the .. is expected to be stripped - parsing the test purl should return the components parsed from the test canonical purl
• passed, as the .. is expected to be stripped - parsing the test purl then re-building a purl from these parsed components should return the test canonical purl
• passing - building a purl from the test components should return the test canonical purl

[matt-phylum]

Should this be with the .. or without? If we're using the value as input to verify that formatting removes the .., then we want .. here, but if we're verifying that parsing removes the .. we wouldn't want it here.

[jkowalleck]

here, but if we're verifying that parsing removes the .. we wouldn't want it here

The same thing probably confused the original author (and me at first review).

that was the reason i looked up the intended test scenarios.

purl-spec/PURL-SPECIFICATION.rst

Lines 473 to 485 in 65d98c4

	To test ``purl`` parsing and building, a tool can use this test suite and for
	every listed test object, run these tests:
	- parsing the test canonical ``purl`` then re-building a ``purl`` from these parsed
	components should return the test canonical ``purl``
	- parsing the test ``purl`` should return the components parsed from the test
	canonical ``purl``
	- parsing the test ``purl`` then re-building a ``purl`` from these parsed components
	should return the test canonical ``purl``
	- building a ``purl`` from the test components should return the test canonical ``purl``

so there is no case where any result is compared against the test components.
there are only cases where the test components are used as input and are expected to produce a canonicalized PURL.