Delete dead code, sanitaze submodule URLs before cloning#6408
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes unused endpoints/CLI functionality and adds validation to prevent dangerous repository URLs (notably submodule URLs) from being used during cloning in VMR patch creation.
Changes:
- Added
GitRepoUrlUtils.IsValidRemoteRepoUriplus NUnit coverage for allowlisted HTTPS remotes and known-dangerous URL formats. - Added a submodule URL validation gate in
VmrPatchHandlerbefore cloning. - Removed dead code: PCS
AzDevControllerand DARCclonecommand implementation/registration.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Maestro/Maestro.Common.Tests/GitRepoUrlUtilsTests.cs | Adds tests for allowlisted vs. dangerous repo URL inputs. |
| src/Maestro/Maestro.Common/GitRepoUrlUtils.cs | Introduces repo URL/path allowlist helper used to protect cloning. |
| src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs | Validates submodule URL before invoking clone manager. |
| src/ProductConstructionService/ProductConstructionService.Api/Controllers/AzDevController.cs | Deletes unused API controller. |
| src/Microsoft.DotNet.Darc/Darc/Program.cs | Removes clone verb registration from CLI options list. |
| src/Microsoft.DotNet.Darc/Darc/Options/CloneCommandLineOptions.cs | Deletes unused clone verb options type. |
| src/Microsoft.DotNet.Darc/Darc/Operations/CloneOperation.cs | Deletes unused deep-clone operation implementation. |
| { | ||
| var checkoutCommit = change.Before == Constants.EmptyGitObject ? change.After : change.Before; | ||
|
|
||
| if (!GitRepoUrlUtils.IsValidRemoteRepoUri(change.Url)) |
There was a problem hiding this comment.
I don't understand the URI validation. We can have submodules from wherever?
There was a problem hiding this comment.
Do you mean submodules don't have to be in GitHub or AzDo?
There was a problem hiding this comment.
Not necessarily. We used to have one from Google from whatever address.
But also how does this fix the issue that we'd send the token from the parent repo to the submodule when getting it? Isn't that what the issue was about?
https://dev.azure.com/dnceng/internal/_workitems/edit/11164
https://dev.azure.com/dnceng/internal/_workitems/edit/11165
https://dev.azure.com/dnceng/internal/_workitems/edit/11167
Deletes AzDevController and
darc cloneoperation as they were dead code.Also sanitizes submodule URLs