Repository navigation
Conversation
Generate changelog in
|
| if part == "." || part == ".." || url.PathEscape(part) != part { | ||
| return "", "", fmt.Errorf("invalid remote value: %s", r.Remote) | ||
| } | ||
| } |
There was a problem hiding this comment.
While this is reasonable as a hardening measure, I don't think its required and it could impact all callers. Maybe this could be its own PR and swapped to be applied as one of the optional functions for each client? Alternatively, only in the new LoadRemoteRef func?
| // LoadRemoteRef returns a literal file without parsing references or searching | ||
| // other paths or owner defaults. Path and ref defaults are defined by [RemoteRef]. | ||
| // Missing files are errors; empty files retain their source metadata. | ||
| // | ||
| // clientOwner must be client's original installation owner, not a loaded file's | ||
| // owner. It is trusted caller context. See [WithPrivateRemotes] for cross-owner | ||
| // access and its information disclosure warning. | ||
| // | ||
| // On load errors, Config identifies the remote file, with a ref once resolved. | ||
| // An invalid remote repository value returns an empty Config and an error. |
There was a problem hiding this comment.
This is a bit hard to follow, maybe:
// LoadRemoteRef loads the file identified by remote. Unlike LoadConfig, it
// does not follow remote references, try other paths, or fall back to owner
// defaults. A missing file is an error.
//
// clientOwner is the owner of the installation for that client.
// It determines whether WithPrivateRemotes selects a different installation.
| } else if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if calls != 1 || cfg.IsUndefined() || len(cfg.Content) != 0 || cfg.Source != "remote/config@main" || cfg.Path != "first.yml" || !cfg.IsRemote { |
There was a problem hiding this comment.
This makes it hard to understand what failed. These should be split up.
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think most of this testing is already covered by appconfig_test.go, so could be removed. I'd reduce it to one table test of about three cases (the literal content returned unparsed, missing file is an error, ref defaults to the default branch), and add it as cases in appconfig_test.go using the response player.
this exposes the existing remote config fetch as LoadRemoteRef so policy-bot can load the repo policy and shared parent separately, then combine them in a later change.
it returns the file contents without following another remote reference or falling back to an org default. it uses the same client selection, private-remotes setting, caching, cancellation and error handling we already have.
also rejects malformed repo references that could change the request URL before any installation lookup or fetch.