Feat/rbac v1

[Marfuen]

What does this PR do?

• Fixes #XXXX (GitHub issue number)
• Fixes COMP-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Visual Demo (For contributors especially)

A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).

Video Demo (if applicable):

• Show screen recordings of the issue or feature.
• Demonstrate how to reproduce the issue, the behavior before and after the change.

Image Demo (if applicable):

• Add side-by-side screenshots of the original and updated change.
• Highlight any significant change(s).

Mandatory Tasks (DO NOT REMOVE)

• I have self-reviewed the code (A decent size PR without self-review might be rejected).
• I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
• I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

• Are there environment variables that should be set?
• What are the minimal test data to have?
• What is expected (happy path) to have (input and output)?
• Any other important info that could help to test that PR

Checklist

• I haven't read the contributing guide
• My code doesn't follow the style guidelines of this project
• I haven't commented my code, particularly in hard-to-understand areas
• I haven't checked if my changes generate no new warnings

^{Cursor Bugbot found 1 potential issue for commit 7ef29c3}

[cursor]

PR Summary

High Risk
High risk because it replaces core authentication verification with better-auth session resolution and introduces permission gating across many controllers; misconfiguration could block access or weaken authorization (API keys currently bypass PermissionGuard).

Overview
Moves API auth to better-auth session + RBAC enforcement. The API now embeds better-auth (auth.server.ts) via @thallesp/nestjs-better-auth, switches HybridAuthGuard and PlatformAdminGuard from manual JWT/JWKS verification to auth.api.getSession (bearer or cookies), and enriches auth context with memberId/memberDepartment.

Adds a permissions layer and wires it into endpoints. Introduces PermissionGuard + @RequirePermission decorator (with tests) and applies them broadly across controllers (e.g., organization, people, policies, findings, attachments, browserbase, etc.), removing X-Organization-Id swagger header docs and shifting to permission-based access.

Expands org/people/policy capabilities behind RBAC. Adds org logo upload/remove (S3), role-notification settings update, API key create/revoke (now salted/hash-based), people email-preference update endpoint, new controls/frameworks CRUD endpoints, department-based policy visibility filtering, and policy PDF management endpoints (upload/delete/signed inline URL) plus control mapping and a policy-change denial flow that can leave an audit comment.

^{Written by Cursor Bugbot for commit 7ef29c3. This will update automatically on new commits. Configure here.}

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
app	Ready	Preview, Comment	Feb 4, 2026 9:54pm

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
portal	Skipped		Feb 4, 2026 9:54pm

[cursor]

Missing validation for permission value types causes crash

Medium Severity

The validatePermissions method iterates over permission values with for (const action of actions) without checking that actions is actually an array. The DTO only validates that permissions is an object via @IsObject(), not that nested values are arrays. If a client sends { "permissions": { "control": null } }, the iteration throws a TypeError (null is not iterable), causing a 500 error instead of a proper 400 validation error.

Additional Locations (1)

• apps/api/src/roles/dto/create-role.dto.ts#L27-L30

 

In general, to fix this issue we should ensure that user-controlled domain is validated and normalized before being used in any outbound HTTP request, and that only syntactically valid domain names are accepted. We do not need an allow-list of concrete hostnames because the host is fixed by vercelApi’s base URL, but we should still reject invalid or hostile input (e.g., containing path separators, whitespace, or control characters) and consistently use the validated value.

The single best fix with minimal functional change is:

1. Add a small helper method in TrustPortalService that:

   • Trims the incoming string.
   • Converts it to lower case.
   • Checks it against a conservative domain-name regular expression (labels of 1–63 characters, allowed a-z0-9-, no label starting/ending with -, total length ≤ 253, and at least one dot).
   • Optionally strips a trailing dot if present.
   • Throws BadRequestException if invalid.

2. Use this helper in addCustomDomain to derive a sanitized normalizedDomain from the incoming domain argument and then use normalizedDomain everywhere in that method:

   • For the trust lookup (currentTrust?.domain === normalizedDomain).
   • When comparing against existingDomains.
   • When querying db.trust.findUnique for the owner.
   • In the Vercel delete and post URLs.
   • In the db.trust.upsert call.

3. Leave the controller contract unchanged; it still accepts { domain: string }, but now invalid values will be rejected consistently at the service layer.

Concretely, all changes are confined to apps/api/src/trust-portal/trust-portal.service.ts in the snippet you provided. We will:

• Insert a private normalizeAndValidateDomain(domain: string): string method near the VercelDomainVerification interface or close to addCustomDomain.
• Update addCustomDomain to call this helper at the top and use the resulting normalizedDomain instead of the raw domain.

No changes are required in trust-portal.controller.ts, since the service will enforce the necessary validation and normalization.

---

General approach: validate and normalize the user-provided domain before using it in the outbound request, and ensure only well-formed hostnames are accepted. Reject inputs containing characters that could affect URL structure (slashes, schemes, spaces, etc.), and enforce a strict domain/hostname pattern and basic structure (at least one dot, no leading/trailing dots or hyphens, reasonable length). Use this sanitized value both in the external API call and in any derived value like rootDomain.

Best concrete fix in this code: in TrustPortalService.checkDnsRecords, add a private helper method (within the same service class) to validate and normalize the domain parameter. Call this helper at the start of checkDnsRecords to obtain a normalizedDomain and derive rootDomain from it. Then use normalizedDomain and rootDomain in all axios.get calls. The helper should enforce a conservative regex for DNS hostnames and strip any trailing dot. If validation fails, throw BadRequestException so behavior is explicit to callers and existing business logic remains intact except for rejecting bad domains that would previously have been (incorrectly) accepted.

Concretely:

• In apps/api/src/trust-portal/trust-portal.service.ts, inside the TrustPortalService class:
  • Add a private static method, e.g. sanitizeDomain(domain: string): string, that:
    • Trims whitespace.
    • Optionally strips a final dot.
    • Rejects:
      • Empty strings.
      • Length > 253.
      • Strings containing /, \, space, @, :.
      • Strings that don’t match a strict domain regex like /^(?=.{1,253}$)(?:[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,63}$/.
    • Throws BadRequestException('Invalid domain') if invalid.
• Modify checkDnsRecords:
  • At the beginning, replace direct use of domain with:
    • const normalizedDomain = TrustPortalService.sanitizeDomain(domain);
    • const rootDomain = normalizedDomain.split('.').slice(-2).join('.');
  • Use normalizedDomain instead of domain in the first axios.get URL, and keep rootDomain for the others.
    No new imports are needed; BadRequestException is already imported.

This keeps functionality the same for valid domains, but blocks malformed or potentially dangerous inputs and satisfies the SSRF check.

---

In general, to mitigate SSRF in this context, we should validate and normalize the user-provided domain before using it in any outbound HTTP request, enforcing that it is a syntactically valid domain name and not an arbitrary string. Because the hostname (networkcalc.com) is already fixed, the remaining risk comes from attacker-controlled path/query values; constraining domain to a safe, canonical domain format eliminates this.

The best targeted fix here is:

1. Add a private helper method in TrustPortalService (in trust-portal.service.ts) that:

   • Accepts a domain string.
   • Trims whitespace, lowercases it.
   • Validates it against a conservative regular expression for valid DNS hostnames (labels of 1–63 chars, allowed characters a-z0-9-, no label starting/ending with -, at least one dot, overall length ≤ 253).
   • Rejects IP addresses and malformed values.
   • Throws BadRequestException if invalid.

2. In checkDnsRecords, call this helper:

   • Normalize/validate the incoming domain via the helper before using it.
   • Derive rootDomain from the normalized domain, then optionally validate rootDomain as well with the same helper (or simpler: only accept if the normalized domain has at least two labels and the resulting rootDomain still matches the domain regex).

3. Use the validated safeDomain and safeRootDomain in the axios URLs:

   • https://networkcalc.com/api/dns/lookup/${safeDomain}
   • https://networkcalc.com/api/dns/lookup/${safeRootDomain}?type=TXT
   • https://networkcalc.com/api/dns/lookup/_vercel.${safeRootDomain}?type=TXT

This preserves existing functionality—clients still pass domain in the same way and receive the same DNS checks—while adding strong constraints so that only legitimate domain names are ever interpolated into the outbound URLs. No new external dependencies are needed; the regex and helper can be implemented inline. All changes are confined to the shown trust-portal.service.ts code.

In general, the problem is resolved by validating and constraining the user-controlled domain input before using it to build URLs. Rather than interpolating arbitrary strings into the request path, we should ensure the value is a well‑formed domain name (no protocol, no slashes, no spaces, no path or query components, reasonable length, allowed characters) and then use that sanitized value in the call to axios.get. If the validation fails, we throw a BadRequestException and avoid making any external request.

For this codebase, the minimal, non‑breaking approach is:

1. Add a small domain‑validation helper method inside TrustPortalService that:
   • Trims whitespace.
   • Rejects values containing /, \, spaces, ?, #, @, or starting with a protocol like http:// or https://.
   • Enforces a reasonable length (e.g., 1–253 chars) and basic hostname pattern (labels of letters/digits/hyphens separated by dots, no empty labels).
   • Lowercases the domain (optional, but helpful).
2. At the start of checkDnsRecords, call this helper to get a normalized domain, and use that in place of the raw parameter.
3. Compute rootDomain from the validated domain (as currently done), and use that in the two TXT lookup URLs.

All changes happen in apps/api/src/trust-portal/trust-portal.service.ts within the shown snippet. No changes are needed in the controller, and we don’t need new external dependencies; we can implement validation with simple string and regex logic.

[cursor]

Non-null assertion on optional userId with API key bypass

Medium Severity

The denyPolicyChanges endpoint uses authContext.userId! (non-null assertion) but userId is optional and undefined for API key authentication. Since PermissionGuard explicitly bypasses permission checks for API keys, an API key request can reach this endpoint with userId being undefined, causing the service's comment creation logic to silently fail rather than properly attributing the action.

 

[cursor]

Cross-tenant entity connections bypass organization validation

High Severity

The ControlsService.create method connects policyIds, taskIds, and requirementMappings to a new control without verifying these entities belong to the same organization. Similarly, PoliciesService.mapControls connects controlIds without organization validation. A user could potentially connect their resources to entities from other organizations if they know or guess valid IDs, violating multi-tenant isolation and potentially exposing cross-tenant data.

Additional Locations (1)

• apps/api/src/policies/policies.service.ts#L1235-L1244

 

[cursor]

Control creation with requirementMappings lacks transaction atomicity

Medium Severity

The create method performs db.control.create followed by separate db.requirementMap.create calls wrapped in Promise.all, but these are not in a database transaction. If any requirementMap.create fails (e.g., invalid requirementId or frameworkInstanceId), the control is already committed to the database with potentially partial requirement mappings, leaving data in an inconsistent state while returning an error to the caller.

 

[cursor]

Organization logo operations leak orphaned S3 files

Medium Severity

The removeLogo method sets the database logo field to null but never deletes the actual file from S3, leaving orphaned files. Similarly, uploadLogo uploads a new file without first fetching and deleting the organization's existing logo. Over time, each logo change accumulates orphaned files in S3 storage. Compare this to deletePdf in policies.service.ts which properly calls attachmentsService.deletePolicyVersionPdf before clearing the reference.

Additional Locations (1)

• apps/api/src/organization/organization.service.ts#L341-L391

 

[cursor]

PlatformAdminGuard missing error handling around session resolution

Medium Severity

The PlatformAdminGuard calls auth.api.getSession({ headers }) without a try-catch block, unlike the HybridAuthGuard which properly wraps this call and converts errors to UnauthorizedException. If getSession throws an error (network failure, malformed response, etc.), it will bubble up as a 500 Internal Server Error instead of a 401 Unauthorized. The old JWT-based implementation had proper error handling that returned null on error, which was then handled as an authentication failure.

 

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[cursor]

Download endpoint bypasses department visibility filter

Medium Severity

The downloadAllPolicies endpoint calls policiesService.downloadAllPoliciesPdf(organizationId) without applying a visibility filter, while getAllPolicies applies buildPolicyVisibilityFilter based on authContext.memberDepartment and authContext.userRoles. This allows employees and contractors to download all published policies via the PDF bundle, bypassing the department-based visibility restrictions that apply when listing policies.