openvibers/openvibe.bot Commits

Close the owner-fence hole that the independent review of Bot PR 9 found (#10)

OP OpenVibers committed 2 Oct 2026 · dae7c59cb6 · 3 files, +110 −9

local checks ✓ 4/4 · no CI checks · review SHIP by DeepSeek harness

https://github.com/OpenVibers/OpenVibe.Bot/pull/10

Changes

ddiff --git a/docs/protocol.md b/docs/protocol.md
iindex f5dc3c0..3724857 100644
---- a/docs/protocol.md
++++ b/docs/protocol.md
@@@ -301,12 +301,15 @@ role that may drive (a queue robot's turn holder, not someone waiting) and an on
Every error is RFC 9457 `application/problem+json`. Authenticate with a Network **user token**
(`Authorization: Bearer`, people act on their own things) or a **service token** with the capability
--shown; a service acts for `X-OV-Subject` / the body's `owner`.
++shown; a service acts for `X-OV-Subject` / the body's `owner`. A person acts only as themself: naming
++anyone else (`?owner=`, the body's `owner`, `X-OV-Subject`) is `403 bot.forbidden`. Without a token, a
++route that acts for an owner answers `401 bot.sign_in`; a node token gets `403 bot.forbidden` everywhere
++but `POST /devices/bind`.
| Method & path | Auth (capability) | Answer |
|---|---|---|
| `GET /profiles`, `GET /profiles/:id` | public | `{ profiles[] }` / `{ profile }` |
--| `GET /robots` | user, or `bot.robot.read` + `?owner=` | `{ robots[] }` |
++| `GET /robots` | user (own robots), or `bot.robot.read` + `?owner=` | `{ robots[] }` |
| `POST /robots` | the owner, or `bot.robot.manage` | `201 { robot, pairing{code,expires_at,installer} }` |
| `GET /robots/:id` | a member, or `bot.robot.read` | `{ robot, role }` |
| `PATCH /robots/:id` | owner, or `bot.robot.manage` | `{ robot }`; new `limits` (including `allow`) re-send `config` to the connected device |
ddiff --git a/server/api/v1.js b/server/api/v1.js
iindex 3683d1e..89e65d9 100644
---- a/server/api/v1.js
++++ b/server/api/v1.js
@@@ -9,7 +9,7 @@
* | Method & path | Capability (services) | People |
* |------------------------------------------------|------------------------|-------------------------------------|
* | GET /profiles, /profiles/:id | — (public) | anyone |
-- * | GET /robots | bot.robot.read | own robots (or ?owner=usr_… if the owner) |
++ * | GET /robots | bot.robot.read | own robots (?owner= only themself) |
* | POST /robots | bot.robot.manage | the owner (new robot + pairing code)|
* | GET /robots/:id | bot.robot.read | a member (owner/operator/viewer) |
* | PATCH /robots/:id | bot.robot.manage | owner |
@@@ -46,15 +46,27 @@ function v1Router({ domain, apiAuth, limits, hub }) {
const wrap = (fn) => (req, res, next) => Promise.resolve(fn(req, res, next)).catch(next);
const me = (req) => (req.principal.kind === 'user' ? req.principal.subject : null);
++ /** Only a person or a service acts for an owner: no token → 401, any other kind (a node token) → 403. */
++ const fence = (req) => {
++ if (req.principal.kind === 'anonymous') fail(401, 'bot.sign_in', 'sign in to do that');
++ if (req.principal.kind !== 'user' && req.principal.kind !== 'service') fail(403, 'bot.forbidden', 'this token may not act for an owner');
++ };
++ /** A person acts only as themself: naming anyone else (owner / X-OV-Subject) is refused, never ignored. */
++ const self = (req, ...named) => {
++ const subject = req.principal.subject;
++ if (named.some((s) => s != null && s !== '' && s !== subject)) fail(403, 'bot.forbidden', 'you may only act for yourself');
++ return subject;
++ };
/** The subject a call acts for: a person's own subject, or a service's X-OV-Subject / body.owner. */
const actedFor = (req, extra) => {
-- if (req.principal.kind === 'user') return req.principal.subject;
-- const s = req.principal.subject || extra || req.headers['x-ov-subject'] || req.body?.owner;
++ fence(req);
++ if (req.principal.kind === 'user') return self(req, extra, req.headers['x-ov-subject'], req.body?.owner);
++ const s = extra || req.headers['x-ov-subject'] || req.body?.owner;
if (!s) fail(422, 'bot.invalid_input', 'a service must act for a subject (X-OV-Subject or owner)');
return userSubject(s, 'subject');
};
const requireManage = (req, extra) => { if (req.principal.kind === 'service' && !apiAuth.granted(req.principal, CAP.manage)) fail(403, 'capability.denied', `${CAP.manage} not granted`); return actedFor(req, extra); };
-- const requireRead = (req) => { if (req.principal.kind === 'service' && !apiAuth.granted(req.principal, CAP.read)) fail(403, 'capability.denied', `${CAP.read} not granted`); };
++ const requireRead = (req) => { fence(req); if (req.principal.kind === 'service' && !apiAuth.granted(req.principal, CAP.read)) fail(403, 'capability.denied', `${CAP.read} not granted`); };
const requireControlCap = (req) => { if (req.principal.kind === 'service' && !apiAuth.granted(req.principal, CAP.control)) fail(403, 'capability.denied', `${CAP.control} not granted`); };
async function member(req, robotId, roles = ['owner', 'operator', 'viewer']) {
@@@ -87,9 +99,9 @@ function v1Router({ domain, apiAuth, limits, hub }) {
// ── Robots ────────────────────────────────────────────────────────────────────────────────────
r.get('/robots', wrap(async (req, res) => {
-- const ownerSubject = req.principal.kind === 'service' ? null : me(req);
-- if (req.principal.kind === 'service') requireRead(req);
-- const target = ownerSubject || (req.query.owner ? userSubject(req.query.owner, 'owner') : null);
++ requireRead(req);
++ const target = req.principal.kind === 'user' ? self(req, req.query.owner, req.headers['x-ov-subject'])
++ : (req.query.owner ? userSubject(req.query.owner, 'owner') : null);
if (!target) fail(422, 'bot.invalid_input', 'owner is required');
res.json({ robots: (await domain.robots.list(target)).map(domain.present.robot) });
}));
ddiff --git a/test/owner-fence.test.js b/test/owner-fence.test.js
nnew file mode 100644
iindex 0000000..243e296
---- /dev/null
++++ b/test/owner-fence.test.js
@@@ -0,0 +1,86 @@
++'use strict';
++// The owner fence on /api/v1: only a person (as themself) or a service with the capability (for the
++// subject it names) reads or creates robots for an owner. No token → 401 bot.sign_in; a node token →
++// 403 bot.forbidden; a person naming someone else (?owner, body owner, X-OV-Subject) → 403 bot.forbidden.
++// Public routes, POST /pair (its code) and POST /devices/bind (a node token) keep their own auth.
++const assert = require('assert');
++const { boot, check, done } = require('./helpers/app');
++
++(async () => {
++ const t = await boot();
++ const alex = t.network.newUser('alex');
++ const bob = t.network.newUser('bob');
++ const { robot: bobsRobot } = await t.robot(bob);
++ const nodeToken = t.network.signNode(t.network.addNode({ owner: bob.subject, ref: bobsRobot.id }));
++
++ const robotsOf = async (u) => Number((await t.db.maybe('SELECT count(*) AS n FROM robots WHERE owner_subject = $1', [u.subject])).n);
++ const listFor = (u, opts) => t.call('GET', `/api/v1/robots?owner=${u.subject}`, opts);
++ const createFor = (u, opts) => t.call('POST', '/api/v1/robots', { ...opts, body: { name: 'Not yours', profile_id: 'sim.rover', owner: u.subject } });
++ const refused = (r, status, code) => { assert.strictEqual(r.status, status, r.text); assert.strictEqual(r.json.code, code, r.text); assert.ok(!r.json.robots && !r.json.robot && !r.json.pairing, r.text); };
++
++ await check('no token: listing or creating another person\'s robots → 401 bot.sign_in', async () => {
++ const before = await robotsOf(bob);
++ refused(await listFor(bob, { token: null }), 401, 'bot.sign_in');
++ refused(await createFor(bob, { token: null }), 401, 'bot.sign_in');
++ refused(await t.call('POST', '/api/v1/robots', { token: null, headers: { 'X-OV-Subject': bob.subject }, body: { name: 'x', profile_id: 'sim.rover' } }), 401, 'bot.sign_in');
++ assert.strictEqual(await robotsOf(bob), before);
++ });
++
++ await check('a node token: listing or creating robots for its owner → 403 bot.forbidden', async () => {
++ const before = await robotsOf(bob);
++ refused(await listFor(bob, { token: nodeToken }), 403, 'bot.forbidden');
++ refused(await createFor(bob, { token: nodeToken }), 403, 'bot.forbidden');
++ assert.strictEqual(await robotsOf(bob), before);
++ });
++
++ await check('a person naming another owner (?owner, body owner or X-OV-Subject) → 403 bot.forbidden', async () => {
++ const before = await robotsOf(bob);
++ refused(await listFor(bob, { user: alex }), 403, 'bot.forbidden');
++ refused(await t.call('GET', '/api/v1/robots', { user: alex, headers: { 'X-OV-Subject': bob.subject } }), 403, 'bot.forbidden');
++ refused(await createFor(bob, { user: alex }), 403, 'bot.forbidden');
++ refused(await t.call('POST', '/api/v1/robots', { user: alex, headers: { 'X-OV-Subject': bob.subject }, body: { name: 'x', profile_id: 'sim.rover' } }), 403, 'bot.forbidden');
++ assert.strictEqual(await robotsOf(bob), before);
++ });
++
++ await check('a person acts as themself, named or not', async () => {
++ const created = await createFor(alex, { user: alex });
++ assert.strictEqual(created.status, 201, created.text);
++ const plain = await t.call('POST', '/api/v1/robots', { user: alex, body: { name: 'Mine', profile_id: 'sim.rover' } });
++ assert.strictEqual(plain.status, 201, plain.text);
++ const named = await listFor(alex, { user: alex });
++ assert.strictEqual(named.status, 200, named.text);
++ const own = await t.call('GET', '/api/v1/robots', { user: alex });
++ assert.strictEqual(own.status, 200, own.text);
++ assert.deepStrictEqual(named.json.robots.map((r) => r.id).sort(), [created.json.robot.id, plain.json.robot.id].sort());
++ assert.deepStrictEqual(own.json.robots, named.json.robots);
++ });
++
++ await check('a service acts for the subject it names, with the capability (unchanged)', async () => {
++ const list = await listFor(bob, { cap: ['bot.robot.read'] });
++ assert.strictEqual(list.status, 200, list.text);
++ assert.deepStrictEqual(list.json.robots.map((r) => r.id), [bobsRobot.id]);
++ refused(await listFor(bob, { cap: ['bot.robot.control'] }), 403, 'capability.denied');
++ refused(await t.call('GET', '/api/v1/robots', { cap: ['bot.robot.read'] }), 422, 'bot.invalid_input');
++ const before = await robotsOf(bob);
++ const viaBody = await createFor(bob, { cap: ['bot.robot.manage'] });
++ assert.strictEqual(viaBody.status, 201, viaBody.text);
++ const viaHeader = await t.call('POST', '/api/v1/robots', { cap: ['bot.robot.manage'], headers: { 'X-OV-Subject': bob.subject }, body: { name: 'Via header', profile_id: 'sim.rover' } });
++ assert.strictEqual(viaHeader.status, 201, viaHeader.text);
++ assert.strictEqual(await robotsOf(bob), before + 2);
++ refused(await createFor(bob, { cap: ['bot.robot.read'] }), 403, 'capability.denied');
++ refused(await t.call('POST', '/api/v1/robots', { cap: ['bot.robot.manage'], body: { name: 'x', profile_id: 'sim.rover' } }), 422, 'bot.invalid_input');
++ });
++
++ await check('public routes, POST /pair and POST /devices/bind keep their own auth', async () => {
++ assert.strictEqual((await t.call('GET', '/api/v1/profiles', { token: null })).status, 200);
++ assert.strictEqual((await t.call('GET', '/api/v1/profiles/sim.rover', { token: null })).status, 200);
++ const pair = await t.call('POST', '/api/v1/pair', { token: null, body: { robot: bobsRobot.id, code: 'AAAA-AAAA' } });
++ assert.strictEqual(pair.status, 403, pair.text);
++ assert.match(pair.json.code, /^bot\.pairing_code_/);
++ const bind = await t.call('POST', '/api/v1/devices/bind', { token: nodeToken });
++ assert.strictEqual(bind.status, 201, bind.text);
++ });
++
++ await t.close();
++ done();
++})();