fix: URL-escape placeholder values in BuildLookasideURL and dist-git URL construction#74
Open
fix: URL-escape placeholder values in BuildLookasideURL and dist-git URL construction#74
Conversation
…DistGitURL Apply url.PathEscape() to all placeholder values before substitution in BuildLookasideURL so reserved URL characters (/, ?, #, %) don't alter URL structure. Parse and validate the final URL before returning. Create BuildDistGitURL for dist-git URL construction with the same path-escaping and validation protections. Update the two raw strings.ReplaceAll call sites in fedorasourceprovider.go to use the new function. Add comprehensive test coverage for both functions with reserved URL characters in filename and packageName inputs. Agent-Logs-Url: https://github.com/microsoft/azure-linux-dev-tools/sessions/c56e9983-31d5-4a6d-9ac6-e6fee388466f Co-authored-by: dmcilvaney <23200982+dmcilvaney@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix URL encoding in BuildLookasideURL for valid substitutions
fix: URL-escape placeholder values in BuildLookasideURL and dist-git URL construction
Apr 7, 2026
dmcilvaney
approved these changes
Apr 7, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens Fedora lookaside and dist-git URL generation by URL-escaping substituted placeholder values and validating the resulting URLs to prevent reserved characters from silently altering URL structure.
Changes:
- Path-escape placeholder substitutions in
BuildLookasideURLand validate the resulting URL. - Introduce
BuildDistGitURL(escaped + validated) and use it in the Fedora source provider. - Add tests covering reserved characters in filenames and package names for both URL builders.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/providers/sourceproviders/fedorasource/fedorasource.go | Escapes placeholder substitutions, adds URL validation, and introduces BuildDistGitURL. |
| internal/providers/sourceproviders/fedorasourceprovider.go | Switches dist-git URL construction to BuildDistGitURL and propagates errors. |
| internal/providers/sourceproviders/fedorasource/fedorasource_test.go | Adds regression tests for reserved characters and malformed percent sequences. |
internal/providers/sourceproviders/fedorasource/fedorasource.go
Outdated
Show resolved
Hide resolved
internal/providers/sourceproviders/fedorasource/fedorasource.go
Outdated
Show resolved
Hide resolved
Contributor
|
@copilot Can you have a look at the review comments? Two seem worth investigating. Please double check the error formatting one, and imo the template count isn't an issue (but open to pushback). |
…ideURL The placeholder-in-value guard checked raw hashType but substitution uses strings.ToLower(hashType). This meant hashType="$PKG" bypassed the guard and was substituted as "$pkg" (matching PlaceholderPkg). Check the normalized (lowercased) hashType against placeholders to close this bypass. Add test case for the scenario. Agent-Logs-Url: https://github.com/microsoft/azure-linux-dev-tools/sessions/ac27db32-1692-4bde-bb94-c7c0b4f0fa5d Co-authored-by: dmcilvaney <23200982+dmcilvaney@users.noreply.github.com>
Contributor
Author
Investigated both items:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
internal/providers/sourceproviders/fedorasource/fedorasource.go:333
- When the placeholder check fails for
hashType, the error reports the normalized (lowercased) value rather than the caller-provided value (e.g.,$PKGwill be reported as$pkg). To keep error messages actionable, consider checking with the normalized value but reporting the original input in the error (or include both original and normalized forms).
// Check the normalized (lowercased) hashType since that is the form actually substituted.
for _, v := range []string{packageName, fileName, strings.ToLower(hashType), hash} {
for _, p := range allPlaceholders {
if strings.Contains(v, p) {
return "", fmt.Errorf("value %#q contains placeholder %s, which would cause ambiguous substitution", v, p)
}
internal/providers/sourceproviders/fedorasource/fedorasource.go
Outdated
Show resolved
Hide resolved
internal/providers/sourceproviders/fedorasource/fedorasource.go
Outdated
Show resolved
Hide resolved
Replace the validateAbsoluteURL helper with inline validation per reviewer suggestion: url.Parse() for syntax, then check u.Scheme and u.Host are non-empty. This catches misconfigured templates (e.g. missing scheme) that url.Parse alone would silently accept. Agent-Logs-Url: https://github.com/microsoft/azure-linux-dev-tools/sessions/661f751e-7043-4004-9580-01a1ea6036f3 Co-authored-by: dmcilvaney <23200982+dmcilvaney@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
dmcilvaney
approved these changes
Apr 7, 2026
PawelWMS
reviewed
Apr 7, 2026
|
|
||
| for _, v := range []string{packageName, fileName, hashType, hash} { | ||
| // Check the normalized (lowercased) hashType since that is the form actually substituted. | ||
| for _, v := range []string{packageName, fileName, strings.ToLower(hashType), hash} { |
Contributor
There was a problem hiding this comment.
Nit-pick: we're doing strings.ToLower(hashType) twice inside BuildLookasideURL. What do you think about doing hashType = strings.ToLower(hashType) at the beginning of the function to avoid repetition?
PawelWMS
reviewed
Apr 7, 2026
| // resulting URL is not valid. | ||
| func BuildDistGitURL(template, packageName string) (string, error) { | ||
| if strings.Contains(packageName, PlaceholderPkg) { | ||
| return "", fmt.Errorf("package name %#q contains placeholder %s, which would cause ambiguous substitution", |
Contributor
There was a problem hiding this comment.
Suggested change
| return "", fmt.Errorf("package name %#q contains placeholder %s, which would cause ambiguous substitution", | |
| return "", fmt.Errorf("package name %#q contains placeholder %#q, which would cause ambiguous substitution", |
PawelWMS
approved these changes
Apr 7, 2026
Contributor
PawelWMS
left a comment
PawelWMS
left a comment
There was a problem hiding this comment.
Left small nit-pick comments but looks good even if they are ignored.
Contributor
|
@copilot please look at PawelWMS's comments. |
Address PawelWMS's review nit-picks: - Normalize hashType with strings.ToLower() once at the top of BuildLookasideURL instead of calling it twice - Use %#q instead of %s for placeholder values in error messages for consistency with other format verbs in the same messages Agent-Logs-Url: https://github.com/microsoft/azure-linux-dev-tools/sessions/577c691c-31b1-42bc-8827-4f7cdad70ddd Co-authored-by: dmcilvaney <23200982+dmcilvaney@users.noreply.github.com>
Contributor
Author
Both nit-picks addressed in
|
PawelWMS
approved these changes
Apr 8, 2026
Contributor
|
@copilot would it be worth just making a validator function here? |
Extract the duplicated url.Parse + scheme/host validation into a shared validateAbsoluteURL helper used by both BuildLookasideURL and BuildDistGitURL. Agent-Logs-Url: https://github.com/microsoft/azure-linux-dev-tools/sessions/a9b06a03-7631-4307-a461-8a366892b221 Co-authored-by: dmcilvaney <23200982+dmcilvaney@users.noreply.github.com>
Contributor
Author
Extracted |
Contributor
|
@copilot validateAbsoluteURL and BuildLookasideURL comments seem to have merged a bit, is that intentional formatting? The linter does seem ok with it, but it reads oddly to me. If it is not intentional, can you fix it please? |
Move the BuildLookasideURL doc comment to directly precede its function declaration, so it is no longer merged with the validateAbsoluteURL comment block. Agent-Logs-Url: https://github.com/microsoft/azure-linux-dev-tools/sessions/30580b36-4aef-450d-ad65-a38597d233ec Co-authored-by: dmcilvaney <23200982+dmcilvaney@users.noreply.github.com>
Contributor
Author
Not intentional — when I extracted |
dmcilvaney
approved these changes
Apr 8, 2026
PawelWMS
approved these changes
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BuildLookasideURLand dist-git URL construction perform raw string substitution for$pkg,$filename,$hashtype, and$hashwithout URL-encoding. Reserved characters (/,?,#,%) in substituted values alter URL structure silently.Changes
fedorasource.go: Applyurl.PathEscape()to all placeholder values before substitution inBuildLookasideURL; validate the final URL via a sharedvalidateAbsoluteURLhelper that callsurl.Parse()and verifies a non-empty scheme and hostfedorasource.go: AddBuildDistGitURLwith the same escaping and validation for$pkgsubstitutionfedorasource.go: ExtractvalidateAbsoluteURLhelper to deduplicate URL validation logic shared by bothBuildLookasideURLandBuildDistGitURLfedorasource.go: NormalizehashTypeto lowercase once at the start ofBuildLookasideURLto avoid redundantstrings.ToLower()calls and ensure the placeholder guard validates the actual substituted formfedorasource.go: Validate constructed URLs are absolute with a non-empty scheme and host, rejecting misconfigured templates thaturl.Parsealone would silently accept (e.g.example.com/$pkg)fedorasource.go: Use%#qfor placeholder values in error messages for consistency with other format verbsfedorasource.go: Fix doc comment placement sovalidateAbsoluteURLandBuildLookasideURLeach have properly separated, correctly attached doc commentsfedorasourceprovider.go: Replace two rawstrings.ReplaceAlldist-git URL constructions withBuildDistGitURLfedorasource_test.go: Add test coverage for/,?,#,%zzin bothfilenameandpackageNamefor both builders, plus tests for the uppercase hashType placeholder bypass and missing-scheme template rejectionExample