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); } }; }