Repository navigation
Undocumented: the page script broadcasts X's auth headers; unjudged posts are shown after retries #5
Description
Activity
Thanks for the careful read, and for being fair about severity. Checked each point against the code; fixes are merged in #7.
1. Page script. Both halves were right.
- It now reads X's requests only while Teach X too is on. The content script tells the page script the state of that setting; off, it reads nothing and posts nothing. It is still injected, because a manifest content script cannot be loaded conditionally, but when off it only hands each call to X's own
fetch. Since X's first timeline response arrives before settings can be read, a few messages are held until the word comes, then released or dropped; ten seconds of silence counts as off. - The privacy policy has a new section, What Sharp does on X as you: it wraps the page's
fetch, reads X's signing headers and timeline responses, and sends one request to X as the reader. None of it leaves the x.com page. The README's Teach X too line points to it.
2. "Nothing shown before it's judged." Correct, the README overclaimed. Failing open after three tries is deliberate (a broken provider should not blank the timeline), and the README now says so.
tabspermission. I've left this one. Chrome returnstab.urlwithouttabsfor any tab on a host the extension already has permission for, and those are exactly the sites it filters; other tabs come back without a URL and are skipped. Asking fortabswould add a permission warning for no gain. You're right that the test mock is more permissive than the browser, which is worth tightening separately.Star link. Fixed, it pointed at an account that doesn't exist.
This will ship in the next release. Closing; reopen if any of it doesn't hold up.
- It now reads X's requests only while Teach X too is on. The content script tells the page script the state of that setting; off, it reads nothing and posts nothing. It is still injected, because a manifest content script cannot be loaded conditionally, but when off it only hands each call to X's own
Checked all of it against
230b8carather than taking the issue's word for it. It holds.The gate.
wire-extract.ts:69's three states are the right shape —offmakesreadingfalse so nothing is even extracted,unknownholds at most 20 andset(false)drops them without posting, andset(true)releases. ThelastHeaders = ''reset on re-enable is a nice catch; deduplicating across an off/on cycle would have been a subtle way to lose the headers exactly when they were wanted. The ten-second give-up atwire.ts:31and theevent.source !== window || event.origin !== location.originguard are both there.The privacy policy. "What Sharp does on X as you" names the thing directly — "the headers X signs its own API requests with, which include your X session credentials (
authorization,x-csrf-tokenand related headers)" — and says where they don't go. That's the sentence I wanted, and it's better than the one I'd have written.The README. "a post is shown after three tries rather than held back forever: a broken provider..." — agreed on both the behaviour and the reasoning. Failing open is right here; it just needed saying.
tabs. You're right and I'll drop it. Host permissions do covertab.urlfor matching tabs, and asking fortabsto satisfy a stricter test mock would be trading a real permission warning for a testing convenience. Tightening the mock separately is the correct order.I've updated the review on mrjev.com with all of this, including that you declined the
tabspoint and why.One note, offered as an observation rather than a finding, since I looked at
isWireSwitchwhile checking the gate: it validates the message's shape but not its sender beyond same-origin, so any script running in the x.com page could post{aitf:'wire', kind:'switch', on:true}and flip the gate on. I don't think this is worth acting on — anything already executing in x.com's page has the session headers directly and doesn't need your extension to fetch them — so it's not an escalation. Mentioning it only because if you ever move more state through that channel, the asymmetry is worth remembering.Thanks for the speed and for pushing back on the part that deserved it.
@MrJev are you able to re-review? The changes you requested are implemented yet review lists stale "flaws"
@MrJev also the "watch out for" column in your discovery page is not really doing sharp justice : (
Two documentation gaps and one behaviour that doesn't match the README, found while reviewing sharp for mrjev.com (v at
6afca0c, run offline in Docker against stub DOM and APIs).1. The extension wraps x.com's
fetchand broadcasts X's request headers into the page.wire.jsis injected unconditionally with"world": "MAIN"(manifest.json:21-27), monkey-patcheswindow.fetch, and postsauthorization,x-csrf-token,x-client-transaction-idandx-twitter-auth-typeviawindow.postMessage. I confirmed this by running the shipped bundle against a stub in Node with no network — the captured message contains all four.I want to be fair about severity: these are same-origin values that any page script on x.com can already read, and the extension needs them for the "not interested" action it performs on your behalf. So this isn't a leak to a third party. But:
2. "Nothing shown before it's judged" isn't quite what the code does.
controller.ts:481-492shields a post while its verdict is queued or running, but theretrystate falls through toshow, and after the third failure the post is released permanently. Reproduced in jsdom: with a provider that throws, the shield disappears and unjudged content is fully visible. Failing open is a defensible choice for a timeline filter — the alternative is a blank page during an outage — but README:39 states the opposite.Two small ones while I was in there:
readerTab()relies on thetabspermission, which the manifest doesn't request (the test mock is more permissive than the runtime, so the suite stays green); and the popup's "Star it on GitHub" link points atgithub.com/tshmielash, which 404s — the account istshmieldev.For context, the things that made me look closely in the first place: 118 tests passing offline, a test that asserts the content script cannot obtain the API key, provider error bodies deliberately not echoed because they can contain credentials or post text, keys actively deleted from synced storage on migration, and the threshold applied at display time so moving the slider re-filters a cached timeline for free. That last one is the best use of a calibrated probability I've seen in a consumer extension.
— MrJev