Repository navigation
Conversation
auth/upstream was extracted from basecamp-mcp-server before its #265 finished, and missed two of #265's last changes. basecamp-mcp-server is about to adopt this package in place of its own copy, and would lose both. - Strip endpoint text to printable ASCII before redacting and rendering it (upstream.PrintableASCII, replacing LogSafeRune). LogSafeRune drops controls and format characters but keeps combining marks: a client secret with U+034F or U+FE0F after every third byte splits into pieces shorter than both the fragment and token-run redactions, and a viewer that drops the marks shows the secret whole. TestInvisibleMarkCannotSplitASecret fails 3 of 3 against main. OAuth error descriptions are ASCII by specification (RFC 6749 §5.2), so a conformant endpoint loses nothing. - Failure.Error() appends a fixed phrase for its cause's class (deadline exceeded, connection refused, TLS certificate verification failed, ...), so an operator can tell failures apart without the cause's text. Nothing is read from the cause except its type, sentinel identity, and a JSON type error's struct-field path. - upstreamtest gains the control-split error_description kind, and AssertClean reads text as a viewer that drops invisible characters would. - The exchange tests gain basecamp-mcp-server's U+FFFD/invalid-UTF-8 split test.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Cause labels can expose unchecked operation text, and normalized assertions can miss literal credential leaks.
2 open issues
What changed in this PR
Ports upstream error-handling hardening needed for Basecamp’s adoption of the shared exchange client.
Changes:
- Normalizes endpoint descriptions to printable ASCII before redaction.
- Adds cause-class labels to infrastructure errors.
- Expands hostile-endpoint fixtures and credential-redaction tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| auth/upstream/upstreamtest/hostile.go | Adds invisible-split fixtures and normalized leak assertions. |
| auth/upstream/failure.go | Adds ASCII filtering and cause classification. |
| auth/upstream/failure_test.go | Tests filtering and cause labels. |
| auth/exchange/oautherror.go | Shares ASCII normalization with error rendering. |
| auth/exchange/exchange_test.go | Adds split-credential regression tests. |
Review effort: Balanced
| case errors.As(err, &protoError): | ||
| return "malformed HTTP response" | ||
| case errors.As(err, &opErr): | ||
| return "network error during " + opErr.Op |
| // credential threaded with any of them still counts as present. | ||
| func AssertClean(t testing.TB, sink, text string, secrets ...string) { | ||
| t.Helper() | ||
| text = strings.Map(func(r rune) rune { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dffd14205
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case errors.As(err, &opErr): | ||
| return "network error during " + opErr.Op |
There was a problem hiding this comment.
When a supplied HTTPClient/transport returns a *net.OpError, Op is an exported, unconstrained string rather than a type-derived constant, so concatenating it directly breaks Failure.Error()'s safe-for-any-sink guarantee and can expose credentials or inject control characters. Keep this class entirely fixed or map Op through an explicit allowlist instead of rendering the field.
Useful? React with 👍 / 👎.
auth/upstreamwas extracted (#10) from a basecamp-mcp-server snapshot taken before its #265 finished. Two of #265's last changes didn't come across. basecamp-mcp-server is about to delete its copy and import this package, so without them it would regress on both. This PR moves them over as they were reviewed there.Changes
upstream.PrintableASCIIreplacesLogSafeRune, and bothexchange.stripControlsandScrubDescriptionuse it.LogSafeRunedrops controls and format characters but keeps combining marks. So a client secret echoed with U+034F, U+FE0F or U+20DD after every third byte splits into pieces shorter than both the 8-byte fragment pass and the 20-byte token-run pass. A viewer that drops the marks then shows the whole secret.TestInvisibleMarkCannotSplitASecretfails 3 of 3 against main and passes here. OAutherror_descriptionis ASCII by specification (RFC 6749 §5.2), so a conformant endpoint loses nothing. Non-ASCII letters are now dropped:caférenders ascaf, and the test case says so.Failure.Error()appends a fixed phrase for its cause's class, for exampledeadline exceeded,connection refused,TLS certificate verification failedorunexpected JSON value for expires_in. An operator can then tell failures apart without the cause's text. Only the cause's type, sentinel identity and a JSON type error's struct-field path (our own names) are read from it.TestCauseClasschecks that no class renders the cause's text.upstreamtestgains theControlSplitDescriptionkind.AssertCleannow reads text the way a viewer that drops invisible characters would.TestErrorDescriptionRedactsReplacementSplitCredentials). It already passes on main and pins that behaviour.API change
LogSafeRuneis removed andPrintableASCIIis added. No consumer callsLogSafeRune: fizzy-mcp-server importsauth/exchangeonly.make readypasses: vet, race tests and build.