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. |
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)
}
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:
|
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
Member
|
@copilot Will you please rebase this PR against latest changes in |
…kaside-url-substitution # Conflicts: # internal/providers/sourceproviders/fedorasource/fedorasource.go # internal/providers/sourceproviders/fedorasource/fedorasource_test.go Co-authored-by: reubeno <10508433+reubeno@users.noreply.github.com>
Contributor
Author
Merged latest |
Tonisal-byte
approved these changes
Apr 22, 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.
url.PathEscape()to all placeholder values before substitution inBuildLookasideURLBuildDistGitURLwith the same escaping for$pkgsubstitutionstrings.ReplaceAlldist-git URL constructions withBuildDistGitURLhashTypeplaceholder guard to validate the lowercased valuehashTypeonce at function start instead of twice%#qfor placeholder values in error messagesvalidateAbsoluteURLhelper to deduplicate URL validationvalidateAbsoluteURLandBuildLookasideURLmaininto branch (resolving conflicts with newFormatSourcesEntryand related changes)