feat(rbac): redesign the access page and harden security - #138
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
WhiteMuush
changed the title
The three lists on /access rendered an empty bordered box until their fetch came back, so the page looked finished and wrong for a beat, then jumped as rows appeared. Extract the idiom the dashboard route skeleton already uses (a pulsing muted block) into a Skeleton primitive plus a SkeletonRows list, sized like the real rows so nothing shifts when the data lands. Roles and People clear the flag once and never raise it again: the reloads that follow a mutation keep the list on screen rather than flashing placeholders over rows the user is reading. The audit trail is the exception and re-arms on every page change, because paging swaps the whole set and the placeholders are the feedback that the page turned. Its Prev/Next are disabled mid-fetch so a fast double click cannot outrun the response.
This reverts commit d0c79f7.
Two gaps on the login page. The pending state only swapped the button label, with nothing moving, and on success setLoading(false) ran before the push to /dashboard, so the button snapped back to "Sign in" while the navigation was still under way. The slowest part of signing in had the least feedback. Add a spinning icon next to the label, and keep the pending state alive through the redirect on every path that navigates. Error paths and the two-factor branch still hand control back, since those stay on the page. Track which action is pending rather than a bare boolean. A shared flag made the passkey button read "Signing in..." when the password form was the one submitting, which is wrong on its face and collides with the accessible name the e2e disambiguates on.
Scope the animation to the one moment that had no feedback: the stretch between credentials being accepted and the dashboard painting. Nothing covered it, because the pending state was released as soon as the auth call returned, so the button read "Sign in" again while the navigation was still running. A full-screen shield and spinner now replace the form once authentication has succeeded, on all four paths that navigate: password, passkey, TOTP and email code. The credential check itself keeps its plain label, and error paths hand control straight back since they stay on the page. Amends the previous commit, which put spinners on the buttons themselves. That animated the wrong moment.
Moving between Employees, Alerts, Reports and the rest flashed a placeholder grid before the real page. Routes are already prefetched, so the pause it covered is short, and a shape that briefly pretends to be content reads worse than a beat of nothing. Loading feedback now exists in exactly one place: entering the workspace after signing in, which is the only wait long enough to need it. Without a loading.tsx, Next keeps the current page on screen until the next one is ready rather than swapping in a fallback.
Opening the permission editor made /access unusable on any viewport shorter than the content: the panel could not be closed and the page would not scroll. The dashboard shell is h-screen with overflow-hidden at every level, so each page owns its scrolling. Every other page opens with "h-full overflow-y-auto p-6" and puts its max-width on an inner wrapper. This one opened straight into "mx-auto max-w-4xl space-y-6 p-6", so the shell clipped it: measured at a 620px viewport, 2233px of content shown through 572px with no way to reach the rest. It also nested a second main inside the shell's own. The regression test walks out from the form and fails on any ancestor clipping content it is too short to show. Asserting the Cancel button's position instead would pass or fail on how many roles happen to be seeded, and Playwright can force a click on a clipped node where a user cannot. It runs at 620px because the editor does fit in the 720px default.
It was the only page capped at max-w-4xl. The convention splits on content: list pages run full width (alerts, dashboard, employees, register, reports), settings pages sit at max-w-3xl (data-api, data-sources, notifications). This page holds three lists, so it belongs with the first group. The cap came from the plan and matched neither.
The page read as a prototype next to a finished app. It used none of the vocabulary the rest of the product already has: bare ul lists instead of the card-wrapped tables every other list page uses, an unstyled search input where EmployeeTable insets a Search icon at max-w-sm, pagination floating outside any container, and stacked h2 headings instead of the page-header shape. Three lists stacked on one page also ran past 2000px. They move behind a plain underlined tab strip, no pills and no colour, so each fits a viewport. Every surface now reuses the existing classes: rounded-xl border-border/60 bg-card, thead on bg-muted/30 with uppercase tracked labels, rows on hover:bg-muted/40, empty states at py-12 centered, pagination in a bordered footer with size-7 icon buttons. Row actions reveal on hover, the pattern PresetTab and SetupGuides already use. The audit trail finally shows what it recorded. It listed only action, actor and date, so a row read "role.update" with no way to tell which role or what changed, while the API had been returning targetId, before and after all along. It now resolves ids to names and renders a real diff: "+alerts:close, -reports:export" for a permission change, "Viewer -> Security Manager" for an assignment. Name lookups degrade to a short id when the viewer holds audit:read without roles:read or users:read.
"Viewer -> Security Manager" read like terminal output, and gave both values the same weight when only the destination matters. The column now returns a fragment rather than a string. A reassignment steps the previous role back in muted text behind a small arrow glyph, with the new role in foreground. A permission change lists additions in foreground and struck-through removals in muted, instead of joining "+a, -b" into one line.
Webhook targets are supplied by an admin, so from the server's side they are attacker-controlled: a request the app makes reaches whatever the network reaches. Forcing https already blocked the AWS metadata endpoint, which is http only, but nothing stopped an internal host behind TLS. parseOutboundUrl now rejects a literal private address, embedded credentials and a non-https scheme, covering loopback, RFC1918, link-local (including 169.254.169.254), carrier-grade NAT, unique-local v6, and IPv4-mapped v6 that wraps a private v4. A hostname is also resolved and every returned address checked, because a name that looks public can point into the private space. That runs at write time so a bad endpoint is refused up front, and again before each delivery so a record valid when saved cannot be repointed afterwards. Deliveries no longer follow redirects, which would otherwise route around both checks.
Both limiters counted in a per-process Map. On more than one instance the real ceiling was the configured limit times the number of nodes, and every deploy reset it, which makes the protection decorative anywhere but a single long-lived process. Better Auth switches to its database storage, and the application limiter behind scan, SCIM and SIEM now counts in an ApiRateLimit row. The counter is a single INSERT ON CONFLICT that rolls the window over inside the statement, so two requests arriving together cannot both read a stale count and both decide they are under the limit. An integration test drives 20 concurrent calls against a limit of 5 and asserts exactly 5 get through. The limiter helpers become async, so their callers await. The SCIM rate-limit tests move out of the unit suite into an itest: the counter lives in Postgres now, and a mocked store would only prove the mock counts. Expired rows are swept by the existing cron tick, which has no timing requirement.
The production hash ran at cost 10 while every seed script used 12. That one line is the only place a real user's password is hashed, and it was the lowest. Existing hashes keep verifying unchanged: bcrypt stores its cost inside the hash, so old and new coexist and rotate naturally on the next password change. No minimum was configured either, leaving Better Auth's default of 8 with no complexity requirement. Thin for a product whose job is finding exposed credentials. The 72-byte cap is bcrypt's own truncation point, made explicit so a long passphrase is refused rather than silently cut.
CodeQL flagged this as a user-controlled bypass of a security check. The alert predates this branch, the route last changed in c7b7167, but the reasoning holds: `scope` arrives as untrusted JSON while its type annotation is only a compile-time claim, so any value other than the two members skipped the `scope === "COMPANY"` guard and its permission check. That was not exploitable, because Prisma rejects an unknown enum member a few lines later, but it left an authorization decision resting on a database constraint instead of an explicit check. Narrow the value against a literal set first and answer 400 otherwise.
WhiteMuush
force-pushed
the
feat/rbac-management-ui
branch
from
August 5, 2026 07:29
08aea8c to
2bc6f93
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #137, which shipped RBAC Plan 2 tasks 12 to 14. This branch carries what came after.
Access page
Rebuilt on the app own design language: it used none of the vocabulary the rest of the product has (bare
ullists instead of card-wrapped tables, an unstyled search input, pagination floating outside any container). Three stacked lists ran past 2000px, so they move behind a plain underlined tab strip.Two defects fixed along the way. The page had no scroll container while the dashboard shell is
h-screenwithoverflow-hiddenat every level, so opening the permission editor clipped 1661px of content with no way to reach it and no way to close the panel. And it was the only page capped atmax-w-4xlwhen list pages run full width.The audit trail finally shows what it recorded: it listed only action, actor and date, so a row read
role.updatewith no way to tell which role or what changed, while the API had been returningtargetId,beforeandafterall along.Sign-in
Loading feedback was in the wrong places. Route-level skeletons flashed on every navigation between entries, while the one genuinely slow moment, the stretch between credentials being accepted and the dashboard painting, showed nothing at all: the pending state was released before the redirect. Skeletons removed, transition screen added.
Security
Four findings from a review of the OWASP categories, brute force, MITM and request tampering. Account lockout and credential stuffing are deliberately out of scope: they belong at the edge, not in application code.
Mapmultiplied the real ceiling by the instance count and reset on every deploy. The counter is now a singleINSERT ON CONFLICTthat rolls the window inside the statement; an integration test drives 20 concurrent calls against a limit of 5 and asserts exactly 5 pass.Verification
Note for local runs: the rate limiter is persistent now, so a
npm run devserver started withoutE2E=1will be reused by Playwright, the test-only bypass will not arm, and sign-ins will 429. Run the suite with no dev server up and Playwright starts its own correctly.