From 915c939539f49df4e61161ad414fda871a4bcbc6 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Wed, 8 Jul 2026 14:49:48 +0200 Subject: [PATCH] fix(list): allow listing an ancestor folder when permission is only granted on a descendant A user granted read/write on a deep path (e.g. /folder2/a/b/c) got 403 listing any ancestor folder, including root, since permission was only checked against the exact requested path. Add hasDescendantPermission as a fallback and filter each listed child individually so only folders leading to a permitted descendant (or directly permitted entries) are shown. Also fixes the top-level authorized gate in index.js, which short-circuited before the list route's own check ever ran. Co-Authored-By: Claude Sonnet 5 Signed-off-by: kptdobe --- src/index.js | 12 ++- src/routes/list.js | 13 +++- src/storage/object/list.js | 24 +++++- src/utils/auth.js | 31 ++++++++ test/index.test.js | 44 +++++++++++ test/routes/list.test.js | 74 +++++++++++++++++- test/storage/object/list.test.js | 58 ++++++++++++++ test/utils/auth.test.js | 127 +++++++++++++++++++++++++++++++ 8 files changed, 374 insertions(+), 9 deletions(-) diff --git a/src/index.js b/src/index.js index b35699b2..5599e5fd 100644 --- a/src/index.js +++ b/src/index.js @@ -39,13 +39,21 @@ export default { return daResp({ status: 500, error: e.message }); } - const { users, authorized, key } = daCtx; + const { + users, authorized, key, api, + } = daCtx; // Anonymous users are not permitted const anon = users.some((user) => user.email === 'anonymous'); if (anon) return daResp({ status: 401 }); - if (!authorized) return daResp({ status: 403 }); + // `authorized` only reflects permission on the exact requested path. A user + // granted permission on some deeper descendant only (e.g. /folder2/a/b/c) + // is not "authorized" for a listing of an ancestor folder by that measure, + // but should still be able to list it to reach their descendant. The list + // route itself knows how to fall back to descendant permission, so this + // blanket gate must not shadow it. + if (!authorized && api !== 'list') return daResp({ status: 403 }); if (key?.startsWith('.da-versions')) { return daResp({ status: 404 }); diff --git a/src/routes/list.js b/src/routes/list.js index 78c35300..d6f7d1ea 100644 --- a/src/routes/list.js +++ b/src/routes/list.js @@ -11,13 +11,20 @@ */ import listBuckets from '../storage/bucket/list.js'; import listObjects from '../storage/object/list.js'; -import { getChildRules, hasPermission } from '../utils/auth.js'; +import { getChildRules, hasDescendantPermission, hasPermission } from '../utils/auth.js'; export default async function getList({ env, daCtx }) { if (!daCtx.org) return listBuckets(env, daCtx); - if (!hasPermission(daCtx, daCtx.key, 'read')) return { status: 403 }; + + const canReadDir = hasPermission(daCtx, daCtx.key, 'read'); + if (!canReadDir && !hasDescendantPermission(daCtx, daCtx.key, 'read')) { + return { status: 403 }; + } // Get the child rules of the current folder and store this in daCtx.aclCtx getChildRules(daCtx); - return /* await */ listObjects(env, daCtx); + // When the user can't read this folder directly but has permission on some + // descendant, only the folder itself is authorized as an ancestor - each + // child must still be checked individually before being shown. + return /* await */ listObjects(env, daCtx, undefined, !canReadDir); } diff --git a/src/storage/object/list.js b/src/storage/object/list.js index 72860f26..081322bb 100644 --- a/src/storage/object/list.js +++ b/src/storage/object/list.js @@ -16,6 +16,7 @@ import { import getS3Config from '../utils/config.js'; import formatList from '../utils/list.js'; +import { hasDescendantPermission, hasPermission } from '../../utils/auth.js'; function buildInput({ bucket, org, key, maxKeys, continuationToken, @@ -30,7 +31,24 @@ function buildInput({ return input; } -export default async function listObjects(env, daCtx, maxKeys) { +// Only used when the caller couldn't read the listed folder directly, but got +// in because some descendant is permitted (see src/routes/list.js). Every +// child then needs its own check: folders may lead to a permitted descendant +// even if not directly readable, but files have no descendants to fall back on. +function filterUnauthorized(daCtx, items) { + // item.path is bucket-relative (starts with /{org}/...) since one bucket is + // shared across orgs, but permissions are org-scoped and keyed off + // org-relative paths (like daCtx.key) - so the org segment must be stripped + // before checking. + const orgPrefix = `/${daCtx.org}`; + return items.filter((item) => { + const relPath = item.path.slice(orgPrefix.length) || '/'; + return hasPermission(daCtx, relPath, 'read') + || (!item.ext && hasDescendantPermission(daCtx, relPath, 'read')); + }); +} + +export default async function listObjects(env, daCtx, maxKeys, restrictToPermitted = false) { const config = getS3Config(env); const client = new S3Client(config); @@ -42,7 +60,9 @@ export default async function listObjects(env, daCtx, maxKeys) { try { const resp = await client.send(command); // console.log(resp); - const body = formatList(resp); + const body = restrictToPermitted + ? filterUnauthorized(daCtx, formatList(resp)) + : formatList(resp); const nextContinuationToken = resp.IsTruncated && resp.NextContinuationToken && resp.NextContinuationToken !== daCtx.continuationToken diff --git a/src/utils/auth.js b/src/utils/auth.js index 71199fcc..2b18b984 100644 --- a/src/utils/auth.js +++ b/src/utils/auth.js @@ -453,6 +453,37 @@ export function getChildRules(daCtx) { daCtx.aclCtx.childRules = [`${probeDir}**=${[...resultSet].join(',')}`]; } +/** + * Whether the user has `action` on some path at or below `path` (a descendant, + * or `path` itself). Used to let a listing of an ancestor folder proceed even + * when the user has no permission on the folder itself - e.g. a user granted + * read on `/folder2/a/b/c` only should still see `folder2` when listing `/`. + * Keyword paths (CONFIG, ACLTRACE) are ignored since they don't represent + * content and must not leak directory visibility. + */ +export function hasDescendantPermission(daCtx, path, action = 'read') { + if (path === null || path === undefined) return false; + const { pathLookup } = daCtx.aclCtx; + if (pathLookup.size === 0) return true; + + const p = !path.startsWith('/') ? `/${path}` : path; + const dirKey = p === '/' ? '/' : `${p.endsWith('/') ? p.slice(0, -1) : p}/`; + + return daCtx.users.every((u) => getIdents(u).some((ident) => (pathLookup.get(ident) || []) + .some((r) => { + if (!r.path.startsWith('/')) return false; + if (r.path === 'CONFIG' || r.path.endsWith('/CONFIG')) return false; + if (!r.actions.includes(action)) return false; + + let base = r.path; + if (base.endsWith('/+**')) base = base.slice(0, -3); + else if (base.endsWith('/**')) base = base.slice(0, -2); + else base = base.endsWith('/') ? base : `${base}/`; + + return base.startsWith(dirKey); + }))); +} + export function hasPermission(daCtx, path, action, keywordPath = false) { if (path === null || path === undefined) return false; if (daCtx.aclCtx.pathLookup.size === 0) { diff --git a/test/index.test.js b/test/index.test.js index 529add43..d884cd87 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -67,6 +67,50 @@ describe('fetch', () => { assert.strictEqual(resp.status, 403); }); + it('defers to the list route when not authorized on the exact path but api is list', async () => { + // daCtx.authorized reflects exact-path permission only. A user who is only + // granted access on a deep descendant (e.g. /folder2/a/b/c) has + // authorized=false for the root listing request, but routes/list.js knows + // how to fall back to descendant permission - so the blanket 403 gate here + // must not shadow it for the list api. + const hnd = await esmock('../src/index.js', { + '../src/utils/daCtx.js': { + default: async () => ({ + authorized: false, + users: [{ email: 'acapt@adobe.com' }], + path: '/list/kptdobe', + api: 'list', + org: 'kptdobe', + key: '', + }), + }, + '../src/handlers/get.js': { + default: async () => ({ status: 200, body: '[]', contentType: 'application/json' }), + }, + }); + + const resp = await hnd.fetch({ method: 'GET', url: 'http://www.example.com/list/kptdobe' }, {}); + assert.strictEqual(resp.status, 200); + }); + + it('still returns 403 for a non-list api when not authorized on the exact path', async () => { + const hnd = await esmock('../src/index.js', { + '../src/utils/daCtx.js': { + default: async () => ({ + authorized: false, + users: [{ email: 'acapt@adobe.com' }], + path: '/source/kptdobe/test/index.html', + api: 'source', + org: 'kptdobe', + key: 'test/index.html', + }), + }, + }); + + const resp = await hnd.fetch({ method: 'GET', url: 'http://www.example.com/source/kptdobe/test/index.html' }, {}); + assert.strictEqual(resp.status, 403); + }); + it('return 404 for unknown get route', async () => { const hnd = await esmock('../src/index.js', { '../src/utils/daCtx.js': { diff --git a/test/routes/list.test.js b/test/routes/list.test.js index 95f9d531..c1e8ef15 100644 --- a/test/routes/list.test.js +++ b/test/routes/list.test.js @@ -15,8 +15,10 @@ import esmock from 'esmock'; describe('List Route', () => { it('Test getList with permissions', async () => { const loCalled = []; - const listObjects = (e, c) => { - loCalled.push({ e, c }); + const listObjects = (e, c, maxKeys, restrictToPermitted) => { + loCalled.push({ + e, c, maxKeys, restrictToPermitted, + }); return {}; }; @@ -27,6 +29,8 @@ describe('List Route', () => { } return true; }; + // No descendant access either - the 403 must still hold. + const hasDescendantPermission = () => false; const getList = await esmock('../../src/routes/list.js', { '../../src/storage/object/list.js': { @@ -34,6 +38,7 @@ describe('List Route', () => { }, '../../src/utils/auth.js': { hasPermission, + hasDescendantPermission, }, }); const resp = await getList({ env: {}, daCtx: ctx, aclCtx: {} }); @@ -49,9 +54,74 @@ describe('List Route', () => { }); assert.strictEqual(1, loCalled.length); assert.strictEqual('q/q', loCalled[0].c.key); + assert.strictEqual(false, loCalled[0].restrictToPermitted, 'a normally-permitted dir must not be filtered'); const { childRules } = aclCtx; assert.strictEqual(1, childRules.length); assert(childRules[0].startsWith('/q/q/**='), 'Should have defined some child rule'); }); + + it('lists an ancestor folder when the user only has permission on a descendant', async () => { + const loCalled = []; + const listObjects = (e, c, maxKeys, restrictToPermitted) => { + loCalled.push({ c, restrictToPermitted }); + return {}; + }; + + // No direct permission on "" (root), but the user has read somewhere below it. + const hasPermission = (c, k, a) => !(k === '' && a === 'read'); + const hasDescendantPermission = (c, k, a) => k === '' && a === 'read'; + + const aclCtx = { pathLookup: new Map([['x', []]]) }; + const getList = await esmock('../../src/routes/list.js', { + '../../src/storage/object/list.js': { + default: listObjects, + }, + '../../src/utils/auth.js': { + hasPermission, + hasDescendantPermission, + }, + }); + + const resp = await getList({ + env: {}, + daCtx: { + org: 'bar', key: '', users: [], aclCtx, + }, + }); + + assert.notStrictEqual(resp?.status, 403); + assert.strictEqual(1, loCalled.length, 'listObjects should still be invoked'); + assert.strictEqual(true, loCalled[0].restrictToPermitted, 'children must be filtered per-permission'); + }); + + it('still returns 403 when the user has neither direct nor descendant permission', async () => { + const loCalled = []; + const listObjects = (...args) => { + loCalled.push(args); + return {}; + }; + const hasPermission = () => false; + const hasDescendantPermission = () => false; + + const getList = await esmock('../../src/routes/list.js', { + '../../src/storage/object/list.js': { + default: listObjects, + }, + '../../src/utils/auth.js': { + hasPermission, + hasDescendantPermission, + }, + }); + + const resp = await getList({ + env: {}, + daCtx: { + org: 'bar', key: '', users: [], aclCtx: { pathLookup: new Map([['x', []]]) }, + }, + }); + + assert.strictEqual(403, resp.status); + assert.strictEqual(0, loCalled.length); + }); }); diff --git a/test/storage/object/list.test.js b/test/storage/object/list.test.js index 993f2a88..74229cb7 100644 --- a/test/storage/object/list.test.js +++ b/test/storage/object/list.test.js @@ -105,6 +105,64 @@ describe('List Objects', () => { assert.strictEqual(resp.continuationToken, undefined); }); + it('filters out entries the user cannot reach when restrictToPermitted is set', async () => { + s3Mock.on(ListObjectsV2Command, { + Bucket: 'bkt', + Prefix: 'adobe/', + Delimiter: '/', + }).resolves({ + $metadata: { httpStatusCode: 200 }, + CommonPrefixes: [{ Prefix: 'adobe/folder2/' }, { Prefix: 'adobe/folder3/' }], + Contents: [{ Key: 'adobe/index.html', LastModified: new Date() }], + }); + + // The user is only granted read on a page deep inside folder2 - not on + // folder2 itself, not on folder3, and not on the root index.html. + const pathLookup = new Map([ + ['deep@bloggs.org', [{ group: 'deep@bloggs.org', path: '/folder2/a/b/c', actions: ['read'] }]], + ]); + const daCtx = { + bucket: 'bkt', + org: 'adobe', + key: '', + users: [{ email: 'deep@bloggs.org' }], + aclCtx: { pathLookup }, + }; + + const resp = await listObjects({}, daCtx, undefined, true); + const data = JSON.parse(resp.body); + assert.deepStrictEqual(data.map((item) => item.name), ['folder2']); + }); + + it('does not filter entries when restrictToPermitted is not set (default, backward compatible)', async () => { + s3Mock.on(ListObjectsV2Command, { + Bucket: 'bkt', + Prefix: 'adobe/', + Delimiter: '/', + }).resolves({ + $metadata: { httpStatusCode: 200 }, + CommonPrefixes: [{ Prefix: 'adobe/folder2/' }, { Prefix: 'adobe/folder3/' }], + Contents: [{ Key: 'adobe/index.html', LastModified: new Date() }], + }); + + // Same restrictive ACL as above, but the caller did not ask for filtering + // (e.g. because the exact-path permission check already passed). + const pathLookup = new Map([ + ['deep@bloggs.org', [{ group: 'deep@bloggs.org', path: '/folder2/a/b/c', actions: ['read'] }]], + ]); + const daCtx = { + bucket: 'bkt', + org: 'adobe', + key: '', + users: [{ email: 'deep@bloggs.org' }], + aclCtx: { pathLookup }, + }; + + const resp = await listObjects({}, daCtx); + const data = JSON.parse(resp.body); + assert.deepStrictEqual(data.map((item) => item.name).sort(), ['folder2', 'folder3', 'index'].sort()); + }); + it('does not return same continuation token again', async () => { s3Mock.on(ListObjectsV2Command, { Bucket: 'rt-bkt', diff --git a/test/utils/auth.test.js b/test/utils/auth.test.js index f4359507..f9de0234 100644 --- a/test/utils/auth.test.js +++ b/test/utils/auth.test.js @@ -23,6 +23,7 @@ import { getAclCtx, getChildRules, getUserActions, + hasDescendantPermission, hasPermission, logout, pathSorter, @@ -1195,4 +1196,130 @@ describe('DA auth', () => { assert.strictEqual(1, rules.length); assert(rules[0] === '/foo/**=read,write' || rules[0] === '/foo/**=write,read'); }); + + describe('hasDescendantPermission (ancestor listing)', async () => { + const DA_CONFIG = { + test: { + ':type': 'multi-sheet', + permissions: { + data: [ + // Only a deep exact page is granted - the reported customer bug. + { path: '/folder2/a/b/c', groups: 'deep@bloggs.org', actions: 'read' }, + // A folder granted via a recursive-descendants-only wildcard. + { path: '/folder/**', groups: 'wild@bloggs.org', actions: 'read' }, + // A folder granted via +** (folder itself and descendants), write-only + // (write implies read). + { path: '/team/proj/+**', groups: 'plus@bloggs.org', actions: 'write' }, + // A user with both a wildcard folder and a deep exact page - should + // reveal both "folder" and "folder2" when listing "/". + { path: '/folder/**', groups: 'both@bloggs.org', actions: 'read' }, + { path: '/folder2/a/b/c', groups: 'both@bloggs.org', actions: 'read' }, + // A user who shares one path with another co-author (AND semantics). + { path: '/shared/**', groups: 'sharedA@bloggs.org', actions: 'read' }, + { path: '/shared/**', groups: 'sharedB@bloggs.org', actions: 'read' }, + // Unrelated grants that must not leak directory visibility. + { path: '/other/x', groups: 'other@bloggs.org', actions: 'write' }, + { path: 'CONFIG', groups: 'cfgonly@bloggs.org', actions: 'write' }, + { path: '/site/CONFIG', groups: 'sitecfg@bloggs.org', actions: 'write' }, + ], + }, + }, + }; + const env = { DA_CONFIG: { get: (name) => DA_CONFIG[name] } }; + + async function aclCtxFor(...emails) { + const users = emails.map((email) => ({ email })); + const aclCtx = await getAclCtx(env, 'test', users, '/irrelevant'); + return { users, aclCtx }; + } + + it('returns false for a null or undefined path', async () => { + const { users, aclCtx } = await aclCtxFor('deep@bloggs.org'); + assert(!hasDescendantPermission({ users, aclCtx }, null, 'read')); + assert(!hasDescendantPermission({ users, aclCtx }, undefined, 'read')); + }); + + it('returns true for every ancestor of a deep exact grant, and the exact path itself', async () => { + const { users, aclCtx } = await aclCtxFor('deep@bloggs.org'); + const ctx = { users, aclCtx }; + assert(hasDescendantPermission(ctx, '/', 'read')); + assert(hasDescendantPermission(ctx, '/folder2', 'read')); + assert(hasDescendantPermission(ctx, '/folder2/a', 'read')); + assert(hasDescendantPermission(ctx, '/folder2/a/b', 'read')); + assert(hasDescendantPermission(ctx, '/folder2/a/b/c', 'read')); + assert(hasDescendantPermission(ctx, 'folder2', 'read'), 'should normalize missing leading slash'); + }); + + it('returns false beyond the exact grant and for unrelated siblings', async () => { + const { users, aclCtx } = await aclCtxFor('deep@bloggs.org'); + const ctx = { users, aclCtx }; + // An exact (non-wildcard) grant does not cover its own children. + assert(!hasDescendantPermission(ctx, '/folder2/a/b/c/d', 'read')); + assert(!hasDescendantPermission(ctx, '/folder3', 'read')); + // Prefix-collision guard: "/team" must not match "/teamx". + assert(!hasDescendantPermission(ctx, '/foo', 'read')); + }); + + it('respects the action being checked', async () => { + const { users, aclCtx } = await aclCtxFor('deep@bloggs.org'); + const ctx = { users, aclCtx }; + assert(hasDescendantPermission(ctx, '/folder2', 'read')); + assert(!hasDescendantPermission(ctx, '/folder2', 'write')); + }); + + it('returns true when the deepest grant is via a recursive wildcard', async () => { + const { users, aclCtx } = await aclCtxFor('wild@bloggs.org'); + const ctx = { users, aclCtx }; + assert(hasDescendantPermission(ctx, '/', 'read')); + assert(hasDescendantPermission(ctx, '/folder', 'read')); + assert(!hasDescendantPermission(ctx, '/folderx', 'read'), 'prefix-collision guard'); + }); + + it('returns true for ancestors of a +** grant', async () => { + const { users, aclCtx } = await aclCtxFor('plus@bloggs.org'); + const ctx = { users, aclCtx }; + // "/team" and "/team/proj" are ancestors of the granted root and need + // this fallback to be listable. "/team/proj/sub" is already directly + // covered by hasPermission's forward +** match, so it's out of scope + // for this ancestor-only helper (hasPermission is checked first by + // callers and would already return true there). + assert(hasDescendantPermission(ctx, '/team', 'read')); + assert(hasDescendantPermission(ctx, '/team/proj', 'read')); + assert(!hasDescendantPermission(ctx, '/teamx', 'read'), 'prefix-collision guard'); + }); + + it('reveals every distinct top-level folder that leads to a grant for the same user', async () => { + const { users, aclCtx } = await aclCtxFor('both@bloggs.org'); + const ctx = { users, aclCtx }; + assert(hasDescendantPermission(ctx, '/', 'read')); + assert(hasDescendantPermission(ctx, '/folder', 'read')); + assert(hasDescendantPermission(ctx, '/folder2', 'read')); + assert(!hasDescendantPermission(ctx, '/folder3', 'read')); + }); + + it('requires every co-author to have descendant permission (AND semantics)', async () => { + const both = await aclCtxFor('sharedA@bloggs.org', 'sharedB@bloggs.org'); + assert(hasDescendantPermission(both, '/shared', 'read')); + + const mixed = await aclCtxFor('deep@bloggs.org', 'other@bloggs.org'); + assert(!hasDescendantPermission(mixed, '/folder2', 'read'), 'other@bloggs.org has no grant there'); + }); + + it('does not leak directory visibility from CONFIG-only grants', async () => { + const cfgOnly = await aclCtxFor('cfgonly@bloggs.org'); + assert(!hasDescendantPermission(cfgOnly, '/', 'read')); + + const siteCfg = await aclCtxFor('sitecfg@bloggs.org'); + assert(!hasDescendantPermission(siteCfg, '/', 'read')); + assert(!hasDescendantPermission(siteCfg, '/site', 'read')); + }); + + it('returns true unconditionally when there is no ACL config at all', async () => { + const openEnv = { DA_CONFIG: { get: () => undefined } }; + const users = [{ email: 'anyone@bloggs.org' }]; + const aclCtx = await getAclCtx(openEnv, 'test', users, '/irrelevant'); + assert(hasDescendantPermission({ users, aclCtx }, '/', 'read')); + assert(hasDescendantPermission({ users, aclCtx }, '/anything/at/all', 'read')); + }); + }); });