feat: add ability to dismiss discovered host suggestions#278
feat: add ability to dismiss discovered host suggestions#278devin-ai-integration[bot] wants to merge 3 commits into
Conversation
Add dismiss functionality to the discovered hosts banner on the Services
page. Users can dismiss individual hosts via an X button or dismiss all
hosts at once via a 'Dismiss all' button in the banner header.
Backend:
- Store dismissed hosts as a JSON array in vault settings (no migration needed)
- Filter dismissed hosts from the GET discovered-hosts response
- Add DELETE /v1/vaults/{name}/discovered-hosts/{host} for single dismiss
- Add DELETE /v1/vaults/{name}/discovered-hosts for dismiss-all
Frontend:
- Add X dismiss button to each discovered host row
- Add 'Dismiss all' button in the banner header
- Both buttons are admin-only, consistent with existing 'Add as service'
Co-Authored-By: jake <jake@infisical.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
| Filename | Overview |
|---|---|
| internal/server/handle_host_discovery.go | Adds dismiss-single and dismiss-all handlers for discovered hosts; uses requireVaultMember instead of the intended requireVaultAdmin, and stores arbitrary host strings without validation or a size cap. |
| internal/server/server.go | Registers two new DELETE routes with the same middleware chain as the existing GET endpoint; straightforward and correct. |
| web/src/pages/vault/ServicesTab.tsx | Adds per-host dismiss button and "Dismiss all" link gated behind isAdmin; errors are silently swallowed with no user feedback on failure. |
Reviews (1): Last reviewed commit: "feat: add ability to dismiss discovered ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return | ||
| } | ||
|
|
||
| if _, err := s.requireVaultMember(w, r, ns.ID); err != nil { |
There was a problem hiding this comment.
The PR description explicitly states "Both controls are admin-only, matching existing Add as service visibility," and the UI correctly gates both dismiss buttons behind isAdmin. However, both new DELETE handlers call requireVaultMember instead of requireVaultAdmin. Any vault member — not just an admin — can call DELETE /v1/vaults/{name}/discovered-hosts/{host} or DELETE /v1/vaults/{name}/discovered-hosts directly via the API, silently bypassing the intended access control.
There was a problem hiding this comment.
Fixed in 7e5fe0c — changed both handlers from requireVaultMember to requireVaultAdmin.
| dismissed := loadDismissedHosts(ctx, s.store, ns.ID) | ||
| dismissed[host] = true | ||
| if err := saveDismissedHosts(ctx, s.store, ns.ID, dismissed); err != nil { |
There was a problem hiding this comment.
handleDismissDiscoveredHost stores any host string from the URL path directly into vault settings without checking that the host actually exists in the discovered-hosts list. A vault member can dismiss thousands of fabricated host strings, causing the dismissed_discovered_hosts setting to grow unboundedly. Validating against the live discovered-host set (or capping the list) before persisting would prevent this.
| dismissed := loadDismissedHosts(ctx, s.store, ns.ID) | |
| dismissed[host] = true | |
| if err := saveDismissedHosts(ctx, s.store, ns.ID, dismissed); err != nil { | |
| const maxDismissed = 1000 | |
| dismissed := loadDismissedHosts(ctx, s.store, ns.ID) | |
| if !dismissed[host] && len(dismissed) >= maxDismissed { | |
| jsonError(w, http.StatusUnprocessableEntity, "Dismissed hosts limit reached") | |
| return | |
| } | |
| dismissed[host] = true | |
| if err := saveDismissedHosts(ctx, s.store, ns.ID, dismissed); err != nil { |
There was a problem hiding this comment.
Fixed in 7e5fe0c — added maxDismissedHosts = 1000 cap with a check before persisting.
| return | ||
| } | ||
|
|
||
| jsonOK(w, map[string]any{"dismissed": len(dismissed)}) |
There was a problem hiding this comment.
dismissed count in dismiss-all response includes previously dismissed hosts
len(dismissed) reflects the total map size after merging all previously dismissed entries plus the newly dismissed ones, so the response value is misleading for API consumers who expect it to reflect how many hosts were dismissed in this call.
| jsonOK(w, map[string]any{"dismissed": len(dismissed)}) | |
| jsonOK(w, map[string]any{"dismissed": len(unmatched)}) |
There was a problem hiding this comment.
Fixed in 7e5fe0c — response now reports len(unmatched) (newly dismissed count) instead of len(dismissed) (total map size).
| await apiFetch( | ||
| `/v1/vaults/${encodeURIComponent(vaultName)}/discovered-hosts/${encodeURIComponent(host)}`, | ||
| { method: "DELETE" } | ||
| ); | ||
| await fetchDiscoveredHosts(discoveredExpanded ? 100 : 5); | ||
| } catch { | ||
| // Degrade silently. | ||
| } finally { | ||
| setDismissingHost(null); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Both dismissDiscoveredHost and dismissAllDiscoveredHosts catch all errors and discard them. If the server returns a 500 or the network drops, the UI state and the server state will diverge: the host will remain in the server's discovered list but the user sees no indication that anything went wrong.
There was a problem hiding this comment.
Fixed in 6f01df5 — both dismiss functions now check resp.ok and call setError(...) on failure, matching the pattern used by saveServices and deleteService in the same file.
| dismissed := loadDismissedHosts(ctx, s.store, ns.ID) | ||
| dismissed[host] = true | ||
| if err := saveDismissedHosts(ctx, s.store, ns.ID, dismissed); err != nil { | ||
| s.logger.Warn("dismiss-host: save failed", "vault", vaultName, "err", err.Error()) |
There was a problem hiding this comment.
The warn log for a failed dismiss save records vault and err but not the actor/user who made the request, making it harder to correlate incidents in cloud log services. The team's logging convention expects something like "user", actor.Username or the [username=…] format to appear on mutation-path log lines.
Rule Used: Follow a specific logging pattern for easier query... (source)
Learned From
Infisical/infisical#3651
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 6f01df5 — both dismiss handlers now capture the actor from requireVaultAdmin and include "actor", actor.ID, "actor_type", actor.Type in all warn log lines.
Address Greptile review feedback: - Change requireVaultMember to requireVaultAdmin for both DELETE handlers - Add maxDismissedHosts (1000) cap to prevent unbounded growth - Fix dismiss-all response to report count of newly dismissed hosts Co-Authored-By: jake <jake@infisical.com>
Manual Test ResultsTested locally (Go server + Vite dev, SQLite seeded with 4 discovered hosts). Devin session Dismiss Feature (5/5 passed)
Screenshots
Also addressed Greptile review feedback in 7e5fe0c: upgraded |
- Show error feedback on dismiss failure instead of silently swallowing - Add actor identity (ID + type) to dismiss log lines for incident correlation Co-Authored-By: jake <jake@infisical.com>
Summary
Adds the ability to dismiss discovered host suggestions from the Services page banner. Dismissed hosts are stored per-vault using the existing vault settings infrastructure (JSON array), so no database migration is required.
Backend:
DELETE /v1/vaults/{name}/discovered-hosts/{host}— dismiss a single hostDELETE /v1/vaults/{name}/discovered-hosts— dismiss all currently visible hostsGET /v1/vaults/{name}/discovered-hostsnow filters out previously dismissed hostsrequireVaultAdmin(not just member)Frontend:
Add as servicevisibilitysetError()instead of silently swallowingDismissed hosts persist across page reloads.
Type of change
Test plan
make test)All existing discovered-hosts tests continue to pass (7/7). Go build and TypeScript typecheck both succeed. Manual UI testing confirmed dismiss single, dismiss all, and persistence across reloads.
Security checklist
Both new endpoints use the same
requireInitialized → requireAuth → actorAuthedmiddleware chain, withrequireVaultAdminenforced inside handlers. Dismissed hosts list is capped at 1000 entries per vault.Link to Devin session: https://app.devin.ai/sessions/d95feaed4e29455aa99c9d8c31b9f5cc