Fix 401 retry bug, XSS in OAuth callback, and codebase cleanup - #18
Conversation
… up codebase - Fix io.Reader consumed on first request attempt causing 401 retries to send empty bodies for POST/PUT/multipart requests. Buffer body as []byte and create fresh readers for each attempt. - Fix reflected XSS in OAuth error callback by escaping user-controlled Strava error parameter with html.EscapeString(). - Consolidate Post/Put into shared jsonRequest helper. - Remove dead Config.Debug field (debug handled via CLI flag). - Remove stale section-divider comments and outdated references. - Use loop for optional field forwarding in HandleCreateActivity for consistency with HandleUpdateActivity.
There was a problem hiding this comment.
Pull request overview
Fixes Strava client request retry behavior and hardens the OAuth callback against reflected XSS, alongside some tool/config comment cleanup.
Changes:
- Buffer/replay request bodies in
doRequestto ensure 401 refresh retries resend POST/PUT/multipart bodies correctly. - Escape the OAuth callback
errorquery param before rendering it into the HTML error page. - Minor cleanup/DRY in tools and configuration (remove stale comments and dead config field).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/strava/client.go | Adds body replay for 401 retry; refactors POST/PUT into jsonRequest. |
| internal/auth/oauth.go | Escapes Strava OAuth error parameter before inserting into HTML. |
| internal/config/config.go | Removes unused Config.Debug field. |
| internal/server/server.go | Removes outdated phase-related comment. |
| internal/tools/activities.go | Simplifies optional field forwarding in HandleCreateActivity. |
| internal/tools/helpers.go | Removes stale ticket reference in comment. |
| internal/tools/register.go | Removes stale tool-count/category comment. |
| internal/tools/athlete.go | Removes section-divider comments. |
| internal/tools/clubs.go | Removes section-divider comments. |
| internal/tools/streams.go | Removes section-divider comments. |
| internal/tools/uploads.go | Removes section-divider comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Buffer the body so we can replay it on 401 retry. | ||
| var bodyBytes []byte | ||
| if body != nil { | ||
| var err error | ||
| bodyBytes, err = io.ReadAll(body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("buffer request body: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
doRequest now buffers the entire request body into memory via io.ReadAll. This can be problematic for upload/multipart requests (potentially multi‑MB FIT/GPX files) and can lead to high memory usage or OOM under concurrency. Consider avoiding full buffering when the body is seekable (e.g., if it implements io.ReadSeeker, rewind between attempts), and/or enforce a reasonable max buffer size for non-seekable bodies and return a clear error when exceeded.
| @@ -169,14 +178,22 @@ func (c *Client) doRequest(ctx context.Context, method, fullURL string, body io. | |||
| if refreshErr != nil { | |||
| return nil, fmt.Errorf("token refresh after 401: %w", refreshErr) | |||
| } | |||
| // Retry with new token — if this also fails, return the error directly | |||
| return c.executeRequest(ctx, method, fullURL, body, contentType, tokens.AccessToken) | |||
| // Retry with new token and a fresh reader | |||
| return c.executeRequest(ctx, method, fullURL, newReader(bodyBytes), contentType, tokens.AccessToken) | |||
There was a problem hiding this comment.
There’s comprehensive retry testing for Get() (401 refresh + retry), but no equivalent test coverage for Post/Put/PostMultipart ensuring the body is correctly replayed on the 401 retry. Since this change is specifically about replaying consumed bodies, adding a test that forces a 401 on the first request and asserts the second request receives the full (non-empty) body would prevent regressions.
| func (c *Client) jsonRequest(ctx context.Context, method, path string, body interface{}) ([]byte, error) { | ||
| jsonBody, err := json.Marshal(body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshal request body: %w", err) | ||
| } | ||
| return c.doRequest(ctx, http.MethodPut, fullURL, bytes.NewReader(jsonBody), "application/json") | ||
| return c.doRequest(ctx, method, c.baseURL+path, bytes.NewReader(jsonBody), "application/json") | ||
| } |
There was a problem hiding this comment.
jsonRequest marshals to jsonBody and then passes a reader to doRequest, which immediately io.ReadAlls it back into a new byte slice. For JSON requests this introduces an extra allocation/copy on every call. Consider changing doRequest to accept []byte (or a func() io.Reader factory) so JSON requests can reuse the already-marshaled bytes without a second read/copy, while still allowing fresh readers per attempt.
… retry body test Address Copilot PR feedback: - Change doRequest to accept []byte instead of io.Reader, eliminating the extra io.ReadAll buffering step. JSON requests pass marshaled bytes directly; PostMultipart reads upfront at the call site. - Use a bodyReader closure to create fresh readers per attempt. - Add TestPostReplaysBodyOn401Retry to verify POST body is fully replayed after token refresh, preventing regression of the consumed-reader bug.
Summary
doRequest401 retry was reusing a consumedio.Reader, causing POST/PUT/multipart retries to send empty bodies. Now buffers as[]byteand creates fresh readers per attempt.errorquery parameter was injected into HTML without escaping. Now useshtml.EscapeString().Post/Putinto sharedjsonRequesthelper; loop-based optional field forwarding inHandleCreateActivityto matchHandleUpdateActivity.Config.Debugfield, stale section-divider comments, outdated phase references, and ticket IDs.Test plan
go build ./...passesgo test ./... -count=1— all packages greenstrava-mcp auth, complete OAuth flow, confirm success page renders