Skip to content

Fix IDN normalization for authorities with ports#6860

Open
Gautam-aman wants to merge 2 commits into
line:mainfrom
Gautam-aman:fix-idn-host-port
Open

Fix IDN normalization for authorities with ports#6860
Gautam-aman wants to merge 2 commits into
line:mainfrom
Gautam-aman:fix-idn-host-port

Conversation

@Gautam-aman

Copy link
Copy Markdown

Summary

Fixes #6859

Currently, IDN.toASCII() is applied to the entire authority string before host/port parsing.

For authorities containing a long hostname and a port, such as:

abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefg:4567

IDN.toASCII() may throw:

IllegalArgumentException: The label in the input is too long

even though the hostname itself is within the RFC label limit.

This change applies IDN normalization only to the hostname portion after authority parsing.

Changes

  • Added a regression test for a long hostname with a port.
  • Moved IDN normalization from the full authority to the parsed hostname.
  • Preserved existing authority parsing behavior for IPv6 addresses and authorities with ports.

Reproduction

IDN.toASCII(
    "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefg:4567",
    IDN.ALLOW_UNASSIGNED
);

This throws:

IllegalArgumentException: The label in the input is too long

because the entire authority is passed to IDN.toASCII() instead of only the hostname.

Expected Behavior

The hostname:

abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefg

is only 59 characters long and should be considered valid. The presence of a port should not affect hostname validation.

@CLAassistant

CLAassistant commented Jul 16, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3e90b37c-4aac-4f76-abfc-9e3da9aeb458

📥 Commits

Reviewing files that changed from the base of the PR and between 5604f91 and 1d29df8.

📒 Files selected for processing (1)
  • core/src/test/java/com/linecorp/armeria/internal/common/SchemeAndAuthorityTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/src/test/java/com/linecorp/armeria/internal/common/SchemeAndAuthorityTest.java

📝 Walkthrough

Walkthrough

SchemeAndAuthority.of now parses IPv6 and non-IPv6 authorities before applying IDN normalization to hosts. A regression test verifies preservation and port parsing for a long host.

Changes

Authority parsing

Layer / File(s) Summary
Host extraction and validation
core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java, core/src/test/java/com/linecorp/armeria/internal/common/SchemeAndAuthorityTest.java
IPv6 scope handling precedes normalization, non-IPv6 host conversion is applied separately from ports, and a long-host authority with port 4567 is tested.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: ikhoon, jrhee17, minwoox

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: fixing IDN normalization for authorities with ports.
Description check ✅ Passed The description accurately describes the bug, fix, and regression test in this pull request.
Linked Issues check ✅ Passed The code and test changes address #6859 by parsing host and port before IDN normalization.
Out of Scope Changes check ✅ Passed The changes stay focused on the reported IDN/authority parsing fix and its regression test.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mergify

mergify Bot commented Jul 16, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java (1)

90-94: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Use string concatenation instead of regex replacement.

Since originalHost is guaranteed to be the exact prefix of rawAuthority (as it is not IPv6, userinfo has been removed, and Guava's HostAndPort does not decode or modify the prefix), you can use simple string concatenation. This is more efficient and resolves the static analysis warning about compiling regular expressions from variable inputs.

♻️ Proposed refactor
-                if (!host.equals(originalHost)) {
-                    rawAuthority = rawAuthority.replaceFirst(
-                            java.util.regex.Pattern.quote(originalHost),
-                            java.util.regex.Matcher.quoteReplacement(host));
-                }
+                if (!host.equals(originalHost)) {
+                    rawAuthority = host + rawAuthority.substring(originalHost.length());
+                }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java`
around lines 90 - 94, Replace the regex-based rawAuthority update in
SchemeAndAuthority with direct string concatenation: remove the replaceFirst
call and construct the authority using host plus the unchanged suffix after
originalHost. Preserve the existing condition and all remaining authority
content exactly.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java`:
- Around line 90-94: Replace the regex-based rawAuthority update in
SchemeAndAuthority with direct string concatenation: remove the replaceFirst
call and construct the authority using host plus the unchanged suffix after
originalHost. Preserve the existing condition and all remaining authority
content exactly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: be8da550-175e-4bf9-8445-30702ab38f18

📥 Commits

Reviewing files that changed from the base of the PR and between a953182 and 5604f91.

📒 Files selected for processing (2)
  • core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java
  • core/src/test/java/com/linecorp/armeria/internal/common/SchemeAndAuthorityTest.java

@Gautam-aman

Copy link
Copy Markdown
Author

The remaining CI failures appear to be in example.armeria.athenz.AthenzTest and other unrelated jobs.

This PR only modifies:

  • core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java
  • core/src/test/java/com/linecorp/armeria/internal/common/SchemeAndAuthorityTest.java

Please let me know if there is a specific failure related to this change that I should investigate further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The label in the input is too long exception triggered by a long domain with a port

2 participants