Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 66.77% 72.37% +5.60%
==========================================
Files 34 34
Lines 1845 1564 -281
==========================================
- Hits 1232 1132 -100
+ Misses 493 309 -184
- Partials 120 123 +3
... and 21 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to upgrade the project to Go 1.26.0 and update dependencies. The changes include:
Changes:
- Upgrade Go version from 1.24.2 to 1.26.0 in go.mod
- Update golang.org/x dependencies (oauth2, sync, time, net, text) to newer versions
- Update github.com/twpayne/go-gpx and github.com/rogpeppe/go-internal dependencies
- Add SSRF protection validation for Zwift S3 URLs in activity exports
- Add gosec linter suppressions with justifications
- Remove loop variable reassignment pattern
tt := ttfrom one test file - Change Photo.Type field from string to int in Strava model
- Replace fmt.Sprintf with fmt.Appendf in xfer.go
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Upgrades Go version to 1.26.0 and updates dependency versions |
| go.sum | Updates checksums for upgraded dependencies |
| zwift/activity.go | Adds SSRF validation for S3 URLs and gosec suppression comment |
| zwift/zwift.go | Adds gosec linter suppression for OAuth endpoint |
| strava/strava.go | Adds gosec linter suppression for OAuth endpoint |
| cyclinganalytics/cyclinganalytics.go | Adds gosec linter suppression for OAuth endpoint |
| rwgps/model.go | Adds gosec linter suppression for auth token field |
| xfer_test.go | Removes loop variable reassignment pattern |
| xfer.go | Replaces fmt.Sprintf with fmt.Appendf for JSON marshaling |
| strava/model.go | Changes Photo.Type field from string to int |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // validateZwiftS3URL validates that the URL is a legitimate Zwift S3 bucket URL | ||
| func validateZwiftS3URL(rawURL string) error { | ||
| parsedURL, err := url.Parse(rawURL) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid URL: %w", err) | ||
| } | ||
|
|
||
| // Ensure HTTPS scheme | ||
| if parsedURL.Scheme != "https" { | ||
| return fmt.Errorf("invalid URL scheme: expected https, got %s", parsedURL.Scheme) | ||
| } | ||
|
|
||
| // Validate that the host is an S3 bucket in amazonaws.com | ||
| if !strings.HasSuffix(parsedURL.Host, ".s3.amazonaws.com") { | ||
| return fmt.Errorf("invalid host: expected *.s3.amazonaws.com, got %s", parsedURL.Host) | ||
| } | ||
|
|
||
| // Validate bucket name contains "zwift" (case-insensitive) | ||
| bucketName := strings.TrimSuffix(parsedURL.Host, ".s3.amazonaws.com") | ||
| if !strings.Contains(strings.ToLower(bucketName), "zwift") { | ||
| return fmt.Errorf("invalid bucket: expected Zwift bucket, got %s", bucketName) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The new validateZwiftS3URL function lacks test coverage. Given that this function is implementing important security validation to prevent SSRF attacks, it should have comprehensive tests covering:
- Valid Zwift S3 URLs
- Invalid schemes (http, ftp, etc.)
- Invalid hosts (non-S3, non-amazonaws.com, wrong region)
- Invalid bucket names (without "zwift")
- Malformed URLs
- Edge cases like case sensitivity
This is especially important since other test files in the codebase (e.g., activity_test.go, model_test.go, auth_test.go) have comprehensive test coverage for their respective functionality.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.