From 5d3fdde4dad5dd8c0028a0347fd32cc4497b7073 Mon Sep 17 00:00:00 2001 From: rays Date: Fri, 11 Sep 2026 13:42:38 +0000 Subject: [PATCH] Upload an OPML file to import; tests for every way in and out - The import screen has a file picker beside the paste box. The page reads the file, checks it looks like OPML before sending, and clears the picker when it is refused and after it is imported. The file is sent as text and never written to disk on the server. - The server parses the OPML before touching anything and answers 400 "that is not an OPML file" (was a 500). subscribe_opml takes a parsed document, so ipx import also refuses a non-OPML file by name. - Tests: Settings' Export OPML download and paste import; uploading an RSS file (refused) and a real OPML; the server's 400; the admin's export round-tripped into a second account; ipx import/export in a scratch config. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016HdTEWQNrzyFULijigkmMn --- CHANGELOG.md | 19 +++++ src/main.rs | 11 ++- src/web.rs | 7 +- tests/page-smoke.js | 1 + tests/ui/app.spec.js | 136 +++++++++++++++++++++++++++------ tests/ui/fixtures/imported.xml | 5 ++ web/index.html | 18 ++++- 7 files changed, 165 insertions(+), 32 deletions(-) create mode 100644 tests/ui/fixtures/imported.xml diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b15061..f2ee5f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,19 @@ matters, what was wrong before -- the reasoning is the point, not the diff. See [README.md](README.md) for what the thing is, and [docs/](docs/) for how to run it. +## 2026-09-11 — Import an OPML file by uploading it + +The import screen now has a file picker as well as the paste box. The page reads the file and +checks it looks like OPML before sending anything. If it doesn't, the page says so and clears the +picker, and it also clears the picker after an import. The file travels as text and is never +written to disk on the server, so there is nothing to clean up there. + +The server now parses the OPML before touching anything, and returns `400` "that is not an OPML +file" instead of a `500`. `subscribe_opml` takes a parsed document, so `ipx import` also refuses a +non-OPML file by name before changing anything. + +--- + ## 2026-09-11 — OPML import and export are per person Importing an OPML now subscribes you to every feed in it. Feeds already in the catalogue cost @@ -17,6 +30,12 @@ through `subscribe_opml`. Export now lists only your own subscriptions. It used to write out the whole catalogue to anyone signed in, including other people's feeds and any private URLs in them. +Tests now cover every way in and out: +- Settings' Export OPML download and paste-to-import screen. +- A round trip that imports the admin's actual export into a second account and checks both + exports match. That round trip is exactly what failed. +- `ipx import` and `ipx export`, run in their own scratch config and database. + --- ## 2026-09-11 — The log is admin-only diff --git a/src/main.rs b/src/main.rs index 8a40df7..3d4a7e3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -611,7 +611,9 @@ async fn import(ctx: &Ctx, config_path: &std::path::Path, file: &std::path::Path .into_iter() .find(|u| u.is_admin) .ok_or_else(|| anyhow::anyhow!("no admin account to subscribe: ipx user add --admin"))?; - let (added, had) = subscribe_opml(ctx, config_path, &text, admin.id)?; + let doc = opml::OPML::from_str(&text) + .map_err(|e| anyhow::anyhow!("{} is not OPML: {e}", file.display()))?; + let (added, had) = subscribe_opml(ctx, config_path, &doc, admin.id)?; println!("subscribed {} to {added} feed(s); {had} already there", admin.name); Ok(()) } @@ -623,14 +625,15 @@ async fn import(ctx: &Ctx, config_path: &std::path::Path, file: &std::path::Path /// Before accounts, importing only added unknown URLs to config.toml. Once subscriptions /// decided what each person sees, that imported nothing at all for a feed someone else /// already had, and a new one had no subscriber, so it was never scanned. +/// +/// The caller parses the document, so each refuses a file that is not OPML in its own terms, +/// before anything is touched: a 400 from the web, a message from the CLI. pub fn subscribe_opml( ctx: &Ctx, config_path: &std::path::Path, - xml: &str, + doc: &opml::OPML, user_id: i64, ) -> Result<(usize, usize)> { - let doc = opml::OPML::from_str(xml) - .map_err(|e| anyhow::anyhow!("that does not parse as OPML: {e}"))?; let mut found = vec![]; collect_outlines(&doc.body.outlines, &mut found); diff --git a/src/web.rs b/src/web.rs index 04258a2..21e037d 100644 --- a/src/web.rs +++ b/src/web.rs @@ -1172,8 +1172,11 @@ async fn import_opml( user: crate::db::User, Json(body): Json, ) -> Result, ApiError> { - let (added, already) = - crate::subscribe_opml(&state.ctx, &state.config_path, &body.xml, user.id)?; + // Refused before anything is touched. Nothing reaches the disk either way: an uploaded + // file arrives as text, is read here, and is gone when the request ends. + let doc = opml::OPML::from_str(&body.xml) + .map_err(|e| ApiError::bad_request(format!("that is not an OPML file: {e}")))?; + let (added, already) = crate::subscribe_opml(&state.ctx, &state.config_path, &doc, user.id)?; Ok(Json(serde_json::json!({ "added": added, "already": already }))) } diff --git a/tests/page-smoke.js b/tests/page-smoke.js index 9c2ce9c..482e6d3 100644 --- a/tests/page-smoke.js +++ b/tests/page-smoke.js @@ -82,6 +82,7 @@ const drive = [ ['removeFeed', () => ctx.removeFeed(feed)], ['prefsModal', () => ctx.prefsModal()], ['usersModal', () => ctx.usersModal()], + ['opmlModal', () => ctx.opmlModal()], ['logsModal', () => ctx.logsModal()], // `const S` is not reachable from here: top-level const/let do not become properties // of a vm context the way var and function declarations do. diff --git a/tests/ui/app.spec.js b/tests/ui/app.spec.js index 7cf5132..039eae7 100644 --- a/tests/ui/app.spec.js +++ b/tests/ui/app.spec.js @@ -413,9 +413,67 @@ test('the only admin cannot be demoted or removed', async ({ page }) => { expect((await page.request.delete(`/api/users/${me.id}`)).status()).toBe(400); }); -test('importing an OPML subscribes you, and export lists only your feeds', async ({ browser }) => { - // Regression: import only added URLs the catalogue lacked and subscribed nobody, so a feed - // someone else already had imported as nothing at all. +test('Settings exports your OPML and imports a pasted one', async ({ page }) => { + await page.locator('#prefs').click(); + const [dl] = await Promise.all([ + page.waitForEvent('download'), + page.locator('#modalCard a', { hasText: 'Export OPML' }).click(), + ]); + expect(dl.suggestedFilename()).toBe('ipx-subscriptions.opml'); + const out = require('fs').readFileSync(await dl.path(), 'utf8'); + for (const f of ['show.xml', 'pics.xml', 'multi.xml', 'subs.opml']) expect(out).toContain(f); + // A feed from an OPML subscription comes back with the OPML itself, not on its own. + expect(out).not.toContain('other.xml'); + + // One feed new to everyone, one the admin already has. + await page.locator('#gopml').click(); + await page.locator('#opmlText').fill('t' + + '' + + ''); + await page.locator('#oimp').click(); + await expect(page.locator('.toast', { hasText: 'Subscribed to' })) + .toHaveText('Subscribed to 1 feed(s), 1 you already had'); + // Named from the OPML's id until the first scan reads the feed's own title. + await expect(page.locator('#feedlist .feed', { hasText: /Imported Show|imported-show/ })) + .toBeVisible({ timeout: 20_000 }); +}); + +test('an uploaded OPML file imports, and a file that is not OPML is refused', async ({ page }) => { + await page.locator('#prefs').click(); + await page.locator('#gopml').click(); + const pick = page.locator('#opmlFile'); + + // An RSS feed is XML but not OPML: refused in the page, and the picker lets go of it. + await pick.setInputFiles({ + name: 'feed.xml', mimeType: 'application/xml', + buffer: require('fs').readFileSync(require('path').join(__dirname, 'fixtures', 'show.xml')), + }); + await page.locator('#oimp').click(); + await expect(page.locator('.toast.bad', { hasText: 'feed.xml is not an OPML file' })).toBeVisible(); + expect(await pick.evaluate(i => i.files.length)).toBe(0); + + // Something that gets past the page's quick look is still refused by the server, untouched. + const sneaky = await page.request.post('/api/opml', { + data: { xml: '<opml>' }, + }); + expect(sneaky.status()).toBe(400); + expect(await sneaky.text()).toContain('not an OPML file'); + + // A real one. Multi Show is already the admin's, so it counts as already had. + await pick.setInputFiles({ + name: 'subs.opml', mimeType: 'text/x-opml', + buffer: Buffer.from('t' + + ''), + }); + await page.locator('#oimp').click(); + await expect(page.locator('.toast', { hasText: 'Subscribed to' })) + .toHaveText('Subscribed to 0 feed(s), 1 you already had'); + await expect(page.locator('#modal.on')).toBeHidden(); +}); + +test('an export from one account imports into another', async ({ page, browser }) => { + // Regression: import only added URLs the catalogue lacked and subscribed nobody, so importing + // the admin's export into a second account did nothing at all. const { execFileSync } = require('child_process'); const setup = require('./global-setup'); const env = { @@ -429,26 +487,60 @@ test('importing an OPML subscribes you, and export lists only your feeds', async if (!String(e.stderr || e.stdout).includes('already exists')) throw e; } const ctx = await browser.newContext(); - const page = await ctx.newPage(); - await page.goto('/login'); - await page.locator('#name').fill('opal'); - await page.locator('#pw').fill('opalpassword'); - await page.locator('button[type=submit]').click(); - await expect(page.locator('#feedlist')).toContainText('No feeds.'); + const opal = await ctx.newPage(); + await opal.goto('/login'); + await opal.locator('#name').fill('opal'); + await opal.locator('#pw').fill('opalpassword'); + await opal.locator('button[type=submit]').click(); + await expect(opal.locator('#feedlist')).toContainText('No feeds.'); - // Test Show is already in the catalogue, because the admin reads it. - const xml = 't' + - ''; - expect(await (await page.request.post('/api/opml', { data: { xml } })).json()) - .toEqual({ added: 1, already: 0 }); - expect(await (await page.request.post('/api/opml', { data: { xml } })).json()) - .toEqual({ added: 0, already: 1 }); - await page.reload(); - await expect(page.locator('.feed', { hasText: 'Test Show' })).toBeVisible({ timeout: 20_000 }); + // Export used to hand anyone the whole catalogue. Opal has nothing yet, so gets nothing. + const empty = await opal.request.get('/api/opml'); + expect(empty.status()).toBe(200); + expect(await empty.text()).not.toContain('xmlUrl'); - // The admin also reads Picture Blog; that is not opal's to export. - const out = await (await page.request.get('/api/opml')).text(); - expect(out).toContain('show.xml'); - expect(out).not.toContain('pics.xml'); + const urlsIn = xml => [...xml.matchAll(/xmlUrl="([^"]+)"/g)].map(m => m[1]).sort(); + const exported = await (await page.request.get('/api/opml')).text(); // the admin's + const urls = urlsIn(exported); + expect(urls.length).toBeGreaterThan(2); + expect(await (await opal.request.post('/api/opml', { data: { xml: exported } })).json()) + .toEqual({ added: urls.length, already: 0 }); + expect(await (await opal.request.post('/api/opml', { data: { xml: exported } })).json()) + .toEqual({ added: 0, already: urls.length }); + + await opal.reload(); + await expect(opal.locator('.feed', { hasText: 'Test Show' })).toBeVisible({ timeout: 20_000 }); + // The round trip closes: opal's own export now lists what the admin's did. + expect(urlsIn(await (await opal.request.get('/api/opml')).text())).toEqual(urls); await ctx.close(); }); + +test('ipx import subscribes the admin, and ipx export writes the feeds out', async () => { + // Its own config and database. The CLI works in-process, and the suite's running daemon + // reads config.toml once at start, so it would not see what the CLI added anyway. + const fs = require('fs'); + const path = require('path'); + const { execFileSync } = require('child_process'); + const setup = require('./global-setup'); + const dir = path.join(setup.root, 'cli'); + fs.rmSync(dir, { recursive: true, force: true }); + fs.mkdirSync(path.join(dir, 'data'), { recursive: true }); + fs.writeFileSync(path.join(dir, 'config.toml'), + `[general]\ndownload_dir = "${dir}/downloads"\nsocket = "${dir}/ipx.sock"\n`); + const env = { ...process.env, IPX_CONFIG: path.join(dir, 'config.toml'), IPX_DATA_DIR: path.join(dir, 'data') }; + const ipx = (args, input) => execFileSync('./target/debug/ipx', args, { env, input, encoding: 'utf8' }); + + ipx(['user', 'add', 'boss'], 'bosspassword'); // the first account is the admin + const opml = path.join(dir, 'in.opml'); + fs.writeFileSync(opml, 't' + + '' + + ''); + expect(ipx(['import', opml])).toContain('subscribed boss to 2 feed(s); 0 already there'); + expect(ipx(['import', opml])).toContain('subscribed boss to 0 feed(s); 2 already there'); + + const out = path.join(dir, 'out.opml'); + ipx(['export', out]); + const xml = fs.readFileSync(out, 'utf8'); + expect(xml).toContain('http://127.0.0.1:8792/one.xml'); + expect(xml).toContain('http://127.0.0.1:8792/two.xml'); +}); diff --git a/tests/ui/fixtures/imported.xml b/tests/ui/fixtures/imported.xml new file mode 100644 index 0000000..9c85c93 --- /dev/null +++ b/tests/ui/fixtures/imported.xml @@ -0,0 +1,5 @@ + +Imported Showhttp://127.0.0.1:8792/ +Only ever arrives through an OPML import. +Imported Epimp-1x + diff --git a/web/index.html b/web/index.html index 614acb4..dbf9a79 100644 --- a/web/index.html +++ b/web/index.html @@ -1390,15 +1390,25 @@ function opmlModal(){
Export
-
+
+
`); $('#oimp').onclick=async()=>{ + // A chosen file is read here and sent as text, so the server never stores it. Clearing + // the picker lets go of it on this side too, whether it was refused or imported. + const pick=$('#opmlFile'), file=pick.files[0]; + const xml=file ? await file.text() : $('#opmlText').value; + const letGo=()=>{ pick.value=''; }; + // A quick look before sending anything. The server parses it properly and has the last word. + if(!/]/i.test(xml)){ + letGo(); toast(`${file?file.name:'That'} is not an OPML file`,true); return; + } try{ - const r=await api('/api/opml',{method:'POST',body:JSON.stringify({xml:$('#opmlText').value})}); - closeModal(); toast(`Subscribed to ${r.added} feed(s)`+(r.already?`, ${r.already} you already had`:'')); loadFeeds(true); - }catch(e){ toast(e.message,true); } + const r=await api('/api/opml',{method:'POST',body:JSON.stringify({xml})}); + letGo(); closeModal(); toast(`Subscribed to ${r.added} feed(s)`+(r.already?`, ${r.already} you already had`:'')); loadFeeds(true); + }catch(e){ letGo(); toast(e.message,true); } }; }