fix(source/cloud-sql-admin): validate source instance for backup-run restore (#3555)
## Description
`Source.RestoreBackup` in
`internal/sources/cloudsqladmin/cloud_sql_admin.go` validates the wrong
field on the BackupRun-ID restore path. When `backup_id` is a numeric
BackupRun ID, the code populates `RestoreBackupContext` from
`sourceProject` and `sourceInstance`, but the guard checks
`sourceProject` and `targetInstance`:
```go
if backupRunID, err := strconv.ParseInt(backupID, 10, 64); err == nil {
if sourceProject == "" || targetInstance == "" { // checks targetInstance
return nil, fmt.Errorf("source project and instance are required when restoring via backup ID")
}
request.RestoreBackupContext = &sqladmin.RestoreBackupContext{
Project: sourceProject,
InstanceId: sourceInstance, // sourceInstance is what's actually consumed
BackupRunId: backupRunID,
}
}
```
`targetInstance` is already mandatory for the API call itself
(`service.Instances.RestoreBackup(targetProject, targetInstance,
request)`), so the guard never adds anything for it. The only thing it
lets through is an empty `source_instance`, which then sends a malformed
`RestoreBackupContext{InstanceId: ""}` to the API.
The error message already reads "source project and instance are
required", and both the tool parameter description
(`internal/tools/cloudsql/cloudsqlrestorebackup/cloudsqlrestorebackup.go`)
and the docs
(`docs/en/integrations/cloud-sql-admin/tools/cloudsqlrestorebackup.md`)
state that both `source_project` and `source_instance` are required when
`backup_id` is a BackupRun ID. So this is a one-character copy/paste
bug, not intended behavior.
**Fix:** guard on `sourceInstance == ""` instead of `targetInstance ==
""`. The error string is unchanged. A regression row is added to the
existing `tests/cloudsql/cloud_sql_restore_backup_test.go` table for the
case where `source_project` is set but `source_instance` is omitted,
which fails before the fix (an empty `InstanceId` reaches the API) and
passes after.
## PR Checklist
- [x] Make sure you reviewed
[CONTRIBUTING.md](https://github.com/googleapis/mcp-toolbox/blob/main/CONTRIBUTING.md)
- [ ] Make sure to open an issue as a
[bug/issue](https://github.com/googleapis/mcp-toolbox/issues/new/choose)
before writing your code! That way we can discuss the change, evaluate
designs, and agree on the general idea
- [x] Ensure the tests and linter pass
- [x] Code coverage does not decrease (if any source code was changed)
- [ ] Appropriate docs were updated (if necessary)
- [ ] Make sure to add `!` if this involve a breaking change
> No tracking issue was opened: this is a small, self-contained
correctness fix with a regression test, and the open `type: bug` issues
are already assigned. Happy to file one and link it with `Fixes #<n> 🦕`
if you would prefer that first. The docs and the tool parameter
description already mark both source fields as required, so no docs
change is needed; this is not a breaking change (validation is only
tightened for input that would already have failed at the API).
Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com>
This commit is contained in:
@@ -412,7 +412,7 @@ func (s *Source) RestoreBackup(ctx context.Context, targetProject, targetInstanc
|
||||
// 'projects/{project-id}/backups/{backup-uid}'. In this case, the Backup
|
||||
// field should be populated.
|
||||
if backupRunID, err := strconv.ParseInt(backupID, 10, 64); err == nil {
|
||||
if sourceProject == "" || targetInstance == "" {
|
||||
if sourceProject == "" || sourceInstance == "" {
|
||||
return nil, fmt.Errorf("source project and instance are required when restoring via backup ID")
|
||||
}
|
||||
request.RestoreBackupContext = &sqladmin.RestoreBackupContext{
|
||||
|
||||
@@ -186,6 +186,12 @@ func TestRestoreBackupToolEndpoints(t *testing.T) {
|
||||
body: `{"target_project": "p1", "target_instance": "instance-project-level", "backup_id": "12345"}`,
|
||||
want: `{"error":"error processing GCP request: source project and instance are required when restoring via backup ID"}`,
|
||||
},
|
||||
{
|
||||
name: "missing source instance with source project for standard backup",
|
||||
toolName: "restore-backup",
|
||||
body: `{"target_project": "p1", "target_instance": "instance-project-level", "backup_id": "12345", "source_project": "p1"}`,
|
||||
want: `{"error":"error processing GCP request: source project and instance are required when restoring via backup ID"}`,
|
||||
},
|
||||
{
|
||||
name: "missing backup identifier",
|
||||
toolName: "restore-backup",
|
||||
|
||||
Reference in New Issue
Block a user