diff --git a/background/background.js b/background/background.js index 8d473a4..71f5068 100644 --- a/background/background.js +++ b/background/background.js @@ -10,6 +10,17 @@ const COMMAND_ID = "ask-chatgpt"; // A commands invocation confers activeTab (ext-commands.js grants it before // firing onCommand), so executeScript works with no host permission. // +// Every frame keeps its own independent Selection, and a selection made in a +// non-focused frame is not cleared when the user selects elsewhere. Without a +// filter, a stale selection in an unfocused (possibly cross-origin) iframe +// could win over — or be pulled alongside — the selection the user actually +// made, with the winner among multiple non-empty results decided by an +// executeScript ordering that isn't specified. The injected snippet is +// therefore gated on document.hasFocus(), which is true for the focused +// document and all of its ancestors: the top frame wins when the user +// selected there, and an iframe's selection is only picked up when it is +// itself the focused frame. +// // Failure has three measured shapes, all handled here: a thrown // "Missing host permission for the tab", a thrown variant naming frames, and a // SILENT resolution to [null] on parent-process about: pages. Never index into @@ -32,7 +43,7 @@ async function readSelection() { let results; try { results = await browser.tabs.executeScript(tabId, { - code: "window.getSelection().toString()", + code: "document.hasFocus() ? window.getSelection().toString() : ''", allFrames: true, matchAboutBlank: true, }); diff --git a/lib/defaults.js b/lib/defaults.js index c7e67f7..b3dd454 100644 --- a/lib/defaults.js +++ b/lib/defaults.js @@ -4,6 +4,13 @@ export const OPEN_IN_VALUES = ["new-tab", "background-tab", "new-window"]; +// An unbounded model string can, on its own, exceed MAX_URL_CHARS in +// lib/build-url.js — fitToBudget() only ever trims `q`, so a long enough +// model silently pushes the whole URL over budget and drops the query +// entirely with no signal to the user. 200 is generous for any real model +// slug while keeping the URL budget meaningful. +export const MAX_MODEL_CHARS = 200; + export const DEFAULT_SETTINGS = { model: "", temporaryChat: false, diff --git a/lib/settings.js b/lib/settings.js index d3d4856..1060a0a 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -1,4 +1,4 @@ -import { DEFAULT_SETTINGS, OPEN_IN_VALUES } from "./defaults.js"; +import { DEFAULT_SETTINGS, OPEN_IN_VALUES, MAX_MODEL_CHARS } from "./defaults.js"; // Pure. Layers stored values over the defaults, dropping unknown keys and // falling back on wrong types or out-of-range values. A fresh or signed-out @@ -8,7 +8,7 @@ export function mergeSettings(stored) { if (stored === null || typeof stored !== "object") { return merged; } - if (typeof stored.model === "string") { + if (typeof stored.model === "string" && stored.model.length <= MAX_MODEL_CHARS) { merged.model = stored.model; } if (typeof stored.temporaryChat === "boolean") { diff --git a/options/options.html b/options/options.html index 07f9e8d..8920222 100644 --- a/options/options.html +++ b/options/options.html @@ -9,7 +9,7 @@
- +

ChatGPT may ignore this and use your account default. Leave blank to always use your account default. diff --git a/options/options.js b/options/options.js index bd244f7..cc1ef03 100644 --- a/options/options.js +++ b/options/options.js @@ -8,12 +8,8 @@ const field = (id) => document.getElementById(id); let saveTimer = null; let statusTimer = null; -// Suppresses the change/input handlers while we are programmatically writing -// values into the form, so a remote sync cannot trigger a save loop. -let applying = false; function render(settings) { - applying = true; for (const [id, value] of Object.entries(settings)) { const element = field(id); // Never overwrite the control the user is actively editing — this page's @@ -27,7 +23,6 @@ function render(settings) { element.value = value; } } - applying = false; } function collect() { @@ -60,9 +55,6 @@ async function persist(settings) { // Debounced so typing in a text field cannot approach any write-rate quota. function scheduleSave() { - if (applying) { - return; - } clearTimeout(saveTimer); saveTimer = setTimeout(() => persist(collect()), SAVE_DEBOUNCE_MS); } diff --git a/test/settings.test.js b/test/settings.test.js index f514e57..4a1ce78 100644 --- a/test/settings.test.js +++ b/test/settings.test.js @@ -1,7 +1,7 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { DEFAULT_SETTINGS } from "../lib/defaults.js"; +import { DEFAULT_SETTINGS, MAX_MODEL_CHARS } from "../lib/defaults.js"; import { mergeSettings } from "../lib/settings.js"; test("empty object yields the defaults", () => { @@ -78,3 +78,17 @@ test("mergeSettings does not mutate DEFAULT_SETTINGS", () => { mergeSettings({ model: "mutated" }); assert.equal(DEFAULT_SETTINGS.model, ""); }); + +// An unbounded model string can, on its own, push the finished URL over +// MAX_URL_CHARS (lib/build-url.js), which silently drops `q` — the user's +// selection vanishes with no signal. mergeSettings() must bound `model` the +// same way it already bounds an out-of-range openIn: fall back to the default. +test("a model at exactly MAX_MODEL_CHARS is accepted", () => { + const model = "m".repeat(MAX_MODEL_CHARS); + assert.equal(mergeSettings({ model }).model, model); +}); + +test("a model one character over MAX_MODEL_CHARS falls back to the default", () => { + const model = "m".repeat(MAX_MODEL_CHARS + 1); + assert.equal(mergeSettings({ model }).model, DEFAULT_SETTINGS.model); +});