Skip to content

fix(v3): reject U+0085 (NEXT LINE) in URL validation#5704

Open
greymoth-jp wants to merge 1 commit into
wailsapp:masterfrom
greymoth-jp:fix-url-validator-nel
Open

fix(v3): reject U+0085 (NEXT LINE) in URL validation#5704
greymoth-jp wants to merge 1 commit into
wailsapp:masterfrom
greymoth-jp:fix-url-validator-nel

Conversation

@greymoth-jp

@greymoth-jp greymoth-jp commented Jun 30, 2026

Copy link
Copy Markdown

Description

ValidateAndSanitizeURL (v3/pkg/application/urlvalidator.go) rejects control characters and a set of dangerous/invisible Unicode characters in URLs. The unicodeDangerous class lists every non-ASCII whitespace character (U+00A0, U+1680, U+2000-U+200A, U+2028, U+2029, U+202F, U+205F, U+3000) except U+0085 (NEXT LINE).

U+0085 has White_Space=Yes in the Unicode character database (PropList.txt), the same property as the other spaces already in the class. It is also a C1 control character, so it is not caught by the r < 32 loop earlier in the function (that loop only covers C0 controls). The result is that a URL containing NEL passes the validator untouched, even though it is the same kind of invisible whitespace as the characters the class already rejects.

This adds U+0085 to the class so the whitespace coverage is complete, and adds a test case alongside the existing zero-width / RTL-override cases.

Changes

  • v3/pkg/application/urlvalidator.go: add U+0085 to unicodeDangerous, with a short comment noting it is a C1 control that the r < 32 loop does not reach.
  • v3/pkg/application/urlvalidator_test.go: add a next line (NEL) case that expects the dangerous unicode error.
  • v3/UNRELEASED_CHANGELOG.md: add a Fixed entry.

Testing

go test ./pkg/application/ -run TestValidateURL passes. Reverting only the regex change (keeping the new test) makes the next line (NEL) case fail with expected error containing 'dangerous unicode', got nil, which confirms the test exercises this change and that nothing else is affected.

Summary by CodeRabbit

  • Bug Fixes
    • URL validation now rejects an additional hidden control character, preventing unsafe or malformed URLs from being accepted.
    • Validation errors now consistently flag this input as containing dangerous Unicode characters.
  • Tests
    • Added coverage for the newly rejected control character to confirm the validation error is returned as expected.
ValidateAndSanitizeURL lists every other non-ASCII White_Space
character in its dangerous-unicode class but omits U+0085 (NEXT
LINE). Because U+0085 is a C1 control it is also missed by the
r < 32 loop, so a URL containing it passes the validator. Add it
to the class and a test case.
@coderabbitai

coderabbitai Bot commented Jun 30, 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: 8880dc8c-d5b6-42f5-ad04-159dfde94178

📥 Commits

Reviewing files that changed from the base of the PR and between 0407ac2 and cc82c2a.

📒 Files selected for processing (3)
  • v3/UNRELEASED_CHANGELOG.md
  • v3/pkg/application/urlvalidator.go
  • v3/pkg/application/urlvalidator_test.go

Walkthrough

ValidateAndSanitizeURL is updated to explicitly reject U+0085 (NEXT LINE), a C1 control character not caught by the existing ASCII control loop. The unicodeDangerous regex is extended, a matching test case is added, and the fix is noted in the changelog.

URL Validator: U+0085 fix

Layer / File(s) Summary
Regex fix, test, and changelog
v3/pkg/application/urlvalidator.go, v3/pkg/application/urlvalidator_test.go, v3/UNRELEASED_CHANGELOG.md
unicodeDangerous regex is updated to include U+0085 with comments explaining why it escapes the ASCII < 32 check; a "next line (NEL)" table entry is added to the test suite expecting a "dangerous unicode" error; the changelog records the fix.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • wailsapp/wails#4484: Introduced the ValidateAndSanitizeURL validator and original dangerous-unicode detection that this PR extends.
  • wailsapp/wails#4500: Also modifies ValidateAndSanitizeURL and its tests for URL sanitization in browser opening.

Suggested labels

Bug, Implemented in v3

Suggested reviewers

  • leaanthony

Poem

A sneaky NEL slipped through the gate,
U+0085, you'll have to wait!
The regex expanded, the test now runs,
No rogue control char ever wins. 🐇✨

🚥 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 is concise, specific, and accurately summarizes the main URL validation fix.
Description check ✅ Passed The description covers the change, motivation, and testing, but it omits the fixed issue reference and most checklist details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies"


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.

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

Labels

None yet

1 participant