fix(getdata): apply row level security to selectbox source values #341
Assignees
allan (Allan)
dcbot
DylanLeong
henrik
hermes (hermes)
mihajlo
sabir
sead
trevor
Yury
Clear assignees
allan (Allan)
dcbot
DylanLeong
henrik
hermes (hermes)
mihajlo
sabir
sead
trevor
Yury
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: dc/dc#341
Reference in new issue
Block a user
Summary
The cell dropdown (selectbox) values sourced from a
library.member.columnreference are read directly from the source table ineditors/getdata.sas(%dq_selects), without applying Row Level Security. The grid rows are filtered by%mpe_filtermaster, but the dropdown was not, so values from restricted rows leaked into the dropdown.This runs
%mpe_filtermasterover the source table and reads the distinct values through the filtered view. EDIT mode mirrors the grid on this (editor) page. A data step view is used because a fileref cannot be%inc'd inside a proc sql where clause.Closes #340
Test
getdata.test.1.sasrestrictsSOME_BESTNUMinMPE_X_TESTwith a Row Level Security rule (only the value 44 is visible) and asserts the selectbox values returned bygetdataare restricted to 44. Without the fix the dropdown returns the other distinct values and the test fails.Note: the SAS test suite needs a live estate and has not been executed here. The test requires the runner not to be a member of the
&mpeadminsgroup (Row Level Security is skipped for admins) - the same assumption made bympe_filtermaster.test.1.sas.Checks
sasjs lintclean (the only warning is pre-existing, ingetstagetable.test.sas).sasjs cb -t v4builds;mf_getuniquefileref,mf_getuniquenameandmpe_filtermasterare inlined in the builtservices/editors/getdata.sas.Note on CI:
Build / Build-and-ng-testfails at the Check audit step, on new upstream advisories foraxios(via@sasjs/adapter) andbraces(via@sasjs/cli). This change touches only two.sasfiles and no dependency manifests, so the audit result is identical onmain- it is a repo-wide dependency-advisory condition, not something introduced by this PR. TheLighthouseandBuild-and-test-developmentjobs are unaffected by the SAS change.Reviewed the full base...head diff (selectbox RLS fix in getdata.sas plus the new SAS test), the mpe_filtermaster / mf_getuniquefileref / mf_getuniquename macro contracts, the SASjs mock seed data, CI workflows and the audit / lighthouse job logs, npm manifests and .npmrc files, and the secret-scanning hook.
4b6c2df2) failed identically and this diff touches only .sas files, so the pipeline cannot go green on any PR until a separate chore PR bumps sas/package.json @sasjs/cli 4.20.4 -> 4.20.9 and runs npm audit fix for axios. The Lighthouse job fails separately with ECONNREFUSED 127.0.0.1:5000 while deploying mock services (SASjs server never came up) - runner infra, also pre-existing.4 findings above for review.
Re-review at new head
0a1edb7(delta since the reviewede6ba472is one comment-only commit); re-checked the full base...head diff against the mpe_filtermaster contract, the seeded demo data, the npm manifests and .npmrc files, the CI workflows plus a locally reproduced audit, and the secret-scanning hook.CI is red at this head. Reproduced locally against the head's lockfiles:
npm audit --omit=devin sas/ reports 7 high advisories - axios via transitive @sasjs/adapter 4.19.0 (GHSA-vh66-26gq-q6x8, GHSA-r4gj-5m52-g5wh and others) and braces GHSA-vfj7-8cjw-p6xm via the @sasjs/cli 4.20.4 chain; the audit fix resolves by installing @sasjs/cli 4.20.9. Pre-existing (4b6c2df2ande6ba4722failed identically, and this diff touches only .sas files), so the pipeline stays red until a separate chore PR bumps sas/package.json. The Lighthouse job fails separately with ECONNREFUSED 127.0.0.1:5000 deploying the mock SASjs server - runner infra, also pre-existing.The new test sas/sasjs/services/editors/getdata.test.1.sas is never executed by CI: .gitea/workflows/build.yaml runs audit, lint, Angular tests, a production build and Cypress only - no
sasjs teststep exists (the sas/package.jsonviyascript runs it, but nothing invokes it), and Build-and-test-development is skipped as a dependent of the failed build. The fix therefore ships with no automated verification. Note the test premise only holds on SAS-seeded data: mpe_makedata.sas:2169-2177 seeds 490 ranuni SOME_BESTNUM rows after the seven static 44s, but the SASjs server mock (sas/mocks/sasjs/services/admin/makedata.js:1937-2015) seeds exactly those 7 rows all with SOME_BESTNUM 44, so the distinct_bestnum>1 precondition would fail on the server-ci target - worth knowing before wiring the suite into CI.Conventional commits: the new commit subject
Update sas/sasjs/services/editors/getdata.sasis non-conventional, and this repo releases from commit messages (semantic-release in .releaserc, default merge style merge, so commits land verbatim). Nothing is lost - the fix(getdata) commit drives the release - but the Update commit only trims the comment the fix commit added; fold it into the fix commit before merge so main carries one clean release-driving subject.Still open from the previous review, minor: per-source loop artifacts are not cleaned up. work.&srcview (sas/sasjs/services/editors/getdata.sas:649), work.&filterds (:646) and the temp fileref persist while the same loop drops its temp table (drop table dqdata&x at :671) - dropping the view and filter table after the select would match the surrounding pattern; cosmetic since WORK is session-scoped. The comment trim also removed the one-line why (a fileref cannot be %inc'd inside a proc sql where clause, documented in mpe_filtermaster.sas:11) - consider restoring it.
Pre-existing hardening, advisory: client/.npmrc is not committed - CI writes it from secrets.NPMRC (.gitea/workflows/build.yaml:24-26) - so ignore-scripts=true and save-exact=true do not cover the client tree for local developers, and client/package.json carries ^/~ ranges accordingly.
5 findings above for review.
0a1edb7c14to768d33c618Re-review at new head
768d33c: the force-push squashed the branch into the single conventional fix(getdata) commit, the test moved to getdata.test.2.sas, and the loop cleanup and comment restoration from the prior reviews are in. Re-checked the full base...head diff against the mpe_filtermaster / mf_getuniquefileref contracts, the SAS and SASjs-mock seed data, the CI workflows and job statuses, and the secret-scanning hook.CI is green at this head: Build-and-ng-test (including the strict npm audit steps) and Lighthouse both pass, and the previously blocking axios/braces advisories are gone from the sas/ tree (sas/package.json now pins @sasjs/cli 4.20.5). Build-and-test-development was still running when the review was posted; it exercises Cypress against the mock services, not the new SAS test.
The new test sas/sasjs/services/editors/getdata.test.2.sas is still not executed by any CI workflow: no workflow runs
sasjs testor any SAS test step, so the RLS fix ships without automated verification. The file name and structure are correct for the suite (the CLI's isTestFile regexp matches .test.2.sas, testsetUp/testterm are wired in sasjsconfig.json), so wiring it in is purely a CI change. Note the premise holds only on SAS-seeded data: mpe_makedata.sas:2202 seeds the 490 ranuni SOME_BESTNUM rows, but the SASjs server mock (sas/mocks/sasjs/services/admin/makedata.js:1970-2037) seeds only the seven 44 rows, so the distinct_bestnum>1 precondition assertion would fail on a server-ci target.sas/sasjs/services/editors/getdata.sas:705 - the per-source cleanup clears the fileref but mpe_filtermaster itself created it with
filename &outref temp;(mpe_filtermaster.sas:75).filename ... clearon a temp fileref is harmless in a session, but note the grid path at getdata.sas:197 leaves its own filtref alive, so the two paths now differ in cleanup style; cosmetic.getdata.sas:671 - %mpe_filtermaster(EDIT,&lib..&ds,...) reads RLS for the source table only; if the source table is not registered in mpe_tables the varfrom lookup inside filtermaster silently yields 0 rows and the RLS scope is still applied, which matches the grid behaviour, so this is consistent. No issue - verified the mode/scope mapping (EDIT) and the group logic against the mpe_row_level_security contract.
Test hygiene, minor: getdata.test.2.sas deletes all MPE_GROUPS rows for the runner and restores them from work.groups_backup via proc append at the end, but MPE_GROUPS holds a tx_from column that the backup keeps - fine - while an abort between the delete and the restore leaves the runner with no group rows (the same exposure exists in mpe_filtermaster.test.1.sas, so this follows the established pattern; noting it only because two tests now share it).
Pre-existing hardening, advisory and unchanged from the prior review: client/.npmrc is written from secrets.NPMRC in CI (.gitea/workflows/build.yaml:40-44) and not committed, so ignore-scripts=true and save-exact=true do not cover the client tree for local developers, and client/package.json carries ^/~ ranges accordingly.
3 findings above for review.
Re-review at new head
0d3d475(delta since the reviewed768d33cis one test commit, test(getdata): make selectbox RLS test runnable on admin estates); re-checked the full base...head diff against the mpe_filtermaster / mx_execute / mp_assert macro contracts, compiled the test suite with @sasjs/cli 4.20.5 to inspect the assembled test program, ran sasjs lint, confirmed both CI jobs green at this head, and checked the conventional-commit subjects and the gitleaks hook.sas/sasjs/services/editors/getdata.test.2.sas:67 - "%return;" is in open code. The CLI compiles each test into a single submitted program (testinit + macros + test body + testterm, verified in the compiled sasjsbuild/tests/services/editors/getdata.test.2.sas), so the admin-skip block at lines 58-68 sits outside any macro definition. The SAS macro language reference restricts %RETURN to macro definitions ("Valid only in a macro definition"), and the SAS error reference lists "The %RETURN statement is not valid in open code" - the 9.4M5 open-code exception covers %IF-%THEN/%ELSE only, not %RETURN. This is the only open-code %return in the tree: every service wraps its body first (getdata.sas:207 %macro mpestp_getdata();, viewlibs.sas, postdata.sas) and bitemporal_dataloader.test.2.sas wraps its tests in %macro bitemporal_tester. As written the intended clean skip on admin estates is not delivered: the macro facility errors on the statement and execution falls through into the demote + assertion section, where Test 1 (grid RLS control) fails because admin status comes from SAS identity group membership, which the MPE_GROUPS delete cannot change. Wrap the test body in a %macro ... %mend invocation as the services do, or replace the early exit with a %do ... %end guard around the assertions.
Still current from the prior review: no CI workflow executes this test (or any SAS test). "sasjs test" appears only in the viya npm script and no workflow invokes it, so the RLS fix ships without automated verification. The new header documents why the SASjs mock target cannot run it (only one distinct SOME_BESTNUM seeded), so wiring the suite into CI will need a real SAS runner.
Pre-existing, unchanged from prior reviews: client/.npmrc is written from secrets.NPMRC in CI rather than committed, so ignore-scripts=true / save-exact=true do not govern local installs in the client tree and client/package.json carries ^/~ ranges accordingly.
3 findings above for review.
Re-review at new head
78d436f(delta since the reviewed0d3d475is one test-only commit: the open-code %return is replaced by an %if/%else %do guard - valid under the 9.4M5 open-code exception - and the restricted value is now derived from the data instead of hardcoded); re-checked the full base...head diff, the dc_admin_group resolution through the deployed settings.sas (it is declared %global there, so the admin-skip guard does resolve in a test session), the mpe_makedata seed rows behind the derived value, the CI jobs (Build-and-ng-test green including the strict audit step; the other two pending, none red), and the commit subject (conventional).sas/sasjs/services/editors/getdata.test.2.sas:110 -
select distinct some_bestnum into :rls_value trimmeddoes not create the macro variable at all when the query returns zero rows (SAS SQL Users Guide: "If the query returns no rows... the macro variable is not created"), so the new precondition meant to catch that case at line 150,%length(&rls_value)>0, resolves the unresolved reference as literal text - 11 characters - and falsely PASSES; the garbage is meanwhile already written into MPE_ROW_LEVEL_SECURITY at line 131, where RLS_RAW_VALUE="(&rls_value)" stores the literal string. Add%let rls_value=;before the select so the zero-rows case leaves the variable empty and the precondition fails cleanly.Still current from the prior review: no CI workflow executes this test or any SAS test. "sasjs test" appears only in the viya npm script (sas/package.json:25) and no workflow invokes it, so the RLS fix ships without automated verification; the header already documents why the SASjs mock target cannot run it (only one distinct SOME_BESTNUM seeded), so wiring the suite into CI needs a real SAS runner.
Pre-existing, unchanged from prior reviews: client/.npmrc is written from secrets.NPMRC in CI rather than committed, so ignore-scripts=true / save-exact=true do not govern local installs in the client tree and client/package.json carries ^/~ ranges accordingly.
3 findings above for review.