fix: update canonical URL validation for 6.2.11 - #1008
bshek-xitaso wants to merge 2 commits into
Conversation
8accbf1 to
f118d8f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The shared implementation unintentionally applies CSAF 2.1 requirements to CSAF 2.0 and lacks regression tests.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates canonical URL validation for CSAF 2.1’s revised requirements.
Changes:
- Requires a slash-delimited filename and non-empty authority.
- Updates rule documentation.
File summaries
| File | Description |
|---|---|
csaf-rs/src/validations/test_6_2_11.rs |
Documents revised requirements. |
csaf-rs/src/csaf/traits/document_trait.rs |
Implements stricter URL filtering. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
peinjoh
left a comment
There was a problem hiding this comment.
During review, I've added some more test cases for the unit test coverage. I'll also provde a patch file to you for convenience.
// empty
#[case::no_references(None, 0)]
// canonical urls
#[case::match_simple_path(Some(vec![make_ref21("self", HTTPS_MATCH)]), 1)]
#[case::match_well_known(Some(vec![make_ref21("self", HTTPS_MATCH_WELL_KNOWN)]), 1)]
// non-canonical urls
#[case::reference_category_external(Some(vec![make_ref21("external", HTTPS_MATCH)]), 0)]
#[case::url_schema_http(Some(vec![make_ref21("self", HTTP_MATCH)]), 0)]
#[case::url_schema_ftp(Some(vec![make_ref21("self", "ftp://example.com/example-company-2019-yh3234.json")]), 0)]
#[case::wrong_filename(Some(vec![make_ref21("self", "https://example.com/example-company-2019-yh3235.json")]), 0)]
#[case::only_domain(Some(vec![make_ref21("self", "https://example.com")]), 0)]
#[case::only_domain_with_delim(Some(vec![make_ref21("self", "https://example.com/")]), 0)]
#[case::empty_authority(Some(vec![make_ref21("self", "https:///example-company-2019-yh3234.json")]), 0)]
#[case::empty_authority_additional_path(Some(vec![make_ref21("self", "https:////example-company-2019-yh3234.json")]), 0)]
#[case::filename_as_authority(Some(vec![make_ref21("self", "https://example-company-2019-yh3234.json")]), 0)]
#[case::with_fragment(Some(vec![make_ref21("self", "https://example.com/example-company-2019-yh3234.json#fragment")]), 0)]
#[case::with_param(Some(vec![make_ref21("self", "https://example.com/example-company-2019-yh3234.json?foo=1")]), 0)]
could still pass this check even though they do not contain a valid non-empty hostname. Please also add those as a) unit test coverage with an accompanying note and b) as passing test cases. |
474600e to
8ba0433
Compare
Gronner
left a comment
There was a problem hiding this comment.
LGTM, just a remark. Otherwise I'd wait for peinjoh's approval after adding the test cases.
peinjoh
left a comment
There was a problem hiding this comment.
LGTM, thank you for integrating the improved test coverage and adding the known limitations!
I'm with @Gronner here, rsplit_once is actually what we want, both in intention and performance. Please make that change, then we can merge 👍
There was a problem hiding this comment.
On a second reading:
I think the known limitations are actually forbidden by the prose. The 4th constraint in the prose is: "The hostname is not empty.", in the comments, we describe the authority to be non-empty and check for that in the code.
Both in RFC 3986 and the HTTP RFC's, host is distinguished from username (http does not have that) / port, see the RFC 3986 ABNF: authority = [ userinfo "@" ] host [ ":" port ].
If we go of this, we should split / rsplit the authority again, and check if host is actually empty. The 3 limitations could then be used as more supplemental test cases.
At this point, would it make sense to include the |
We should probably benchmark the cost of that but yeah. |
6ec8b50 to
a7b75ff
Compare
a7b75ff to
eb43885
Compare
This PR resolves #297.
It updates the canonical URL validation for rule 6.2.11 according to the changes introduced in OASIS CSAF Editor Revision 2026-09-02 (non-empty hostname, filename preseded with "/").
The implementation has been checked against the newly added OASIS test cases 04 and 05, and both are rejected as expected. However, these test cases are currently only available in the latest version of the OASIS repository.
Discussion Note:
As discussed, the current hostname check should be considered an MVP. The implementation checks that the authority part between
https://and thepathis non-empty, but does not fully parse the URL authority.Because of that, edge cases such as:
https://:443/file.jsonhttps://@:/file.jsonhttps://userinfo@/file.jsoncould still pass this check even though they do not contain a valid non-empty hostname.
If stricter URL validation is needed, we should consider using an URL parser.