diff --git a/backend/src/app/rpc/commands/viewer.clj b/backend/src/app/rpc/commands/viewer.clj index 988cb26f5f..39ffbb1444 100644 --- a/backend/src/app/rpc/commands/viewer.clj +++ b/backend/src/app/rpc/commands/viewer.clj @@ -89,18 +89,27 @@ (mapv (fn [{:keys [id] :as lib}] (merge lib (bfc/get-file cfg id))))) - links (cond->> (->> (db/query conn :share-link {:file-id file-id}) - (mapv (fn [row] - (-> row - (update :pages db/decode-pgarray #{}) - ;; NOTE: the flags are deprecated but are still present - ;; on the table on old rows. The flags are pgarray and - ;; for avoid decoding it (because they are no longer used - ;; on frontend) we just dissoc the column attribute from - ;; row. - (dissoc :flags))))) - (= :share-link (:type perms)) - (filterv #(= (:id %) share-id))) + decode-link + (fn [row] + (-> row + (update :pages db/decode-pgarray #{}) + ;; NOTE: the flags are deprecated but are still present + ;; on the table on old rows. The flags are pgarray and + ;; for avoid decoding it (because they are no longer used + ;; on frontend) we just dissoc the column attribute from + ;; row. + (dissoc :flags))) + + ;; NOTE: on the share-link path we fetch at most the caller's own + ;; row with a composite (id, file-id) predicate, so sibling tokens + ;; never leave postgres. The membership path keeps the full list + ;; the share-management UI needs. A nil share-id never falls back + ;; to the full query; it simply resolves to an empty vector. + links (if (= :share-link (:type perms)) + (if-some [row (db/get* conn :share-link {:id share-id :file-id file-id})] + [(decode-link row)] + []) + (mapv decode-link (db/query conn :share-link {:file-id file-id}))) fonts (db/query conn :team-font-variant {:team-id (:id team) diff --git a/backend/test/backend_tests/rpc_viewer_test.clj b/backend/test/backend_tests/rpc_viewer_test.clj index cbf9e4493a..fde2679936 100644 --- a/backend/test/backend_tests/rpc_viewer_test.clj +++ b/backend/test/backend_tests/rpc_viewer_test.clj @@ -204,4 +204,52 @@ ;; Team member should see both share-links (t/is (= 2 (count share-links))) (t/is (some #(= link-a-id (:id %)) share-links)) - (t/is (some #(= link-b-id (:id %)) share-links)))))) + (t/is (some #(= link-b-id (:id %)) share-links)))) + + (t/testing "share-link viewer does not query sibling share-links" + ;; Direct regression test for the predicate-pushdown invariant: + ;; on the share-link path the bundle must resolve the caller's + ;; row with a composite (id, file-id) single-row lookup and must + ;; never run the full {:file-id} query that would load sibling + ;; tokens into the backend process. The with-redefs spies only + ;; record and delegate, following the instrumentation style used + ;; elsewhere in this suite (e.g. auth-ldap-test). + (let [share-queries (atom []) + share-gets (atom []) + orig-query @#'db/query + orig-get* @#'db/get*] + (with-redefs [db/query (fn [conn table params & opts] + (when (= :share-link table) + (swap! share-queries conj params)) + (apply orig-query conn table params opts)) + db/get* (fn [conn table params & opts] + (when (= :share-link table) + (swap! share-gets conj params)) + (apply orig-get* conn table params opts))] + (let [out (th/command! {::th/type :get-view-only-bundle + :share-id link-a-id + :file-id (:id file)}) + result (:result out)] + (t/is (nil? (:error out))) + (t/is (= 1 (count (:share-links result)))) + (t/is (= link-a-id (:id (first (:share-links result))))) + ;; No full-file sibling query ran on this path. + (t/is (empty? @share-queries)) + ;; Only composite single-row lookups ran (the permission + ;; check plus the bundle itself, same predicate in both). + (t/is (seq @share-gets)) + (t/is (every? #(= {:id link-a-id :file-id (:id file)} %) + @share-gets)))))) + + (t/testing "cross-file share-id replay fails closed" + (let [other-file (th/create-file* 2 {:profile-id (:id owner) + :project-id proj-id + :is-shared false}) + out (th/command! {::th/type :get-view-only-bundle + :share-id link-a-id + :file-id (:id other-file)}) + error (:error out) + error-data (ex-data error)] + (t/is (th/ex-info? error)) + (t/is (= :not-found (:type error-data))) + (t/is (= :object-not-found (:code error-data)))))))