Skip to content

Feat/copilot money fix#135

Merged
MaryWylde merged 6 commits into
mainfrom
feat/copilot-money-fix
Jun 2, 2026
Merged

Feat/copilot money fix#135
MaryWylde merged 6 commits into
mainfrom
feat/copilot-money-fix

Conversation

@manager
Copy link
Copy Markdown
Contributor

@manager manager commented Jun 2, 2026

Description

Briefly describe the changes.

🔗 Related Issue

Closes #123 or relates to #456

*(Optional: You can also open a pull request directly from an issue — GitHub will auto-link it.)

✅ Checklist

  • My code follows the project's coding style
  • I have linked the relevant issue in the PR description
  • I have manually tested the changes
  • All CI checks have passed

manager and others added 4 commits May 31, 2026 07:55
Adds api-js.mixpanel.com, *.analytics.google.com, stats.g.doubleclick.net
and www.google.com to connect-src (+ www.google.com to img-src) so GA4
regional collection, the Mixpanel JS shard, and the Google Ads conversion
beacon are not CSP-blocked on staging. Mirrors fixes already live on main;
prevents a future dev->main merge from reverting prod's analytics fixes.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The per-navigation "you're on X" landing line fires a paid Claude call
on every page hop, even when the pill is closed and the visitor never
interacts — so cost scaled with raw visitor count. Now the organic
greeting only spends when the panel is open AND the visitor has already
engaged (asked a question / clicked a card / picked a suggestion) this
session. Curated landings stay local/free; card-click landings already
imply both signals and are unchanged.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…cache

Tighten the organic greeting cost gate per product decision:
- Engagement is now set ONLY on a manually typed message. Card and
  suggestion clicks no longer count — clicking existing buttons is
  navigation, not a conversation, and must not incur paid greetings.
- Engagement expires after 30 min of no typed input (timestamp + TTL),
  so a long-idle tab starts neutral again.
- Never pay twice for the same page in a session: the organic greeting
  is cached per canonical path; revisits and back/forth are free.

Curated (local) landings and the chat answer path are unchanged.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…+ bot filter

Remember the widget's open/closed panel in sessionStorage so it follows the
visitor across page loads (incl. hard reloads into UX Core), instead of always
booting closed. Switch the paid organic-greeting gate from "typed within 30 min"
to simply "panel open" — opening the pill is a deliberate human gesture — and
add a known-bot user-agent backstop on /api/concierge-landing so we never pay a
crawler that reaches the route.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@MaryWylde MaryWylde changed the base branch from dev to main June 2, 2026 17:28
@MaryWylde
Copy link
Copy Markdown
Contributor

@claude review this PR. Also, check for security issues.

@claude
Copy link
Copy Markdown

claude Bot commented Jun 2, 2026

Claude finished @MaryWylde's task in 1m 54s —— View job


PR Review: Feat/copilot money fix

Tasks

  • Gather context and understand request
  • Read changed files and git diff
  • Review next.config.js changes
  • Review concierge-landing.ts API route
  • Review widget/src/AskUxCore.tsx changes
  • Check for security issues
  • Post full review findings

Summary

This PR makes three changes across 4 commits:

  1. CSP expansion (next.config.js) — adds Google Ads and Mixpanel endpoints to connect-src/img-src
  2. Bot UA filter (concierge-landing.ts) — rejects known crawlers before triggering paid LLM calls
  3. Gate relaxation (AskUxCore.tsx) — replaces the "typed + 30-min TTL" engagement gate with a simpler "panel open" gate, and persists open state across page loads via sessionStorage

Overall the direction is correct — the previous gate (requiring typed input) was too strict and broke the organic greeting for returning users. The new approach is reasonable. However there are some concerns detailed below.


Security Issues

🔴 Critical — No rate limiting on the paid LLM endpoint

src/pages/api/concierge-landing.ts has no rate limiting whatsoever. The bot UA filter is trivially bypassed by setting any browser-like user-agent. Once a visitor opens the panel, every page navigation calls this endpoint:

// fireOrganicLanding — fires on every new page the visitor visits with panel open
if (!openRef.current) return;
const greetKey = canonicalPathKey(rawUrl);
if (hasGreetedPage(greetKey)) return;
markGreetedPage(greetKey);
// → POST /api/concierge-landing (paid call)

hasGreetedPage only lives in sessionStorage — server has no memory of it. Any script hitting the endpoint directly (with a browser UA and no cookies) can exhaust your API budget:

for i in $(seq 1 1000); do
  curl -s -X POST https://keepsimple.io/api/concierge-landing \
    -H "Content-Type: application/json" \
    -H "User-Agent: Mozilla/5.0" \
    -d '{"url":"https://keepsimple.io/uxcore/'$i'","title":"test","lang":"en","mode":"organic"}'
done

Recommendation: Add server-side rate limiting by IP — e.g. upstash/ratelimit (they have a Next.js-native adapter) or even a simple in-memory sliding window with lru-cache. A reasonable limit might be 30 calls/hour per IP for this endpoint. Fix this →

🟡 Medium — Bot UA filter is a cost-reduction hint, not a security control

BOT_UA_RE in concierge-landing.ts:26 is the right instinct but should be framed correctly in the codebase. Any attacker spoofing a browser UA bypasses it entirely. The comment on line 20–24 accurately says "Human backstop" but the comment should make clearer that this is a cost hint, not abuse prevention. The real protection must come from rate limiting (see above).

Also: isBotUserAgent returns true when UA is empty/absent (if (!ua || !ua.trim()) return true). This is correct and good. However some legitimate embedded browser environments send no UA — if that's a real concern, worth noting.

🟡 Medium — No input length validation before LLM calls

concierge-landing.ts:292–300:

const { url, title, prevQuery, prevAnswer, lang, mode } = (req.body ?? {}) as { ... };

title, prevQuery, and prevAnswer have no length cap before being injected into the system prompt at line 323–328. A caller can send a multi-megabyte prevAnswer string, inflating token usage significantly. Fix this →

🟢 Low — CSP connect-src: https://www.google.com is broad

next.config.js:51:

'https://www.google.com', // Google Ads conversion

The comment explains it's for www.google.com/ccm/collect. CSP doesn't support path-level restrictions in connect-src (paths are ignored), so this is technically unavoidable if you need that endpoint — but worth acknowledging in review that this broadly allows connections to www.google.com. Not a vulnerability, just a wider attack surface acknowledgement.


Code Quality / Logic

Behavior change in gate relaxation (by design, but worth flagging)

AskUxCore.tsx — the removal of markEngaged() at handleSubmit and the switch from open && hasEngaged() to just open means:

  • Before: Only visitors who typed at least once in the last 30 min got the organic greeting
  • After: Any visitor with an open panel gets the greeting on every new page (once per page per tab session via hasGreetedPage)

This is the stated intent of the PR, but it does widen the surface of paid calls for "panel-openers who just browse without typing." Given the concurrent removal of markEngaged() from the submit handler, the engagement concept is fully removed and no dead code lingers — clean.

writeOpenFlag + open state initialization

AskUxCore.tsx:1816–1825:

const [open, setOpen] = useState<boolean>(() =>
  typeof window !== 'undefined' ? readOpenFlag() : false,
);
const openRef = useRef(open);
openRef.current = open;
useEffect(() => {
  writeOpenFlag(open);
}, [open]);

This correctly initializes from sessionStorage and stays in sync. The openRef mirrors it correctly for the nav-effect closure. No issues.

OPEN_KEY / ENGAGED_KEY rename

Old ENGAGED_KEY = 'ks_aux_engaged_v1' stored a timestamp; new OPEN_KEY = 'ks_aux_open_v1' stores '1'/'0'. There's no migration — on first load, both keys will be absent or the old key will exist but be ignored (it's a different key name). No stale-data issue.


Positive Observations

  • Silent 200 { text: '' } for rejected bots is the right UX pattern — no error surfaces to crawlers.
  • hasGreetedPage + markGreetedPage correctly scope dedup to the tab session.
  • organicAbortRef correctly cancels in-flight requests when a faster nav occurs.
  • The CSP img-src and connect-src additions look correct for the described analytics integrations.
  • credentials: 'same-origin' on the fetch is correct.

Verdict

Approve with a request to address the rate limiting before this goes to production. The CSP changes and bot filter are solid. The gate logic change is intentional and clean. The missing rate limit is a real financial exposure since the server-side has no memory of per-IP call counts.
| Branch: feat/copilot-money-fix

manager and others added 2 commits June 2, 2026 17:46
Address tech-lead review on /api/concierge-landing:
- per-IP sliding-window rate limit (30/hr) as the real budget guard;
  the bot-UA filter is bypassable and only a cost hint now (comment fixed)
- clamp title/prevQuery/prevAnswer before prompt injection to cap tokens

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@MaryWylde MaryWylde merged commit 42b98b6 into main Jun 2, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants