fix(brief): parse URL pathname in opinion-classifier (backport from PR #3748)#3750
Merged
Merged
Conversation
…#3748) PR #3748 added the same fix to feelgood-classifier.js and explicitly documented the latent gap in opinion-classifier.js:93 as a follow-up backport. Greptile's review of #3748 also flagged this. Closing the gap now. Mechanism (same as adv-002 in #3748): - Pre-fix: `lowerLink.includes('/opinion/')` on the raw link string returns true for any URL whose query string OR fragment contains "/opinion/" — including legitimate aggregator tracking params like `?utm_campaign=/opinion/promo` or news.google.com style redirects whose original `?url=...` carries an /opinion/ section. Hard news with that tracking shape would silently classify as opinion and drop from the brief. - Fix: `safePathname(link)` runs `new URL(link).pathname.toLowerCase()` inside try/catch and returns '' on malformed URLs. Matching is then done against the parsed pathname only — query strings and fragments cannot spoof a section match. Mirrors the helper in feelgood-classifier.js byte-for-byte. Two sites in classifyOpinion shared the bug: - STRONG #1 URL match (line 93) - CORROBORATING URL match (line 103) for /analysis/ + /analyses/ Both now run against the parsed pathname. 6 new regression tests prove the injection vector is closed (tracking param, URL fragment, malformed URL defensive) AND that the fix does not over-correct (genuine /opinion/ + genuine /analysis/ with tracking params still classify correctly). All 12 original tests still pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR backports the pathname-parsing hardening from
Confidence Score: 4/5Safe to merge; the fix correctly narrows URL matching to the parsed pathname and the tests cover both the new protection and regressions on legitimate opinion URLs. The logic change is small and well-contained. The only non-trivial note is that
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[classifyOpinion called] --> B[Extract title / link / description]
B --> C[safePathname link]
C -->|valid URL| D[pathname = URL.pathname.toLowerCase]
C -->|malformed / empty| E[pathname = '']
D --> F{pathname &&\nSTRONG_URL_SEGMENTS\n.some seg in pathname?}
E --> F
F -->|yes| G[return true — STRONG #1]
F -->|no| H{STRONG_HEADLINE_PREFIX_RE.test title?}
H -->|yes| I[return true — STRONG #2]
H -->|no| J[corroborating = 0]
J --> K{isWholeHeadlineQuoted?}
K -->|yes| L[corroborating += 1]
K -->|no| M[unchanged]
L --> N{CORROBORATING_DESCRIPTION_RE.test description?}
M --> N
N -->|yes| O[corroborating += 1]
N -->|no| P[unchanged]
O --> Q{pathname && pathname includes /analysis/ or /analyses/?}
P --> Q
Q -->|yes| R[corroborating += 1]
Q -->|no| S[unchanged]
R --> T{corroborating >= 2?}
S --> T
T -->|yes| U[return true — CORROBORATING]
T -->|no| V[return false]
Reviews (1): Last reviewed commit: "fix(brief): parse URL pathname in opinio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+80
to
+96
| /** | ||
| * Parse the URL pathname defensively. Malformed URL → empty string | ||
| * (skip URL signal entirely; do not throw). Closes the tracking-param | ||
| * injection vector — aggregator tracking params (?utm=/opinion/promo) | ||
| * and URL fragments (#/opinion/footer) live OUTSIDE the pathname and | ||
| * must not trigger STRONG (or CORROBORATING) via raw-string includes() | ||
| * matching on the full link. Backport of the same helper added in | ||
| * feelgood-classifier.js (PR #3748 / adv-002). | ||
| */ | ||
| function safePathname(link) { | ||
| if (typeof link !== 'string' || link.length === 0) return ''; | ||
| try { | ||
| return new URL(link).pathname.toLowerCase(); | ||
| } catch { | ||
| return ''; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
safePathname is duplicated verbatim between opinion-classifier.js and feelgood-classifier.js. If the helper ever needs a behavioural tweak (e.g. normalising percent-encoding, stripping a trailing slash, or handling protocol-relative URLs), the two copies can silently diverge. Extracting it to a shared utility file server/_shared/url-utils.js and importing it in both classifiers would eliminate the drift risk.
Suggested change
| /** | |
| * Parse the URL pathname defensively. Malformed URL → empty string | |
| * (skip URL signal entirely; do not throw). Closes the tracking-param | |
| * injection vector — aggregator tracking params (?utm=/opinion/promo) | |
| * and URL fragments (#/opinion/footer) live OUTSIDE the pathname and | |
| * must not trigger STRONG (or CORROBORATING) via raw-string includes() | |
| * matching on the full link. Backport of the same helper added in | |
| * feelgood-classifier.js (PR #3748 / adv-002). | |
| */ | |
| function safePathname(link) { | |
| if (typeof link !== 'string' || link.length === 0) return ''; | |
| try { | |
| return new URL(link).pathname.toLowerCase(); | |
| } catch { | |
| return ''; | |
| } | |
| } | |
| // safePathname is defined in server/_shared/url-utils.js and imported | |
| // by both opinion-classifier.js and feelgood-classifier.js so a single | |
| // change keeps both in sync. | |
| import { safePathname } from './url-utils.js'; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sibling backport of PR #3748's pathname-parsing fix to
server/_shared/opinion-classifier.js. The original PR closed the tracking-param injection vector infeelgood-classifier.jsand explicitly flagged the same latent gap inopinion-classifier.js:93as a follow-up. Greptile's review of #3748 also called it out.Mechanism (identical to adv-002 closed in #3748)
Before:
```js
const lowerLink = link.toLowerCase();
if (STRONG_URL_SEGMENTS.some((seg) => lowerLink.includes(seg))) return true;
```
Raw
lowerLink.includes('/opinion/')returns true for any URL whose query string OR fragment contains/opinion/— e.g.?utm_campaign=/opinion/promo, news.google.com style redirects whose?url=…carries an/opinion/section, etc. A hard-news article with that tracking shape silently classifies as opinion and drops from the brief.After:
```js
const pathname = safePathname(link);
if (pathname && STRONG_URL_SEGMENTS.some((seg) => pathname.includes(seg))) return true;
```
safePathname()runsnew URL(link).pathname.toLowerCase()insidetry/catchand returns''on malformed URLs. Match runs against the parsed pathname only — query strings and fragments cannot spoof a section match. Helper mirrorsfeelgood-classifier.jsbyte-for-byte.Two sites fixed
classifyOpinionhad the bug at:server/_shared/opinion-classifier.js:93) —STRONG_URL_SEGMENTS.some(...):103) —lowerLink.includes('/analysis/') || lowerLink.includes('/analyses/')Both now go through the same parsed-pathname check.
Test plan
tests/opinion-classifier.test.mjs:/opinion/→ not STRONG/commentary/→ not STRONG/analysis/→ no CORROBORATING contributionfalse/opinion/pathname WITH?utm_source=newsletter→ still STRONG (fix does not over-correct)/analysis/pathname WITH?ref=twitter→ still contributes CORROBORATINGOut of scope
Same scope as PR #3748 — purely the pathname-parse hardening. No signal-list changes, no threshold changes, no telemetry changes. The opinion classifier's behavior on legitimate URLs is unchanged.
🤖 Generated with Claude Code