From 6fe1bd7d9be538cded5961825ede152043f497a3 Mon Sep 17 00:00:00 2001 From: dev9-bb Date: Fri, 23 Oct 2026 20:49:05 +0800 Subject: [PATCH] fix(acl): skip a dirent with no Authority kind instead of hiding the page `filter_chunk()` aborted the whole 512-entry batch when a child was neither a regular file nor a directory, and `cf_acl_filter_dirents()` turns one aborted chunk into a NULL listing: a single native symlink emptied its entire directory page. `server/pack-dir.c` already carries S_IFLNK, so that dirent is reachable in the data model. - names that are empty, '/'-bearing, '.' or '..' still abort the chunk: that is corrupt input, not a per-child decision - an entry without a kind is skipped; it gets no permission below and stays hidden, exactly like a denied sibling - `included` keeps the checks[] order, so permissions[i] still maps to the child that produced it, and an all-skipped chunk returns an empty page successfully instead of a fail-closed NULL Cheap contracts updated: skip-vs-abort separation, index alignment and the empty-chunk path. `python3 -m unittest discover -s tests -p 'test_cf_*.py'` passes (12 tests). Native MariaDB/Hub integration is unchanged and still needs the manual environment. --- common/cf-acl.c | 25 ++++++++++++--- tests/test_cf_acl_dirent_kind.py | 53 ++++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/common/cf-acl.c b/common/cf-acl.c index 2e5fb795..3d25eec7 100644 --- a/common/cf-acl.c +++ b/common/cf-acl.c @@ -92,6 +92,9 @@ filter_chunk (const char *repo, const char *dir, const char *user, json_t *req=json_object(), *checks=json_array(), *reply=NULL; GString *body=g_string_new(NULL); char *payload=NULL; + /* Same order as checks: an entry without an Authority kind is skipped, + * so position i of permissions no longer maps to position i of begin. */ + GList *included=NULL; if (!req || !checks || !body || !repo || !dir || !user) goto done; for (GList *p=begin; p && count > 0; p=p->next, --count) { GObject *item=p->data; @@ -99,11 +102,18 @@ filter_chunk (const char *repo, const char *dir, const char *user, int mode=0; g_object_get(item,"obj_name",&name,"mode",&mode,NULL); if (!name || !*name || strchr(name,'/') || - !strcmp(name,".") || !strcmp(name,"..") || - (!S_ISDIR(mode) && !S_ISREG(mode))) { + !strcmp(name,".") || !strcmp(name,"..")) { g_free(name); goto done; } + /* A dirent that is neither a regular file nor a directory (a native + * symlink, for example) has no Authority kind. Skip that child only: + * aborting the chunk would hide every sibling on the page. A hidden + * child simply gets no permission below, like a denied sibling. */ + if (!S_ISDIR(mode) && !S_ISREG(mode)) { + g_free(name); + continue; + } char *path=g_build_filename(dir,name,NULL); g_free(name); /* File rights follow the containing directory; a directory entry @@ -117,9 +127,13 @@ filter_chunk (const char *repo, const char *dir, const char *user, goto done; } json_decref(check); + included=g_list_append(included,item); } unsigned size=(unsigned)json_array_size(checks); - if (!size || size>512 || + /* Every child was skipped: nothing to authorize, the page stays empty + * instead of failing closed over the whole directory. */ + if (!size) { result=0; goto done; } + if (size>512 || json_object_set_new(req,"repo_id",json_string(repo)) || json_object_set_new(req,"username",json_string(user)) || json_object_set_new(req,"partial",json_true()) || @@ -131,14 +145,14 @@ filter_chunk (const char *repo, const char *dir, const char *user, if (!json_is_true(json_object_get(reply,"allowed")) || g_strcmp0(json_string_value(json_object_get(reply,"contract")),"cf-acl-v1") || !json_is_array(permissions) || json_array_size(permissions)!=size) goto done; - GList *p=begin; + GList *p=included; for (unsigned i=0;inext) { const char *permission=json_string_value(json_array_get(permissions,i)); if (!json_is_null(json_array_get(permissions,i)) && (!permission || (strcmp(permission,"r") && strcmp(permission,"rw")))) goto done; } - p=begin; + p=included; for (unsigned i=0;inext) { const char *permission=json_string_value(json_array_get(permissions,i)); if (permission) { @@ -149,6 +163,7 @@ filter_chunk (const char *repo, const char *dir, const char *user, } result=0; done: + g_list_free(included); json_decref(reply);json_decref(req);json_decref(checks); g_free(payload); if(body)g_string_free(body,TRUE); diff --git a/tests/test_cf_acl_dirent_kind.py b/tests/test_cf_acl_dirent_kind.py index 123b1583..faf6c7ac 100644 --- a/tests/test_cf_acl_dirent_kind.py +++ b/tests/test_cf_acl_dirent_kind.py @@ -1,18 +1,53 @@ -"""Cheap contract: native batch sends file vs directory kind to Hub Authority.""" +"""Cheap contract: native batch sends file vs directory kind to Hub Authority. + +An entry with no Authority kind (a native symlink) must be skipped on its own: +aborting the chunk hides every sibling on the page. +""" from pathlib import Path import unittest +ROOT = Path(__file__).resolve().parents[1] + + +def chunk(): + source = (ROOT / 'common' / 'cf-acl.c').read_text(encoding='utf-8') + return source.split('filter_chunk (', 1)[1].split('\nGList *\ncf_acl_filter_dirents', 1)[0] + class NativeDirentKindTests(unittest.TestCase): def test_native_permission_batch_uses_real_dirent_mode(self): - root = Path(__file__).resolve().parents[1] - source = (root / 'common' / 'cf-acl.c').read_text(encoding='utf-8') - chunk = source.split('filter_chunk (', 1)[1].split('\nGList *\ncf_acl_filter_dirents', 1)[0] - self.assertIn('"mode",&mode', chunk) - self.assertIn('S_ISDIR(mode) ? "dir" : "file"', chunk) - self.assertIn('"path",path,"kind",kind,"action","read"', chunk) - self.assertIn('cf_v2_acl_check_json(seaf,payload,body)', chunk) - self.assertIn('json_array_size(permissions)!=size', chunk) + text = chunk() + self.assertIn('"mode",&mode', text) + self.assertIn('S_ISDIR(mode) ? "dir" : "file"', text) + self.assertIn('"path",path,"kind",kind,"action","read"', text) + self.assertIn('cf_v2_acl_check_json(seaf,payload,body)', text) + self.assertIn('json_array_size(permissions)!=size', text) + + def test_invalid_name_still_aborts_the_chunk(self): + text = chunk() + name_guard = text.split('g_object_get(item,"obj_name",&name,"mode",&mode,NULL);', 1)[1] + name_guard = name_guard.split('/* A dirent that is neither', 1)[0] + self.assertIn('strchr(name,\'/\')', name_guard) + self.assertIn('goto done;', name_guard) + + def test_unknown_dirent_kind_skips_only_that_child(self): + text = chunk() + mode_guard = text.split('if (!S_ISDIR(mode) && !S_ISREG(mode)) {', 1)[1].split('}', 1)[0] + self.assertIn('continue;', mode_guard) + self.assertNotIn('goto done', mode_guard) + + def test_permissions_map_to_the_checked_children_only(self): + text = chunk() + mapping = text.split('json_array_size(permissions)!=size', 1)[1].split('result=0;', 1)[0] + self.assertIn('included=g_list_append(included,item);', text) + self.assertIn('GList *p=included;', mapping) + self.assertEqual(mapping.count('p=included;'), 2) + self.assertNotIn('GList *p=begin;', mapping) + self.assertIn('g_list_free(included);', text) + + def test_all_children_skipped_is_not_a_batch_failure(self): + text = chunk() + self.assertIn('if (!size) { result=0; goto done; }', text) if __name__ == '__main__':