Conversation
Self-registration silently ignored a tenant's EmailVerificationMode=
"never" setting because the rule was only consulted when the user
already had a tenant context — and a brand-new user has none yet. The
admin sets "never" on the only tenant in their deployment, a new user
self-registers, and the system still demands email verification because
the resolution function defaulted to the platform-level SMTP / env
config.
Two fixes:
1. Single-tenant fallback in shouldRequireEmailVerification. When the
caller has no tenant context AND the platform has exactly one
active tenant, treat that tenant as the obvious target and apply
its EmailVerificationMode. This covers the typical OSS deployment
(one company = one tenant) without any new configuration. With
more than one tenant the heuristic can't pick the "right" one and
we fall back to the platform default, same as before.
2. invitation_token pass-through on /auth/register. RegisterInput
gains an optional InvitationToken field; when present the register
flow looks up the invitation, resolves the inviting tenant, and
uses THAT tenant's rule. This handles the multi-tenant case for
the invitation-link flow specifically — the invitation is the
ground truth for which tenant the new user is going to land in.
Failure to look up the token is non-fatal (we fall back to the
single-tenant heuristic / platform default) so a stale or invalid
token does not break registration; the token is validated for real
later when the user POSTs to /invitations/{token}/accept.
Includes 4 new unit subtests on TestAuthService_Register:
- single tenant with mode=never overrides global verification
- single tenant with mode=always forces verification
- multiple tenants falls back to global config
- invitation token resolves to inviting tenant rule
A multi-role audit of the member-suspension feature surfaced two
critical security gaps and several performance hot paths. This commit
addresses the highest-impact items.
SECURITY
S1. Suspended members could still hit JWT-claim-scoped routes
(/api/v1/me/*, /api/v1/notifications, /api/v1/api-keys, scanning,
access_control, ...) until their access token expired. The
URL-path tenant routes were already protected by RequireMembership
in tenant.go but the JWT-tenant routes built via
buildTokenTenantMiddlewares only checked tenant presence, not
membership status. Add a new middleware
RequireActiveMembershipFromJWT and append it automatically inside
buildTokenTenantMiddlewares so every token-scoped route inherits
the check. The middleware reads tenant id from JWT claims (not
URL path), looks up the membership, and rejects suspended ones.
S2. ReactivateMember did not invalidate the permission cache, so a
reactivated user could see stale "empty permissions" for up to
5 minutes (the SuspendMember invalidation is what put them
there). Mirror SuspendMember by calling invalidateUserPermissions.
S3. SuspendMember did not revoke sessions / refresh tokens. The
permission cache was cleared and the URL-path middleware caught
the user, but their existing JWT was still valid for non-tenant
routes — and now that S1 closes that gap, S3 makes the closure
immediate (a 30-min JWT lifetime would otherwise leave the gap
open for half an hour). Wire SessionService into TenantService
via SetSessionService and call RevokeAllSessions on suspend.
While here, delete the dead Services.SetEmailEnqueuer helper that
silently rebuilt s.Tenant with nil repo + nil log — it was never
called from anywhere but would panic on first use if it was.
Re-attach SetSessionService in main.go where the tenant service
is reconstructed with the email enqueuer.
S4. Add composite indexes on tenant_members:
- (user_id, status) for GetUserMemberships filter
- (tenant_id, status) for GetMemberStats counts
Migration 000110 — both partial-index-friendly with the data we
already have. The existing UNIQUE (user_id, tenant_id) constraint
already covers GetMembership lookups.
PERFORMANCE
P2. Login was issuing two sequential queries to tenant_members for
opposite status filters: GetUserMemberships (active) +
GetUserSuspendedMemberships (the rest). Add
GetUserMembershipsWithStatus that returns BOTH partitioned by
status in a single round trip, and use it from Login. The
legacy methods stay for now — they're still used by other call
sites (RefreshToken, AcceptInvitationWithRefreshToken, etc.).
TESTS
- New middleware test cases for RequireActiveMembershipFromJWT
(active allowed / suspended rejected / no tenant claim 401).
- Existing TenantService and AuthService tests still pass.
GET /api/v1/tenants/{tenant}/members?include=user,roles previously
issued one DB query to fetch members and then ONE QUERY PER MEMBER
to fetch their roles. A tenant with 100 members made 101 queries to
render a single page of the users table.
Replace the per-member loop in enrichMembersWithRoles with one batch
query:
- new role.Repository.GetUsersRoles(tenantID, []userIDs)
fetches all user→role assignments matching the given user IDs in
a single SELECT joined with the roles table. Permissions for the
matched roles are still loaded via the existing batch loader, so
the total cost is at most 2 round trips regardless of member count
- new RoleService.GetUsersRoles wrapper that parses the IDs and
delegates to the repo
- tenant_handler.enrichMembersWithRoles calls GetUsersRoles once,
indexes the result by user_id, and attaches the roles to each
MemberWithUserResponse in a tight loop
- mockRoleRepo gains the new method so the unit suite still
builds and runs
ListAssetOwnersWithNames, GetPrimaryOwnerBrief, and
GetPrimaryOwnersByAssetIDs all guarded the result set with this
predicate:
AND (ao.user_id IS NULL OR ao.user_id IN
(SELECT id FROM tenant_members WHERE tenant_id = $2))
The intent was "only return user owners who are members of this
tenant", but `tenant_members.id` is the MEMBERSHIP row id, not the
user id — they are two completely different UUIDs. The IN clause
therefore NEVER matched, so every user-owner row was filtered out
regardless of whether the user was actually a tenant member.
Symptom (reported by an operator): the unified picker added a user
to an asset successfully (POST returned 200, the asset_owners row is
in the database), but the asset detail Owners tab kept showing
"Owners (0) — No owners assigned" because the GET endpoint dropped
the row at the SQL level.
Fix: change the subquery to `SELECT user_id FROM tenant_members ...`
in all three call sites. Group owners were unaffected because their
filter uses the groups.tenant_id column directly.
No schema change. The bug pre-dates the unified-picker UI work but
was masked by everyone happening to assign group owners only.
GetMemberStats was issuing three sequential queries against the same table set: one for total/active counts, one for role counts via the v_user_effective_role view, and one for pending invitations. Every admin dashboard render hit the DB three times for a single stat card. Replace with a single query using a CTE that materialises the member rows once and feeds COUNT(*) FILTER aggregates from the CTE plus a correlated sub-SELECT for invitations. Same result shape, one third the round trips.
The legacy branch of ListMembers handler called the unpaginated ListMembersWithUserInfo when no search/limit/offset parameters were present. A tenant with thousands of members would load the entire table into memory and serialise it to JSON in one shot, which is both a memory hazard and a slow request. Always route through SearchMembersWithUserInfo with a default limit of 100. Clients that need more rows can opt in via ?limit=N up to a hard cap of 500. The unpaginated path is gone from the request flow (the repository method stays for now to avoid touching the interface, but no handler invokes it).
… DB hit)
Every authenticated tenant-scoped request — and every JWT-claim
tenant request after the S1 fix — went through the membership status
check, which translated to one database SELECT against tenant_members
per HTTP call. A typical dashboard load fires 30-50 API calls in a
burst, so the middleware was hammering the database 30-50 times for
exactly the same (user, tenant) lookup. With composite indexes the
queries are fast (single-row by index) but the round trips dominate.
Replace the direct repo lookup with a Redis-backed wrapper:
- new internal/app/membership_cache_service.go: thin
MembershipCacheService that satisfies a slim
middleware.MembershipReader interface. Stores a small
CachedMembership DTO (id, role, status, joinedAt) with a 5-minute
TTL. On hit it reconstructs the entity via
ReconstituteMembershipWithStatus so middleware downstream code
reads the same shape it always did. On miss it falls through to
tenant.Repository and writes the cache. Cache failures are logged
and never fatal — correctness comes from the repo, the cache is
pure performance.
- middleware refactor: RequireMembership and
RequireActiveMembershipFromJWT now take a MembershipReader instead
of tenant.Repository. The repo still satisfies the interface so
call sites without a cache continue to work; the wiring code
prefers the cache when available and falls back to the repo when
it isn't.
- TenantService gains SetMembershipCache + invalidateMembershipCache
helper. Every mutation that could shift role or status drops the
cached entry immediately so the next request sees fresh state
without waiting for the TTL:
- SuspendMember (next request → 403)
- ReactivateMember (next request → permitted)
- UpdateMemberRole (next request → new role)
- RemoveMember (next request → 403 not-a-member)
- services.go wires NewMembershipCacheService alongside the existing
permission cache and threads it through routes.Register down to
the middleware constructors. main.go's routes.Register call passes
services.MembershipCache.
Net effect: a 50-call dashboard burst goes from 50 indexed DB SELECTs
to 1 SELECT (cold cache, first call) + 49 Redis GETs. Suspension still
takes effect on the very next request because the invalidation runs
synchronously inside SuspendMember.
No schema changes, no new tests required for the legacy paths (the
middleware unit tests still pass against the in-memory mock repo
which trivially satisfies the new interface).
… trail - Campaign team roles (lead/tester/reviewer/observer) with 5-layer authorization - Finding CRUD with pentest-specific metadata (steps, PoC, impact, remediation) - Status workflow with role-based transitions (tester passed != auto-verified) - Attachment system: upload/download/delete with campaign membership check - File dedup (SHA-256 per finding), per-tenant storage (Local/S3/MinIO) - S3 credentials encrypted via AES-256-GCM, storage_provider tracked per file - Activity trail logging for create/update/status/retest operations - Refresh token cleanup (used/revoked with 1h grace for replay detection) - Performance indexes: severity sort, campaign stats, activities, attachments - Security: SVG blocked, content-type sniffing, nosniff header, body size limit - Unit tests: 69 tests (pentest service + attachment service) - E2E tests: 2 bash scripts with 114 assertions
- GET /campaigns/{id}/findings/export — CSV download with all finding fields
- Supports ?format=json for JSON export alternative
- Includes: ID, Title, Severity, Status, CVSS, CWE, CVE, Affected Assets, Created
- Handler reads both flat metadata.poc_code and nested metadata.pentest.poc_code - Merges nested fields into flat map for backward compatibility
- CommentSection: fetch + display existing comments with instant update
- Add Details button linking to /findings/{id} detail page
- CSV export includes all pentest fields: steps, PoC, impact, remediation, references - UTF-8 BOM for Excel compatibility - Migration 000119: finding_number column with backfill - Legacy nested metadata merge for seed data
- FindingImportService: parse Burp Suite XML and generic CSV formats
- Auto-detect format from file extension (.xml → burp, .csv → csv)
- Burp: maps severity, strips HTML, extracts request/response pairs
- CSV: flexible column mapping (title, severity, description, etc)
- POST /campaigns/{id}/findings/import (lead only)
- Migration 000119: finding_number column for human-readable IDs
… backend
- HTML report generator with Go templates (pkg/report/)
- Campaign stats extended: status breakdown, CVSS aggregates
- Campaign metadata JSONB for OWASP checklist persistence
- Report download endpoint: GET /campaigns/{id}/reports/download
- Auto-remediation workflow templates (configs/workflow-templates/)
- Asynq task for scheduled report generation
- Input validation: watermark sanitization, classification whitelist
- Migrations 000120-000122: simulations, threat actors, report schedules, performance indexes - Domain models: simulation (entity, run, control test), threatactor, reportschedule - Repositories: SimulationRepository, ThreatActorRepository with tenant isolation - Services: SimulationService (nil-safe controlRepo), ThreatActorService - Handlers + routes: /api/v1/simulations, /api/v1/control-tests, /api/v1/threat-actors - MITRE ATT&CK mapping, BAS result ingestion, STIX/TAXII connector foundation - Pagination bounds validation, rows.Err() checks, error wrapping
- Asset correlation: FindByIP/FindByHostname search across name + properties - Auto-rename: IP-named hosts get renamed when hostname arrives from another source - Freshness-aware merge: newer data wins during upsert, stale fills gaps only - Host IP standardized: properties.ip (string) → ip_addresses (array) - Migration 000123: indexes for IP/hostname correlation queries - Migration 000124: data migration converting legacy ip to ip_addresses - ControlTest Postgres repository with GetStatsByFramework - Ingest processor: normalizeHostIPProperties for new host ingestion
- ArchiveStaleAssets: find and archive assets unseen > N days - Asynq task for scheduled lifecycle cleanup (maintenance queue) - Fixed tests for CreateAsset upsert behavior (GetByName instead of ExistsByName) - Added IPCorrelation test case - Asset detail: name uses break-words instead of truncate
- New file: pkg/domain/asset/category.go with TypeToCategory mapping - Asset entity: Category() derived method (not stored in DB) - API response: includes 'category' field for UI grouping - Categories: external_surface, application, infrastructure, network, cloud, data, code, identity, other
- Migration 000125: remediation_campaigns table with priority, progress, risk tracking
- Domain model: Campaign entity with status lifecycle (draft→active→validating→completed)
- Repository: Postgres CRUD with priority-ordered listing
- Service: create, update status, progress tracking, risk reduction
- Handler: REST API for campaigns
- Routes: GET/POST /api/v1/remediation/campaigns, GET/PATCH/DELETE /{id}
- Risk tracking: before/after risk scores, reduction calculation
- Overdue detection: IsOverdue() based on due_date
… + auto-escalation
- GetMTTRMetrics: average hours to remediate per severity - GetRiskVelocity: new vs resolved per week (12-week window) - RiskVelocityPoint type: positive = losing ground, negative = improving - Ready for UI consumption via dashboard API
- GET /api/v1/dashboard/mttr — MTTR in hours by severity - GET /api/v1/dashboard/velocity?weeks=12 — new vs resolved per week - Service layer wraps repo queries - Repository interface extended with GetMTTRMetrics + GetRiskVelocity
- Migration 000126: business_units + business_unit_assets tables, crown jewel columns on assets - Domain model: BusinessUnit entity with risk aggregation stats - Repository: CRUD + asset linking (many-to-many) - Service: create, list, add/remove assets - Handler + routes: GET/POST /api/v1/business-units, asset linking - Crown jewels: is_crown_jewel, business_impact_score columns on assets table - Fixed dashboard test mock for MTTR/velocity interface
…d trigger - Workflow template #5: fix_applied → auto trigger verification scan + notify team - Added TriggerTypeFindingStatusChanged for clearer intent - Uses existing trigger_scan action type
- PATCH /api/v1/assets/{id}/crown-jewel — mark/unmark crown jewel with impact scoring
- SaveAsset() — direct entity persist for handlers that modify entity directly
- Route registered with AssetsWrite permission
- 5 network segments (PROD-LAN, DMZ, MGMT, DEV, BACKUP) - 3 firewalls (Palo Alto perimeter, Fortinet internal, F5 WAF) - 2 load balancers (F5 trading, HAProxy web) - 3 switches (Cisco core + 2 access) - 1 wireless AP (Cisco Catalyst) - 1 WAN router (Juniper MX204) - All with vendor, model, firmware, management IP, serial number
- properties: stores CIDR, VLAN, gateway for network segments - group_type: manual, dynamic, network_segment, business_unit - Enables network segments to be managed as asset groups instead of assets
- Migration 000128: sub_type column, backfill 21 types, consolidate to 15 core types - Fix asset_type_categories: add external_surface + network, fix assignments - TypeAliases map + ResolveTypeAlias() for ingest normalization - Asset entity: SubType() getter, SetSubType() setter - API response includes sub_type field - Fully reversible: down migration restores from sub_type
Bug fixes: - CHECK constraint: add 'application', 'identity', 'kubernetes' to allowed values - Repository SELECT: include sub_type column in query - Repository INSERT/UPDATE: persist sub_type on create and update - AllAssetTypes(): add 3 new consolidated types - CategoryForType(): add application, identity, kubernetes mappings Tests: - TestResolveTypeAlias: 28 cases (21 aliases + 7 pass-through) - TestParseAssetType_NewTypes: application, identity, kubernetes - TestCategoryForType: 12 type→category mappings - TestSubTypeOnEntity: getter/setter on entity All unit tests pass (12.5s)
…sset_type SAFE migration: only adds sub_type column and backfills from current type. Does NOT change asset_type values. Zero risk of breaking UI/API. Rollback: DROP COLUMN sub_type.
- Filter: is_crown_jewel=true for crown jewel assets - Filter: sub_type=firewall for sub-type filtering - nilIfEmpty() helper for optional string query params - Crown jewel filter wired: Filter → Repository → Handler
- FetchEPSSScores: top 1000 CVEs from FIRST.org API - FetchKEVCatalog: full CISA KEV catalog JSON - FetchEPSSForCVEs: targeted EPSS lookup for specific CVEs - Ready for Asynq job handler integration
…types) Requires 000128 (sub_type backfill) and 000129 (legacy DB types). Changes: firewall→network, website→application, iam_user→identity, etc. Original type preserved in sub_type column for rollback.
- SQL: GROUP BY sub_type for assets with sub_type set - AssetStatsData: added BySubType map - DashboardStats: added AssetsBySubType - API response: by_sub_type field in asset stats
GetAllStats (used by main dashboard endpoint) was missing asset_by_sub_type CTE. Only GetAssetStats (simple query) had it. Now both return by_sub_type data.
…ub_type stats - Add GET /assets/facets endpoint — returns distinct property keys + values for dynamic filtering UI (uses GIN index on properties JSONB) - Add ?properties=key:value query param to GET /assets — server-side JSONB containment filter with parameterized queries (injection-safe) - Add ?sub_type= param to GET /assets/stats — stats scoped to sub_type - Add by_sub_type breakdown to /assets/stats response - Add PropertiesFilter to domain Filter struct + repository WHERE clause - Add GetPropertyFacets to Repository interface + Postgres implementation - Migration 000131: normalize network sub_types (core_switch/access_switch → switch) - Update test mocks for new interface methods
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Collectors can now send known fields inside properties JSONB and the system auto-promotes them to proper columns: - sub_type → entity.SubType column - type → resolved via TypeAliases (e.g., "firewall" → type=network) - scope, exposure, description → override if empty - tags → merged with input tags - name, tenant_id, criticality, status, owner_ref → removed from JSONB Also: - Fix LIKE wildcard injection in business_unit search (escape % and _) - Fix CodeQL warning: remove unused argIdx increment - Add Properties field to CreateAssetInput + CreateAssetRequest
Unit tests (23 tests): - TestPromoteKnownProperties: sub_type, type alias, scope/exposure, tags (array + string), column name removal, empty properties, description - TestParsePropertiesFilter: single/multi, spaces, colon in value, empty key/value, max 5 pairs, injection chars, unicode, underscore Integration test (18 assertions): - Stats endpoint: total, by_type, by_sub_type, sub_type filter - Facets endpoint: network facets, host facets - Properties filter: vendor:Cisco, non-existent, multi-filter - Promote on create: sub_type, scope, vendor preserved, type alias - Identity type: list, sub_type filter - Modules: asset sub-modules count, identity module exists - Cleanup: deletes test assets after run Also: export PromoteKnownProperties + ParsePropertiesFilter for testability
Codify module changes into a reversible migration: - Deprecate: cloud_resources, kubernetes, serverless, ports, web_apps, compute - Rename: containers → Containers & K8s, data_stores → Storage - Upgrade: subdomains → released - Add: assets.identity (Identity & Access) - Clean orphan tenant_modules for deprecated modules - Down migration: restore pre-consolidation state, clean FK before DELETE
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.
No description provided.