Repository navigation
fix(feedback): keep page links on tempo.xyz and strip invisible characters - #1136
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review Summary: ✅ CompletedThis 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. |
![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: 182fa612b8
ℹ️ 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".
| const url = new URL(path.startsWith('/') ? path : `/${path}`, 'https://tempo.xyz/developers') | ||
| return isTempoHost(url.hostname) ? url.toString() : defaultPageUrl |
There was a problem hiding this comment.
When MCP feedback supplies a protocol-relative IPv4 resource such as //192.0.2.1/docs without a separate page URL or path, cleanResource now converts it to //192[.]0[.]2[.]1/docs. Resolving that value here throws Invalid URL because the defanged hostname contains brackets. sendSlackFeedback catches the exception and returns delivered: false without submitting anything to Slack, silently dropping the entire feedback report. Catch invalid fallback URLs and use defaultPageUrl, or avoid resolving defanged resources as URLs.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the latest push and didn't find any new bugs. Because this is sanitization for text that ends up in Slack, built on fairly intricate regexes, a human look would still be worthwhile.
What was reviewed:
- allowedUrl now checks the raw host against plain ASCII labels before parsing, so hosts like
evil.com,.tempo.xyzand userinfo forms liketempo.xyz@ evil.comget defanged. - stripInvisible now drops every Cc/Cf code point except tab, CR and LF. Bare hostnames are defanged unless they are on tempo.xyz. Rejected pageUrls are rebuilt under
/developersfrom the raw path. - Not done: I didn't run the AGENTS.md checks (pnpm test, check:types, Biome). I only read the code.
Extended reasoning...
The change touches only src/lib/feedback.ts and its tests. It hardens how docs and MCP feedback is normalized before it is posted to Slack: URL allowlisting to tempo.xyz, defanging other links, and stripping invisible characters. The security-sensitive surface is content injection into Slack, through clickable phishing links and hidden text. No auth or crypto is involved. The latest commits appear to address the earlier inline findings, but the regex-heavy host matching is subtle enough that a human should still look.
Feedback page URLs and related resources now link only to https://tempo.xyz and its subdomains; other URLs are dropped, keeping their path so the page is still named. Links to other hosts in free-text fields are defanged (hxxp[://]example[.]com) so they show as text, not links. Control, zero-width and bidi-override characters are stripped before whitespace is collapsed.
Tests: pnpm test, pnpm check:types, biome.