fix(console): add missing tls for queryParams in asset context#290
fix(console): add missing tls for queryParams in asset context#290
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's ability to manage and display TLS certificate information. It establishes a robust foundation on the backend with a new database view and a flexible API for querying TLS assets, supporting pagination, sorting, and various filters. On the frontend, it integrates these capabilities by adding a new 'TLS' tab to the assets page, complete with a faceted filter and a detailed table, providing users with a clear and interactive overview of their TLS certificates and their expiration statuses. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for viewing and filtering TLS assets. It adds a new tab to the assets page, a faceted filter for TLS hosts, and the necessary backend support including a new API endpoint and a database view. While the new functionality is well-implemented, there are a few areas for improvement. The error handling in the new TLS assets tab is buggy. A significant performance regression has been introduced on the dashboard's TLS expiration table, which now fetches all assets and filters on the client. Additionally, navigation from the dashboard could be improved to leverage the new specific filters, and there's a minor naming inconsistency in a backend DTO that affects maintainability.
| const { data, isLoading, error } = useAssetsControllerGetTlsAssets( | ||
| {}, | ||
| { | ||
| query: { | ||
| queryKey: ['tls-assets', selectedWorkspace], | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
This component now fetches all TLS assets and filters them on the client-side to find certificates that are expiring soon. This is a performance regression, as the previous API implementation handled this filtering on the backend. With a large number of TLS assets, this will be inefficient and could slow down the dashboard. The assetsControllerGetTlsAssets endpoint should be updated to support filtering by expiration date to resolve this.
| const { data, isLoading } = useAssetsControllerGetTlsAssets( | ||
| { | ||
| page: queryParams.page, | ||
| limit: queryParams.limit, | ||
| sortBy: queryParams.sortBy, | ||
| sortOrder: queryParams.sortOrder, | ||
| search: queryParams.value, | ||
| targetIds: queryParams.targetIds, | ||
| }, | ||
| { | ||
| query: { | ||
| ...queryOptions.query, | ||
| queryKey: ['tlsAssets', ...queryOptions.query.queryKey], | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const tlsAssets = data?.data ?? []; | ||
| const total = data?.total ?? 0; | ||
|
|
||
| if (!data && !isLoading) return <div>Error loading TLS assets.</div>; |
There was a problem hiding this comment.
The error handling logic is incorrect. The condition !data && !isLoading will also be true for a successful response with an empty list of assets, causing an error message to be displayed to the user when it shouldn't. You should use the isError flag from the useAssetsControllerGetTlsAssets hook to correctly handle API errors.
const { data, isLoading, isError } = useAssetsControllerGetTlsAssets(
{
page: queryParams.page,
limit: queryParams.limit,
sortBy: queryParams.sortBy,
sortOrder: queryParams.sortOrder,
search: queryParams.value,
targetIds: queryParams.targetIds,
},
{
query: {
...queryOptions.query,
queryKey: ['tlsAssets', ...queryOptions.query.queryKey],
},
},
);
const tlsAssets = data?.data ?? [];
const total = data?.total ?? 0;
if (isError) return <div>Error loading TLS assets.</div>;
| </CardHeader> | ||
| <CardContent className="p-4 py-0"> | ||
| <DataTable | ||
| onRowClick={(row) => navigate(`/assets?filter=${row.host}`)} |
There was a problem hiding this comment.
The onRowClick handler navigates using a generic text filter. With the addition of TLS-specific filters, it would provide a better user experience to navigate directly to the TLS assets tab and apply the specific host filter. This will give the user a more focused view.
| onRowClick={(row) => navigate(`/assets?filter=${row.host}`)} | |
| onRowClick={(row) => navigate(`/assets?tab=tls&tlsHosts=${row.host}`)} |
| @IsOptional() | ||
| @ApiProperty({ required: false, type: [String] }) | ||
| @IsArray() | ||
| @IsString({ each: true }) | ||
| @Transform(({ value }): string[] => | ||
| Array.isArray(value) ? (value as string[]) : [value as string], | ||
| ) | ||
| hosts?: string[]; |
There was a problem hiding this comment.
For consistency and clarity, the hosts property in GetTlsQueryDto should be renamed to tlsHosts. This would align it with the naming in GetAssetsQueryDto and make its purpose of filtering TLS hosts more explicit. This change would also simplify the mapping logic in assets.service.ts.
| @IsOptional() | |
| @ApiProperty({ required: false, type: [String] }) | |
| @IsArray() | |
| @IsString({ each: true }) | |
| @Transform(({ value }): string[] => | |
| Array.isArray(value) ? (value as string[]) : [value as string], | |
| ) | |
| hosts?: string[]; | |
| @IsOptional() | |
| @ApiProperty({ required: false, type: [String] }) | |
| @IsArray() | |
| @IsString({ each: true }) | |
| @Transform(({ value }): string[] => | |
| Array.isArray(value) ? (value as string[]) : [value as string], | |
| ) | |
| tlsHosts?: string[]; |
There was a problem hiding this comment.
Pull request overview
This PR expands TLS certificate support across the stack by introducing a TypeORM view for TLS assets, upgrading the /api/assets/tls endpoint to be paginated/filterable/sortable, and adding a new TLS tab + faceted filtering in the console assets UI.
Changes:
- Add
tls_assets_view(TypeORM@ViewEntity) and wire it intoAssetServicerelations and module registration. - Replace the old “expiring soon” TLS endpoint behavior with a paginated, searchable, sortable TLS certificates listing.
- Add TLS tab + TLS host faceted filter in the console and update generated API clients/DTOs to include additional TLS fields.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| worker/services/core-api/api.ts | Update worker-generated client types and TLS assets endpoint to accept query params. |
| core-api/src/modules/assets/entities/tls-assets.entity.ts | New TypeORM view entity flattening TLS JSONB into queryable columns. |
| core-api/src/modules/assets/entities/asset-services.entity.ts | Add tlsAssets relation from AssetService to the TLS view. |
| core-api/src/modules/assets/dto/tls.dto.ts | Expand TLS response shape; add TLS query DTO for filtering. |
| core-api/src/modules/assets/dto/get-faceted-data.dto.ts | Extend faceted DTO with tlsHosts. |
| core-api/src/modules/assets/dto/assets.dto.ts | Add tlsHosts filter to asset-service listing query DTO. |
| core-api/src/modules/assets/assets.service.ts | Join TLS view into base query and implement new paginated TLS listing logic. |
| core-api/src/modules/assets/assets.service.spec.ts | Update DI setup for the new TLS view repository. |
| core-api/src/modules/assets/assets.module.ts | Register TlsAssetsView in TypeORM module feature set. |
| core-api/src/modules/assets/assets.controller.ts | Accept TLS query params and forward to service. |
| console/src/services/apis/gen/queries.ts | Update console-generated client types/hooks for new TLS query params & response fields. |
| console/src/pages/dashboard/components/tls-expiration-table.tsx | Adapt dashboard TLS table to new query-enabled hook signature. |
| console/src/pages/assets/list-assets.tsx | Add a new “TLS” tab to the assets page. |
| console/src/pages/assets/context/asset-context.tsx | Add tlsHosts into shared assets filter context. |
| console/src/pages/assets/components/tls-assets-tab.tsx | New TLS assets tab that fetches and renders paginated TLS certificates. |
| console/src/pages/assets/components/tls-assets-column.tsx | Define TLS table columns and expiry badge rendering. |
| console/src/pages/assets/components/filter-form-infinite.tsx | Include TLS host facet in the filter form and “isFiltered” detection. |
| console/src/pages/assets/components/faceted-filter.tsx | Add TlsFacetedFilter using the TLS infinite query hook. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Faceted host filter (query.hosts maps to tlsHosts in buildBaseQuery | ||
| // via the whereBuilder, but GetTlsQueryDto uses `hosts` directly) | ||
| if (query.hosts && query.hosts.length > 0) { | ||
| qb.andWhere('"tlsAssets"."host" = ANY(:tlsHostFilter)', { | ||
| tlsHostFilter: query.hosts, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
TLS host filtering is applied twice: once via buildBaseQuery() (through tlsHosts) and again with the explicit andWhere on tlsAssets.host. Keeping both is redundant and makes it harder to reason about which params are actually in effect; it also risks diverging parameter names (:tlsHosts vs :tlsHostFilter). Consider relying on a single filter path.
| // Faceted host filter (query.hosts maps to tlsHosts in buildBaseQuery | |
| // via the whereBuilder, but GetTlsQueryDto uses `hosts` directly) | |
| if (query.hosts && query.hosts.length > 0) { | |
| qb.andWhere('"tlsAssets"."host" = ANY(:tlsHostFilter)', { | |
| tlsHostFilter: query.hosts, | |
| }); | |
| } |
| .leftJoin('asset.ipAssets', 'ipAssets') | ||
| .leftJoin('asset_service.statusCodeAssets', 'statusCodeAssets') | ||
| .leftJoin('asset_service.tlsAssets', 'tlsAssets') | ||
| .where('asset_service."isErrorPage" = false') |
There was a problem hiding this comment.
buildBaseQuery() now always left-joins asset_service.tlsAssets, which will affect all asset-service list queries even when TLS data isn't needed. Consider making this join conditional (only when query.tlsHosts is present or when building the TLS endpoint query) to avoid extra work and potential query-plan regressions on the main assets listing.
| // Type cho raw database response từ TLS query | ||
| interface TlsRawData { | ||
| host: string; | ||
| sni: string; | ||
| subject_dn: string; | ||
| subject_an: string[]; | ||
| not_after: string; | ||
| not_before: string; | ||
| tls_connection: string; | ||
| } | ||
| // interface TlsRawData { | ||
| // host: string; | ||
| // sni: string; | ||
| // subject_dn: string; | ||
| // subject_an: string[]; | ||
| // not_after: string; | ||
| // not_before: string; | ||
| // tls_connection: string; | ||
| // } |
There was a problem hiding this comment.
There are large blocks of commented-out TypeScript interfaces left in the service. These add noise and can drift out of date; consider removing them entirely (or converting them to real types if they’re still needed).
| @IsOptional() | ||
| @ApiProperty({ required: false, type: [String] }) | ||
| @IsArray() | ||
| @IsString({ each: true }) | ||
| @Transform(({ value }): string[] => | ||
| Array.isArray(value) ? (value as string[]) : [value as string], | ||
| ) | ||
| targetIds?: string[]; |
There was a problem hiding this comment.
targetIds is validated as string[], but it is used to filter asset.targetId (UUIDs). To match the rest of the assets filters (e.g. GetAssetsQueryDto.targetIds), this should use @IsUUID(4, { each: true }) to prevent invalid IDs from reaching the query builder.
| sortBy: queryParams.sortBy, | ||
| sortOrder: queryParams.sortOrder, | ||
| search: queryParams.value, | ||
| targetIds: queryParams.targetIds, |
There was a problem hiding this comment.
The TLS assets request does not include the selected TLS-host facet (queryParams.tlsHosts). As a result, picking a “TLS Host” filter updates the URL/queryKey but won’t actually filter /api/assets/tls results. Consider mapping queryParams.tlsHosts into the API param (hosts as currently defined by GetTlsQueryDto, or rename the API to accept tlsHosts for consistency with the assets filters).
| targetIds: queryParams.targetIds, | |
| targetIds: queryParams.targetIds, | |
| hosts: queryParams.tlsHosts, |
| return items | ||
| .filter((e) => !!e.host) | ||
| .map((e) => ({ | ||
| value: e.host?.toString() ?? '', | ||
| label: e.host?.toString() ?? '', | ||
| })); |
There was a problem hiding this comment.
The TLS host facet options are derived by mapping over certificate rows and can contain many duplicates (same host across multiple cert records/pages). Consider de-duplicating hosts in the select mapping (e.g., via a Set) so the faceted filter list is stable and doesn’t spam repeated options.
| return items | |
| .filter((e) => !!e.host) | |
| .map((e) => ({ | |
| value: e.host?.toString() ?? '', | |
| label: e.host?.toString() ?? '', | |
| })); | |
| const seenHosts = new Set<string>(); | |
| const options = | |
| items | |
| .filter((e) => !!e.host) | |
| .reduce<{ value: string; label: string }[]>((acc, e) => { | |
| const host = e.host?.toString() ?? ''; | |
| if (!host || seenHosts.has(host)) { | |
| return acc; | |
| } | |
| seenHosts.add(host); | |
| acc.push({ | |
| value: host, | |
| label: host, | |
| }); | |
| return acc; | |
| }, []); | |
| return options; |
| public async getManyTls( | ||
| query: GetTlsQueryDto, | ||
| workspaceId: string, | ||
| ): Promise<GetManyBaseResponseDto<GetTlsResponseDto>> { | ||
| // Hard-coded pagination for notification purposes | ||
| const page = 1; | ||
| const limit = 10; | ||
|
|
||
| // Calculate date range: 30 days before and 30 days after current date | ||
| const now = new Date(); | ||
| const thirtyDaysAgo = new Date(now); | ||
| thirtyDaysAgo.setDate(now.getDate() - 30); | ||
| const thirtyDaysFromNow = new Date(now); | ||
| thirtyDaysFromNow.setDate(now.getDate() + 30); | ||
|
|
||
| // Query to get total count with date range filter | ||
| const allowedSortColumns = [ | ||
| 'host', | ||
| 'sni', | ||
| 'not_after', | ||
| 'not_before', | ||
| 'subject_dn', | ||
| 'tls_version', | ||
| ]; | ||
| if (!allowedSortColumns.includes(query.sortBy)) { | ||
| query.sortBy = 'not_after'; | ||
| } | ||
|
|
||
| const offset = (query.page - 1) * query.limit; | ||
|
|
||
| // Re-use the base query (workspace isolation + all standard filters). | ||
| // Cast to GetAssetsQueryDto so we can forward targetIds / hosts filters. |
There was a problem hiding this comment.
getManyTls() now has non-trivial behavior (workspace isolation via joins, pagination, sorting allowlist, text search, and deduplication). There are currently no unit/integration tests covering these behaviors, so regressions (e.g., incorrect filtering due to param name collisions) are easy to miss. Consider adding tests that assert filtering by targetIds/hosts/search and that pagination totals match the result set.
| // Cast to GetAssetsQueryDto so we can forward targetIds / hosts filters. | ||
| const baseQuery: GetAssetsQueryDto = { ...query, tlsHosts: query.hosts }; |
There was a problem hiding this comment.
baseQuery is built via { ...query, tlsHosts: query.hosts }, which also forwards hosts into buildBaseQuery() and unintentionally applies the asset service host filter (asset.value = ANY(:hosts)) in addition to the intended TLS-host filter. This can silently drop TLS rows when TLS host != asset service value. Consider omitting hosts from the forwarded object (or renaming the TLS query param to tlsHosts) and only setting tlsHosts when filtering TLS hosts.
| // Cast to GetAssetsQueryDto so we can forward targetIds / hosts filters. | |
| const baseQuery: GetAssetsQueryDto = { ...query, tlsHosts: query.hosts }; | |
| // Cast to GetAssetsQueryDto so we can forward targetIds filters. | |
| const { hosts, ...restQuery } = query; | |
| const baseQuery: GetAssetsQueryDto = { | |
| ...(restQuery as unknown as GetAssetsQueryDto), | |
| tlsHosts: hosts, | |
| }; |
* feat(asset): add tls filter for asset * fix(core): fix asset test * fix(asset): fix based on bot reviews * fix(console): fix small typo in tls tab * fix(console): add missing tls for queryParams in asset context * fix(console): fix tls query hook in dashboard --------- Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com>
* feat(console, admin): add list users for administration * feat(console): redesign user detail sheet * fix(console): fix sorting for server datatable * refactor(console): restyle user-detail-sheet * feat(console): add confirm dialog for action ban * feat(console): add user detail section * fix(console): fix small typo in tls tab (#290) * feat(asset): add tls filter for asset * fix(core): fix asset test * fix(asset): fix based on bot reviews * fix(console): fix small typo in tls tab * fix(console): add missing tls for queryParams in asset context * fix(console): fix tls query hook in dashboard --------- Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com> * chore(deps): bump multer from 2.0.2 to 2.1.0 (#292) Bumps [multer](https://github.com/expressjs/multer) from 2.0.2 to 2.1.0. - [Release notes](https://github.com/expressjs/multer/releases) - [Changelog](https://github.com/expressjs/multer/blob/main/CHANGELOG.md) - [Commits](expressjs/multer@v2.0.2...v2.1.0) --- updated-dependencies: - dependency-name: multer dependency-version: 2.1.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat(targets): create multiple targets (#291) * refactor(targets): combine logic create target in transaction * feat(targets): add bulk target creation endpoint * feat(targets): add bulk creation support * fix(console): move tabList into component to avoid out of context (#293) Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com> * fix(assets): select asset relations in query (#297) * refactor(ui): improve flow onboarding with first workspace creation and re-design settings ui (#299) * feat(console): add all workspaces navigation and improve 404 page UI * refactor(layout): extract header into dedicated HeaderBar component * refactor(console): add workspace-aware header layout * refactor(console): convert workspace creation to page and add route protection * refactor(console): update workspaces UI from table to card layout * feat(workspaces): add member and target counts to workspace list * refactor(settings): reorganize settings page with sidebar layout * feat(settings): add API keys management * refactor(settings): improve API key display layout * fix(screenshot-cell): add type assertion for screenshotPath * refactor(workspaces): use workspace selector hook * feat(auth): add session retry with exponential backoff * chore(agent): migrate ai agent * feat(router): add admin users route * feat(console): implement create user * feat(console): add change name, email and reset password in user detail * fix(console): fix duplicate tlsHosts in context * fix(console): use loading state of data table and improve client user type * fix(console): add admin route * feat(console): Implement role-based access control for settings tabs and sidebar menu items based on user roles. * style(console): update 'Add User' button to outline variant * refactor(console): move add user button to table toolbar * fix(console): add autoComplete to user detail input --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: l1ttps <l1ttps443@gmail.com>
No description provided.