Escape delegated role name when reading cached metadata#755
Open
sueun-dev wants to merge 1 commit into
Open
Conversation
persistMetadata writes cached metadata under url.PathEscape(roleName), but loadTargets read it back using the raw role name. A delegated role whose name contains a character PathEscape rewrites (a space, "/", ...) was therefore never found in the cache and re-downloaded on every refresh, and a name like ".." resolved outside LocalMetadataDir. Share a single localMetadataPath helper between the write and the delegated read so the two agree on the file name. The ".json" suffix is added before the join so a "." or ".." name stays inside the directory. Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
There was a problem hiding this comment.
Pull request overview
Fixes delegated-targets cache misses (and potential path traversal) by ensuring delegated role names are consistently URL-escaped for both cache writes and reads, so cached metadata is found even when role names contain special characters.
Changes:
- Introduced
localMetadataPathand used it in bothpersistMetadataand delegated-role cache reads inloadTargets. - Updated delegated-role cache read to use the escaped filename (matching the write path).
- Added a round-trip test covering delegated role names with spaces,
/,.,.., and%.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| metadata/updater/updater.go | Aligns delegated cached-metadata read path with the existing escaped write path via a shared helper. |
| metadata/updater/updater_delegated_cache_test.go | Adds coverage for escaped delegated-role cache filenames, including edge-case role names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.Equal(t, dir, filepath.Dir(path)) | ||
| assert.Equal(t, url.PathEscape(roleName)+".json", filepath.Base(path)) | ||
|
|
||
| // The read loadTargets performs recovers the persisted bytes. |
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.
loadTargetsreads cached delegated metadata fromfilepath.Join(LocalMetadataDir, roleName), butpersistMetadatawrites it underurl.PathEscape(roleName). When a delegated role name contains a characterPathEscaperewrites (a space,/,%, ...), the read and write paths differ, so the cached file is never found and the role's metadata is re-downloaded on every refresh. A name like..also makes the read resolve outsideLocalMetadataDir.The escape landed on the write and download paths in #675, but the delegated read still used the raw name.
What changed:
localMetadataPathand used it for both the write inpersistMetadataand the delegated read inloadTargets, so the two always agree on the file name. Appending.jsonbefore the join keeps a.or..name inside the metadata directory./,.,.., and%.Checked:
GODEBUG=rsa1024min=0 go test ./metadata/...go vet ./...