Allow label scoped resources in POLICIES_STRING_REGEX - #7178
Conversation
Signed-off-by: Goyam Jain <goyam24224@iiitd.ac.in>
✅ Deploy Preview for pipecd-site canceled.
|
| "resources=(" + | ||
| rbacResourceTypes() | ||
| .map((v) => v.replace(/\*/, "\\*")) | ||
| .map((v) => v.replace(/\*/, "\\*") + "(\\{[^}]*\\})?") |
There was a problem hiding this comment.
[^}]* also allows ;, which the parser can't handle. resources=application{a=b;c:d};actions=get passes validation but gets silently dropped by parseRBACPolicies, leaving the role with no policies
\\{[^;{}]*\\} fixes this while keeping the valid cases above. similarly, {} / {garbage} parse to empty labels, so the policy gets saved un-scoped. i think we should guard against that too
There was a problem hiding this comment.
Thanks for catching both of these — the semicolon-in-label escape and the empty/malformed label case. Fixed: the label block now excludes ;, {, }, :, , from key/value characters (so application{a=b;c:d} no longer escapes into the resources/actions separator), and it requires at least one valid key:value pair, so {} and {garbage} are rejected too. Pushed in ce7d41d.
| ])("matches policy with label scoped resources: %s", (policy) => { | ||
| expect(POLICIES_STRING_REGEX.test(policy)).toBe(true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
could you add resources=application{a=b;c:d};actions=get and resources=application{};actions=get to the invalid cases once the pattern is tightened? these are the cases where form validation and parseRBACPolicies currently disagree
There was a problem hiding this comment.
Added both cases to the invalid-policy test list — resources=application{a=b;c:d};actions=get and resources=application{};actions=get — and confirmed the full suite still passes (50 suites / 185 tests) with all existing valid/invalid cases intact.
Could you rewrite the description in English? a few lines are in Hinglish .. 🤔 |
…pty/malformed labels Addresses review feedback from @areebahmeddd on pipe-cd#7178. - The label block now requires at least one valid key:value pair and excludes ';', '{', '}', ':', ',' from key/value characters, so a semicolon inside a label can no longer escape into the resources/actions separator (e.g. application{a=b;c:d} is now rejected instead of being silently dropped by parseRBACPolicies). - Empty ("{}") or malformed ("{garbage}") label blocks are now rejected instead of passing validation as bare/broken labels. - Added the two invalid cases the reviewer requested to project.test.ts and confirmed all existing valid/invalid cases still pass. Signed-off-by: Goyam Jain <goyam24224@iiitd.ac.in>
|
Apologies for that — the description had a few lines in Hinglish. I've rewritten it fully in English while keeping the same technical content. |
|
Hi @areebahmeddd, just checking in on this one — I've updated the description as requested. Let me know if there are any further changes needed, or if this is ready for another look. Thanks for the feedback so far! |
|
looks good. maybe @khanhtc1202 will give a final look and merge 🙂 |
rahulshendre
left a comment
rahulshendre
left a comment
There was a problem hiding this comment.
LGTM @Goyamjain06, thank you : )
Thanks @areebahmeddd for the review.
There was a problem hiding this comment.
Pull request overview
This PR updates the web UI RBAC policy-string validation so that label-scoped resources (e.g. application{env:prod}) are accepted by POLICIES_STRING_REGEX, unblocking Add/Edit Role dialogs from rejecting policies that the parser/formatter already supports.
Changes:
- Extend
POLICIES_STRING_REGEXto allow an optional{key:value(,key:value)*}label block after each resource type. - Add a dedicated unit test file covering valid label-scoped policies and several invalid formats.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
web/src/constants/project.ts |
Expands the RBAC policy regex with reusable label-pair/label-block subpatterns. |
web/src/constants/project.test.ts |
Adds unit tests validating label-scoped policy strings and rejecting invalid forms. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // A single label pair, e.g. "env:prod". Key/value exclude the characters | ||
| // that are structurally significant elsewhere in the grammar (`;{}:,`) so a | ||
| // label block can't accidentally swallow the resources/actions separator. | ||
| const LABEL_PAIR = "[^;{}:,]+:[^;{}:,]+"; |
There was a problem hiding this comment.
Good catch — fixed. LABEL_PAIR now excludes \s alongside ;{}:,, so a \n\n inside a label key/value can no longer act as a policy separator. Added a regression case for this (newline inside a label value) and reran the full suite — 186/186 passing. Pushed in 20dd7ac.
| "resources=application", // missing actions | ||
| "resources=application{a=b;c:d};actions=get", // semicolon escapes the label block | ||
| "resources=application{};actions=get", // empty label block |
There was a problem hiding this comment.
Good catch — fixed. LABEL_PAIR now excludes \s alongside ;{}:,, so a \n\n inside a label key/value can no longer act as a policy separator. Added a regression case for this (newline inside a label value) and reran the full suite — 186/186 passing. Pushed in 20dd7ac.
…jection Signed-off-by: Goyam Jain <goyam24224@iiitd.ac.in>
POLICIES_STRING_REGEXnow accepts an optional{key:value,...}label block after each resource type, using the same shape asRESOURCES_LABELS_REGEX(used inparseRBACPolicies).key:valuepair and excludes;,{,},:,,from key/value characters, so a semicolon inside a label can no longer escape into the resources/actions separator (e.g.application{a=b;c:d}is rejected instead of being silently dropped byparseRBACPolicies), and empty ({}) or malformed ({garbage}) label blocks are rejected instead of passing validation.web/src/constants/project.test.tswith 14 cases: no-label regression guard, label-scoped valid cases, and invalid formats (including the semicolon-escape and empty-label cases from review).make test/websuite passes (50 suites / 185 tests), including theadd-role-dialogandedit-role-dialogtests.