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

[bug] ./ and ../ are not valid subpath prefix #67

Closed
ridhoq opened this issue Dec 2, 2023 · 5 comments · Fixed by #68
Closed

[bug] ./ and ../ are not valid subpath prefix #67

ridhoq opened this issue Dec 2, 2023 · 5 comments · Fixed by #68

Comments

@ridhoq
Copy link
Contributor

ridhoq commented Dec 2, 2023

A bug was discovered after upgrading to v0.1.2 where ./ and ../ are not valid prefixes as seen here: guacsec/guac#1545. It appears to be due to this logic:

if s == "." || s == ".." {
	return fmt.Errorf("invalid Package URL subpath: %q", p.Subpath)
}

As per the SPDX spec, file paths are generally prefixed with ./. If a purl was to be constructed using the files read in from an SPDX SBOM, it would throw an error.

@lumjjb
Copy link

lumjjb commented Dec 8, 2023

Hmm seems like the current specification doesn't allow allow some of these subpaths, so I think it would also be a question whether it makes sense to have the first index be an exception due to convention or do a implicit translation of removing the ./ prefix.

I think the ./ case is a little easier to reason about, but the ../ one is a bit tricky (although i've seen some of this, e.g. dynamic references)

@pombredanne do you have any thoughts on this? We are seeing quite a bit of ./ and ../ prefix, and would be good to be able to handle them.

@lumjjb
Copy link

lumjjb commented Jan 11, 2024

we are still running into some of these issues where ./ and ../ carry some meaning with the PURLs that are generated that include them... I do realize this is a bit more of an involved discussion, and wondering if @pombredanne and other PURL maintainers would be amicable to relax this requirement in the code only for now?

@pombredanne
Copy link
Member

Why not stripping a leading ./ on input? The fact that this is supported by SPDX does not mean this is requirement for SPDX nor here.

And reporting an error for ../ which as a relative path which may be eventually impossible to make sense of as this would be outside of a package file tree?

Where did these PURLs come from BTW? If this is from https://github.com/microsoft/sbom-tool from reading through this and related issue then you may want to report a bug there to get the fix upstream? Can you provide some exmaple?

Note that in general, I am not too keen on changing the spec based on the output of one tool in another spec.

@lumjjb
Copy link

lumjjb commented Jan 11, 2024

Yea agreed about the "../" values not making semantic sense for now.. i'm wondering if we can provide a build flag or alternate function that will parse legacy documents while things get eventually fixed? It is likely we'll still see a lot of these "bad PURLs" around for the near future?

@pombredanne
Copy link
Member

@lumjjb it could make sense to publish a simple doc with fixes to apply to well known problematic inputs.

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 a pull request may close this issue.

3 participants