Prevent redirect bypasses via backslash-encoded paths

[lunny]

This change tightens relative URL validation to reject raw backslashes and %5c (encoded backslash), since browsers and URL normalizers can treat backslashes as path separators. That normalization can turn seemingly relative paths into scheme-relative URLs, creating open-redirect risk.

Visiting below URL to reproduce the problem.

http://localhost:3000/user/login?redirect_to=/a/../\example.com

http://localhost:3000/user/login?redirect_to=/a/../%5cexample.com

[silverwind]

Review by @silverwind, written with Claude Code.

Looks correct. The fix addresses open redirect via backslash normalization: browsers can normalize \ to / in URLs, so a path like /%5cexample.com decodes to /\example.com which the browser may then treat as //example.com (protocol-relative redirect to example.com). The existing two-character prefix check on line 24 doesn't catch these cases because the raw string starts with /% rather than /\.

With the new blanket backslash check, the backslash cases in the old prefix check on line 24 (/\, \\, \/) are now redundant — only the // case still needs it. I'd simplify it to:

if len(s) >= 2 && s[0] == '/' && s[1] == '/' {
    return false
}

Other observations:

• strings.Contains(s, "\\") rejects backslashes anywhere in the URL (including query/fragment), which is slightly aggressive but appropriate for redirect safety — legitimate redirect targets shouldn't contain literal backslashes.
• The strings.ToLower for %5c correctly handles both %5c and %5C. Double-encoding (%255c) is out of scope here since that would need a second decode pass.
• The test cases cover the key vectors: mid-path backslash with traversal (/a/../\example.com), encoded backslash at root (/%5cexample.com), and the combination (/a/../%5cexample.com).
• The blanket rejection approach is the right call. The alternative (normalizing \/%5c to / and then checking) would also need .. path resolution to catch cases like /a/../%5cexample.com → //example.com, adding complexity for no gain.

The PR description is empty — it would be good to mention this is a security fix for open redirect via backslash normalization, for changelog/backport purposes.

[wxiaoguang]

wrong

[silverwind]

no, it can't be right

wrong

I'm sorry but such reviews are unactionable. You need to at least give some hint what you feel is wrong.

[wxiaoguang]

no, it can't be right

wrong

I'm sorry but such reviews are unactionable. You need to at least give some hint what you feel is wrong.

I can only see you copy-paste the AI response.

Since you have approved, can you answer the questions?

1. Why "/%5cexample.com" causes problem? Are you able to reproduce?
2. Why "/a/../%5cexample.com" causes problem?
3. If %5c causes problem, why %2f doesn't?
4. If you check %5c in s, what if s is like path?param=... ?

[wxiaoguang]

@silverwind @lunny , you see, even if I explained, there is no progress. Why should I waste time on explaining?

That's why I only say "wrong" to the people who don't understand the problem and never learn from lessons.

[silverwind]

I would help if the PR description contains some actual reasoning for the change or a link to the security report it relates to.

[lunny]

I will continue the work. Thank you for the review.

[lunny]

no, it can't be right

wrong

I'm sorry but such reviews are unactionable. You need to at least give some hint what you feel is wrong.

I can only see you copy-paste the AI response.

Since you have approved, can you answer the questions?

1. Why "/%5cexample.com" causes problem? Are you able to reproduce?

This one will not, I removed it from test.

2. Why "/a/../%5cexample.com" causes problem?

%5c will be decoded to \ and the whole string will be considered as //example.com but IsRelative doesn't detected it.

3. If %5c causes problem, why %2f doesn't?

I think it might that Web browsers thinks %2f/ as a /.

4. If you check %5c in s, what if s is like path?param=... ?

The code will be used on IsRelative which should not prevent such parameters.

[wxiaoguang]

Still, completely wrong.

If 1 doesn't, why 2 does? Are you able to reproduce in urlIsRelative or ctx.Redirect? Do you see %5c in IsRelative?

3 I think it might that Web browsers thinks: it's just your "think", have you studied it or tested it? since I have pointed it out.

Have you read code for 4? What if /path?param=%5c?

The logic itself is wrong. Really curious that whether you use your brain when you write code.

[wxiaoguang]

Still, completely wrong.

If 1 doesn't, why 2 does? Are you able to reproduce in urlIsRelative or ctx.Redirect? Do you see %5c in IsRelative?

3 I think it might that Web browsers thinks: it's just your "think", have you studied it or tested it? since I have pointed it out.

Have you read code for 4? What if /path?param=%5c?

The logic itself is wrong. Really curious that whether you use your brain when you write code.

@silverwind @lunny the questions are still not addressed

[silverwind]

Written by @silverwind using Claude Code.

I pushed a reworked fix to the branch that addresses wxiaoguang's feedback. The key issues with the previous approach:

1. strings.Contains(s, "\\") is too broad — it rejects backslashes anywhere in the raw URL string, including query parameters (e.g. /path?param=\value would be wrongly rejected).
2. Checking for %5c in the raw string is wrong — by the time the string reaches IsRelativeURL, Go's http.Request.FormValue() has already URL-decoded query parameters, so %5c → \. The %5c literal would never be present in s under normal circumstances. And if it were (double-encoding), it could false-positive on query parameters like /path?param=%5c.

The new approach: Check u.Path (the parsed, decoded path component from url.Parse) for backslashes via strings.ContainsRune(u.Path, '\\').

• u.Path excludes query parameters → no false positives on ?param=\value
• url.Parse already decodes %5c → \ in u.Path → no need for a separate %5c check
• Backslashes are not valid in URL paths per RFC 3986, so rejecting them in the path is safe and targeted

Test cases include both exploit vectors (/a/../\example.com, /a/..\example.com, \) and valid URLs with backslashes in query params (/foo?k=\&v=\, /foo?k=%5c#abc) that should remain allowed.

[silverwind]

Proper fix applied and added test cases so this does not reject valid backslashes in query.

[wxiaoguang]

You deleted all 4 test cases I added in 2260d0d. Can you restore them?

What's wrong? Have you read the new code?

[silverwind]

Your strings.Contains(u.Path, "\\") is equvialent to my strings.ContainsRune(u.Path, '\\'), but yours is slower. I don't see why you would want to drop my test cases, they will still pass.

[wxiaoguang]

Your strings.Contains(u.Path, "\\") is equvialent to my strings.ContainsRune(u.Path, '\\'), but yours is slower. I don't see why you would want to drop my test cases, they will still pass.

Can you stop guessing or imagining? How slow it could be? Have you benchmarked it?

[wxiaoguang]

I don't see why you would want to drop my test cases, they will still pass.

I just reset all files from previous commit, because the code need to be rewritten with comments, the tests can be simplified and focused. What's wrong?

[silverwind]

Can you stop guessing or imagining? How slow it could be? Have you benchmarked it?

There is a slight difference in the implementations in golang, but practically not meaningful.

I just reset all files from previous commit, because the code need to be rewritten with comments, the tests can be simplified and focused. What's wrong?

More test cases are always valuable imho so you can be sure the implemenation catches all edge cases.

[wxiaoguang]

I just reset all files from previous commit, because the code need to be rewritten with comments, the tests can be simplified and focused. What's wrong?

More test cases are always valuable imho so you can be sure the implemenation catches all edge cases.

Show me, what edge cases. Don't just guess.

[silverwind]

Your tests validate the same as mine. That's fine but you could have just mentioned that and we save this whole discussion.

[wxiaoguang]

Your tests validate the same as mine. That's fine but you could have just mentioned that and we save this whole discussion.

But why you don't read code but only guess?

[silverwind]

I saw you reverting my stuff and then I compared the tests only in a fleeting fashion.