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

Encode the origin of the package in PURL #523

Merged
merged 49 commits into from
Nov 12, 2023
Merged

Conversation

Shnatsel
Copy link
Contributor

Fixes #501 and #231

Supersedes #69

@Shnatsel Shnatsel requested a review from a team as a code owner October 31, 2023 18:14
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Comment on lines 29 to 30
//! 2. <https://url.spec.whatwg.org/#application-x-www-form-urlencoded-percent-encode-set>
//! to be used for form submission, so not our case?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a mistake to believe that this is only used for form submission. This same exact encoding is used to set the query string of a URL (because that's where form submission data goes when the method is GET anyways).

The spec even states that this set will be used for anything in the URLSearchParams class, which is used for generating query strings (or body data in the case of POST form submission).

That is why, for example, the whatwg-url package (which is synchronized with the WHATWG URL spec), encodes these characters when set using the searchParam attribute. For example:

import { URL } from 'whatwg-url';
const x = new URL('pkg:generic/bitwarderl');
x.searchParams.set('vcs_url', 'vcs_url=git+https://github.com/rustsec/rustsec.git@6f82509cb43a')
console.log(x.toString());

Outputs:

pkg:generic/bitwarderl?vcs_url=vcs_url%3Dgit%2Bhttps%3A%2F%2Fgithub.com%2Frustsec%2Frustsec.git%406f82509cb43a

It is fine to use characters such as + unencoded in the query string, but not if you are parsing that query string as search parameters (as PURL does).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even that cannot be used to produce the official example of ?vcs_url=git%2Bhttps://git.fsfe.org/dxtr/bitwarderl%40cc55108da32, because the application-x-www-form-urlencoded set encodes : and / but they are left unencoded in the official example.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it would be a question to the PURL maintainers to see if they actually intend qualifiers to be interoperable with URLSearchParams. If so, mandatory percent encoding of + should be mentioned in the spec...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pombredanne is this something you can maybe weigh in on?

… the WHATWG spec

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
…load_url field

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
…o measurable difference in performance.

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
…with more specific details and references, document the criteria for revisiting the current design decision.

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
…oding scheme, and the rationale for the current behavior

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
@Shnatsel
Copy link
Contributor Author

Shnatsel commented Nov 3, 2023

Turning into a draft until the reproducibility issues are addressed.

@Shnatsel Shnatsel marked this pull request as draft November 3, 2023 20:42
… being passed around, because it doesn't change over time

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
…kspace root.

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
…paths within the workspace

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
…s located under the workspace root) as requested in code review

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
@Shnatsel Shnatsel marked this pull request as ready for review November 8, 2023 02:50
@Shnatsel
Copy link
Contributor Author

Shnatsel commented Nov 8, 2023

I've implemented relative paths for packages within the workspace to allow for more reproducible builds (at least in PURLs, there are other aspects of the SBOM that are not reproducible).

This needs even more tests but other than that it's ready to go, PTAL.

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
@lfrancke
Copy link
Contributor

For full reproducability we have another issue

Copy link
Contributor

@lfrancke lfrancke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clippy warnings are only in test code but still. Otherwise looks good. Thanks for the tests and the attention to detail!

let purl = get_purl(
&crates_io_package,
&crates_io_package,
&Utf8Path::new("/foo/bar"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&Utf8Path::new("/foo/bar"),
Utf8Path::new("/foo/bar"),

#[test]
fn git_purl() {
let git_package: Package = serde_json::from_str(GIT_PACKAGE_JSON).unwrap();
let purl = get_purl(&git_package, &git_package, &Utf8Path::new("/foo/bar"), None).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let purl = get_purl(&git_package, &git_package, &Utf8Path::new("/foo/bar"), None).unwrap();
let purl = get_purl(&git_package, &git_package, Utf8Path::new("/foo/bar"), None).unwrap();

let purl = get_purl(
&root_package,
&root_package,
&Utf8Path::new("/home/shnatsel/Code/cargo-cyclonedx/"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&Utf8Path::new("/home/shnatsel/Code/cargo-cyclonedx/"),
Utf8Path::new("/home/shnatsel/Code/cargo-cyclonedx/"),

let purl = get_purl(
&root_package,
&root_package,
&Utf8Path::new("/home/shnatsel/Code/cargo-cyclonedx/"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&Utf8Path::new("/home/shnatsel/Code/cargo-cyclonedx/"),
Utf8Path::new("/home/shnatsel/Code/cargo-cyclonedx/"),

let purl = get_purl(
&workspace_package,
&root_package,
&Utf8Path::new("/home/shnatsel/Code/cargo-cyclonedx/"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&Utf8Path::new("/home/shnatsel/Code/cargo-cyclonedx/"),
Utf8Path::new("/home/shnatsel/Code/cargo-cyclonedx/"),

let purl = get_purl(
&workspace_package,
&root_package,
&Utf8Path::new("/foo/bar/"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&Utf8Path::new("/foo/bar/"),
Utf8Path::new("/foo/bar/"),

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
@Shnatsel Shnatsel merged commit c8658cc into CycloneDX:main Nov 12, 2023
8 checks passed
@lfrancke
Copy link
Contributor

This means #69 can be closed, right?

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.

Include information on dependency origin (crates.io, git, custom registry)
3 participants