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

Address review comments #1

Merged

Conversation

aristotelos
Copy link

Revive package-url#245 by adding the following changes:

  • registry_url -> repository_url
  • registry_version -> port_revision
  • Remove percent escaping from repository_url to be more consistent with other uses of repository_url in the purl spec

@aristotelos
Copy link
Author

Hi @michaelbprice, because security is important I am trying to revive your package-url#245 by addressing some open review comments. I see you didn't yet respond to my comments. I hope you have some time to review this PR and if it is OK to you, merge it, so that your PR can be moved forward.

Revive package-url#245 by adding the
following changes:

- `registry_url` -> `repository_url`
- `registry_version` -> `port_revision`
- Remove percent escaping from `repository_url` to be more consistent
  with other uses of `repository_url` in the purl spec
@michaelbprice
Copy link
Owner

Currently very busy with CppCon. I'll take a look at your comment in my PR and this one that will update it when I get a chance. Feel free to ping me on it next week if I don't get to it by this weekend.

@aristotelos
Copy link
Author

Do take the time to enjoy CppCon!

@aristotelos
Copy link
Author

aristotelos commented Sep 30, 2024

Feel free to ping me on it next week if I don't get to it by this weekend.

@michaelbprice Ping! 😄

@aristotelos
Copy link
Author

@michaelbprice Ping 2!

@michaelbprice
Copy link
Owner

I'm still not convinced that percent-encoding the registry URL is "wrong", but I can live with removing that language. As was pointed out on the PR into the upstream, percent encoding could theoretically still be done, and parsers would need to deal with it. The situations where it would be absolutely needed (a registry URL that has qualifiers itself separated by ampersands) are unlikely to occur.

@michaelbprice
Copy link
Owner

I'd still like to retain the registry_version qualifier, but we have language that still gives us some flexibility going forward. The reason why I think it is crucial is that along with the repository_url it allows you to narrow down to a specific point-in-time. Otherwise, it's possible that a PURL could not be traced back to the real version of the registry that was used when the PURL was generated. The simplest case I could think of would be that a user makes a mistake and changes a port in a way that is meaningful but doesn't update the port version.

Without that snapshot in time, I'm worried that accidents will happen that will cause confusion or even that a malicious user could wreak havoc in some way that I haven't yet foreseen. But with that commit SHA there, if something has changed, at least someone investigating and tracing backwards could see that something has gone wrong.

PURL-TYPES.rst Outdated Show resolved Hide resolved
aristotelos and others added 3 commits October 10, 2024 07:43
Co-authored-by: Michael B. Price <[email protected]>
Add optional `repository_revision` so that mistakes in `port_revision` and
`version` can be accounted for. Not relevant for filesystem registries
or overlay ports because that gives no further external traceability.

Along with this, describe the filesystem registries and overlay port
cases.
Extend the port overlay or filesystem registry example with port
revision.

Add an example for additional qualifiers.
@aristotelos
Copy link
Author

I'd still like to retain the registry_version qualifier, but we have language that still gives us some flexibility going forward. The reason why I think it is crucial is that along with the repository_url it allows you to narrow down to a specific point-in-time. Otherwise, it's possible that a PURL could not be traced back to the real version of the registry that was used when the PURL was generated. The simplest case I could think of would be that a user makes a mistake and changes a port in a way that is meaningful but doesn't update the port version.

Without that snapshot in time, I'm worried that accidents will happen that will cause confusion or even that a malicious user could wreak havoc in some way that I haven't yet foreseen. But with that commit SHA there, if something has changed, at least someone investigating and tracing backwards could see that something has gone wrong.

Good point, I have added a registry_revision back for online registries. I however think it is not relevant for filesystem registries or overlay ports because those are not online and therefore traceability will not help. Filesystem registries are an edge case anyways so I think adding additional complexity to the purl spec for vcpkg for that case is not the best thing to do.

I saw I also did not describe the filesystem registry or overlay port concept yet, so added that back.

Also added a full example with additional, unspecified qualifiers.

@michaelbprice
Copy link
Owner

LGTM. Let's go! Hopefully this revives the upstream PR. They've got quite a backlog though.

@michaelbprice michaelbprice merged commit a2b507a into michaelbprice:vcpkg-purl-spec Oct 10, 2024
@aristotelos aristotelos deleted the vcpkg-purl-spec branch October 11, 2024 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants