Skip to content

WEB-932: Fix null dereference in HasPermissionDirective on unauthenticated session#3531

Merged
IOhacker merged 1 commit intoopenMF:devfrom
adity-a34:WEB-932-fix-has-permission-null-crash
Apr 26, 2026
Merged

WEB-932: Fix null dereference in HasPermissionDirective on unauthenticated session#3531
IOhacker merged 1 commit intoopenMF:devfrom
adity-a34:WEB-932-fix-has-permission-null-crash

Conversation

@adity-a34
Copy link
Copy Markdown
Contributor

@adity-a34 adity-a34 commented Apr 26, 2026

Description

Guards against a null dereference crash in HasPermissionDirective. AuthenticationService.getCredentials() returns Credentials | null -- when no credentials exist in storage (unauthenticated session), it returns null via
JSON.parse(null). The directive constructor accessed .permissions directly on this null value, throwing a TypeError before any RBAC logic could run.

Changed savedCredentials.permissions to savedCredentials?.permissions ?? [] so the directive initialises safely with an empty array when no credentials are present.

Jira: https://mifosforge.jira.com/browse/WEB-932

Related issues and discussion

#3527

Screenshots, if any

Screenshot 2026-04-26 105136

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • If you have multiple commits please combine them into one commit by squashing them.

  • Read and understood the contribution guidelines at web-app/.github/CONTRIBUTING.md.

Summary by CodeRabbit

  • Bug Fixes
    • Improved robustness of permission handling to gracefully handle missing credentials and prevent potential errors.

Signed-off-by: Aditya Tiwari <adityatiwari342005@gmail.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 26, 2026

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'pre_merge_checks'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

A defensive initialization pattern was applied to HasPermissionDirective's constructor. The userPermissions property now uses optional chaining and nullish coalescing to safely default to an empty array when credential retrieval returns null or undefined.

Changes

Cohort / File(s) Summary
Defensive Initialization
src/app/directives/has-permission/has-permission.directive.ts
Updated constructor to defensively initialize userPermissions using optional chaining and nullish coalescing operator, providing an empty array fallback when getCredentials() returns null/undefined or permissions are absent.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing a null dereference in HasPermissionDirective during unauthenticated sessions, which aligns with the defensive initialization of userPermissions and the guard against null credentials.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/app/directives/has-permission/has-permission.directive.ts (1)

28-28: Optional: tighten userPermissions type from any[] to string[].

The Credentials.permissions field is typed as string[], and the empty-array fallback is also assignable to string[]. Narrowing this declaration would align with the strict typing convention without behavior change.

Proposed change
-  private userPermissions: any[];
+  private userPermissions: string[];

As per coding guidelines: "Use TypeScript for all application code with strict typing conventions".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/app/directives/has-permission/has-permission.directive.ts` at line 28,
The field userPermissions is declared as any[] but should be strongly typed to
string[] to match Credentials.permissions and the empty-array fallback; update
the declaration of userPermissions in the HasPermissionDirective (or
has-permission.directive.ts) from any[] to string[] and ensure any
assignments/uses of userPermissions (e.g., in the directive's constructor/inputs
and methods like ngOnInit or checkPermission) remain compatible with string[] so
no runtime behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/app/directives/has-permission/has-permission.directive.ts`:
- Line 28: The field userPermissions is declared as any[] but should be strongly
typed to string[] to match Credentials.permissions and the empty-array fallback;
update the declaration of userPermissions in the HasPermissionDirective (or
has-permission.directive.ts) from any[] to string[] and ensure any
assignments/uses of userPermissions (e.g., in the directive's constructor/inputs
and methods like ngOnInit or checkPermission) remain compatible with string[] so
no runtime behavior changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0722807f-e965-41c5-b86a-4480e5981d1e

📥 Commits

Reviewing files that changed from the base of the PR and between bd48f54 and 4a93b42.

📒 Files selected for processing (1)
  • src/app/directives/has-permission/has-permission.directive.ts

Copy link
Copy Markdown
Contributor

@IOhacker IOhacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@IOhacker IOhacker merged commit 2a6f1e3 into openMF:dev Apr 26, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants