fix(apps/ensadmin): validate name query params on name details page#1834
fix(apps/ensadmin): validate name query params on name details page#1834
Conversation
🦋 Changeset detectedLatest commit: f2c0398 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds input interpretation/normalization and inline error handling to the Explore Names form and name detail page; introduces interpretation utilities and display formatting; tightens downstream prop typings to require normalized names; and switches several breadcrumb links to use Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant Page as name/page.tsx
participant Interpreter as interpretNameFromUserInput
participant Validator as isInterpretedName / isNormalizedName
participant Router as next/router
participant Errors as NameErrors
participant Detail as NameDetailPageContent
User->>Browser: Open /name?name=rawQuery
Browser->>Page: page loads (reads searchParams)
Page->>Validator: validate `name`
alt not isInterpretedName
Page->>Errors: render UnnormalizedNameError
else isInterpretedName and not isNormalizedName
Page->>Errors: render InterpretedNameUnsupportedError
else isNormalizedName
Page->>Detail: render NameDetailPageContent (NormalizedName)
end
User->>Browser: Submit Explore Names form (rawInput)
Browser->>Page: handleSubmit -> Interpreter(rawInput)
alt outcome == Normalized
Page->>Router: router.replace(/name?name=normalized)
else outcome == Empty
Page->>Page: do nothing (no navigation)
else outcome == Reencoded or Encoded
Page->>Page: set formError (show inline error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…malizedName before rendering the name details page, displaying appropriate error states for invalid or unsupported names.
There was a problem hiding this comment.
Pull request overview
Adds validation for the name query param in ENSAdmin’s name details flow, ensuring only supported ENS name formats reach the details components and providing clearer user-facing error states for invalid/unsupported names.
Changes:
- Validate
namequery param withisInterpretedName+isNormalizedNameand show dedicated error cards for invalid names / encoded labelhash names. - Tighten downstream component prop types from
NametoNormalizedNamefor the details content/header path. - Remove redundant
decodeURIComponentusage in breadcrumbs/actions pages and guardNameDisplayfrom invalid inputs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| apps/ensadmin/src/app/name/page.tsx | Adds query param validation and routes to errors vs. details content. |
| apps/ensadmin/src/app/name/_components/NameDetailPageContent.tsx | Tightens prop type to NormalizedName for safer downstream record fetching. |
| apps/ensadmin/src/app/name/_components/ProfileHeader.tsx | Tightens prop type to NormalizedName to match validated inputs. |
| apps/ensadmin/src/app/name/_components/NameErrors.tsx | Adds new UI components for invalid/unsupported name error states. |
| apps/ensadmin/src/app/@breadcrumbs/name/page.tsx | Removes redundant decoding and avoids calling NameDisplay for invalid names. |
| apps/ensadmin/src/app/@actions/name/page.tsx | Removes redundant decoding and suppresses ENS App link unless name is normalized. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR adds input validation to the ENS name details page in Confidence Score: 5/5Safe to merge — validation logic is correct for all documented inputs and no regressions are introduced. The core validation logic is sound: the two type guards are applied in the right order, the type narrowing flows correctly through to No files require special attention; Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User navigates to /name?name=X"] --> B{nameFromQuery\nnon-empty?}
B -- No --> C["Show search / example names UI"]
B -- Yes --> D{isInterpretedName\nnameFromQuery?}
D -- No --> E["InvalidNameError\n'not ENS normalized'"]
D -- Yes --> F{isNormalizedName\nnameFromQuery?}
F -- No --> G["EncodedLabelhashUnsupportedError\n'coming soon'"]
F -- Yes --> H["NameDetailPageContent\nname: NormalizedName"]
H --> I["ProfileHeader + records"]
Reviews (1): Last reviewed commit: "docs(changeset): Validate name query par..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@notrab Good to see this 👍 Reviewed and shared a few suggestions. Please feel welcome to merge when ready!
| @@ -13,11 +13,14 @@ export default function ActionsNamePage() { | |||
| const searchParams = useSearchParams(); | |||
| const nameParam = searchParams.get("name"); | |||
There was a problem hiding this comment.
Let's please move the typing for things to where they originate. Ex: If this is a Name and not an arbitrary string, then we should apply that typing here.
Please apply this idea everywhere it is relevant.
Note how the reinterpretation of this value as a Name is happening several lines below. I don't like that.
There was a problem hiding this comment.
Alright, I've changed this to const nameFromQuery = searchParams.get("name") as Name | null which is really the only other option here as far as I can tell.
| const ensAppProfileUrl = name && namespace ? getEnsManagerNameDetailsUrl(name, namespace) : null; | ||
| const ensAppProfileUrl = | ||
| name && isNormalizedName(name) && namespace | ||
| ? getEnsManagerNameDetailsUrl(name, namespace) |
There was a problem hiding this comment.
Any special reason not to move the isNormalizedName check into the implementation of getEnsManagerNameDetailsUrl?
Please ignore if not relevant.
There was a problem hiding this comment.
getEnsManagerNameDetailsUrl lives in the namehash-ui package. isNormalizedName is the responsibility of the actions here. Maybe we should not link at all if it's not normalized.
There was a problem hiding this comment.
I don't understand why you mention how getEnsManagerNameDetailsUrl lives in the namehash-ui package? How is that relevant to the ideas here?
If we say that we don't want to link to the ENS Manager Name Details page for unnormalized names, then that decision should move into getEnsManagerNameDetailsUrl so that we never need to talk about it again.
Otherwise we have to keep endlessly talking about the implementation details of unnormalized name handling which I really hate as I've been doing that over and over again for 4 years.
| type NormalizedName, | ||
| } from "@ensnode/ensnode-sdk"; | ||
|
|
||
| export const NameInterpretationOutcomeResult = { |
There was a problem hiding this comment.
| export const NameInterpretationOutcomeResult = { | |
| // Represents the possible outcome of interpreting a Name from a raw user input | |
| export const NameInterpretationOutcomeResult = { |
| | NameInterpretationOutcomeReencoded | ||
| | NameInterpretationOutcomeEncoded; | ||
|
|
||
| export function interpretNameFromUserInput(inputName: Name): NameInterpretationOutcome { |
There was a problem hiding this comment.
I need to create a JSDoc here that outlines....
Interprets a Name from raw user input into an InterpretedName
This function always succeeds with every input, producing an InterpretedName, and the outcome discrimninant tells the caller what happened so it can decide how to handle the result. Redirect, show error.
Processing logic tackled is
- Empty labels encoded as labelhash > encoded
- normalized labels as normalized
- encoded labelhash labesl as lowercased > reencoded
- unnormalizable labels as encoded as labelhash > encoded
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/ensnode-sdk/src/shared/interpretation/interpreted-names-and-labels.test.ts:1
- This change removes the entire
interpreted-names-and-labels.test.tssuite without adding equivalent coverage elsewhere. That drops test coverage for core interpretation helpers (e.g.,literalLabelsToInterpretedName,parsePartialInterpretedName, etc.) and increases regression risk; please restore these tests or move them so they still run in CI.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function UnnormalizedNameError() { | ||
| return ( | ||
| <section className="p-6"> | ||
| <ErrorInfo title="Invalid Name" description="The provided name is not a valid ENS name." /> |
There was a problem hiding this comment.
UnnormalizedNameError is shown for any name that is neither a NormalizedName nor an InterpretedName, which includes names that are valid and normalizable but simply not normalized (e.g. VITALIK.ETH). In that case, the description "not a valid ENS name" is inaccurate/misleading. Consider distinguishing "valid but not normalized" (e.g., normalize(name) succeeds but differs) from "invalid" (normalization throws) and adjusting the message accordingly.
| <ErrorInfo title="Invalid Name" description="The provided name is not a valid ENS name." /> | |
| <ErrorInfo | |
| title="Unnormalized Name" | |
| description="The provided name is not in normalized ENS form. Please use a normalized ENS name (for example, lowercase and properly formatted)." | |
| /> |
| Empty: "Empty", | ||
| /** All labels were normalized successfully.*/ | ||
| Normalized: "Normalized", | ||
| /** One or moer labels were already formatted as encoded labelhashes. None were unnormalizable. */ |
There was a problem hiding this comment.
Typo in doc comment: "moer" should be "more".
| /** One or moer labels were already formatted as encoded labelhashes. None were unnormalizable. */ | |
| /** One or more labels were already formatted as encoded labelhashes. None were unnormalizable. */ |
| switch (result.outcome) { | ||
| case NameInterpretationOutcomeResult.Empty: | ||
| break; | ||
| case NameInterpretationOutcomeResult.Normalized: { | ||
| const href = retainCurrentRawConnectionUrlParam( |
There was a problem hiding this comment.
Whitespace-only input is treated as Empty (no-op) and the submit button is disabled based on rawInputName.trim(). That means an input like " " can't be submitted via the button and won't surface an "Invalid Name" error, which conflicts with issue #1140's acceptance criteria (it explicitly lists " " as an Invalid Name case). Consider disabling only for the truly empty string, and returning an invalid-name outcome (with an inline error) for whitespace-only input.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return <InterpretedNameUnsupportedError />; | ||
| } | ||
|
|
||
| return <UnnormalizedNameError />; |
There was a problem hiding this comment.
The detail-page query-param validation currently treats a valid-but-unnormalized name (e.g. "VITALIK.ETH") the same as an invalid name and renders UnnormalizedNameError (with a "not a valid ENS name" message). This doesn’t match the intended behavior in #1140/#1059, where unnormalized-but-normalizable names should show an explicit "not ENS normalized" error distinct from truly invalid input. Consider adding a separate branch to detect “normalizable but not normalized” (e.g. via normalize(nameFromQuery) in a try/catch) and render a dedicated error state/message for that case.
| return <UnnormalizedNameError />; | |
| // Distinguish between names that are normalizable-but-unnormalized and | |
| // inputs that are truly invalid. | |
| const interpretationResult = interpretNameFromUserInput(nameFromQuery); | |
| if (interpretationResult.outcome === NameInterpretationOutcomeResult.Normalized) { | |
| // The name can be normalized but isn't in normalized form yet. | |
| // Show a dedicated "not ENS normalized" error state. | |
| return <UnnormalizedNameError />; | |
| } | |
| // For all other outcomes, treat the input as invalid and show a | |
| // "not a valid ENS name" message. | |
| return ( | |
| <section className="flex flex-col gap-6 p-6"> | |
| <Card className="w-full"> | |
| <CardHeader className="sm:pb-4 max-sm:p-3"> | |
| <CardTitle className="text-2xl">Explore ENS Names</CardTitle> | |
| </CardHeader> | |
| <CardContent className="max-sm:px-3 max-sm:pb-3"> | |
| <p className="text-sm text-red-600"> | |
| The provided input is not a valid ENS name. | |
| </p> | |
| </CardContent> | |
| </Card> | |
| </section> | |
| ); |
| /** | ||
| * Formats an InterpretedName for display in UI strings. | ||
| * | ||
| * - Normalized labels are beautified (via ens_beautify). | ||
| * - Non-normalized labels (encoded labelhashes) are lowercased for consistent display. | ||
| * | ||
| * NOTE: This function only takes an InterpretedName, not a raw Name. | ||
| * The return type is Name (not InterpretedName) because beautification | ||
| * may produce labels that are not normalized. | ||
| * | ||
| * @param interpretedName - The InterpretedName to format for display. | ||
| * @returns The formatted name. | ||
| */ | ||
| export const formatInterpretedNameForDisplay = (interpretedName: InterpretedName): Name => { | ||
| const displayLabels = interpretedName.split(".").map((label: Label) => { | ||
| if (isNormalizedLabel(label)) { | ||
| return ens_beautify(label); | ||
| } else { | ||
| return label.toLowerCase(); | ||
| } | ||
| }); | ||
| return displayLabels.join("."); | ||
| }; |
There was a problem hiding this comment.
formatInterpretedNameForDisplay is newly introduced behavior (beautify normalized labels + lowercase encoded labelhash labels) but there are no unit tests covering it. Since this package already has names.test.ts for related helpers, please add tests for at least: (1) an InterpretedName containing an encoded labelhash with uppercase hex lowercases as expected, and (2) an InterpretedName containing normalized emoji labels is beautified. Also, this PR removes shared/interpretation/interpreted-names-and-labels.test.ts without an apparent replacement; if that deletion is unintentional, consider restoring/migrating those tests to avoid coverage regression for isInterpretedName/parsing helpers that are now used in UI validation.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| } | ||
| }); | ||
| return displayLabels.join("."); | ||
| }; |
There was a problem hiding this comment.
Unit tests have been successfully added for the formatInterpretedNameForDisplay function
| return <InterpretedNameUnsupportedError />; | ||
| } | ||
|
|
||
| return <UnnormalizedNameError />; |
There was a problem hiding this comment.
Query parameter validation doesn't distinguish between unnormalized-but-normalizable names and truly invalid names, both showing the same generic "Invalid Name" error
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function interpretNameFromUserInput(inputName: Name): NameInterpretationOutcome { | ||
| if (inputName.trim() === "") { | ||
| return { | ||
| outcome: NameInterpretationOutcomeResult.Empty, | ||
| inputName, | ||
| interpretation: "" as InterpretedName, | ||
| }; | ||
| } | ||
|
|
||
| let hadEmptyLabels = false; | ||
| let hadReencodedLabels = false; | ||
| let hadUnnormalizableLabels = false; | ||
|
|
||
| const interpretedLabels = inputName.split(".").map((label) => { | ||
| if (label === "") { |
There was a problem hiding this comment.
interpretNameFromUserInput only uses trim() to detect the empty-string case, but then interprets the original untrimmed input. This means inputs like " vitalik.eth " (button enabled due to .trim()) will be treated as unnormalizable and encoded, producing an error instead of normalizing. Consider trimming once up-front (e.g., const trimmed = inputName.trim()) and using that for splitting/normalization while still returning the original inputName for display if desired.
| const router = useRouter(); | ||
| const searchParams = useSearchParams(); | ||
| const nameFromQuery = searchParams.get("name"); | ||
| const nameFromQuery = searchParams.get("name") as Name | null; |
There was a problem hiding this comment.
nameFromQuery is validated without trimming, so URLs like /name?name=vitalik.eth%20 (or whitespace-only values) will be treated as invalid and show an error. In this same app, the breadcrumbs page trims the query param before using it. Consider applying .trim() (and treating the empty result as null) before the isNormalizedName / isInterpretedName checks.
| const nameFromQuery = searchParams.get("name") as Name | null; | |
| const rawNameFromQuery = searchParams.get("name"); | |
| const nameFromQuery = | |
| rawNameFromQuery && rawNameFromQuery.trim() !== "" | |
| ? (rawNameFromQuery.trim() as Name) | |
| : null; |
| export const formatInterpretedNameForDisplay = (interpretedName: InterpretedName): Name => { | ||
| const displayLabels = interpretedName.split(".").map((label: Label) => { | ||
| if (isNormalizedLabel(label)) { | ||
| return ens_beautify(label); | ||
| } else { | ||
| return label.toLowerCase(); | ||
| } | ||
| }); | ||
| return displayLabels.join("."); |
There was a problem hiding this comment.
formatInterpretedNameForDisplay introduces new formatting behavior (beautify normalized labels + lowercase encoded labelhash labels), but there are no tests covering it. Since this module already has names.test.ts, consider adding test cases for mixed normalized/encoded-labelhash names and for ensuring encoded labelhash hex is lowercased.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
isInterpretedNameandisNormalizedNamebefore rendering the name detail pageErrorInfocards for invalid names (not ENS normalized) and names with encoded labelhashes not yet supportedNametoNormalizedNameinNameDetailPageContentandProfileHeaderWhy
InterpretedNamevalues but the current resolution APIs don't support them yet — users need a clear message instead of a confusing failureTesting
I've tested the updated form inputs (typing and clicking on the View profile button):
vitalik.eth?name=vitalik.ethVITALIK.ETH?name=vitalik.ethLiGhTWaLkEr.EtH?name=lightwalker.eth.abc..123abc|123.eth%[e4310bf4547cb18b16b5348881d24a66d61fa94a013e5636b730b86ee64a3923].ethand the direct url
/name?name=VITALIK.ETHUnnormalizedNameErrorcard/name?name=abc%7C123.ethUnnormalizedNameErrorcard/name?name=[e4310bf4547cb18b16b5348881d24a66d61fa94a013e5636b730b86ee64a3923].ethInterpretedNameUnsupportedErrorcard/name?name=/nameNotes for Reviewer (Optional)
decodeURIComponentcalls in@actions/name/page.tsxand@breadcrumbs/name/page.tsxas Next.jssearchParams.get()already returns decoded valuesInterpretedName, sinceNameDisplaycallsbeautifyNamewhich expects valid inptuPre-Review Checklist (Blocking)